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

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

Best reviewed by each commit. Supersedes #124859.

r? lcnr

rustbot · 2024-05-08T18:22:06Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler-errors · 2024-05-08T18:22:51Z

@bors try @rust-timer queue

since i am using RefCell over a mut ref to simplify mixing self access and delegate calls

…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

bors · 2024-05-08T18:24:02Z

⌛ Trying commit 9af26d0 with merge 4544086...

compiler-errors · 2024-05-08T18:24:33Z

compiler/rustc_hir_typeck/src/mem_categorization.rs

@@ -1,774 +0,0 @@
-//! # Categorization


Goodbye to this comment. I could possibly pull all of the fn cat_* that I moved into expr_use_visitor.rs into a separate impl and copy this comment over. Let me know if necessary.

compiler-errors · 2024-05-08T18:25:12Z

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

-    Copy,
-    /// reference to x where x has a type that moves
-    Move,
+impl<'tcx, D: Delegate<'tcx>> Delegate<'tcx> for &mut D {


All call sites pass in &mut Delegate but I don't want to store a lifetime in the ExprUseVisitor struct lol

compiler-errors · 2024-05-08T18:25:31Z

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

+}
+
+pub trait TypeInformationCtxt<'tcx> {
+    type TypeckResults<'a>: Deref<Target = ty::TypeckResults<'tcx>>


Need to abstract over &'tcx TypeckResults (clippy) and RefCell<TypeckResults> (typeck)

compiler-errors · 2024-05-08T18:27:25Z

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

+    }
+}
+
+impl<'tcx> TypeInformationCtxt<'tcx> for (&LateContext<'tcx>, LocalDefId) {


We store the body id separately rather than trying to get it from the LateContext because although that stores the body, we're often calling the ExprUseVisitor on an inner closure, so the def id would be wrong (which affects upvar analysis!)

Too lazy to make this into a separate type but I could be convinced otherwise.

compiler-errors · 2024-05-08T18:32:01Z

@bors try

bors · 2024-05-08T18:33:12Z

⌛ Trying commit 8b060ab with merge 74abdbd...

…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

bors · 2024-05-08T20:11:22Z

☀️ Try build successful - checks-actions
Build commit: 74abdbd (74abdbd697099bb2e629f4379d23c9b8fe26d919)

rust-timer · 2024-05-08T22:07:01Z

Finished benchmarking commit (74abdbd): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	[4.1%, 4.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.856s -> 674.94s (-0.14%)
Artifact size: 316.00 MiB -> 315.97 MiB (-0.01%)

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

lcnr

nits

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

lcnr · 2024-05-09T15:02:29Z

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

+    /// Like `pat_ty`, but ignores implicit `&` patterns.
+    #[instrument(level = "debug", skip(self), ret)]
+    fn pat_ty_unadjusted(&self, pat: &hir::Pat<'_>) -> McResult<Ty<'tcx>> {
+        let base_ty = self.node_ty(pat.hir_id)?;


does that ICE?

Suggested change

let base_ty = self.node_ty(pat.hir_id)?;

let base_ty = self.typeck_results.pat_ty(pat);

We need to call resolve_type_vars_or_error still

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

tests/ui/traits/next-solver/typeck/normalize-in-upvar-collection.rs

compiler-errors · 2024-05-09T16:06:57Z

src/tools/clippy/clippy_utils/src/lib.rs

@@ -7,6 +7,7 @@
 #![feature(never_type)]
 #![feature(rustc_private)]
 #![feature(assert_matches)]
+#![feature(unwrap_infallible)]


Hi @rust-lang/clippy; I'm pinging you to let y'all know I added a new #![feature(..)] for Result::into_ok() to make the ExprUseVisitor that's being employed by clippy a bit more resilient to error reporting.

None of this error reporting matters for clippy, since we've typechecked the function we're visiting already, so it needs to handle Result<(), !> everywhere and into_ok() is the easiest way to do this.

bors · 2024-05-10T19:08:51Z

☔ The latest upstream changes (presumably #124972) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2024-05-11T21:09:41Z

Ok I understand why this change has fixed #123901.

We used to silently swallow errors when recursing here:

rust/compiler/rustc_hir_typeck/src/expr_use_visitor.rs

Lines 500 to 510 in 3349155

    
           fn walk_local<F>( 
        
               &mut self, 
        
               expr: &hir::Expr<'_>, 
        
               pat: &hir::Pat<'_>, 
        
               els: Option<&hir::Block<'_>>, 
        
               mut f: F, 
        
           ) where 
        
               F: FnMut(&mut Self), 
        
           { 
        
               self.walk_expr(expr); 
        
               let expr_place = return_if_err!(self.mc.cat_expr(expr));

Which now we propagate as an error all the way up the stack. I believe this is totally fine to do.

lcnr · 2024-05-12T17:35:08Z

@bors r+ rollup=iffy

bors · 2024-05-12T17:35:10Z

📌 Commit c697ec4 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-05-12T17:48:37Z

⌛ Testing commit c697ec4 with merge 852a78e...

bors · 2024-05-12T19:57:44Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 852a78e to master...

rust-timer · 2024-05-12T21:33:23Z

Finished benchmarking commit (852a78e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	1

Bootstrap: 676.904s -> 676.465s (-0.06%)
Artifact size: 316.08 MiB -> 316.13 MiB (0.01%)

flip1995

Thanks for the extra ping to the Clippy team. But I got here late anyway. All of this looks fine, only one question for better understanding.

flip1995 · 2024-05-17T01:39:55Z

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

+    pub fn for_clippy(cx: &'a LateContext<'tcx>, body_def_id: LocalDefId, delegate: D) -> Self {
+        Self::new((cx, body_def_id), delegate)
+    }


Late to the party: Why does Clippy require special treatment?

Because I want to hide the the Self::new constructor so clippy users won't be tempted to either 1. implementa a manual TypeInformationContext and/or FnCtxt, and 2. so that users will know exactly what data to pass to make a ExprUseVisitor.

I could've equally called it for_lint, or from_late_ctxt, or something instead.

Clippy was fundamentally using ExprUseVisitor differently from the only usage in the compiler, though, so that's why the separate constructor exists.

rustbot assigned lcnr May 8, 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 8, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 8, 2024

compiler-errors force-pushed the mem-cat-but-better branch from 9af26d0 to 8b060ab Compare May 8, 2024 18:31

compiler-errors commented May 8, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 8, 2024

compiler-errors commented May 8, 2024

View reviewed changes

compiler/rustc_hir_typeck/src/expr_use_visitor.rs Outdated Show resolved Hide resolved

lcnr reviewed May 9, 2024

View reviewed changes

compiler-errors force-pushed the mem-cat-but-better branch from 904ad62 to c6d875c Compare May 9, 2024 15:35

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the mem-cat-but-better branch from c6d875c to e46f870 Compare May 9, 2024 16:05

compiler-errors commented May 9, 2024

View reviewed changes

compiler-errors force-pushed the mem-cat-but-better branch from e46f870 to e465aba Compare May 9, 2024 16:11

This comment has been minimized.

Sign in to view

compiler-errors mentioned this pull request May 11, 2024

internal compiler error when copying borrow of leaked Box into async move closure for FutureExt::then #123334

Open

compiler-errors force-pushed the mem-cat-but-better branch from e465aba to 03fb1ca Compare May 11, 2024 21:15

compiler-errors added 2 commits May 12, 2024 11:52

Inline MemCategorization into ExprUseVisitor

d7595eb

Remove unncessary mut ref

72eccf2

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the mem-cat-but-better branch from f263857 to 0534f66 Compare May 12, 2024 15:52

This comment has been minimized.

Sign in to view

compiler-errors added 4 commits May 12, 2024 12:11

Introduce TypeInformationCtxt to abstract over LateCtxt/FnCtxt

e4209f1

Make LateCtxt be a type info delegate for EUV for clippy

55cf09d

Try structurally resolve

5ab6dca

Apply nits

fb298e8

compiler-errors force-pushed the mem-cat-but-better branch from 0534f66 to 3aef2f5 Compare May 12, 2024 16:11

This comment has been minimized.

Sign in to view

Propagate errors rather than using return_if_err

c697ec4

compiler-errors force-pushed the mem-cat-but-better branch from 3aef2f5 to c697ec4 Compare May 12, 2024 16:50

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 12, 2024

bors merged commit 852a78e into rust-lang:master May 12, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 12, 2024

bors mentioned this pull request May 12, 2024

[DO NOT MERGE] bootstrap with next solver enabled #124812

Draft

matthiaskrgr mentioned this pull request May 12, 2024

ICE: leftover child captures? #123901

Closed

compiler-errors deleted the mem-cat-but-better branch May 13, 2024 02:56

This was referenced May 13, 2024

ICE: explicit deref of non-derefable type #125058

Closed

ICE: explicit index of non-indexable type #125059

Open

compiler-errors mentioned this pull request May 14, 2024

Don't ICE when we have leftover child captures due to ambiguous closure params #124525

Closed

flip1995 reviewed May 17, 2024

View reviewed changes

	let base_ty = self.node_ty(pat.hir_id)?;
	let base_ty = self.typeck_results.pat_ty(pat);

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

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

Conversation

compiler-errors commented May 8, 2024 • edited

rustbot commented May 8, 2024

compiler-errors commented May 8, 2024

This comment has been minimized.

bors commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented May 8, 2024

bors commented May 8, 2024

bors commented May 8, 2024

This comment has been minimized.

rust-timer commented May 8, 2024

Overall result: no relevant changes - no action needed

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

bors commented May 10, 2024

compiler-errors commented May 11, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lcnr commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

bors commented May 12, 2024

rust-timer commented May 12, 2024

Overall result: no relevant changes - no action needed

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented May 8, 2024 •

edited

compiler-errors May 17, 2024 •

edited