Skip to content

Commit

Permalink
Basic name poisoning support
Browse files Browse the repository at this point in the history
carbon-language#4622
When using an unqualified name disallow using that name in all scopes that would make it ambiguous in retrospect.
Doesn't include support for poisoning when importing (see new test for that with TODO).
Implemented by introduce `InstId::PoisonedName` and entries with it to `NameScope`.
  • Loading branch information
bricknerb committed Dec 9, 2024
1 parent bd0f620 commit 24c0d19
Show file tree
Hide file tree
Showing 17 changed files with 590 additions and 74 deletions.
23 changes: 21 additions & 2 deletions toolchain/check/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,14 @@ auto Context::DiagnoseDuplicateName(SemIRLoc dup_def, SemIRLoc prev_def)
.Emit();
}

auto Context::DiagnosePoisonedName(SemIRLoc dup_def) -> void {
CARBON_DIAGNOSTIC(
NameDeclPoisoned, Error,
"cannot declare this name in this scope since is was already used "
"without qualification in the context of this scope");
emitter_->Build(dup_def, NameDeclPoisoned).Emit();
}

auto Context::DiagnoseNameNotFound(SemIRLoc loc, SemIR::NameId name_id)
-> void {
CARBON_DIAGNOSTIC(NameNotFound, Error, "name `{0}` not found", SemIR::NameId);
Expand Down Expand Up @@ -326,16 +334,26 @@ auto Context::LookupUnqualifiedName(Parse::NodeId node_id,
scope_stack().LookupInLexicalScopes(name_id);

// Walk the non-lexical scopes and perform lookups into each of them.
// Collect scopes to poison this name when it's found.
std::vector<LookupScope> scopes_to_poison;
for (auto [index, lookup_scope_id, specific_id] :
llvm::reverse(non_lexical_scopes)) {
if (auto non_lexical_result =
LookupQualifiedName(node_id, name_id,
LookupScope{.name_scope_id = lookup_scope_id,
.specific_id = specific_id},
/*required=*/false);
non_lexical_result.inst_id.is_valid()) {
non_lexical_result.inst_id.is_valid() &&
!non_lexical_result.inst_id.is_poisoned()) {
// Poison the scopes for this name.
for (const auto [scope_id, specific_id] : scopes_to_poison) {
name_scopes().Get(scope_id).AddPoison(name_id);
}

return non_lexical_result;
}
scopes_to_poison.push_back(
{.name_scope_id = lookup_scope_id, .specific_id = specific_id});
}

if (lexical_result.is_valid()) {
Expand Down Expand Up @@ -587,7 +605,8 @@ auto Context::LookupQualifiedName(SemIRLoc loc, SemIR::NameId name_id,
result.specific_id = specific_id;
}

if (required && !result.inst_id.is_valid()) {
if (required &&
(!result.inst_id.is_valid() || result.inst_id.is_poisoned())) {
if (!has_error) {
if (prohibited_accesses.empty()) {
DiagnoseNameNotFound(loc, name_id);
Expand Down
3 changes: 3 additions & 0 deletions toolchain/check/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,9 @@ class Context {
// Prints a diagnostic for a duplicate name.
auto DiagnoseDuplicateName(SemIRLoc dup_def, SemIRLoc prev_def) -> void;

// Prints a diagnostic for a poisoned name.
auto DiagnosePoisonedName(SemIRLoc dup_def) -> void;

// Prints a diagnostic for a missing name.
auto DiagnoseNameNotFound(SemIRLoc loc, SemIR::NameId name_id) -> void;

Expand Down
29 changes: 24 additions & 5 deletions toolchain/check/decl_name_stack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ auto DeclNameStack::NameContext::prev_inst_id() -> SemIR::InstId {
case NameContext::State::Unresolved:
return SemIR::InstId::Invalid;

case NameContext::State::Poisoned:
return SemIR::InstId::PoisonedName;

case NameContext::State::Finished:
CARBON_FATAL("Finished state should only be used internally");
}
Expand Down Expand Up @@ -166,12 +169,15 @@ auto DeclNameStack::AddName(NameContext name_context, SemIR::InstId target_id,
}
}

auto DeclNameStack::AddNameOrDiagnoseDuplicate(NameContext name_context,
SemIR::InstId target_id,
SemIR::AccessKind access_kind)
-> void {
auto DeclNameStack::AddNameOrDiagnose(NameContext name_context,
SemIR::InstId target_id,
SemIR::AccessKind access_kind) -> void {
if (auto id = name_context.prev_inst_id(); id.is_valid()) {
context_->DiagnoseDuplicateName(target_id, id);
if (id.is_poisoned()) {
context_->DiagnosePoisonedName(target_id);
} else {
context_->DiagnoseDuplicateName(target_id, id);
}
} else {
AddName(name_context, target_id, access_kind);
}
Expand Down Expand Up @@ -259,6 +265,9 @@ auto DeclNameStack::ApplyAndLookupName(NameContext& name_context,
// Invalid indicates an unresolved name. Store it and return.
name_context.unresolved_name_id = name_id;
name_context.state = NameContext::State::Unresolved;
} else if (resolved_inst_id.is_poisoned()) {
name_context.unresolved_name_id = name_id;
name_context.state = NameContext::State::Poisoned;
} else {
// Store the resolved instruction and continue for the target scope
// update.
Expand All @@ -276,9 +285,15 @@ static auto CheckQualifierIsResolved(
CARBON_FATAL("No qualifier to resolve");

case DeclNameStack::NameContext::State::Resolved:
if (name_context.resolved_inst_id.is_poisoned()) {
context.DiagnoseNameNotFound(name_context.loc_id,
name_context.unresolved_name_id);
return false;
}
return true;

case DeclNameStack::NameContext::State::Unresolved:
case DeclNameStack::NameContext::State::Poisoned:
// Because more qualifiers were found, we diagnose that the earlier
// qualifier failed to resolve.
context.DiagnoseNameNotFound(name_context.loc_id,
Expand Down Expand Up @@ -366,6 +381,10 @@ auto DeclNameStack::ResolveAsScope(const NameContext& name_context,
return InvalidResult;
}

if (name_context.resolved_inst_id.is_poisoned()) {
return InvalidResult;
}

auto new_params = DeclParams(
name.name_loc_id, name.first_param_node_id, name.last_param_node_id,
name.implicit_param_patterns_id, name.param_patterns_id);
Expand Down
8 changes: 5 additions & 3 deletions toolchain/check/decl_name_stack.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ class DeclNameStack {
// An identifier didn't resolve.
Unresolved,

// An identifier was poisoned in this scope.
Poisoned,

// The name has already been finished. This is not set in the name
// returned by `FinishName`, but is used internally to track that
// `FinishName` has already been called.
Expand Down Expand Up @@ -231,9 +234,8 @@ class DeclNameStack {
SemIR::AccessKind access_kind) -> void;

// Adds a name to name lookup. Prints a diagnostic for name conflicts.
auto AddNameOrDiagnoseDuplicate(NameContext name_context,
SemIR::InstId target_id,
SemIR::AccessKind access_kind) -> void;
auto AddNameOrDiagnose(NameContext name_context, SemIR::InstId target_id,
SemIR::AccessKind access_kind) -> void;

// Adds a name to name lookup, or returns the existing instruction if this
// name has already been declared in this scope.
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/handle_alias.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ auto HandleParseNode(Context& context, Parse::AliasId /*node_id*/) -> bool {

// Add the name of the binding to the current scope.
context.decl_name_stack().PopScope();
context.decl_name_stack().AddNameOrDiagnoseDuplicate(
context.decl_name_stack().AddNameOrDiagnose(
name_context, alias_id, introducer.modifier_set.GetAccessKind());
return true;
}
Expand Down
8 changes: 7 additions & 1 deletion toolchain/check/handle_class.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,12 @@ static auto MergeOrAddName(Context& context, Parse::AnyClassDeclId node_id,
return;
}

if (prev_id.is_poisoned()) {
// This is declaration of a poisoned name.
context.DiagnosePoisonedName(class_decl_id);
return;
}

auto prev_class_id = SemIR::ClassId::Invalid;
auto prev_import_ir_id = SemIR::ImportIRId::Invalid;
auto prev = context.insts().Get(prev_id);
Expand Down Expand Up @@ -546,7 +552,7 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {
}

// Bind the name `base` in the class to the base field.
context.decl_name_stack().AddNameOrDiagnoseDuplicate(
context.decl_name_stack().AddNameOrDiagnose(
context.decl_name_stack().MakeUnqualifiedName(node_id,
SemIR::NameId::Base),
class_info.base_id, introducer.modifier_set.GetAccessKind());
Expand Down
12 changes: 6 additions & 6 deletions toolchain/check/handle_let_and_var.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ static auto BuildAssociatedConstantDecl(Context& context,
auto assoc_id = BuildAssociatedEntity(context, interface_id, decl_id);
auto name_context =
context.decl_name_stack().MakeUnqualifiedName(pattern.loc_id, name_id);
context.decl_name_stack().AddNameOrDiagnoseDuplicate(name_context, assoc_id,
access_kind);
context.decl_name_stack().AddNameOrDiagnose(name_context, assoc_id,
access_kind);
}

// Adds name bindings. Returns the resulting ID for the references.
Expand All @@ -109,16 +109,16 @@ static auto HandleNameBinding(Context& context, SemIR::InstId pattern_id,
auto name_context = context.decl_name_stack().MakeUnqualifiedName(
context.insts().GetLocId(pattern_id),
context.entity_names().Get(bind_name->entity_name_id).name_id);
context.decl_name_stack().AddNameOrDiagnoseDuplicate(
name_context, pattern_id, access_kind);
context.decl_name_stack().AddNameOrDiagnose(name_context, pattern_id,
access_kind);
return bind_name->value_id;
} else if (auto field_decl =
context.insts().TryGetAs<SemIR::FieldDecl>(pattern_id)) {
// Introduce the field name into the class.
auto name_context = context.decl_name_stack().MakeUnqualifiedName(
context.insts().GetLocId(pattern_id), field_decl->name_id);
context.decl_name_stack().AddNameOrDiagnoseDuplicate(
name_context, pattern_id, access_kind);
context.decl_name_stack().AddNameOrDiagnose(name_context, pattern_id,
access_kind);
return pattern_id;
} else {
// TODO: Handle other kinds of pattern.
Expand Down
1 change: 1 addition & 0 deletions toolchain/check/import.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id,
SemIR::InstId::Invalid, SemIR::AccessKind::Public);
if (!inserted) {
auto prev_inst_id = parent_scope->GetEntry(entry_id).inst_id;
CARBON_CHECK(!prev_inst_id.is_poisoned());
if (auto namespace_inst =
context.insts().TryGetAs<SemIR::Namespace>(prev_inst_id)) {
if (diagnose_duplicate_namespace) {
Expand Down
6 changes: 6 additions & 0 deletions toolchain/check/import_ref.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1205,6 +1205,9 @@ static auto AddNameScopeImportRefs(ImportContext& context,
const SemIR::NameScope& import_scope,
SemIR::NameScope& new_scope) -> void {
for (auto entry : import_scope.entries()) {
if (entry.inst_id.is_poisoned()) {
continue;
}
auto ref_id = AddImportRef(context, entry.inst_id);
new_scope.AddRequired({.name_id = GetLocalNameId(context, entry.name_id),
.inst_id = ref_id,
Expand Down Expand Up @@ -2790,6 +2793,9 @@ static auto GetInstForLoad(Context& context,
}

auto LoadImportRef(Context& context, SemIR::InstId inst_id) -> void {
if (inst_id.is_poisoned()) {
return;
}
auto inst = context.insts().TryGetAs<SemIR::ImportRefUnloaded>(inst_id);
if (!inst) {
return;
Expand Down
Loading

0 comments on commit 24c0d19

Please sign in to comment.