-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: Name resolution failures inside include!
d files
#18325
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
Conversation
} | ||
} else { | ||
macro_call_id.as_file() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This id mismatch was blocking us from inserting item for usage search
if loc.item_tree_id().file_id() == file_id { |
LHS: MacroFileId != RHS: EditionFileId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, does it mean we have to make the include!()
d file a macro file instead of an EditionedFileId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about both directions; making included file into MacroFile
as you said and current way and I think that as the ide things triggered from the opened included file are anchored with nodes from EditionedFile
, this way would be more intuitive. But I'm not 100% sure about this so I'll rethink about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'm afraid that making it a macro file would require us to map between InFile<T> { file_id: Editioned, .. }
and InFile<T> { file_id: Macro, .. }
back and forth quite often and missing it in one place might cause another error. This PR seems simpler
} else { | ||
let file_id = node.file_id.file_id()?; | ||
*this.cache.included_file_cache.get(&file_id)? | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the current node is inside the include!
d file, this recursive parent search was stopped on macro_file()?
at the file's outer node.
But if this was None
it was fallbacked to the root module. So, it worked fine if the file was include!
d to root and caused the weird behaviour #18314 if it was inside a block module.
0263e98
to
2c0ebc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we basically skip expanding include
and instead just use the actual file for name resolution? I feel like this might break things
Well, this doesn't skip expanding but just changes the nameress collecting source. |
I think I know where to fix this actually, the macro token descension logic is wrong / needs to handle this |
…Veykril fix: Fix token downmapping failing for include! inputs Supercedes rust-lang/rust-analyzer#18325 Fixes rust-lang/rust-analyzer#18325 Fixes rust-lang/rust-analyzer#18313 Fixes rust-lang/rust-analyzer#18314
Fixes #18313 and fixes #18314