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.

Sign up for GitHub

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

Jump to bottom

Start adding lazy import references to name lookup. #3475

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Start adding lazy import references to name lookup. #3475

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb1a023
Start adding import cross refs.
jonmeow Dec 6, 2023
ed55099
Addressing comments
jonmeow Dec 11, 2023
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
Next Next commit
Start adding import cross refs.
  • Loading branch information
@jonmeow
jonmeow committed Dec 8, 2023
commit bb1a0232faa98e3671bb3810fab73b026f81a45f
8 changes: 6 additions & 2 deletions toolchain/base/value_store.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,9 @@ class ValueStore
auto size() const -> int { return values_.size(); }

private:
llvm::SmallVector<std::decay_t<ValueType>> values_;
// Set inline size to 0 because these will typically be too large for the
// stack, while this does make File smaller.
llvm::SmallVector<std::decay_t<ValueType>, 0> values_;
};

// Storage for StringRefs. The caller is responsible for ensuring storage is
Expand Down Expand Up @@ -207,7 +209,9 @@ class ValueStore<StringId> : public Yaml::Printable<ValueStore<StringId>> {

private:
llvm::DenseMap<llvm::StringRef, StringId> map_;
llvm::SmallVector<llvm::StringRef> values_;
// Set inline size to 0 because these will typically be too large for the
// stack, while this does make File smaller.
llvm::SmallVector<llvm::StringRef, 0> values_;
};

// A thin wrapper around a `ValueStore<StringId>` that provides a different IdT,
Expand Down
87 changes: 71 additions & 16 deletions toolchain/check/check.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,73 @@ struct UnitInfo {
};

// Add imports to the root block.
static auto AddImports(Context& context, UnitInfo& unit_info) -> void {
static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info)
-> void {
// Define the package scope, with an instruction for `package` expressions to
// reference.
auto package_scope_id = context.name_scopes().Add();
CARBON_CHECK(package_scope_id == SemIR::NameScopeId::Package);

auto package_inst = context.AddInst(SemIR::Namespace{
Parse::NodeId::Invalid,
context.GetBuiltinType(SemIR::BuiltinKind::NamespaceType),
SemIR::NameScopeId::Package});
CARBON_CHECK(package_inst == SemIR::InstId::PackageNamespace);

// Add imports from the current package.
auto self_import = unit_info.package_imports_map.find(IdentifierId::Invalid);
if (self_import != unit_info.package_imports_map.end()) {
auto& package_scope =
context.name_scopes().Get(SemIR::NameScopeId::Package);
package_scope.has_load_error = self_import->second.has_load_error;

for (const auto& import : self_import->second.imports) {
const auto& import_sem_ir = **import.unit_info->unit->sem_ir;
const auto& import_scope =
import_sem_ir.name_scopes().Get(SemIR::NameScopeId::Package);

// If an import of the current package caused an error for the imported
// file, it transitively affects the current file too.
package_scope.has_load_error |= import_scope.has_load_error;

auto ir_id = context.sem_ir().cross_ref_irs().Add(&import_sem_ir);

for (const auto& [import_name_id, import_inst_id] : import_scope.names) {
// Translate the name to the current IR.
auto name_id = SemIR::NameId::Invalid;
if (auto import_identifier_id = import_name_id.AsIdentifierId();
import_identifier_id.is_valid()) {
auto name = import_sem_ir.identifiers().Get(import_identifier_id);
name_id =
SemIR::NameId::ForIdentifier(context.identifiers().Add(name));
} else {
// A builtin name ID which is equivalent cross-IR.
name_id = import_name_id;
}

// Leave a placeholder that the inst comes from the other IR.
auto target_id = context.AddInst(
SemIR::LazyImportRef{.ir_id = ir_id, .inst_id = import_inst_id});
// TODO: The scope's names should be changed to allow for ambiguous
// names.
package_scope.names.insert({name_id, target_id});
}
}

// Push the scope.
context.PushScope(package_inst, SemIR::NameScopeId::Package,
package_scope.has_load_error);
} else {
// Push the scope; there are no names to add.
context.PushScope(package_inst, SemIR::NameScopeId::Package);
}

for (auto& [package_id, package_imports] : unit_info.package_imports_map) {
if (!package_id.is_valid()) {
// Current package is handled above.
continue;
}

llvm::SmallVector<const SemIR::File*> sem_irs;
for (auto import : package_imports.imports) {
sem_irs.push_back(&**import.unit_info->unit->sem_ir);
Expand Down Expand Up @@ -105,8 +170,6 @@ static auto ProcessParseNodes(Context& context,
}

// Produces and checks the IR for the provided Parse::Tree.
// TODO: Both valid and invalid imports should be recorded on the SemIR. Invalid
// imports should suppress errors where it makes sense.
static auto CheckParseTree(const SemIR::File& builtin_ir, UnitInfo& unit_info,
llvm::raw_ostream* vlog_stream) -> void {
unit_info.unit->sem_ir->emplace(
Expand All @@ -124,17 +187,7 @@ static auto CheckParseTree(const SemIR::File& builtin_ir, UnitInfo& unit_info,
// Add a block for the file.
context.inst_block_stack().Push();

// Define the package scope, with an instruction for `package` expressions to
// reference.
auto package_scope = context.name_scopes().Add();
auto package_inst = context.AddInst(SemIR::Namespace{
Parse::NodeId::Invalid,
context.GetBuiltinType(SemIR::BuiltinKind::NamespaceType),
package_scope});
CARBON_CHECK(package_inst == SemIR::InstId::PackageNamespace);
context.PushScope(SemIR::InstId::Invalid, package_scope);

AddImports(context, unit_info);
InitPackageScopeAndImports(context, unit_info);

if (!ProcessParseNodes(context, unit_info.err_tracker)) {
context.sem_ir().set_has_errors(true);
Expand Down Expand Up @@ -338,7 +391,7 @@ static auto BuildApiMapAndDiagnosePackaging(
packaging && packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl;

// Add to the `api` map and diagnose duplicates. This occurs before the
// file extension check because we might emit both diagnostics in situation
// file extension check because we might emit both diagnostics in situations
// where the user forgets (or has syntax errors with) a package line
// multiple times.
if (!is_impl) {
Expand Down Expand Up @@ -418,7 +471,9 @@ auto CheckParseTrees(const SemIR::File& builtin_ir,
unit_info.unit->parse_tree->packaging_directive()) {
if (packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl) {
// An `impl` has an implicit import of its `api`.
TrackImport(api_map, nullptr, unit_info, packaging->names);
auto implicit_names = packaging->names;
implicit_names.package_id = IdentifierId::Invalid;
TrackImport(api_map, nullptr, unit_info, implicit_names);
}
}

Expand Down
123 changes: 89 additions & 34 deletions toolchain/check/context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,20 +137,17 @@ auto Context::AddPackageImports(Parse::NodeId import_node,
.type_id = type_id,
.first_cross_ref_ir_id = first_id,
.last_cross_ref_ir_id = last_id});
if (name_id.is_valid()) {
// Add the import to lookup. Should always succeed because imports will be
// uniquely named.
AddNameToLookup(import_node, name_id, inst_id);
// Add a name for formatted output. This isn't used in name lookup in order
// to reduce indirection, but it's separate from the Import because it
// otherwise fits in an Inst.
AddInst(SemIR::BindName{.parse_node = import_node,
.type_id = type_id,
.name_id = name_id,
.value_id = inst_id});
} else {
// TODO: All names from the current package should be added.
}

// Add the import to lookup. Should always succeed because imports will be
// uniquely named.
AddNameToLookup(import_node, name_id, inst_id);
// Add a name for formatted output. This isn't used in name lookup in order
// to reduce indirection, but it's separate from the Import because it
// otherwise fits in an Inst.
AddInst(SemIR::BindName{.parse_node = import_node,
.type_id = type_id,
.name_id = name_id,
.value_id = inst_id});
}

auto Context::AddNameToLookup(Parse::NodeId name_node, SemIR::NameId name_id,
Expand All @@ -169,6 +166,45 @@ auto Context::AddNameToLookup(Parse::NodeId name_node, SemIR::NameId name_id,
}
}

auto Context::ResolveIfLazyImportRef(SemIR::InstId inst_id) -> void {
auto inst = insts().Get(inst_id);
auto lazy_inst = inst.TryAs<SemIR::LazyImportRef>();
if (!lazy_inst) {
return;
}
const SemIR::File& import_ir = *cross_ref_irs().Get(lazy_inst->ir_id);
auto import_inst = import_ir.insts().Get(lazy_inst->inst_id);
switch (import_inst.kind()) {
case SemIR::InstKind::FunctionDecl: {
// TODO: Fill this in better.
auto function_id =
functions().Add({.name_id = SemIR::NameId::Invalid,
.decl_id = inst_id,
.implicit_param_refs_id = SemIR::InstBlockId::Empty,
.param_refs_id = SemIR::InstBlockId::Empty,
.return_type_id = SemIR::TypeId::Invalid,
.return_slot_id = SemIR::InstId::Invalid});
insts().Set(inst_id, SemIR::FunctionDecl{
Parse::NodeId::Invalid,
GetBuiltinType(SemIR::BuiltinKind::FunctionType),
function_id});
break;
}

default:
// TODO: We need more type support. For now we inject an arbitrary
// invalid node that's unrelated to the underlying value. The TODO
// diagnostic is used since this section shouldn't typically be able to
// error.
TODO(Parse::NodeId::Invalid,
(llvm::Twine("TODO: support ") + import_inst.kind().name()).str());
insts().Set(inst_id, SemIR::VarStorage{Parse::NodeId::Invalid,
SemIR::TypeId::Error,
SemIR::NameId::PackageNamespace});
break;
}
}

auto Context::LookupNameInDecl(Parse::NodeId parse_node, SemIR::NameId name_id,
SemIR::NameScopeId scope_id) -> SemIR::InstId {
if (scope_id == SemIR::NameScopeId::Invalid) {
Expand Down Expand Up @@ -201,6 +237,7 @@ auto Context::LookupNameInDecl(Parse::NodeId parse_node, SemIR::NameId name_id,
<< "Should have been erased: " << names().GetFormatted(name_id);
auto result = name_it->second.back();
if (result.scope_index == current_scope_index()) {
ResolveIfLazyImportRef(result.inst_id);
return result.inst_id;
}
}
Expand Down Expand Up @@ -233,23 +270,29 @@ auto Context::LookupUnqualifiedName(Parse::NodeId parse_node,
// it shadows all further non-lexical results and we're done.
if (!lexical_results.empty() &&
lexical_results.back().scope_index > index) {
return lexical_results.back().inst_id;
auto inst_id = lexical_results.back().inst_id;
ResolveIfLazyImportRef(inst_id);
return inst_id;
}

auto non_lexical_result =
LookupQualifiedName(parse_node, name_id, name_scope_id,
/*required=*/false);
if (non_lexical_result.is_valid()) {
if (auto non_lexical_result =
LookupQualifiedName(parse_node, name_id, name_scope_id,
/*required=*/false);
non_lexical_result.is_valid()) {
return non_lexical_result;
}
}

if (!lexical_results.empty()) {
return lexical_results.back().inst_id;
auto inst_id = lexical_results.back().inst_id;
ResolveIfLazyImportRef(inst_id);
return inst_id;
}

// We didn't find anything at all.
DiagnoseNameNotFound(parse_node, name_id);
if (!name_lookup_has_load_error_) {
DiagnoseNameNotFound(parse_node, name_id);
}
return SemIR::InstId::BuiltinError;
}

Expand All @@ -259,28 +302,36 @@ auto Context::LookupQualifiedName(Parse::NodeId parse_node,
-> SemIR::InstId {
CARBON_CHECK(scope_id.is_valid()) << "No scope to perform lookup into";
const auto& scope = name_scopes().Get(scope_id);
auto it = scope.find(name_id);
if (it == scope.end()) {
// TODO: Also perform lookups into `extend`ed scopes.
if (required) {
DiagnoseNameNotFound(parse_node, name_id);
return SemIR::InstId::BuiltinError;
}
return SemIR::InstId::Invalid;
if (auto it = scope.names.find(name_id); it != scope.names.end()) {
ResolveIfLazyImportRef(it->second);
return it->second;
}

return it->second;
// TODO: Also perform lookups into `extend`ed scopes.

if (!required) {
return SemIR::InstId::Invalid;
}
if (!scope.has_load_error) {
DiagnoseNameNotFound(parse_node, name_id);
}
return SemIR::InstId::BuiltinError;
}

auto Context::PushScope(SemIR::InstId scope_inst_id,
SemIR::NameScopeId scope_id) -> void {
scope_stack_.push_back({.index = next_scope_index_,
.scope_inst_id = scope_inst_id,
.scope_id = scope_id});
SemIR::NameScopeId scope_id,
bool name_lookup_has_load_error) -> void {
scope_stack_.push_back(
{.index = next_scope_index_,
.scope_inst_id = scope_inst_id,
.scope_id = scope_id,
.prev_name_lookup_has_load_error = name_lookup_has_load_error_});
if (scope_id.is_valid()) {
non_lexical_scope_stack_.push_back({next_scope_index_, scope_id});
}

name_lookup_has_load_error_ |= name_lookup_has_load_error;

// TODO: Handle this case more gracefully.
CARBON_CHECK(next_scope_index_.index != std::numeric_limits<int32_t>::max())
<< "Ran out of scopes";
Expand All @@ -289,6 +340,9 @@ auto Context::PushScope(SemIR::InstId scope_inst_id,

auto Context::PopScope() -> void {
auto scope = scope_stack_.pop_back_val();

name_lookup_has_load_error_ = scope.prev_name_lookup_has_load_error;

for (const auto& str_id : scope.names) {
auto it = name_lookup_.find(str_id);
CARBON_CHECK(it->second.back().scope_index == scope.index)
Expand Down Expand Up @@ -848,6 +902,7 @@ class TypeCompleter {
case SemIR::InitializeFrom::Kind:
case SemIR::InterfaceDecl::Kind:
case SemIR::IntLiteral::Kind:
case SemIR::LazyImportRef::Kind:
case SemIR::NameRef::Kind:
case SemIR::Namespace::Kind:
case SemIR::NoOp::Kind:
Expand Down
20 changes: 17 additions & 3 deletions toolchain/check/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,13 @@ class Context {
auto NoteIncompleteClass(SemIR::ClassId class_id, DiagnosticBuilder& builder)
-> void;

// Pushes a new scope onto scope_stack_.
// Pushes a scope onto scope_stack_. NameScopeId::Invalid is used for new
// scopes. name_lookup_has_load_error is used to limit diagnostics when a
// given namespace may contain a mix of both successful and failed name
// imports.
auto PushScope(SemIR::InstId scope_inst_id = SemIR::InstId::Invalid,
SemIR::NameScopeId scope_id = SemIR::NameScopeId::Invalid)
-> void;
SemIR::NameScopeId scope_id = SemIR::NameScopeId::Invalid,
bool name_lookup_has_load_error = false) -> void;

// Pops the top scope from scope_stack_, cleaning up names from name_lookup_.
auto PopScope() -> void;
Expand Down Expand Up @@ -387,6 +390,9 @@ class Context {
// The name scope associated with this entry, if any.
SemIR::NameScopeId scope_id;

// The previous state of name_lookup_has_load_error_, restored on pop.
bool prev_name_lookup_has_load_error;
Comment on lines +393 to +394
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use a backup / restore approach here, rather than always looking at the state on the top of the scope stack when querying name_lookup_has_load_error_? (I'm not opposed to this if there's a good reason, but I think maintaining this flag alongside other current-scope information like the names set seems simpler if either would work.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that'd work:

import library "broken";

class C {
  fn Foo() {
    // Top of the scope stack has no load error, but this does name lookup into the package.
    var a: i32 = y;
  }

  // This pops the scope to `C`, `C`'s name scope does not have a load error, but
  // this still shouldn't print because it's unqualified name lookup.
  var b: i32 = y;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea would be that when you push a new scope, its "name lookup has load error" flag is set to that of the enclosing scope.


// Names which are registered with name_lookup_, and will need to be
// unregistered when the scope ends.
llvm::DenseSet<SemIR::NameId> names;
Expand Down Expand Up @@ -427,6 +433,10 @@ class Context {
// instruction to the current block.
auto CanonicalizeTypeAndAddInstIfNew(SemIR::Inst inst) -> SemIR::TypeId;

// If the passed in instruction ID is a LazyImportRef, resolves it for use.
// Called when name lookup intends to return an inst_id.
auto ResolveIfLazyImportRef(SemIR::InstId inst_id) -> void;

auto current_scope() -> ScopeStackEntry& { return scope_stack_.back(); }
auto current_scope() const -> const ScopeStackEntry& {
return scope_stack_.back();
Expand Down Expand Up @@ -499,6 +509,10 @@ class Context {
llvm::DenseMap<SemIR::NameId, llvm::SmallVector<LexicalLookupResult>>
name_lookup_;

// Whether name_lookup_ has load errors, updated whenever scope_stack_ is
// pushed or popped.
bool name_lookup_has_load_error_ = false;

// Cache of the mapping from instructions to types, to avoid recomputing the
// folding set ID.
llvm::DenseMap<SemIR::InstId, SemIR::TypeId> canonical_types_;
Expand Down
3 changes: 2 additions & 1 deletion toolchain/check/decl_name_stack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ auto DeclNameStack::UpdateScopeIfNeeded(NameContext& name_context) -> void {
auto scope_id = resolved_inst.As<SemIR::Namespace>().name_scope_id;
name_context.state = NameContext::State::Resolved;
name_context.target_scope_id = scope_id;
context_->PushScope(name_context.resolved_inst_id, scope_id);
context_->PushScope(name_context.resolved_inst_id, scope_id,
context_->name_scopes().Get(scope_id).has_load_error);
break;
}
default:
Expand Down
Loading