Model var as a pattern operator
#4720
Conversation
| @@ -390,6 +390,13 @@ auto Context::LookupUnqualifiedName(Parse::NodeId node_id, | |||
| } | |||
|
|
|||
| if (lexical_result.is_valid()) { | |||
| if (!full_pattern_stack_.IsBindNameUsable(lexical_result)) { | |||
| CARBON_DIAGNOSTIC(ReadDuringInitialization, Error, | |||
| "`{0}` read before initialization.", SemIR::NameId); | |||
There was a problem hiding this comment.
| "`{0}` read before initialization.", SemIR::NameId); | |
| "`{0}` read before initialization", SemIR::NameId); |
| // The node corresponding to the `returned` keyword (if any). We track this | ||
| // separately from the other modifiers because the parser ensures it cannot | ||
| // coexist with them. | ||
| Parse::NodeId returned_node_id = Parse::NodeId::Invalid; |
There was a problem hiding this comment.
I didn't understand this comment: specifically, because this modifier cannot coexist with any other modifier, it seems like we could model it using ModifierOrder::Decl without adding extra state. Does that not work here?
| // stack. | ||
| auto StartPatternInitializer() -> void { | ||
| for (SemIR::InstId bind_name_id : bind_name_stack_.PeekArray()) { | ||
| CARBON_CHECK(unusable_bind_names_.Insert(bind_name_id).is_inserted()); |
There was a problem hiding this comment.
(Here and in EndPatternInitializer) Please avoid side-effects in CHECK macros.
|
|
||
| // Returns false if the pattern that introduced `bind_inst_id` is currently | ||
| // being initialized. | ||
| auto IsBindNameUsable(SemIR::InstId bind_inst_id) const -> bool { |
There was a problem hiding this comment.
It looks like we're calling this for every unqualified name lookup. That sounds quite expensive; I wonder if we can find another way to approach this. Perhaps we could somehow tag the result in the lexical lookup structure to make it unusable before the end of the pattern. Can we put some kind of tombstone InstId in the lookup table, and replace it with the actual bind name instruction in PopFullPattern?
| cast_type_id, type_node, | ||
| [&] { | ||
| CARBON_DIAGNOSTIC(IncompleteTypeInFieldDecl, Error, | ||
| "Field has incomplete type `{0}`.", SemIR::TypeId); |
There was a problem hiding this comment.
| "Field has incomplete type `{0}`.", SemIR::TypeId); | |
| "field has incomplete type `{0}`", SemIR::TypeId); |
| if (auto interface_scope = context.GetCurrentScopeAs<SemIR::InterfaceDecl>(); | ||
| interface_scope) { |
There was a problem hiding this comment.
I think this is long enough to be worth factoring out into a HandleAssociatedConstantDecl function; I think that'd make the similarity between handling of let and var decls a bit clearer too.
| @@ -20,9 +20,7 @@ static auto GetCurrentFunction(Context& context) -> SemIR::Function& { | |||
| return context.functions().Get(function_id); | |||
| } | |||
There was a problem hiding this comment.
Would it make sense to move this to check/function.cpp now that it's being used more widely? Oh, or, perhaps we should rename it to something like GetCurrentFunctionForReturn given that the new usage is only in returned handling, and what we really mean here is "get me the function that a return here would return from" not some other notion of current / enclosing function.
| // CHECK:STDOUT: name_binding_decl { | ||
| // CHECK:STDOUT: %x.patt: %empty_tuple.type = binding_pattern x | ||
| // CHECK:STDOUT: %.loc12_3.1: %empty_tuple.type = var_pattern %x.patt | ||
| // CHECK:STDOUT: } | ||
| // CHECK:STDOUT: %x.var: ref %empty_tuple.type = var x |
There was a problem hiding this comment.
The order of the output IR here is a bit surprising: first we create the var, then we initialize it (perhaps in-place), and only then do we evaluate the type expression that's involved in computing the type of the var. We're getting away with this because we can directly reference the type constant without mentioning the computation that produced it, but we should probably reorder these eventually so the type computation is spliced in prior to the var storage instruction.
| // TODO: is there a cleaner way to give VarAfterPattern access to the `var` | ||
| // token? | ||
| state.token = *(context.position() - 1); |
There was a problem hiding this comment.
We could change the token used for the VarAs* states to be the var token rather than the first token after it. (It used to be the token after because that was the start of the bracketed subtree, but we don't use the token field for that any more.)
| break; | ||
| } | ||
| break; | ||
| if (loc_id.node_id().is_valid()) { |
There was a problem hiding this comment.
Maybe use early exit here to reduce indentation?
No description provided.