Make type_implements_trait not a query #86901

arora-aman · 2021-07-06T09:45:01Z

nikomatsakis · 2021-07-06T10:16:15Z

compiler/rustc_trait_selection/src/infer.rs

+        let trait_ref =
+            ty::TraitRef { def_id: trait_def_id, substs: self.mk_substs_trait(ty, params) };
+
+        if (trait_ref, param_env).needs_infer() {


Suggested change

if (trait_ref, param_env).needs_infer() {

// FIXME(#86868): If there are inference variables anywhere, just assume

// we don't know the answer. This works around the ICEs that would result

// from using those inference variables within the `infer_ctxt` we create below.

// Really this should be a method on `InferCtxt` instead, which would fix this

// problem.

if (trait_ref, param_env).needs_infer() {

nikomatsakis · 2021-07-06T10:17:44Z

src/test/ui/closures/2229_closure_analysis/migrations/issue-86753-2.rs

@@ -0,0 +1,47 @@
+// edition:2018
+// check-pass
+// compile-flags: -C incremental=tmp/migration_2229_incr_issue_86753


Hmm, this doesn't seem right, let me poke at this...

nikomatsakis · 2021-07-06T11:08:55Z

compiler/rustc_trait_selection/src/infer.rs

+        // FIXME(#86868): If there are inference variables anywhere, just assume
+        // we don't know the answer. This works around the ICEs that would result
+        // from using those inference variables within the `infer_ctxt` we create below.
+        // Really this should be a method on `InferCtxt` instead, which would fix this
+        // problem.
+        if (trait_ref, param_env).needs_infer() {
+            return traits::EvaluationResult::EvaluatedToUnknown;
+        }


Suggested change

// FIXME(#86868): If there are inference variables anywhere, just assume

// we don't know the answer. This works around the ICEs that would result

// from using those inference variables within the `infer_ctxt` we create below.

// Really this should be a method on `InferCtxt` instead, which would fix this

// problem.

if (trait_ref, param_env).needs_infer() {

return traits::EvaluationResult::EvaluatedToUnknown;

}

We don't need this anymore! The problem was that we were creating a fresh inference context, and hence those type variables and things were out of scope in that fresh infrence context. Since we're not doing that, it's the callers problem to pass up types that are appropriate for this inference context.

nikomatsakis · 2021-07-06T11:10:46Z

src/tools/clippy/clippy_utils/src/ty.rs

@@ -128,9 +129,10 @@ pub fn implements_trait<'tcx>(
        return false;
    }
    let ty_params = cx.tcx.mk_substs(ty_params.iter());
-    cx.tcx
-        .type_implements_trait((trait_id, ty, ty_params, cx.param_env))
+    cx.tcx.infer_ctxt().enter(|infcx|


Can you modify the ty.has_infer_types above to ty.needs_infer, which is a more complete fix? (Actually, I think I'd prefer an assertion, but that's more of a matter for @rust-lang/clippy to decide -- I would be surprised though if inference variables make their way here)

For that matter, I think the function should have a comment explaining that it returns false in the case of inference variables, as that is not obvious (also not obviously correct)

The ty.has_infer_types() was added as a fix for a ICE: #72775

Clippy changes LGTM, if needs_infer also works 👍

Makes sense -- needs_infer would probably avoid more ICEs :)

Note though @flip1995 that there's probably a "better fix" that just makes the code work

Oh, I see, but the ICE in question was #72766 and is not part of clippy, right?

it might well be that this can be an assert! in the clippy code

I really don't know if this is required in Clippy, I've never tested it without it since it was added. We call this utils function in many places, so it's likely that this actually prevents an ICE, even if it is just there as a precaution, so that this function can just be used without checking preconditions.

oli-obk · 2021-07-06T18:03:01Z

@bors try @rust-timer queue

rust-timer · 2021-07-06T18:03:02Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-07-06T18:03:10Z

⌛ Trying commit ab3889223a29f3b2b5dad1dfc8c55b389f7332ab with merge c841cc53e27582dd5b02321f0f8d603cbd1076ed...

bors · 2021-07-06T18:19:14Z

💔 Test failed - checks-actions

arora-aman · 2021-07-06T18:38:35Z

woops i realized i forgot to push my changes

arora-aman · 2021-07-06T18:53:46Z

Weird x.py check and x.py test --bless --stage 2 passed

nikomatsakis · 2021-07-06T20:23:47Z

@bors rollup-

I guess @oli-obk is concerned about performance (I am not), but let's ensure it can be isolated.

nikomatsakis · 2021-07-06T20:24:09Z

@bors try @rust-timer queue

rust-timer · 2021-07-06T20:24:10Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-07-06T20:24:19Z

⌛ Trying commit 1bcbc18 with merge 87b32b6144dfeec8b0cf9c1ad5dff144df3823ac...

bors · 2021-07-06T21:13:16Z

☀️ Try build successful - checks-actions
Build commit: 87b32b6144dfeec8b0cf9c1ad5dff144df3823ac (87b32b6144dfeec8b0cf9c1ad5dff144df3823ac)

rust-timer · 2021-07-06T21:13:18Z

Queued 87b32b6144dfeec8b0cf9c1ad5dff144df3823ac with parent 238fd72, future comparison URL.

rust-timer · 2021-07-06T23:29:04Z

Finished benchmarking try commit (87b32b6144dfeec8b0cf9c1ad5dff144df3823ac): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

nikomatsakis · 2021-07-07T00:34:26Z

@bors r+

bors · 2021-07-07T00:34:27Z

📌 Commit 1bcbc18 has been approved by nikomatsakis

bors · 2021-07-07T01:03:47Z

⌛ Testing commit 1bcbc18 with merge c0bd5a5...

bors · 2021-07-07T03:31:18Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing c0bd5a5 to master...

rust-highfive assigned nikomatsakis Jul 6, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2021