Rewrite the `UnconditionalRecursion` lint to use MIR #54490

wesleywiser · 2018-09-22T22:12:35Z

src/librustc_mir/lints.rs

eddyb · 2018-09-24T16:36:49Z

src/librustc_mir/lints.rs

+
+                        //is this a self call?
+                        let is_self_call =
+                            if let Some(Def::Method(_)) = tcx.describe_def(def_id) {


Do you need this describe_def check? I'd suspect that if you have issues with the substs, you'd rather normalize them somehow. Polymorphic recursion wouldn't compile anyway.

Just doing the substs check missed a few cases which the old lint caught. For example:

fn test<T: Copy>() { test::<usize>(); }

The old lint caught this. With the additional check for "are we in a trait method", all the existing tests pass for the new lint.

Ah, that is unfortunate, since it's not polymorphic recursion, and doesn't involve traits. I don't like describe_def though - can you use some functionality on tcx to check whether the function is a trait-associated item? Inherent methods, or methods in impls, need to behave like ordinary functions, it's only trait impls that care about dispatch.

Furthermore, only the part of the substs that corresponds to the trait, is relevant here, e.g. if the method has generics, they should probably be ignored.

wesleywiser · 2018-09-27T02:45:56Z

Hey @eddyb, I think I resolved most of your feedback except for this bit:

Furthermore, only the part of the substs that corresponds to the trait, is relevant here, e.g. if the method has generics, they should probably be ignored.

I'm not really sure how to go about doing that. Do you have any pointers?

src/librustc_mir/build/mod.rs

src/librustc_mir/lints.rs

eddyb · 2018-09-27T06:53:21Z

@wesleywiser You can slice both substs with ..tcx.generics_of(trait_def_id).count().

src/librustc_mir/lints.rs

wesleywiser · 2018-09-28T01:38:04Z

Thanks @eddyb! I think all of your feedback is resolved now.

src/librustc_mir/lints.rs

Part of rust-lang#51002

wesleywiser · 2018-10-04T13:40:34Z

Rebased and ready for review.

TimNN · 2018-10-16T17:56:05Z

Ping from triage @eddyb: This PR requires your review.

oli-obk · 2018-10-25T15:24:08Z

@bors r+

I have nothing more to add, lgtm

bors · 2018-10-25T15:24:11Z

📌 Commit f81d1dd has been approved by oli-obk

wesleywiser · 2018-10-25T15:29:12Z

Thanks @oli-obk!

@eddyb

…i-obk Rewrite the `UnconditionalRecursion` lint to use MIR Part of rust-lang#51002 r? @eddyb

bors · 2018-10-25T20:20:41Z

⌛ Testing commit f81d1dd with merge a4d1ac8e4337a605ee45ae8e706a550ceaa509ae...

bors · 2018-10-25T20:30:11Z

💔 Test failed - status-travis

rust-highfive · 2018-10-25T20:30:12Z

The job x86_64-gnu-distcheck of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:05:36] Caused by:
[00:05:36]   failed to download package from registry
[00:05:36] 
[00:05:36] Caused by:
[00:05:36]   failed to get 200 response from `https://crates.io/api/v1/crates/openssl-src/111.0.1+1.1.1/download`, got 403
[00:05:36] 
[00:05:36] 
[00:05:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "vendor"
[00:05:36] 
[00:05:36] 
[00:05:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test distcheck
[00:05:36] Build completed unsuccessfully in 0:02:08
---
travis_time:end:044c5b6a:start=1540499353942303725,finish=1540499353951328642,duration=9024917
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:19d56c28
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:05e00bc0
travis_time:start:05e00bc0
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:07ccdffe
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

wesleywiser · 2018-10-25T20:35:41Z

Looks like a spurious crates.io download failure?

wesleywiser · 2018-10-25T20:37:09Z

@bors retry

bors · 2018-10-25T20:40:39Z

⌛ Testing commit f81d1dd with merge 4bd4e41...

@eddyb

Rewrite the `UnconditionalRecursion` lint to use MIR Part of #51002 r? @eddyb

oli-obk · 2018-10-25T20:41:52Z

yea this has been going around, I think most of the queue has been hit by it

wesleywiser · 2018-10-25T20:42:41Z

Should we open a tracking issue for that?

oli-obk · 2018-10-25T20:49:49Z

nah, infra is already ahead of it on discord

bors · 2018-10-25T23:25:32Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 4bd4e41 to master...

dropping the param-env on the floor is obviously the wrong thing to do. The ICE was probably exposed by rust-lang#54490 adding the problem-exposing use of `traits::find_associated_item`. Fixes rust-lang#55380.

@nikomatsakis

…=nikomatsakis pass the parameter environment to `traits::find_associated_item` dropping the param-env on the floor is obviously the wrong thing to do. The ICE was probably exposed by rust-lang#54490 adding the problem-exposing use of `traits::find_associated_item`. Fixes rust-lang#55380. r? @nikomatsakis

@nikomatsakis

…=nikomatsakis pass the parameter environment to `traits::find_associated_item` dropping the param-env on the floor is obviously the wrong thing to do. The ICE was probably exposed by rust-lang#54490 adding the problem-exposing use of `traits::find_associated_item`. Fixes rust-lang#55380. r? @nikomatsakis

rust-highfive assigned eddyb Sep 22, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 22, 2018

This comment has been minimized.

Sign in to view

wesleywiser force-pushed the rewrite_it_in_mir branch from a2d81b1 to ad980e8 Compare September 23, 2018 03:21

varkor reviewed Sep 23, 2018

View reviewed changes

src/librustc_mir/lints.rs Show resolved Hide resolved

eddyb reviewed Sep 24, 2018

View reviewed changes