Basic name poisoning support (#4654)

#4622 When using an unqualified name, disallow declaring that name in all scopes that would make it ambiguous in retrospect. Doesn't include support for poisoning in `impl library` (see new test for that with TODO). Implemented by introduce `InstId::PoisonedName` and entries with it to `NameScope`. --------- Co-authored-by: Dana Jansens <[email protected]> Co-authored-by: Richard Smith <[email protected]> Co-authored-by: Geoff Romer <[email protected]> Co-authored-by: Jon Ross-Perkins <[email protected]> Co-authored-by: josh11b <[email protected]> Co-authored-by: josh11b <[email protected]> Co-authored-by: Chandler Carruth <[email protected]> Co-authored-by: Carbon Infra Bot <[email protected]>
carbon-language · Dec 17, 2024 · 9c8773d · 9c8773d
1 parent 6b3307c
commit 9c8773d
Show file tree

Hide file tree

Showing 20 changed files with 997 additions and 91 deletions.
diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp
@@ -222,6 +222,18 @@ auto Context::DiagnoseDuplicateName(SemIRLoc dup_def, SemIRLoc prev_def)
       .Emit();
 }
 
+auto Context::DiagnosePoisonedName(SemIRLoc loc) -> void {
+  // TODO: Improve the diagnostic to replace NodeId::Invalid with the location
+  // where the name was poisoned. See discussion in
+  // https://github.com/carbon-language/carbon-lang/pull/4654#discussion_r1876607172
+  CARBON_DIAGNOSTIC(NameUseBeforeDecl, Error,
+                    "name used before it was declared");
+  CARBON_DIAGNOSTIC(NameUseBeforeDeclNote, Note, "declared here");
+  emitter_->Build(SemIR::LocId::Invalid, NameUseBeforeDecl)
+      .Note(loc, NameUseBeforeDeclNote)
+      .Emit();
+}
+
 auto Context::DiagnoseNameNotFound(SemIRLoc loc, SemIR::NameId name_id)
     -> void {
   CARBON_DIAGNOSTIC(NameNotFound, Error, "name `{0}` not found", SemIR::NameId);
@@ -354,15 +366,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.
+  llvm::SmallVector<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()) {
-      return non_lexical_result;
+        !non_lexical_result.inst_id.is_poisoned()) {
+      if (non_lexical_result.inst_id.is_valid()) {
+        // 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});
     }
   }
 
@@ -616,7 +639,8 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, 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()) {
         DiagnoseMemberNameNotFound(loc_id, name_id, lookup_scopes);

diff --git a/toolchain/check/context.h b/toolchain/check/context.h
@@ -246,6 +246,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 loc) -> void;
+
   // Prints a diagnostic for a missing name.
   auto DiagnoseNameNotFound(SemIRLoc loc, SemIR::NameId name_id) -> void;
 

diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp
@@ -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");
   }
@@ -167,12 +170,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);
   }
@@ -260,6 +266,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.
@@ -277,8 +286,14 @@ 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::Poisoned:
     case DeclNameStack::NameContext::State::Unresolved:
       // Because more qualifiers were found, we diagnose that the earlier
       // qualifier failed to resolve.
@@ -367,6 +382,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);

diff --git a/toolchain/check/decl_name_stack.h b/toolchain/check/decl_name_stack.h
@@ -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.
@@ -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.

diff --git a/toolchain/check/handle_alias.cpp b/toolchain/check/handle_alias.cpp
@@ -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;
 }

diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp
@@ -113,6 +113,12 @@ static auto MergeOrAddName(Context& context, Parse::AnyClassDeclId node_id,
     return;
   }
 
+  if (prev_id.is_poisoned()) {
+    // This is a 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);
@@ -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());

diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp
@@ -123,6 +123,11 @@ static auto TryMergeRedecl(Context& context, Parse::AnyFunctionDeclId node_id,
     return;
   }
 
+  if (prev_id.is_poisoned()) {
+    context.DiagnosePoisonedName(function_info.latest_decl_id());
+    return;
+  }
+
   auto prev_function_id = SemIR::FunctionId::Invalid;
   auto prev_import_ir_id = SemIR::ImportIRId::Invalid;
   CARBON_KIND_SWITCH(context.insts().Get(prev_id)) {

diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp
@@ -66,8 +66,13 @@ static auto BuildInterfaceDecl(Context& context,
   auto existing_id = context.decl_name_stack().LookupOrAddName(
       name_context, interface_decl_id, introducer.modifier_set.GetAccessKind());
   if (existing_id.is_valid()) {
-    if (auto existing_interface_decl =
-            context.insts().Get(existing_id).TryAs<SemIR::InterfaceDecl>()) {
+    if (existing_id.is_poisoned()) {
+      // This is a declaration of a poisoned name.
+      context.DiagnosePoisonedName(interface_decl_id);
+    } else if (auto existing_interface_decl =
+                   context.insts()
+                       .Get(existing_id)
+                       .TryAs<SemIR::InterfaceDecl>()) {
       auto existing_interface =
           context.interfaces().Get(existing_interface_decl->interface_id);
       if (CheckRedeclParamsMatch(

diff --git a/toolchain/check/handle_let_and_var.cpp b/toolchain/check/handle_let_and_var.cpp
@@ -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.
@@ -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.

diff --git a/toolchain/check/handle_namespace.cpp b/toolchain/check/handle_namespace.cpp
@@ -43,10 +43,13 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool {
   auto existing_inst_id = context.decl_name_stack().LookupOrAddName(
       name_context, namespace_id, SemIR::AccessKind::Public);
   if (existing_inst_id.is_valid()) {
-    // If there's a name conflict with a namespace, "merge" by using the
-    // previous declaration. Otherwise, diagnose the issue.
-    if (auto existing =
-            context.insts().TryGetAs<SemIR::Namespace>(existing_inst_id)) {
+    if (existing_inst_id.is_poisoned()) {
+      context.DiagnosePoisonedName(namespace_id);
+    } else if (auto existing = context.insts().TryGetAs<SemIR::Namespace>(
+                   existing_inst_id)) {
+      // If there's a name conflict with a namespace, "merge" by using the
+      // previous declaration. Otherwise, diagnose the issue.
+
       // Point at the other namespace.
       namespace_inst.name_scope_id = existing->name_scope_id;
 

diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp
@@ -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) {
@@ -333,6 +334,9 @@ static auto ImportScopeFromApiFile(Context& context,
   auto& impl_scope = context.name_scopes().Get(impl_scope_id);
 
   for (const auto& api_entry : api_scope.entries()) {
+    if (api_entry.inst_id.is_poisoned()) {
+      continue;
+    }
     auto impl_name_id =
         CopyNameFromImportIR(context, api_sem_ir, api_entry.name_id);
     if (auto ns =

diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp
@@ -1194,6 +1194,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,
@@ -2907,6 +2910,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;