Build the binding EntityName in CompileTimeBindingPatternStart #6224
Conversation
danakj
force-pushed
the
entity-name-binding
branch
from
6d559ae to
cf75142
Compare
josh11b
left a comment
josh11b
left a comment
There was a problem hiding this comment.
I think I'm a bit out of my depth here, I'm going to ask for additional review help.
|
PTAL |
| // Push an error for the EntityName's binding index, since we're not | ||
| // constructing an entity with make_binding_pattern(). | ||
| context.scope_stack().PushCompileTimeBinding( | ||
| SemIR::ErrorInst::InstId); |
There was a problem hiding this comment.
[Just musing here, not requesting changes in this PR.]
I wasn't especially happy when I split the numbering of compile-time bindings into separate Add / Push steps, and now the two calls can't be in the same function any more I'm feeling more unhappy about it :) The invariant we're maintaining here is a bit too distributed across the code and a bit too subtle, I think. I wonder if we can rearrange this somehow to make it correct by construction.
Potential idea: could we store a list of EntityNames for compile time bindings on the scope stack instead of a list of instructions? If that works, we could maybe allocate the compile time binding number and the entity name in a single operation.
There was a problem hiding this comment.
Oh, I think this is not just subtle but actually subtly incorrect (but only in a case that we can't yet reach, using a generic lambda). Consider:
fn F(T:! (fn (U:! type) => U)(i32));
I think we will:
AddCompileTimeBindingfor T -> index 0AddCompileTimeBindingfor U -> index 1 (!!)PushCompileTimeBindingfor U at index 0 (!!)- Build a lambda with one compile-time binding (which incorrectly thinks it's at index 1 but is at index 0)
- Pop the scope containing U
PushCompileTimeBindingfor T at index 0
Moving the AddCompileTimeBinding call earlier seems to be what's exposing this issue, but computing the number earlier is necessary for our .Self handling. I think what we should do is:
- Replace
AddCompileTimeBindingwith a "peek number of current compile time bindings" call, so we assign the index 0 to both T and U. - Make
PushCompileTimeBindingassert that the binding's entity name's index matches the next index to be assigned.
| auto node_kind = Parse::NodeKind::CompileTimeBindingPattern; | ||
| const DeclIntroducerState& introducer = | ||
| context.decl_introducer_state_stack().innermost(); | ||
| if (introducer.kind == Lex::TokenKind::Let) { |
There was a problem hiding this comment.
On rereading this I'm not sure I understand what's going on here: what other kinds of compile-time bindings are there besides Let? (I was first wondering if we could drop the "`let` " from the TODO diagnostic below, but maybe I'm missing something here.)
| // We avoid making a CompileTimeBinding in the case where a compile-time | ||
| // binding is disallowed, and any `.Self` name. |
There was a problem hiding this comment.
| // We avoid making a CompileTimeBinding in the case where a compile-time | |
| // binding is disallowed, and any `.Self` name. | |
| // We avoid making a CompileTimeBinding and any `.Self` name in the case | |
| // where a compile-time binding is disallowed. |
| // | ||
| // We avoid making a CompileTimeBinding in the case where a compile-time | ||
| // binding is disallowed. In that case, we already constructed an | ||
| // `entity_name_id` above. |
There was a problem hiding this comment.
| // | |
| // We avoid making a CompileTimeBinding in the case where a compile-time | |
| // binding is disallowed. In that case, we already constructed an | |
| // `entity_name_id` above. |
(Given that we don't even get here any more in the case this comment is talking about.)
3 participants
We want to build a
.Selfthat is a SymbolicBindingType with the entity name of the binding being constructed. To do so, we will need to know that entity name in CompileTimeBindingPatternStart. This makes that change in isolation, leaving a TODO for the next step.Since we create the EntityName ahead of time, and pass it through to HandleAnyBindingPattern, we also need to create it ahead of time in the handlers for runtime
letandvarbinding patterns.ScopeStack::Restore can not handle having a compile time binding introduced while suspended currently. Since this is not (yet?) a requirement, we take care to avoid adding the compile time binding at all in CompileTimeBindingPatternStart when the binding is in an incorrect scope. Currently ScopeStack::Suspend is only followed by a compile time binding in scopes that do not support compile time bindings, such as a class fn referring to a later compile time binding in the class.