Skip to content

Commit

Permalink
Rearrange name poisoning logic to do a little less work. (#4766)
Browse files Browse the repository at this point in the history 
Insert the poison at the same time we do the name lookup to avoid doing
two hash table lookups into each scope. This adds a bit of complication
because import logic now needs to cope with importing a name that is
already poisoned, but the complexity seems worthwhile to reduce the
number of name lookups performed.

This incidentally fixes a bug where we wouldn't poison any name scopes
if we found the name in an enclosing lexical scope, leading to one extra
diagnostic in existing tests.

Part of #4622
  • Loading branch information
@zygoloid
zygoloid authored Jan 14, 2025
1 parent 9dc450e commit 6bc36b0
Show file tree
Hide file tree
Showing 9 changed files with 251 additions and 77 deletions.
32 changes: 13 additions & 19 deletions toolchain/check/context.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,8 @@ auto Context::LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id,
// // Error, no `F` in `B`.
// fn B.F() {}
auto result = LookupNameInExactScope(loc_id, name_id, scope_id,
name_scopes().Get(scope_id));
name_scopes().Get(scope_id),
/*is_being_declared=*/true);
return {result.inst_id, result.is_poisoned};
}
}
Expand All @@ -381,26 +382,15 @@ 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.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});
non_lexical_result.inst_id.is_valid()) {
return non_lexical_result;
}
}

Expand All @@ -423,12 +413,16 @@ auto Context::LookupUnqualifiedName(Parse::NodeId node_id,

auto Context::LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id,
SemIR::NameScopeId scope_id,
const SemIR::NameScope& scope)
SemIR::NameScope& scope,
bool is_being_declared)
-> LookupNameInExactScopeResult {
if (auto entry_id = scope.Lookup(name_id)) {
if (auto entry_id = is_being_declared ? scope.Lookup(name_id)
: scope.LookupOrPoison(name_id)) {
auto entry = scope.GetEntry(*entry_id);
if (!entry.is_poisoned) {
LoadImportRef(*this, entry.inst_id);
} else if (is_being_declared) {
entry.inst_id = SemIR::InstId::Invalid;
}
return {entry.inst_id, entry.access_kind, entry.is_poisoned};
}
Expand Down Expand Up @@ -593,7 +587,7 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id,
has_error = true;
continue;
}
const auto& name_scope = name_scopes().Get(scope_id);
auto& name_scope = name_scopes().Get(scope_id);
has_error |= name_scope.has_error();

auto [scope_result_id, access_kind, is_poisoned] =
Expand All @@ -612,7 +606,7 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id,
});
}

if (!is_poisoned && (!scope_result_id.is_valid() || is_access_prohibited)) {
if (!scope_result_id.is_valid() || is_access_prohibited) {
// If nothing is found in this scope or if we encountered an invalid
// access, look in its extended scopes.
const auto& extended = name_scope.extended_scopes();
Expand Down Expand Up @@ -657,7 +651,7 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id,
result.is_poisoned = is_poisoned;
}

if (required && (!result.inst_id.is_valid() || result.is_poisoned)) {
if (required && !result.inst_id.is_valid()) {
if (!has_error) {
if (prohibited_accesses.empty()) {
DiagnoseMemberNameNotFound(loc_id, name_id, lookup_scopes);
Expand Down
12 changes: 9 additions & 3 deletions toolchain/check/context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,17 @@ class Context {

// Performs a name lookup in a specified scope, returning the referenced
// instruction. Does not look into extended scopes. Returns an invalid
// instruction if the name is poisoned or not found.
// TODO: Return the poisoning instruction if poisoned.
// instruction if the name is not found.
//
// If `is_being_declared` is false, then this is a regular name lookup, and
// the name will be poisoned if not found so that later lookups will fail; a
// 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.
auto LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id,
SemIR::NameScopeId scope_id,
const SemIR::NameScope& scope)
SemIR::NameScope& scope,
bool is_being_declared = false)
-> LookupNameInExactScopeResult;

// Appends the lookup scopes corresponding to `base_const_id` to `*scopes`.
Expand Down
25 changes: 14 additions & 11 deletions toolchain/check/import.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,16 +110,17 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id,
SemIR::InstId::Invalid, SemIR::AccessKind::Public);
if (!inserted) {
const auto& prev_entry = parent_scope->GetEntry(entry_id);
CARBON_CHECK(!prev_entry.is_poisoned);
auto prev_inst_id = prev_entry.inst_id;
if (auto namespace_inst =
context.insts().TryGetAs<SemIR::Namespace>(prev_inst_id)) {
if (diagnose_duplicate_namespace) {
auto import_id = make_import_id();
CARBON_CHECK(import_id.is_valid());
context.DiagnoseDuplicateName(import_id, prev_inst_id);
if (!prev_entry.is_poisoned) {
auto prev_inst_id = prev_entry.inst_id;
if (auto namespace_inst =
context.insts().TryGetAs<SemIR::Namespace>(prev_inst_id)) {
if (diagnose_duplicate_namespace) {
auto import_id = make_import_id();
CARBON_CHECK(import_id.is_valid());
context.DiagnoseDuplicateName(import_id, prev_inst_id);
}
return {namespace_inst->name_scope_id, prev_inst_id, true};
}
return {namespace_inst->name_scope_id, prev_inst_id, true};
}
}

Expand Down Expand Up @@ -148,9 +149,11 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id,
parent_scope = &context.name_scopes().Get(parent_scope_id);

// Diagnose if there's a name conflict, but still produce the namespace to
// supersede the name conflict in order to avoid repeat diagnostics.
// supersede the name conflict in order to avoid repeat diagnostics. Names are
// poisoned optimistically by name lookup before checking for imports, so we
// may be overwriting a poisoned entry here.
auto& entry = parent_scope->GetEntry(entry_id);
if (!inserted) {
if (!inserted && !entry.is_poisoned) {
context.DiagnoseDuplicateName(namespace_id, entry.inst_id);
entry.access_kind = SemIR::AccessKind::Public;
}
Expand Down
112 changes: 97 additions & 15 deletions toolchain/check/testdata/function/declaration/no_prelude/name_poisoning.carbon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,21 +293,49 @@ class X {
}
}

// --- fail_no_poison_when_lookup_fails.carbon
// --- fail_poison_when_lookup_fails.carbon

library "[[@TEST_NAME]]";

namespace N;
// Here we fail to find C so we don't poison anything.
// CHECK:STDERR: fail_no_poison_when_lookup_fails.carbon:[[@LINE+3]]:11: error: name `C` not found [NameNotFound]
// CHECK:STDERR: fail_poison_when_lookup_fails.carbon:[[@LINE+5]]:11: error: name `C` not found [NameNotFound]
// CHECK:STDERR: fn N.F(x: C);
// CHECK:STDERR: ^
// CHECK:STDERR:
// CHECK:STDERR: fail_poison_when_lookup_fails.carbon: error: name used before it was declared [NameUseBeforeDecl]
fn N.F(x: C);

// No failures below because nothing was poisoned.
// TODO: We should ideally only produce one diagnostic here.
// CHECK:STDERR: fail_poison_when_lookup_fails.carbon:[[@LINE+5]]:1: note: declared here [NameUseBeforeDeclNote]
// CHECK:STDERR: class C {}
// CHECK:STDERR: ^~~~~~~~~
// CHECK:STDERR:
// CHECK:STDERR: fail_poison_when_lookup_fails.carbon: error: name used before it was declared [NameUseBeforeDecl]
class C {}
// CHECK:STDERR: fail_poison_when_lookup_fails.carbon:[[@LINE+4]]:1: note: declared here [NameUseBeforeDeclNote]
// CHECK:STDERR: class N.C {}
// CHECK:STDERR: ^~~~~~~~~~~
// CHECK:STDERR:
class N.C {}

// --- fail_poison_with_lexical_result.carbon
// CHECK:STDERR: fail_poison_with_lexical_result.carbon: error: name used before it was declared [NameUseBeforeDecl]

library "[[@TEST_NAME]]";

fn F() {
class A {}

class B {
var v: A;

// CHECK:STDERR: fail_poison_with_lexical_result.carbon:[[@LINE+3]]:5: note: declared here [NameUseBeforeDeclNote]
// CHECK:STDERR: class A {}
// CHECK:STDERR: ^~~~~~~~~
class A {}
}
}

// CHECK:STDOUT: --- no_poison.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
Expand Down Expand Up @@ -1158,25 +1186,23 @@ class N.C {}
// CHECK:STDOUT:
// CHECK:STDOUT: specific @B(constants.%Self) {}
// CHECK:STDOUT:
// CHECK:STDOUT: --- fail_no_poison_when_lookup_fails.carbon
// CHECK:STDOUT: --- fail_poison_when_lookup_fails.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
// CHECK:STDOUT: %F.type: type = fn_type @F [template]
// CHECK:STDOUT: %F: %F.type = struct_value () [template]
// CHECK:STDOUT: %C.f79: type = class_type @C.1 [template]
// CHECK:STDOUT: %.a95: type = class_type @.1 [template]
// CHECK:STDOUT: %empty_struct_type: type = struct_type {} [template]
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template]
// CHECK:STDOUT: %C.9f4: type = class_type @C.2 [template]
// CHECK:STDOUT: %.fb7: type = class_type @.2 [template]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: file {
// CHECK:STDOUT: package: <namespace> = namespace [template] {
// CHECK:STDOUT: .N = %N
// CHECK:STDOUT: .C = %C.decl.loc12
// CHECK:STDOUT: }
// CHECK:STDOUT: %N: <namespace> = namespace [template] {
// CHECK:STDOUT: .F = %F.decl
// CHECK:STDOUT: .C = %C.decl.loc13
// CHECK:STDOUT: }
// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {
// CHECK:STDOUT: %x.patt: <error> = binding_pattern x
Expand All @@ -1186,25 +1212,81 @@ class N.C {}
// CHECK:STDOUT: %C.ref: <error> = name_ref C, <error> [template = <error>]
// CHECK:STDOUT: %x: <error> = bind_name x, %x.param
// CHECK:STDOUT: }
// CHECK:STDOUT: %C.decl.loc12: type = class_decl @C.1 [template = constants.%C.f79] {} {}
// CHECK:STDOUT: %C.decl.loc13: type = class_decl @C.2 [template = constants.%C.9f4] {} {}
// CHECK:STDOUT: %.decl.loc18: type = class_decl @.1 [template = constants.%.a95] {} {}
// CHECK:STDOUT: %.decl.loc23: type = class_decl @.2 [template = constants.%.fb7] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @C.1 {
// CHECK:STDOUT: class @.1 {
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template = constants.%complete_type]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%C.f79
// CHECK:STDOUT: .Self = constants.%.a95
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @C.2 {
// CHECK:STDOUT: class @.2 {
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template = constants.%complete_type]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%C.9f4
// CHECK:STDOUT: .Self = constants.%.fb7
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @F(%x.param_patt: <error>);
// CHECK:STDOUT:
// CHECK:STDOUT: --- fail_poison_with_lexical_result.carbon
// CHECK:STDOUT:
// CHECK:STDOUT: constants {
// CHECK:STDOUT: %F.type: type = fn_type @F [template]
// CHECK:STDOUT: %F: %F.type = struct_value () [template]
// CHECK:STDOUT: %A: type = class_type @A [template]
// CHECK:STDOUT: %empty_struct_type: type = struct_type {} [template]
// CHECK:STDOUT: %complete_type.357: <witness> = complete_type_witness %empty_struct_type [template]
// CHECK:STDOUT: %B: type = class_type @B [template]
// CHECK:STDOUT: %B.elem: type = unbound_element_type %B, %A [template]
// CHECK:STDOUT: %.96d: type = class_type @.1 [template]
// CHECK:STDOUT: %struct_type.v: type = struct_type {.v: %A} [template]
// CHECK:STDOUT: %complete_type.57e: <witness> = complete_type_witness %struct_type.v [template]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: file {
// CHECK:STDOUT: package: <namespace> = namespace [template] {
// CHECK:STDOUT: .F = %F.decl
// CHECK:STDOUT: }
// CHECK:STDOUT: %F.decl: %F.type = fn_decl @F [template = constants.%F] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @A {
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template = constants.%complete_type.357]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%A
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @B {
// CHECK:STDOUT: %.loc9: %B.elem = field_decl v, element0 [template]
// CHECK:STDOUT: %.decl: type = class_decl @.1 [template = constants.%.96d] {} {}
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %struct_type.v [template = constants.%complete_type.57e]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%B
// CHECK:STDOUT: .v = %.loc9
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: class @.1 {
// CHECK:STDOUT: %complete_type: <witness> = complete_type_witness %empty_struct_type [template = constants.%complete_type.357]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%.96d
// CHECK:STDOUT: complete_type_witness = %complete_type
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: fn @F() {
// CHECK:STDOUT: !entry:
// CHECK:STDOUT: %A.decl: type = class_decl @A [template = constants.%A] {} {}
// CHECK:STDOUT: %B.decl: type = class_decl @B [template = constants.%B] {} {}
// CHECK:STDOUT: return
// CHECK:STDOUT: }
// CHECK:STDOUT:
20 changes: 12 additions & 8 deletions toolchain/check/testdata/interface/no_prelude/import_access.carbon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,17 @@ fn F(i: Test.ForwardWithDef) {}

impl package Test library "[[@TEST_NAME]]";

// CHECK:STDERR: fail_todo_forward.impl.carbon:[[@LINE+4]]:9: error: name `Forward` not found [NameNotFound]
// CHECK:STDERR: fail_todo_forward.impl.carbon:[[@LINE+5]]:9: error: name `Forward` not found [NameNotFound]
// CHECK:STDERR: fn F(i: Forward*) {}
// CHECK:STDERR: ^~~~~~~
// CHECK:STDERR:
// CHECK:STDERR: fail_todo_forward.impl.carbon: error: name used before it was declared [NameUseBeforeDecl]
fn F(i: Forward*) {}

// CHECK:STDERR: fail_todo_forward.impl.carbon:[[@LINE+4]]:1: note: declared here [NameUseBeforeDeclNote]
// CHECK:STDERR: interface Forward {}
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
interface Forward {}

// --- fail_local_forward.carbon
Expand Down Expand Up @@ -406,14 +411,13 @@ private interface Redecl {}
// CHECK:STDOUT: constants {
// CHECK:STDOUT: %F.type: type = fn_type @F [template]
// CHECK:STDOUT: %F: %F.type = struct_value () [template]
// CHECK:STDOUT: %Forward.type: type = facet_type <@Forward> [template]
// CHECK:STDOUT: %Self: %Forward.type = bind_symbolic_name Self, 0 [symbolic]
// CHECK:STDOUT: %.type: type = facet_type <@.1> [template]
// CHECK:STDOUT: %Self: %.type = bind_symbolic_name Self, 0 [symbolic]
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: file {
// CHECK:STDOUT: package: <namespace> = namespace [template] {
// CHECK:STDOUT: .F = %F.decl
// CHECK:STDOUT: .Forward = %Forward.decl
// CHECK:STDOUT: }
// CHECK:STDOUT: %Test.import = import Test
// CHECK:STDOUT: %default.import = import <invalid>
Expand All @@ -422,17 +426,17 @@ private interface Redecl {}
// CHECK:STDOUT: %i.param_patt: <error> = value_param_pattern %i.patt, runtime_param0
// CHECK:STDOUT: } {
// CHECK:STDOUT: %i.param: <error> = value_param runtime_param0
// CHECK:STDOUT: %.loc8: type = splice_block %ptr [template = <error>] {
// CHECK:STDOUT: %.loc9: type = splice_block %ptr [template = <error>] {
// CHECK:STDOUT: %Forward.ref: <error> = name_ref Forward, <error> [template = <error>]
// CHECK:STDOUT: %ptr: type = ptr_type <error> [template = <error>]
// CHECK:STDOUT: }
// CHECK:STDOUT: %i: <error> = bind_name i, %i.param
// CHECK:STDOUT: }
// CHECK:STDOUT: %Forward.decl: type = interface_decl @Forward [template = constants.%Forward.type] {} {}
// CHECK:STDOUT: %.decl: type = interface_decl @.1 [template = constants.%.type] {} {}
// CHECK:STDOUT: }
// CHECK:STDOUT:
// CHECK:STDOUT: interface @Forward {
// CHECK:STDOUT: %Self: %Forward.type = bind_symbolic_name Self, 0 [symbolic = constants.%Self]
// CHECK:STDOUT: interface @.1 {
// CHECK:STDOUT: %Self: %.type = bind_symbolic_name Self, 0 [symbolic = constants.%Self]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = %Self
Expand Down
1 change: 0 additions & 1 deletion toolchain/check/testdata/namespace/merging_with_indirections.carbon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@ fn Run() {
// CHECK:STDOUT: }
// CHECK:STDOUT: %Other: <namespace> = namespace file.%Other.import, [template] {
// CHECK:STDOUT: .F = %import_ref.f04
// CHECK:STDOUT: .NS1 = %NS1
// CHECK:STDOUT: import Other//b
// CHECK:STDOUT: import Other//a
// CHECK:STDOUT: }
Expand Down
Loading

0 comments on commit 6bc36b0

Please sign in to comment.