fix: Fix a bug when synthetic AST node were searched in the AST ID map and caused panics #18555
Conversation
rustbot
added
the
S-waiting-on-review
| // Make up an invalid file id, so that if we will try to actually access it salsa will panic. | ||
| file_id: file_id.into(), |
There was a problem hiding this comment.
Not a fan of this i have to say, I'd rather we pass the original file id even if it shouldn't ever be used. If we do use it and trigger a panic it will end up confusing devs that don't know of this part
There was a problem hiding this comment.
If we use the original file we can also trigger panics, only they will be less consistent and harder to debug. That was my motivation for doing it, although I agree it's not great.
Also note that the file ID is private to LowerCtx, so it should be easier to protect.
| @@ -1973,10 +1974,16 @@ impl SemanticsScope<'_> { | |||
| /// Resolve a path as-if it was written at the given scope. This is | |||
| /// necessary a heuristic, as it doesn't take hygiene into account. | |||
| pub fn speculative_resolve(&self, ast_path: &ast::Path) -> Option<PathResolution> { | |||
There was a problem hiding this comment.
Not a blocker for this, but I wonder if this is a good API here. It feels brittle passing an ast node that isn't even from an actual file/expansion and then lowering it.
Fixes #17321. Fixes #15634.
I don't think this is the same cause as #18387 (I'll try to work on it, no promises), but just in case: CC @compiler-errors.