-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SemIR text format cleanups #4333
Conversation
- Put decl block insts in the scope of the declared entity. - Ignore attempts to name an inst that already has a name (but require those attempts to be in the same scope). - Give `Param` insts a ".param" suffix, to avoid colliding with the corresponding `BindName`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me, but would like @zygoloid to double check the scope rules here to make sure I'm not missing a problem.
Spot checking the changes to the tests, they all seem like pretty nice improvements to the SemIR output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .param
change is a clear improvement.
I think the scope change is also an improvement -- it certainly makes the behavior more consistent between return objects and other names in the decl block. I'm slightly on the fence because this is going to lead to more name collisions in the case of redeclarations, but probably the biggest question is which scope is less surprising, and I think that using the entity scope rather than the declaration instruction's scope is better on that basis.
For what it's worth, my thinking had been that for a case like
fn Outer() {
let template T:! type = i32;
fn Inner() -> T { return 0; }
}
... it'd make more sense for the reference to T
to be in the scope of Outer
(so that we don't have references into a function body scope from outside, and because that's the scope in which the expression is notionally evaluated). But I think a reference here from the scope of Inner
to the scope of Outer
isn't too much of a surprise, and we'll see such references if the body of Inner
names T
anyway.
Note: the contents of this PR were merged as part of #4221, due to a git usage error on my part. My apologies for the confusion. |
Param
insts a ".param" suffix, to avoid colliding with the correspondingBindName
.