-
Notifications
You must be signed in to change notification settings - Fork 14k
Fix ICE caused by invalid spans for shrink_file #148735
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
base: main
Are you sure you want to change the base?
Conversation
|
cc @Muscraft |
| WARN rustc_errors::emitter Invalid span $SRC_DIR/std/src/macros.rs:LL:COL (#4), error=SourceNotAvailable { filename: Real(Remapped { local_path: None, virtual_name: "$SRC_DIR/std/src/macros.rs" }) } | ||
| WARN rustc_errors::emitter Invalid span $SRC_DIR/std/src/macros.rs:LL:COL (#4), error=SourceNotAvailable { filename: Real(Remapped { local_path: None, virtual_name: "$SRC_DIR/std/src/macros.rs" }) } | ||
| WARN rustc_errors::emitter Invalid span $SRC_DIR/std/src/macros.rs:LL:COL (#4), error=SourceNotAvailable { filename: Real(Remapped { local_path: None, virtual_name: "$SRC_DIR/std/src/macros.rs" }) } |
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 have worries that this will not pass aarch64-msvc-1 the same way it was in my PR
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.
ah, it do have the same issue.
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, I read the discussion on zulip, seems there is another standalone bug. so is it a temprary way we skip Windows for this testcase(and add it back after that bug is fixed)?
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.
These warnings are coming from this call to is_different. The reason this is happening with AnnotateSnippetEmitter and not HumanEmitter is because HumanEmitter checks for source code availability before it calls is_different. Changing lines 353 to 368 in annotate_snippet_emitter_writer.rs to the following fixes the issue:
let lo = subst.parts.iter().map(|part| part.span.lo()).min()?;
let lo_file = sm.lookup_source_file(lo);
let hi = subst.parts.iter().map(|part| part.span.hi()).max()?;
let hi_file = sm.lookup_source_file(hi);
// The different spans might belong to different contexts, if so ignore suggestion.
if lo_file.stable_id != hi_file.stable_id {
return None;
}
// We can't splice anything if the source is unavailable.
if !sm.ensure_source_file_source_present(&lo_file) {
return None;
}
// Account for cases where we are suggesting the same code that's already
// there. This shouldn't happen often, but in some cases for multipart
// suggestions it's much easier to handle it here than in the origin.
subst.parts.retain(|p| is_different(sm, &p.snippet, p.span));
if subst.parts.is_empty() { None } else { Some(subst) }|
@bors try jobs=aarch64-msvc-1 |
This comment has been minimized.
This comment has been minimized.
Fix ICE caused by invalid spans for shrink_file try-job: aarch64-msvc-1
This comment has been minimized.
This comment has been minimized.
|
💔 Test for eca91bd failed: CI. Failed jobs:
|
|
r=me once the comments are addressed. @rustbot author |
7262408 to
725b213
Compare
…=Muscraft Prefer to use file.stable_id over file.name from source map From rust-lang#148735 (comment) r? `@Muscraft`
Rollup merge of #148792 - chenyukang:yukang-fix-file-name, r=Muscraft Prefer to use file.stable_id over file.name from source map From #148735 (comment) r? `@Muscraft`
Fixes #148732
There are two issues in this function:
7..7is beyond the end of buffer0) will be reported after fixing the first one, is caused by spans cross file boundaries due to macro expansion. It is fixed in the second commit.r? @nnethercote
edited: also fixes #148684, added a new testcase for it in the last commit.