fix(hir): stop panicking on macro-expanded files in container accessors#311
Merged
Conversation
HirFileId::file_id() looked like an ordinary accessor but panicked for HirFileId::Macro, and that assumption leaked into every container id (ScopeId, ModuleId, BlockId, GenerateBlockId, FileOrModule, SubroutineScope) plus type_infer/scope, all of which returned a vfs FileId by calling the panicking helper. Since HIR genuinely lowers macro expansions into modules/blocks, these accessors would panic whenever a macro-expanded definition was navigated, searched, or rendered. The expanded text is not a user-facing file, so collapsing HirFileId to FileId was the wrong abstraction. The container accessors now return the HirFileId they actually live in (File or Macro), and the panicking HirFileId::file_id() alias is removed in favour of the honest expect_file() and a new source_file_id(db) that maps a macro expansion to the file containing its invocation. IDE features that need a user-facing (FileId, range) resolve macro expansions to their call site via macro_file_expansion: navigation targets, reference search scopes, definition/side-comment rendering, the semantic module index, and inlay-hint link targets. Rename and side-comment rendering skip macro-expanded origins, since their ranges live in expanded text that is not directly editable. Source-context call sites that only ever hold real files use expect_file().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
HirFileId::file_id()looked like an ordinary accessor but panicked forHirFileId::Macro, and that assumption leaked into every container id (ScopeId,ModuleId,BlockId,GenerateBlockId,FileOrModule,SubroutineScope) plustype_infer/scope, all of which returned a vfsFileIdby calling the panicking helper. Since HIR genuinely lowers macro expansions into modules and blocks (seedb.rsparse/hir_fileand the macro expansion tests), these accessors panicked whenever a macro-expanded definition was navigated, searched, or rendered.The expanded text is not a user-facing file, so collapsing
HirFileIdtoFileIdwas the wrong abstraction.Change
HIR core (
crates/hir):HirFileId::file_id()alias. Keep the honestexpect_file(); addsource_file_id(db) -> Option<FileId>which returns the file itself for real files and maps a macro expansion to the file containing its invocation viamacro_file_expansion.FileOrModule/SubroutineScope/ScopeId/BlockId/GenerateBlockIdfile_id()now return theHirFileIdthey actually live in.ModuleId::file_id()is removed (it duplicated thefile_idfield); callers use the field directly.def_id.rs/scope.rs/type_infer.rsdrop theHirFileId::File(x.file_id())/.into()round-trips and use theHirFileIddirectly.IDE boundary (
crates/ide+src/):(FileId, range)resolve macro expansions to their call site throughmacro_file_expansion: navigation targets, reference search scopes, definition/side-comment rendering, the semantic module index, and inlay-hint link targets (nav_location/resolve_source_rangehelpers).semantic_tokensparses macro-expanded blocks with theHirFileIddirectly (db.parse(block_id.file_id(db))) instead of wrapping inHirFileId::File(..), which discarded the expansion.expect_file().Verification
macro_expanded_module_keeps_macro_hir_file_id: lowers a\DECL-expanded module and assertsScopeId::Module(id).file_id(&db) == HirFileId::Macro(..)(no panic) andsource_file_id` maps back to the call-site file.cargo test -p hir(94),-p ide(137),-p vide(143) all green;cargo check --tests --workspaceclean;cargo fmtapplied.Out of scope
Precise per-token origin mapping via
ExpansionSourceMap::map_up(pointing into the\definebody or the call-site argument rather than the whole call site) is intentionally left for later — it is a refinement, not a correctness fix, and has a span-ambiguity design decision best driven by real feedback. The call-site mapping shipped here is the natural fallback for unmappable origins (TokenPaste/Stringify/Builtin`), so nothing needs to be undone when that lands.