Fix `SourceScope` for `if let` bindings. #97931

xldenis · 2022-06-09T22:11:09Z

I'm not sure how to test this properly, is there any way to observe the difference in behavior apart from ui tests? I'm worried that they would be overlooked in the case of a regression.

rust-highfive · 2022-06-09T22:11:12Z

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_mir_build/src/build/matches/mod.rs

xldenis · 2022-06-09T23:24:30Z

compiler/rustc_mir_build/src/build/matches/mod.rs

+        if let Some(scope) = scope {
+            self.source_scope = scope;
+        }


This is done in lower_match for match statements, so I do the same here.

actually, this seems insufficient to solve the underlying issue, though its close.

This reverts commit fe0dedcb06947317d41a8570b7fff7f8690dcbff.

xldenis · 2022-06-10T21:07:37Z

src/test/mir-opt/const_goto.issue_77355_opt.ConstGoto.diff

+      scope 1 {
+      }


I'm not super happy about this, but only creating a scope when we actually created bindings would require much more invasive changes since we need to bubble up the scope from declare_bindings in lower_let_expr

The other option would be to create a scope only for if let statements regardless of if they have bindings.

@nagisa do you consider this to be an issue? I've reduced it to only introduce scopes for if let but it can still happen

I haven't had an opportunity to give this PR a thorough look yet. I'll need to think about the implications of this approach too, so I can't promise I'll be able to get back to you this week.

That's fine, this bug is a blocker for some of my work but I'll be at a conference next week anyways

I think it is perfectly fine to create scopes liberally if it serves some purpose (such as simplifying the code) – it seems like it would likely be much more straightfoward to clean this up as a MIR transformation pass after the fact if this ever came up as a sticking point.

xldenis · 2022-06-10T21:59:02Z

There is one failing test (is ignored on my local machine?) but I would like some validation of the approach anyways if possible.

nagisa · 2022-06-11T11:32:15Z

is there any way to observe the difference in behavior apart from ui tests

A test for this sort of issue would go into src/test/debuginfo/. var-captured-in-sendable-closure.rs might serve as a reasonable template the test necessary here.

wesleywiser · 2022-06-15T22:26:20Z

Thanks for working on this @xldenis! I took the liberty of pushing a debuginfo test into your PR branch.

nagisa · 2022-06-19T14:43:24Z

I feel there might be some nice way to make lower_let_expr simpler somehow, but I don’t think thinking about it needs to block the fix here.

@bors r+

bors · 2022-06-19T14:43:26Z

📌 Commit a434e4c has been approved by nagisa

xldenis · 2022-06-19T20:26:41Z

I feel there might be some nice way to make lower_let_expr simpler somehow, but I don’t think thinking about it needs to block the fix here.

I also think this, but more broadly I think the whole logic including then_else_break could probably be simplified.

bors · 2022-06-20T03:08:55Z

⌛ Testing commit a434e4c with merge 9a0b774...

bors · 2022-06-20T05:37:11Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 9a0b774 to master...

rust-timer · 2022-06-20T06:54:29Z

Finished benchmarking commit (9a0b774): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	0.4%	0.5%	4
Regressions 😿 (secondary)	0.3%	0.3%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.4%	0.5%	4

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	3.1%	6.0%	2
Regressions 😿 (secondary)	1.4%	1.4%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	3.1%	6.0%	2

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.7%	-3.7%	1
All 😿🎉 (primary)	N/A	N/A	0

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

rust-highfive assigned nagisa Jun 9, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 9, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 9, 2022

xldenis force-pushed the fix-if-let-source-scopes branch 2 times, most recently from 2231462 to dbcc102 Compare June 9, 2022 22:33

This comment has been minimized.

Sign in to view

xldenis force-pushed the fix-if-let-source-scopes branch from dbcc102 to 18d0f97 Compare June 9, 2022 23:14

xldenis commented Jun 9, 2022

View reviewed changes

compiler/rustc_mir_build/src/build/matches/mod.rs Show resolved Hide resolved

xldenis commented Jun 9, 2022

View reviewed changes

xldenis force-pushed the fix-if-let-source-scopes branch from 77896a7 to fe0dedc Compare June 9, 2022 23:25

xldenis added 4 commits June 10, 2022 13:01

Fix SourceScope for if let bindings.

b48f3d8

More minimal changes

53deccf

Revert "More minimal changes"

cc75f0e

This reverts commit fe0dedcb06947317d41a8570b7fff7f8690dcbff.

Actually fix

e39c92e

xldenis force-pushed the fix-if-let-source-scopes branch from af8713a to e39c92e Compare June 10, 2022 20:06

xldenis commented Jun 10, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Only create scopes for if let

a5e27a1

xldenis force-pushed the fix-if-let-source-scopes branch from 6cc4cf2 to a5e27a1 Compare June 15, 2022 01:45

Add debuginfo test for if let lexical scopes

a434e4c

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 Jun 19, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 20, 2022

bors merged commit 9a0b774 into rust-lang:master Jun 20, 2022

rustbot added this to the 1.63.0 milestone Jun 20, 2022

bors mentioned this pull request Jun 20, 2022

Pull Derefer before ElaborateDrops #98145

Merged

rustbot added the perf-regression Performance regression. label Jun 20, 2022

xldenis deleted the fix-if-let-source-scopes branch June 21, 2022 02:24

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `SourceScope` for `if let` bindings. #97931

Fix `SourceScope` for `if let` bindings. #97931

xldenis commented Jun 9, 2022

rust-highfive commented Jun 9, 2022

This comment has been minimized.

xldenis Jun 9, 2022

xldenis Jun 10, 2022

xldenis Jun 10, 2022

xldenis Jun 10, 2022

xldenis Jun 11, 2022

nagisa Jun 11, 2022

xldenis Jun 11, 2022

nagisa Jun 19, 2022

This comment has been minimized.

xldenis commented Jun 10, 2022

nagisa commented Jun 11, 2022

wesleywiser commented Jun 15, 2022

nagisa commented Jun 19, 2022

bors commented Jun 19, 2022

xldenis commented Jun 19, 2022

bors commented Jun 20, 2022

bors commented Jun 20, 2022

rust-timer commented Jun 20, 2022

Fix SourceScope for if let bindings. #97931

Fix SourceScope for if let bindings. #97931

Conversation

xldenis commented Jun 9, 2022

rust-highfive commented Jun 9, 2022

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

This comment has been minimized.

xldenis commented Jun 10, 2022

nagisa commented Jun 11, 2022

wesleywiser commented Jun 15, 2022

nagisa commented Jun 19, 2022

bors commented Jun 19, 2022

xldenis commented Jun 19, 2022

bors commented Jun 20, 2022

bors commented Jun 20, 2022

rust-timer commented Jun 20, 2022

Footnotes

Fix `SourceScope` for `if let` bindings. #97931

Fix `SourceScope` for `if let` bindings. #97931