Silence unecessary !Sized binding error #122119

estebank · 2024-03-07T00:10:50Z

When gathering locals, we introduce a Sized obligation for each
binding in the pattern. After doing so, we typecheck the init
expression. If this has a type failure, we store {type error}, for
both the expression and the pattern. But later we store an inference
variable for the pattern.

We now avoid any override of an existing type on a hir node when they've
already been marked as {type error}, and on E0277, when it comes from
VariableType we silence the error in support of the type error.

Fix #117846

rustbot · 2024-03-07T00:10:58Z

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

estebank · 2024-03-07T00:11:10Z

@bors try @rust-timer queue

Silence unecessary !Sized binding error When gathering locals, we introduce a `Sized` obligation for each binding in the pattern. *After* doing so, we typecheck the init expression. If this has a type failure, we store `{type error}`, for both the expression and the pattern. But later we store an inference variable for the pattern. We now avoid any override of an existing type on a hir node when they've already been marked as `{type error}`, and on E0277, when it comes from `VariableType` we silence the error in support of the type error. Fix rust-lang#117846

bors · 2024-03-07T00:12:22Z

⌛ Trying commit 4f36520 with merge 36df47e...

Nadrieril · 2024-03-07T00:17:58Z

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

+        if let Some(ty) = node_ty.remove(id)
+            && let ty::Error(e) = ty.kind()
+        {
+            // Do not overwrite nodes that were already marked as `{type error}`. This allows us to
+            // silence unnecessary errors from obligations that were set earlier than a type error
+            // was produced, but that is overwritten by later analysis.  This happens in particular
+            // for `Sized` obligations introduced in gather_locals.  (#117846)
+            node_ty.insert(id, ty);


Why not node_ty.get(id) instead of remove+insert?

We don't have LocalTableInContextMut::get.

Wouldn't get_mut work here?

It would. It's just that the type would get discarded and overwritten regardless of the success or not of the let chain, but yeah, I can make it use get_mut.

Ah, my surprise came from the fact that you're reinserting the ty you just removed. I guess you intended to insert the ty you got as input to the function?

I intended to reinsert the type I got because it is a ty::Err, which should remain, any other type would get rewritten.

I see. I would indeed prefer if you used get_mut

Or implemented get tbh

If you really prefer remove+insert could you then name the variable something like err_ty to clarify the intent?

Nadrieril · 2024-03-07T00:20:02Z

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

+        let mut typeck = self.typeck_results.borrow_mut();
+        let mut node_ty = typeck.node_types_mut();
+        if let Some(ty) = node_ty.remove(id)
+            && let ty::Error(e) = ty.kind()


Would there be value in a ty.error_reported() check here to catch more errors?

Isn't it enough to check we have an ErrorGuaranteed for that?

error_reported() looks for ty::Error even deep within the type, so it would also catch e.g. (T, {type error}). I don't know if that's relevant to this check tho

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

bors · 2024-03-07T01:40:50Z

☀️ Try build successful - checks-actions
Build commit: 36df47e (36df47edbb1b0f4b1ee7e1aabd63d17d8efa6aaa)

rust-timer · 2024-03-07T10:38:50Z

Finished benchmarking commit (36df47e): 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)

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)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	1.3%	[1.3%, 1.3%]	1

Cycles

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

Binary size

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

Bootstrap: 647.359s -> 647.517s (0.02%)
Artifact size: 175.04 MiB -> 175.06 MiB (0.01%)

Nadrieril · 2024-03-14T00:59:58Z

r=me once the remove+insert is clarified in one way or another

Nadrieril · 2024-03-17T12:54:57Z

@rustbot author

When gathering locals, we introduce a `Sized` obligation for each binding in the pattern. *After* doing so, we typecheck the init expression. If this has a type failure, we store `{type error}`, for both the expression and the pattern. But later we store an inference variable for the pattern. We now avoid any override of an existing type on a hir node when they've already been marked as `{type error}`, and on E0277, when it comes from `VariableType` we silence the error in support of the type error. Fix rust-lang#117846.

estebank · 2024-03-19T23:01:12Z

@bors r=Nadrieril

bors · 2024-03-19T23:01:15Z

📌 Commit b1575b7 has been approved by Nadrieril

It is now in the queue for this repository.

bors · 2024-03-20T02:36:40Z

⌛ Testing commit b1575b7 with merge b7dcabe...

bors · 2024-03-20T04:51:19Z

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing b7dcabe to master...

rust-timer · 2024-03-20T06:47:41Z

Finished benchmarking commit (b7dcabe): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

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)	5.8%	[5.8%, 5.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 670.108s -> 669.559s (-0.08%)
Artifact size: 312.77 MiB -> 312.80 MiB (0.01%)

…rors Do not attempt to write `ty::Err` on binding that isn't from current HIR Owner Fix rust-lang#123009. Follow up to rust-lang#122119.

rustbot assigned Nadrieril Mar 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 Mar 7, 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 Mar 7, 2024

This comment has been minimized.

Sign in to view

Nadrieril reviewed Mar 7, 2024

View reviewed changes

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved

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 Mar 7, 2024

estebank force-pushed the issue-117846 branch from 4f36520 to ccb0579 Compare March 7, 2024 22:29

rustbot 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 Mar 17, 2024

estebank added 2 commits March 19, 2024 20:56

Add test for rust-lang#117846

3f2159f

estebank force-pushed the issue-117846 branch from ccb0579 to b1575b7 Compare March 19, 2024 21:26

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 20, 2024

bors merged commit b7dcabe into rust-lang:master Mar 20, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Mar 20, 2024

matthiaskrgr mentioned this pull request Mar 24, 2024

ICE: node HirId cannot be placed in TypeckResults with hir_owner DefId #123009

Closed

estebank mentioned this pull request Mar 29, 2024

Do not attempt to write ty::Err on binding that isn't from current HIR Owner #123202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Silence unecessary !Sized binding error #122119

Silence unecessary !Sized binding error #122119

estebank commented Mar 7, 2024

rustbot commented Mar 7, 2024

estebank commented Mar 7, 2024

This comment has been minimized.

bors commented Mar 7, 2024

This comment has been minimized.

Nadrieril Mar 7, 2024

estebank Mar 7, 2024

Nadrieril Mar 8, 2024

estebank Mar 9, 2024

Nadrieril Mar 9, 2024

estebank Mar 14, 2024

Nadrieril Mar 14, 2024

Nadrieril Mar 14, 2024

Nadrieril Mar 14, 2024

Nadrieril Mar 7, 2024

estebank Mar 7, 2024

Nadrieril Mar 7, 2024

bors commented Mar 7, 2024

This comment has been minimized.

rust-timer commented Mar 7, 2024

Nadrieril commented Mar 14, 2024

Nadrieril commented Mar 17, 2024

estebank commented Mar 19, 2024

bors commented Mar 19, 2024

bors commented Mar 20, 2024

bors commented Mar 20, 2024

rust-timer commented Mar 20, 2024

Silence unecessary !Sized binding error #122119

Silence unecessary !Sized binding error #122119

Conversation

estebank commented Mar 7, 2024

rustbot commented Mar 7, 2024

estebank commented Mar 7, 2024

This comment has been minimized.

bors commented Mar 7, 2024

This comment has been minimized.

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 7, 2024

This comment has been minimized.

rust-timer commented Mar 7, 2024

Overall result: no relevant changes - no action needed

Nadrieril commented Mar 14, 2024

Nadrieril commented Mar 17, 2024

estebank commented Mar 19, 2024

bors commented Mar 19, 2024

bors commented Mar 20, 2024

bors commented Mar 20, 2024

rust-timer commented Mar 20, 2024

Overall result: ❌ regressions - no action needed