Add a location to indirect imports. #4098
Conversation
| auto import_ir_inst_id = context.import_ir_insts().Add( | ||
| {.ir_id = SemIR::ImportIRId::ApiForImpl, | ||
| .inst_id = api_imports->import_decl_id}); | ||
| import_decl_id = context.AddInst<SemIR::ImportDecl>( | ||
| import_ir_inst_id, {.package_id = SemIR::NameId::ForIdentifier( | ||
| api_imports_entry.first)}); |
There was a problem hiding this comment.
Have you considered making this an ImportRef naming the API's file's ImportDecl, instead of an ImportDecl that's a copy of the one from the API file? I'm not really sure which makes more sense here and not pushing for one over the other, but my first instinct was that this is naming an import in a different file so maybe ImportRef might make sense.
There was a problem hiding this comment.
I did, but right now we're only using ImportRef for entities. That is, ImportRef points at a typed entity in name lookup.
While imports from other packages can create a namespace that's in name lookup, the import declaration itself isn't in name lookup, and the current package's imports don't have any name lookup data. So it felt like this approach, closer to Namespace handling than entity handling, was a better fit.
Also, presumably it'd be an ImportRefLoaded (ImportRefUnloaded feels a little odd to me since we do load the import), and that ImportRefLoaded should be linked to a resolved ImportDecl and have type information. Building that would require defining a type for ImportDecl, and I'm not sure what that would be (maybe engaging in a more circular definition of ImportDecl as a back-reference to the Namespace?)
toolchain/sem_ir/file.cpp
Outdated
| case ImportDecl::Kind: | ||
| CARBON_FATAL() << "Not valid in an expression."; |
There was a problem hiding this comment.
We have an ExprCategory::NotExpr for this.
There was a problem hiding this comment.
Done, although after debugging #4089 I have apprehension about this: it feels like it makes it really difficult to debug issues, that we don't do more to prevent calls here.
| // The imported IR. | ||
| const File* sem_ir; | ||
| // The `import` declaration. | ||
| InstId decl_id; |
There was a problem hiding this comment.
The equivalents I would draw would be decl_id on AssociatedEntity, or on non-instruction types, Function and Generic.
toolchain/sem_ir/import_ir.h
Outdated
| }; | ||
|
|
||
| static_assert(sizeof(ImportIR) == 16, "Unexpected size"); |
There was a problem hiding this comment.
The size will be 12 for a 32-bit compilation. If it's worth checking this, maybe sizeof(ImportIR) == 8 + sizeof(void*).
There was a problem hiding this comment.
Done (though uintptr_t I think is supposed to be preferred now?).
I did feel it's worth checking because it had been 24 before the re-pack (this is why I reordered fields).
toolchain/sem_ir/inst_namer.cpp
Outdated
| if (inst.package_id.is_valid()) { | ||
| add_inst_name_id(inst.package_id, ".import"); | ||
| } else { | ||
| add_inst_name("<default>.import"); |
There was a problem hiding this comment.
| add_inst_name("<default>.import"); | |
| add_inst_name("default.import"); |
We generally don't use <...> as part of valid names (eg, after a %), only at the top level for error cases (and for builtin types).
By adding an
ImportDeclinstruction, this creates something that can be referenced throughImportIRInst. packages/no_prelude/implicit_imports_entities.carbon is getting a test of this (import_conflict and import_conflict_reverse).Also re-packs ImportIR from 24 bytes to 16 on 64-bit, since I'm touching everywhere that makes one anyways.