-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Separate binding insts for refs and values #6235
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
base: trunk
Are you sure you want to change the base?
Conversation
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 code changes seem generally fine here to me. I skimmed a bunch of the test updates and they all made sense. I didn't try to get GitHub to open literally all of them though.
My biggest question is whether this is the right model that we want in SemIR. Specifically, this appears (unless I misunderstand, sorry if so) to make all of the binding patterns in var
patterns into ref_binding_pattern
insts. Are we happy with that, and walking back through to discover that these aren't just ref
bindings (as created by let ref name: T
or something, but specifically ones referring to a variable? We can also come back and revisit it, but would be good to get a gut check from @jonmeow or @zygoloid on the SemIR model itself here.
// AnyBindNameOrExportDecl -> AnyBindingOrExportDecl | ||
// BindSymbolicName -> SymbolicBinding | ||
// BindAlias -> AliasBinding | ||
// BindValue -> BorrowValue (see also proposal #6231) |
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.
If this is dependent on proposal #6231, it seems odd to reference it in a TODO?
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.
How so? I feel like it's pretty normal to have a TODO that basically says "implement this proposal".
In any event, I'm happy to drop it if you prefer. It was really meant for the reviewer to begin with, and I intend to resolve it as soon as possible.
That sounds like the opposite of my intent. The basic idea of this PR to to model the different kinds of binding patterns using different inst kinds, rather than leaving that information implicit in the inst graph. It doesn't use different inst kinds for Proposal #5545 does give special treatment to some bindings under |
This resolves a TODO in
expr_info.cpp
by using the inst kind rather than the bound value to track the binding's category.Since we're churning all the
bind_name
insts in testdata anyway, I'm also taking this opportunity to align the inst naming with the design's terminology, by calling these insts "bindings" (this aspect of the PR is dependent on #6231 resolving an ambiguity in that terminology). For consistency we'll need to rename several other insts as well (see the TODO onRefBinding
); I'm deferring that to a separate PR to minimize the review load, but I think those name changes are in-scope for this review.