Fix `MemCategorization` and `ExprUse` visitors for new solver #124859

compiler-errors · 2024-05-07T21:02:05Z

Best reviewed per-commit.

First, we add delayed span bugs for the cases where we previously silently quit in MemCategorization.
Second, we just inline FnCtxt into these visitors. Shouldn't change any behavior.
Actually change behavior (in the new solver) to structurally resolve aliases before calling Ty::kind and Ty::builtin_deref.

This fixes an ICE in icu4x when building it in the new trait solver.

r? lcnr

…gracefully

rust-log-analyzer · 2024-05-07T21:09:55Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling declare_clippy_lint v0.1.80 (/checkout/src/tools/clippy/declare_clippy_lint)
error[E0624]: associated function `new` is private
   --> src/tools/clippy/clippy_utils/src/sugg.rs:835:25
    |
835 |           ExprUseVisitor::new(&mut visitor, &infcx, def_id, cx.param_env, cx.typeck_results()).consume_body(closure_body);
    |
   ::: /checkout/compiler/rustc_hir_typeck/src/expr_use_visitor.rs:125:5
    |
125 | /     pub(crate) fn new(
125 | /     pub(crate) fn new(
126 | |         delegate: &'a mut (dyn Delegate<'tcx> + 'a),
127 | |         fcx: &'a FnCtxt<'a, 'tcx>,
128 | |         body_id: LocalDefId,
    | |_____________- private associated function defined here

error[E0061]: this function takes 3 arguments but 5 arguments were supplied
   --> src/tools/clippy/clippy_utils/src/sugg.rs:835:9
   --> src/tools/clippy/clippy_utils/src/sugg.rs:835:9
    |
835 |         ExprUseVisitor::new(&mut visitor, &infcx, def_id, cx.param_env, cx.typeck_results()).consume_body(closure_body);
    |         ^^^^^^^^^^^^^^^^^^^               ------          ------------  ------------------- unexpected argument of type `&TypeckResults<'_>`
    |                                           |               unexpected argument of type `rustc_middle::ty::ParamEnv<'_>`
    |                                           |               unexpected argument of type `rustc_middle::ty::ParamEnv<'_>`
    |                                           expected `&FnCtxt<'_, '_>`, found `&InferCtxt<'_>`
    = note: expected reference `&rustc_hir_typeck::fn_ctxt::FnCtxt<'_, '_>`
               found reference `&InferCtxt<'_>`
note: associated function defined here
   --> /checkout/compiler/rustc_hir_typeck/src/expr_use_visitor.rs:125:19
   --> /checkout/compiler/rustc_hir_typeck/src/expr_use_visitor.rs:125:19
    |
125 |     pub(crate) fn new(
    |                   ^^^
help: remove the extra arguments
    |
835 -         ExprUseVisitor::new(&mut visitor, &infcx, def_id, cx.param_env, cx.typeck_results()).consume_body(closure_body);
835 +         ExprUseVisitor::new(&mut visitor, /* &rustc_hir_typeck::fn_ctxt::FnCtxt<'_, '_> */, def_id).consume_body(closure_body);

    Checking url v2.5.0
error[E0624]: associated function `new` is private
   --> src/tools/clippy/clippy_utils/src/usage.rs:21:21
   --> src/tools/clippy/clippy_utils/src/usage.rs:21:21
    |
21  |       ExprUseVisitor::new(
    |                       ^^^ private associated function
    |
   ::: /checkout/compiler/rustc_hir_typeck/src/expr_use_visitor.rs:125:5
    |
125 | /     pub(crate) fn new(
126 | |         delegate: &'a mut (dyn Delegate<'tcx> + 'a),
127 | |         fcx: &'a FnCtxt<'a, 'tcx>,
128 | |         body_id: LocalDefId,
    | |_____________- private associated function defined here

error[E0061]: this function takes 3 arguments but 5 arguments were supplied
   --> src/tools/clippy/clippy_utils/src/usage.rs:21:5
   --> src/tools/clippy/clippy_utils/src/usage.rs:21:5
    |
21  |     ExprUseVisitor::new(
    |     ^^^^^^^^^^^^^^^^^^^
22  |         &mut delegate,
23  |         &infcx,
    |         ------ expected `&FnCtxt<'_, '_>`, found `&InferCtxt<'_>`
24  |         expr.hir_id.owner.def_id,
25  |         cx.param_env,
    |         ------------ unexpected argument of type `rustc_middle::ty::ParamEnv<'tcx>`
26  |         cx.typeck_results(),
    |         ------------------- unexpected argument of type `&TypeckResults<'_>`
    = note: expected reference `&rustc_hir_typeck::fn_ctxt::FnCtxt<'_, '_>`
               found reference `&InferCtxt<'_>`
note: associated function defined here
   --> /checkout/compiler/rustc_hir_typeck/src/expr_use_visitor.rs:125:19
   --> /checkout/compiler/rustc_hir_typeck/src/expr_use_visitor.rs:125:19
    |
125 |     pub(crate) fn new(
    |                   ^^^
help: remove the extra arguments
    |
23  ~         /* &rustc_hir_typeck::fn_ctxt::FnCtxt<'_, '_> */,
24  ~         expr.hir_id.owner.def_id,

   Compiling color-print-proc-macro v0.3.5
    Checking anstyle-wincon v3.0.2
    Checking anstyle-query v1.0.2

compiler-errors · 2024-05-07T21:12:47Z

goddamn clippy

lcnr

ignoring clippy CI issues, this looks good 👍 some further suggestions, then r=me

lcnr · 2024-05-08T02:48:42Z

compiler/rustc_hir_typeck/src/mem_categorization.rs

+                    self.tcx()
+                        .dcx()
+                        .span_delayed_bug(self.tcx().hir().span(id), "encountered type variable");
+                    Err(())


do we ever return Err(()) in places which should successfully compile? If not, please change McResult to Result<T, ErrorGuaranteed>

lcnr · 2024-05-08T02:49:18Z

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

@@ -122,18 +122,12 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
    /// - `delegate` -- who receives the callbacks
    /// - `param_env` --- parameter environment for trait lookups (esp. pertaining to `Copy`)
    /// - `typeck_results` --- typeck results for the code being analyzed
-    pub fn new(
+    pub(crate) fn new(
        delegate: &'a mut (dyn Delegate<'tcx> + 'a),


style nit: pls flip arguments: fcx, body_id, delegate instead of teh current order

lcnr · 2024-05-08T02:53:01Z

compiler/rustc_hir_typeck/src/mem_categorization.rs

+    /// The def id of the body that is being categorized.
+    /// This is **different** from `fcx.body_id`.
+    pub inner_body_id: LocalDefId,


This feels odd, shouldn't we create a separate FnCtxt for nested bodies? I guess is issue is that the scoping is as follows?

typeck_outer { typeck_inner {} closure_capture_analysis_outer {} closure_capture_analysis_inner {} }

i.e. we already dropped the inner FnCtxt again? 🤔

The old FnCtxt is long gone, though we could create a new one (we don't care about any of the fields in FnCtxt except for the body id).

All of this is useless though, because we definitely cannot adapt this with the way that clippy is using ExprUseVisitor, so I'll have to completely rework this PR.

lcnr · 2024-05-08T02:53:31Z

compiler/rustc_hir_typeck/src/mem_categorization.rs

                        .dcx()
-                        .span_delayed_bug(self.tcx().hir().span(id), "encountered type variable");
+                        .span_delayed_bug(self.tcx.hir().span(id), "encountered type variable");
                    Err(())
                } else {
                    Ok(ty)
                }
            }
            // FIXME


existing: what's that FIXME?

Probably due to not wanting to rely on InferCtxt taintedness for expr-use-analysis.

compiler-errors · 2024-05-08T18:21:09Z

I'm going to put up a more involved PR that actually supports clippy's usecases

…r=<try> Fix MemCategorization and ExprUse visitors for new solver (this time it's better) Best reviewed by each commit. Supersedes rust-lang#124859. r? lcnr

…r=lcnr Fix MemCategorization and ExprUse visitors for new solver (this time it's better) Best reviewed by each commit. Supersedes rust-lang#124859. r? lcnr

compiler-errors added 3 commits May 7, 2024 16:21

Assert that MemCategorizationVisitor actually errors when it bails un…

3998a0f

…gracefully

Inline FnCtxt into MemCategorization and ExprUse

7219e79

Structurally resolve in MemCategorization and ExprUse

f2db557

rustbot assigned lcnr May 7, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 7, 2024

compiler-errors changed the title ~~Mem cat~~ Fix MemCategorization and ExprUse visitors for new solver May 7, 2024

compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2024

lcnr reviewed May 8, 2024

View reviewed changes

compiler-errors closed this May 8, 2024

compiler-errors mentioned this pull request May 8, 2024

Fix MemCategorization and ExprUse visitors for new solver (this time it's better) #124902

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `MemCategorization` and `ExprUse` visitors for new solver #124859

Fix `MemCategorization` and `ExprUse` visitors for new solver #124859

compiler-errors commented May 7, 2024

rust-log-analyzer commented May 7, 2024

compiler-errors commented May 7, 2024

lcnr left a comment

lcnr May 8, 2024

lcnr May 8, 2024

lcnr May 8, 2024

compiler-errors May 8, 2024

compiler-errors May 8, 2024

lcnr May 8, 2024

compiler-errors May 8, 2024

compiler-errors commented May 8, 2024

Fix MemCategorization and ExprUse visitors for new solver #124859

Fix MemCategorization and ExprUse visitors for new solver #124859

Conversation

compiler-errors commented May 7, 2024

rust-log-analyzer commented May 7, 2024

compiler-errors commented May 7, 2024

lcnr left a comment

Choose a reason for hiding this comment

lcnr May 8, 2024

Choose a reason for hiding this comment

lcnr May 8, 2024

Choose a reason for hiding this comment

lcnr May 8, 2024

Choose a reason for hiding this comment

compiler-errors May 8, 2024

Choose a reason for hiding this comment

compiler-errors May 8, 2024

Choose a reason for hiding this comment

lcnr May 8, 2024

Choose a reason for hiding this comment

compiler-errors May 8, 2024

Choose a reason for hiding this comment

compiler-errors commented May 8, 2024

Fix `MemCategorization` and `ExprUse` visitors for new solver #124859

Fix `MemCategorization` and `ExprUse` visitors for new solver #124859