mir: don't nest the initialization scope in the remainder scope. #33235
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
| // FIXME #30046 ^~~~ | ||
|
|
||
| // Don't nest the initializer in the remainder scope. | ||
| this.scope_datas[id].parent_scope = parent; |
There was a problem hiding this comment.
Mutating this imperatively feels really hacky. Maybe add a parameter to in_scope? I forget how often we call this.
I'm still not sure if this change is OK-- but I think it's fine to refine as we go.
There was a problem hiding this comment.
The real problem is that we need to create the remainder scope, place variables and schedule drops in it, without entering it.
Do you think it's feasible to disconnect "create a scope" from "push a ScopeId"?
That might also help with the scope bloat, if nested expression scopes can reuse the same ScopeId, I would think.
|
Sorry I've been out of touch last week. Meetings proved far more absorbing than I thought they would be.
I have to admit, when I read this, it suggests to me that the real problem is that we are trying to make the scope smaller than they ought to be. It seems to me that scopes for the purpose of drops, borrowck, etc, are just... slightly different beasts than scopes for the purpose of debuginfo. I hope eventually that we only need to store the latter (at least, after MIR build is done, and drops are encoded). This would also address your concern about there being way too many scopes, since we'd only have to create a I'm still unclear on what's the best thing to do right now though. |
|
☔ The latest upstream changes (presumably #33130) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@eddyb per our conversation last week, I'm convinced by now we should make these the "debug info" scopes. Do you want to keep going with this PR, or do you have some other preferred approach? |
|
@nikomatsakis I want to simplify the handling of scopes based on the fact that you don't need as many visibility scopes as you need the current ones. |
[MIR] Make scopes debuginfo-specific (visibility scopes). Fixes #32949 by having MIR (visibility) scopes mimic the lexical structure. Unlike #33235, this PR also removes all scopes without variable bindings. Printing of scopes also changed, e.g. for: ```rust fn foo(x: i32, y: i32) { let a = 0; let b = 0; let c = 0; } ``` Before my changes: ```rust fn foo(arg0: i32, arg1: i32) -> () { let var0: i32; // "x" in scope 1 at <anon>:1:8: 1:9 let var1: i32; // "y" in scope 1 at <anon>:1:16: 1:17 let var2: i32; // "a" in scope 3 at <anon>:1:30: 1:31 let var3: i32; // "b" in scope 6 at <anon>:1:41: 1:42 let var4: i32; // "c" in scope 9 at <anon>:1:52: 1:53 ... scope tree: 0 1 2 3 { 4 5 6 { 7 8 9 10 11 } } } ``` After my changes: ```rust fn foo(arg0: i32, arg1: i32) -> () { scope 1 { let var0: i32; // "x" in scope 1 at <anon>:1:8: 1:9 let var1: i32; // "y" in scope 1 at <anon>:1:16: 1:17 scope 2 { let var2: i32; // "a" in scope 2 at <anon>:1:30: 1:31 scope 3 { let var3: i32; // "b" in scope 3 at <anon>:1:41: 1:42 scope 4 { let var4: i32; // "c" in scope 4 at <anon>:1:52: 1:53 } } } } ... }
Fixes #32949 by not having the remainder scope as the parent of the initialization scope.
If some MIR analysis will need to know the scope in which a variable "exists", it can be added then.