Skip to content
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

Model var as a pattern operator #4720

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
2 changes: 2 additions & 0 deletions toolchain/check/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ cc_library(
"deduce.h",
"diagnostic_helpers.h",
"eval.h",
"full_pattern_stack.h",
"function.h",
"generic.h",
"global_init.h",
Expand All @@ -72,6 +73,7 @@ cc_library(
"//common:array_stack",
"//common:check",
"//common:map",
"//common:set",
"//common:vlog",
"//toolchain/base:index_base",
"//toolchain/base:kind_switch",
Expand Down
8 changes: 8 additions & 0 deletions toolchain/check/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"`{0}` read before initialization.", SemIR::NameId);
"`{0}` read before initialization", SemIR::NameId);

emitter_->Emit(node_id, ReadDuringInitialization, name_id);
return {.specific_id = SemIR::SpecificId::Invalid,
.inst_id = SemIR::ErrorInst::SingletonInstId};
}
// A lexical scope never needs an associated specific. If there's a
// lexically enclosing generic, then it also encloses the point of use of
// the name.
Expand Down Expand Up @@ -763,6 +770,7 @@ auto Context::AddConvergenceBlockAndPush(Parse::NodeId node_id, int num_blocks)
if (new_block_id == SemIR::InstBlockId::Unreachable) {
new_block_id = inst_blocks().AddDefaultValue();
}
CARBON_CHECK(node_id.is_valid());
AddInst<SemIR::Branch>(node_id, {.target_id = new_block_id});
}
inst_block_stack().Pop();
Expand Down
15 changes: 15 additions & 0 deletions toolchain/check/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "toolchain/check/decl_introducer_state.h"
#include "toolchain/check/decl_name_stack.h"
#include "toolchain/check/diagnostic_helpers.h"
#include "toolchain/check/full_pattern_stack.h"
#include "toolchain/check/generic_region_stack.h"
#include "toolchain/check/global_init.h"
#include "toolchain/check/inst_block_stack.h"
Expand Down Expand Up @@ -679,6 +680,12 @@ class Context {
return bind_name_map_;
}

auto var_storage_map() -> Map<SemIR::InstId, SemIR::InstId>& {
return var_storage_map_;
}

auto full_pattern_stack() -> FullPatternStack& { return full_pattern_stack_; }

private:
// A FoldingSet node for a type.
class TypeNode : public llvm::FastFoldingSetNode {
Expand Down Expand Up @@ -792,8 +799,16 @@ class Context {

Map<SemIR::InstId, BindingPatternInfo> bind_name_map_;

// Map from VarPattern insts to the corresponding VarStorage insts. The
// VarStorage insts are allocated, emitted, and stored in the map after
// processing the enclosing full-pattern.
Map<SemIR::InstId, SemIR::InstId> var_storage_map_;

// Stack of single-entry regions being built.
ArrayStack<SemIR::InstBlockId> region_stack_;

// Stack of full-patterns currently being checked.
FullPatternStack full_pattern_stack_;
};

} // namespace Carbon::Check
Expand Down
5 changes: 5 additions & 0 deletions toolchain/check/decl_introducer_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ struct DeclIntroducerState {

// If there's an `extern library` in use, the library name.
SemIR::LibraryNameId extern_library = SemIR::LibraryNameId::Invalid;

// 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;
Comment on lines +42 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of `DeclIntroducerState` values, representing all the declaration
Expand Down
2 changes: 2 additions & 0 deletions toolchain/check/eval.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1868,6 +1868,7 @@ static auto TryEvalInstInContext(EvalContext& eval_context,
case SemIR::BranchIf::Kind:
case SemIR::BranchWithArg::Kind:
case SemIR::ImportDecl::Kind:
case SemIR::NameBindingDecl::Kind:
case SemIR::OutParam::Kind:
case SemIR::OutParamPattern::Kind:
case SemIR::RequirementEquivalent::Kind:
Expand All @@ -1879,6 +1880,7 @@ static auto TryEvalInstInContext(EvalContext& eval_context,
case SemIR::StructLiteral::Kind:
case SemIR::TupleLiteral::Kind:
case SemIR::ValueParam::Kind:
case SemIR::VarPattern::Kind:
case SemIR::VarStorage::Kind:
break;

Expand Down
70 changes: 70 additions & 0 deletions toolchain/check/full_pattern_stack.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

#ifndef CARBON_TOOLCHAIN_CHECK_FULL_PATTERN_STACK_H_
#define CARBON_TOOLCHAIN_CHECK_FULL_PATTERN_STACK_H_

#include "common/array_stack.h"
#include "common/check.h"
#include "common/set.h"
#include "toolchain/sem_ir/ids.h"

namespace Carbon::Check {

// Stack of full-patterns currently being checked. When a pattern
// is followed by an explicit initializer, name bindings should not be used
// within that initializer, although they are usable before it (within the
// pattern) and after it. This class keeps track of those state transitions.
// It is structured as a stack to handle situations like a pattern that
// contains an initializer, or a pattern in a lambda in an expression pattern.
//
// TODO: Unify this with Context::pattern_block_stack, or differentiate them
// more clearly.
class FullPatternStack {
public:
// Marks the possible start of a new full-pattern (i.e. a pattern which occurs
// in a non-pattern context).
auto PushFullPattern() -> void { bind_name_stack_.PushArray(); }

// Marks the start of the initializer for the full-pattern at the top of the
// 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());
geoffromer marked this conversation as resolved.
Show resolved Hide resolved
}
}

// Marks the end of the initializer for the full-pattern at the top of the
// stack.
auto EndPatternInitializer() -> void {
for (SemIR::InstId bind_name_id : bind_name_stack_.PeekArray()) {
CARBON_CHECK(unusable_bind_names_.Erase(bind_name_id));
}
}

// Marks the end of checking for the full-pattern at the top of the stack.
// This cannot be called while processing an initializer for the top
// pattern.
auto PopFullPattern() -> void { bind_name_stack_.PopArray(); }

// Records that `bind_inst_id` was introduced by the full-pattern at the
// top of the stack.
auto AddBindName(SemIR::InstId bind_inst_id) -> void {
bind_name_stack_.AppendToTop(bind_inst_id);
}

// Returns false if the pattern that introduced `bind_inst_id` is currently
// being initialized.
auto IsBindNameUsable(SemIR::InstId bind_inst_id) const -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return !unusable_bind_names_.Contains(bind_inst_id);
}

private:
ArrayStack<SemIR::InstId> bind_name_stack_;
Set<SemIR::InstId> unusable_bind_names_;
};

} // namespace Carbon::Check

#endif // CARBON_TOOLCHAIN_CHECK_FULL_PATTERN_STACK_H_
7 changes: 5 additions & 2 deletions toolchain/check/handle_alias.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,18 @@ auto HandleParseNode(Context& context, Parse::AliasIntroducerId /*node_id*/)
// TODO: Disallow these in parse, instead of check, so we don't have to do
// this.
context.pattern_block_stack().Push();
context.full_pattern_stack().PushFullPattern();
return true;
}

auto HandleParseNode(Context& /*context*/,
Parse::AliasInitializerId /*node_id*/) -> bool {
auto HandleParseNode(Context& context, Parse::AliasInitializerId /*node_id*/)
-> bool {
context.full_pattern_stack().StartPatternInitializer();
return true;
}

auto HandleParseNode(Context& context, Parse::AliasId /*node_id*/) -> bool {
context.full_pattern_stack().EndPatternInitializer();
auto [expr_node, expr_id] = context.node_stack().PopExprWithNodeId();

auto name_context = context.decl_name_stack().FinishName(
Expand Down
Loading
Loading