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

[NFC] Convert NameScope from struct to class #4623

Merged
merged 22 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e49844e
[NFC] Convert NameScope from struct to class
bricknerb Dec 3, 2024
64dbb92
Style fixes:
bricknerb Dec 4, 2024
a7df529
Move NameScope::AddRequired() and NameScope::LookupOrAdd() to a cpp f…
bricknerb Dec 4, 2024
841ef2b
Add and update comments in NameScope API.
bricknerb Dec 4, 2024
e87e7c9
NameScope unit tests
bricknerb Dec 4, 2024
d8c6954
Change AddNamespace() to be more similar to the code that used NameSc…
bricknerb Dec 4, 2024
ac9b3f7
Simplify NameScope::LookupOrAdd() by passing InstId instead of a func…
bricknerb Dec 4, 2024
a9c5543
Use structured binding for the return value of NameScope::LookupOrAdd()
bricknerb Dec 4, 2024
f62a938
Deduplicate calling NameScope::GetEntry() in AddImportRefOrMerge()
bricknerb Dec 4, 2024
35592db
Use data members directly in NameScope::Print()
bricknerb Dec 5, 2024
0fa5621
Use preincrmenet in name_scope_test.cpp instead of postincrement.
bricknerb Dec 5, 2024
f9e9509
Use full qualification for using statements in name_scope_test.cpp
bricknerb Dec 5, 2024
77b9466
Define and use a dedicate NameScope::Entry gmock matcher
bricknerb Dec 5, 2024
ca252bd
Keep EntryId as the index instead of int
bricknerb Dec 5, 2024
6fd0dd1
Move NameScope::Print() implementation to cpp file
bricknerb Dec 5, 2024
2a1b872
Move NameScope comments from trivial accessors to data members
bricknerb Dec 5, 2024
98a2c5e
Bring back the logic to call TryToDefineType() regardless of whether …
bricknerb Dec 5, 2024
cea32c4
Use assignment operator for initialization
bricknerb Dec 5, 2024
a467335
Merge from trunk
jonmeow Dec 5, 2024
ab79b57
Merge branch 'trunk' into scope
jonmeow Dec 6, 2024
7e61dfe
Update IdBase use
jonmeow Dec 6, 2024
e16e7da
Merge trunk
jonmeow Dec 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions toolchain/check/check.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ static auto ImportCurrentPackage(Context& context, UnitInfo& unit_info,
unit_info.package_imports[import_map_lookup.value()];

if (self_import.has_load_error) {
context.name_scopes().Get(SemIR::NameScopeId::Package).has_error = true;
context.name_scopes().Get(SemIR::NameScopeId::Package).set_has_error();
}

ImportLibrariesFromCurrentPackage(
Expand All @@ -203,7 +203,7 @@ static auto ImportCurrentPackage(Context& context, UnitInfo& unit_info,

context.scope_stack().Push(
package_inst_id, SemIR::NameScopeId::Package, SemIR::SpecificId::Invalid,
context.name_scopes().Get(SemIR::NameScopeId::Package).has_error);
context.name_scopes().Get(SemIR::NameScopeId::Package).has_error());
}

// Imports all other packages (excluding the current package).
Expand Down
12 changes: 6 additions & 6 deletions toolchain/check/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -361,16 +361,16 @@ auto Context::LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id,
SemIR::NameScopeId scope_id,
const SemIR::NameScope& scope)
-> std::pair<SemIR::InstId, SemIR::AccessKind> {
if (auto lookup = scope.name_map.Lookup(name_id)) {
auto entry = scope.names[lookup.value()];
if (auto entry_id = scope.Lookup(name_id)) {
auto entry = scope.GetEntry(*entry_id);
LoadImportRef(*this, entry.inst_id);
return {entry.inst_id, entry.access_kind};
}

if (!scope.import_ir_scopes.empty()) {
if (!scope.import_ir_scopes().empty()) {
// TODO: Enforce other access modifiers for imports.
return {ImportNameFromOtherPackage(*this, loc, scope_id,
scope.import_ir_scopes, name_id),
scope.import_ir_scopes(), name_id),
SemIR::AccessKind::Public};
}
return {SemIR::InstId::Invalid, SemIR::AccessKind::Public};
Expand Down Expand Up @@ -527,7 +527,7 @@ auto Context::LookupQualifiedName(SemIRLoc loc, SemIR::NameId name_id,
continue;
}
const auto& name_scope = name_scopes().Get(scope_id);
has_error |= name_scope.has_error;
has_error |= name_scope.has_error();

auto [scope_result_id, access_kind] =
LookupNameInExactScope(loc, name_id, scope_id, name_scope);
Expand All @@ -548,7 +548,7 @@ auto Context::LookupQualifiedName(SemIRLoc loc, SemIR::NameId name_id,
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;
const auto& extended = name_scope.extended_scopes();
scopes.reserve(scopes.size() + extended.size());
for (auto extended_id : llvm::reverse(extended)) {
// Substitute into the constant describing the extended scope to
Expand Down
10 changes: 5 additions & 5 deletions toolchain/check/decl_name_stack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ auto DeclNameStack::AddName(NameContext name_context, SemIR::InstId target_id,
auto& name_scope =
context_->name_scopes().Get(name_context.parent_scope_id);
if (name_context.has_qualifiers) {
auto inst = context_->insts().Get(name_scope.inst_id);
auto inst = context_->insts().Get(name_scope.inst_id());
if (!inst.Is<SemIR::Namespace>()) {
// TODO: Point at the declaration for the scoped entity.
CARBON_DIAGNOSTIC(
Expand Down Expand Up @@ -231,7 +231,7 @@ auto DeclNameStack::ApplyNameQualifier(const NameComponent& name) -> void {
if (scope_id.is_valid()) {
PushNameQualifierScope(*context_, name_context.resolved_inst_id, scope_id,
specific_id,
context_->name_scopes().Get(scope_id).has_error);
context_->name_scopes().Get(scope_id).has_error());
name_context.parent_scope_id = scope_id;
} else {
name_context.state = NameContext::State::Error;
Expand Down Expand Up @@ -416,12 +416,12 @@ auto DeclNameStack::ResolveAsScope(const NameContext& name_context,
SemIR::InstBlockId::Invalid))) {
return InvalidResult;
}
if (scope.is_closed_import) {
if (scope.is_closed_import()) {
DiagnoseQualifiedDeclInImportedPackage(*context_, name_context.loc_id,
scope.inst_id);
scope.inst_id());
// Only error once per package. Recover by allowing this package name to
// be used as a name qualifier.
scope.is_closed_import = false;
scope.set_is_closed_import(false);
}
return {scope_id, SemIR::SpecificId::Invalid};
}
Expand Down
6 changes: 3 additions & 3 deletions toolchain/check/handle_class.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ auto HandleParseNode(Context& context, Parse::AdaptDeclId node_id) -> bool {
// Extend the class scope with the adapted type's scope if requested.
if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
auto& class_scope = context.name_scopes().Get(class_info.scope_id);
class_scope.extended_scopes.push_back(adapted_inst_id);
class_scope.AddExtendedScope(adapted_inst_id);
}
return true;
}
Expand Down Expand Up @@ -554,9 +554,9 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {
if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
auto& class_scope = context.name_scopes().Get(class_info.scope_id);
if (base_info.scope_id.is_valid()) {
class_scope.extended_scopes.push_back(base_info.inst_id);
class_scope.AddExtendedScope(base_info.inst_id);
} else {
class_scope.has_error = true;
class_scope.set_has_error();
}
}
return true;
Expand Down
4 changes: 2 additions & 2 deletions toolchain/check/handle_export.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ auto HandleParseNode(Context& context, Parse::ExportDeclId node_id) -> bool {
// diagnostic and so that cross-package imports can find it easily.
auto entity_name = context.entity_names().Get(import_ref->entity_name_id);
auto& parent_scope = context.name_scopes().Get(entity_name.parent_scope_id);
auto lookup = parent_scope.name_map.Lookup(entity_name.name_id);
auto& scope_inst_id = parent_scope.names[lookup.value()].inst_id;
auto& scope_inst_id =
parent_scope.GetEntry(*parent_scope.Lookup(entity_name.name_id)).inst_id;
CARBON_CHECK(scope_inst_id == inst_id);
scope_inst_id = export_id;

Expand Down
31 changes: 17 additions & 14 deletions toolchain/check/handle_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ static auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
return std::nullopt;
}
auto& scope = context.name_scopes().Get(scope_id);
if (!scope.inst_id.is_valid()) {
if (!scope.inst_id().is_valid()) {
return std::nullopt;
}
return context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id);
return context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id());
}

static auto GetDefaultSelfType(Context& context) -> SemIR::TypeId {
Expand Down Expand Up @@ -145,7 +145,7 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
CARBON_DIAGNOSTIC(ExtendImplForall, Error,
"cannot `extend` a parameterized `impl`");
context.emitter().Emit(extend_node, ExtendImplForall);
parent_scope.has_error = true;
parent_scope.set_has_error();
return;
}

Expand All @@ -158,7 +158,7 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
// If the explicit self type is not the default, just bail out.
if (self_type_id != GetDefaultSelfType(context)) {
diag.Emit();
parent_scope.has_error = true;
parent_scope.set_has_error();
return;
}

Expand All @@ -176,18 +176,21 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,

if (!context.types().Is<SemIR::FacetType>(constraint_id)) {
context.TODO(node_id, "extending non-facet-type constraint");
parent_scope.has_error = true;
parent_scope.set_has_error();
return;
}
parent_scope.has_error |= !context.TryToDefineType(constraint_id, [&] {
CARBON_DIAGNOSTIC(ExtendUndefinedInterface, Error,
"`extend impl` requires a definition for facet type {0}",
InstIdAsType);
return context.emitter().Build(node_id, ExtendUndefinedInterface,
constraint_inst_id);
});

parent_scope.extended_scopes.push_back(constraint_inst_id);
if (!parent_scope.has_error() && !context.TryToDefineType(constraint_id, [&] {
bricknerb marked this conversation as resolved.
Show resolved Hide resolved
CARBON_DIAGNOSTIC(
ExtendUndefinedInterface, Error,
"`extend impl` requires a definition for facet type {0}",
InstIdAsType);
return context.emitter().Build(node_id, ExtendUndefinedInterface,
constraint_inst_id);
})) {
parent_scope.set_has_error();
};

parent_scope.AddExtendedScope(constraint_inst_id);
}

// Pops the parameters of an `impl`, forming a `NameComponent` with no
Expand Down
2 changes: 1 addition & 1 deletion toolchain/check/handle_let_and_var.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ static auto BuildAssociatedConstantDecl(Context& context,
"single `:!` binding");
context.emitter().Emit(pattern.loc_id,
ExpectedSymbolicBindingInAssociatedConstant);
context.name_scopes().Get(interface_info.scope_id).has_error = true;
context.name_scopes().Get(interface_info.scope_id).set_has_error();
return;
}

Expand Down
9 changes: 6 additions & 3 deletions toolchain/check/handle_namespace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,17 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool {
// Point at the other namespace.
namespace_inst.name_scope_id = existing->name_scope_id;

if (context.name_scopes().Get(existing->name_scope_id).is_closed_import) {
if (context.name_scopes()
.Get(existing->name_scope_id)
.is_closed_import()) {
// The existing name is a package name, so this is a name conflict.
context.DiagnoseDuplicateName(namespace_id, existing_inst_id);

// Treat this as a local namespace name from now on to avoid further
// diagnostics.
context.name_scopes().Get(existing->name_scope_id).is_closed_import =
false;
context.name_scopes()
.Get(existing->name_scope_id)
.set_is_closed_import(false);
} else if (existing->import_id.is_valid() &&
!context.insts().GetLocId(existing_inst_id).is_valid()) {
// When the name conflict is an imported namespace, fill the location ID
Expand Down
Loading