Avoid poisoning non identifier names (carbon-language#4884)

There are different use cases where we call
`Context::LookupQualifiedName()` on non identifiers, like
`NameId::SelfType`, which implicitly triggers poisoning these names. I
don't think poisoning non identifiers like`Self` is ever useful.

Use cases where `Self` is being poisoned:
* Checking allowed access:
https://github.com/carbon-language/carbon-lang/blob/e257051612e4217295e206fd3274fc75e22d206a/toolchain/check/member_access.cpp#L62,
for example, in
https://github.com/carbon-language/carbon-lang/blob/e257051612e4217295e206fd3274fc75e22d206a/toolchain/check/testdata/alias/no_prelude/fail_aliased_name_in_diag.carbon#L21
* Using the type `Self` as a parameter type:
https://github.com/carbon-language/carbon-lang/blob/e257051612e4217295e206fd3274fc75e22d206a/core/prelude/operators/arithmetic.carbon#L78

Part of carbon-language#4622.

toolchain/check/name_lookup.h

@@ -69,6 +69,8 @@ auto LookupUnqualifiedName(Context& context, Parse::NodeId node_id,
 // poisoned name will be treated as if it is not declared. Otherwise, this is
 // a lookup for a name being declared, so the name will not be poisoned, but
 // poison will be returned if it's already been looked up.
+//
+// If `name_id` is not an identifier, the name will not be poisoned.
 auto LookupNameInExactScope(Context& context, SemIR::LocId loc_id,
                             SemIR::NameId name_id, SemIR::NameScopeId scope_id,
                             SemIR::NameScope& scope,

toolchain/sem_ir/name_scope.cpp

@@ -66,6 +66,10 @@ auto NameScope::LookupOrAdd(NameId name_id, InstId inst_id,
 
 auto NameScope::LookupOrPoison(LocId loc_id, NameId name_id)
     -> std::optional<EntryId> {
+  if (!name_id.AsIdentifierId().has_value()) {
+    return Lookup(name_id);
+  }
+
   auto insert_result = name_map_.Insert(name_id, EntryId(names_.size()));
   if (insert_result.is_inserted()) {
     names_.push_back({.name_id = name_id,

toolchain/sem_ir/name_scope.h

@@ -178,6 +178,8 @@ class NameScope : public Printable<NameScope> {
 
   // Searches for the given name. If found, including if a poisoned entry is
   // found, returns the corresponding EntryId. Otherwise, returns nullopt and
+  // poisons the name so it can't be declared later. Names that are not
+  // identifiers will not be poisoned.
   // poisons the name so it can't be declared later.
   auto LookupOrPoison(LocId loc_id, NameId name_id) -> std::optional<EntryId>;
 

toolchain/sem_ir/name_scope_test.cpp

@@ -195,8 +195,9 @@ TEST(NameScope, Lookup) {
   EXPECT_EQ(name_scope.GetEntry(*lookup), entry3);
 
   NameId unknown_name_id(++id);
-  lookup = name_scope.Lookup(unknown_name_id);
-  EXPECT_EQ(lookup, std::nullopt);
+  EXPECT_EQ(name_scope.Lookup(unknown_name_id), std::nullopt);
+  // Check that this is different from LookupOrPoison() - doesn't get poisoned.
+  EXPECT_EQ(name_scope.Lookup(unknown_name_id), std::nullopt);
 }
 
 TEST(NameScope, LookupOrPoison) {
@@ -222,24 +223,50 @@ TEST(NameScope, LookupOrPoison) {
                                  InstId(++id), AccessKind::Private)};
   name_scope.AddRequired(entry3);
 
-  LocId poisoning_loc_id(++id);
-  auto lookup = name_scope.LookupOrPoison(poisoning_loc_id, entry1.name_id);
+  LocId poisoning_loc_id_known_entries(++id);
+  auto lookup =
+      name_scope.LookupOrPoison(poisoning_loc_id_known_entries, entry1.name_id);
   ASSERT_NE(lookup, std::nullopt);
   EXPECT_EQ(static_cast<NameScope&>(name_scope).GetEntry(*lookup), entry1);
   EXPECT_EQ(static_cast<const NameScope&>(name_scope).GetEntry(*lookup),
             entry1);
 
-  lookup = name_scope.LookupOrPoison(poisoning_loc_id, entry2.name_id);
+  lookup =
+      name_scope.LookupOrPoison(poisoning_loc_id_known_entries, entry2.name_id);
   ASSERT_NE(lookup, std::nullopt);
   EXPECT_EQ(name_scope.GetEntry(*lookup), entry2);
 
-  lookup = name_scope.LookupOrPoison(poisoning_loc_id, entry3.name_id);
+  lookup =
+      name_scope.LookupOrPoison(poisoning_loc_id_known_entries, entry3.name_id);
   ASSERT_NE(lookup, std::nullopt);
   EXPECT_EQ(name_scope.GetEntry(*lookup), entry3);
 
   NameId unknown_name_id(++id);
-  lookup = name_scope.LookupOrPoison(poisoning_loc_id, unknown_name_id);
-  EXPECT_EQ(lookup, std::nullopt);
+  LocId poisoning_loc_id_unknown_entry(++id);
+  EXPECT_EQ(name_scope.LookupOrPoison(poisoning_loc_id_unknown_entry,
+                                      unknown_name_id),
+            std::nullopt);
+  // Check that this is different from Lookup() - does get poisoned.
+  lookup = name_scope.Lookup(unknown_name_id);
+  ASSERT_NE(lookup, std::nullopt);
+  EXPECT_EQ(name_scope.GetEntry(*lookup).result,
+            ScopeLookupResult::MakePoisoned(poisoning_loc_id_unknown_entry));
+}
+
+TEST(NameScope, LookupOrPoisonNotIdentifier) {
+  int id = 0;
+
+  InstId scope_inst_id(++id);
+  NameId scope_name_id(++id);
+  NameScopeId parent_scope_id(++id);
+  NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id);
+  LocId poisoning_loc_id(++id);
+
+  EXPECT_EQ(name_scope.LookupOrPoison(poisoning_loc_id, NameId::SelfType),
+            std::nullopt);
+  // Check that this is different from the identifier use case - doesn't get
+  // poisoned.
+  EXPECT_EQ(name_scope.Lookup(NameId::SelfType), std::nullopt);
 }
 
 TEST(NameScope, LookupOrAdd) {