-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Normalize source when loading external foreign source into SourceMap #71048
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
src/librustc_span/lib.rs
Outdated
let src = get_src().map(|mut src| { | ||
normalize_src(&mut src, BytePos::from_usize(0)); | ||
src | ||
}); |
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.
Can you move this inside if self.src_hash.matches(&src) {
? IIRC src_hash
is computed pre-normalization, so we should also check it here in the same way.
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.
Good catch, it's pretty subtle. We really do need a test for 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.
There are some tests that print snippets from other crates, I think you might be able to do a search like rg auxiliary/ src/test/**.stderr
to find some examples.
You can then combine that with an unnormalized file, maybe we have some normalization tests?
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.
That is, I think we can use an error message to print a snippet and trigger this through rustc
instead of rustdoc
.
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.
Oops, just saw your suggestion about how to do the test. I added one as a SourceMap test. Let me know if you'd prefer a integration-style test like you suggested, or if this is sufficient.
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.
run-make
tests are a PITA, and your test is pretty clear, so no worries.
cc @GuillaumeGomez It might be good to add a test, but I'm not sure it's possible right now. |
The compiler normalizes source when reading files initially (removes BOMs, etc), but not when loading external sources. Fixes rust-lang#70874 by normalizing when loading external sources too. Adds a test to verify normalization.
32d2bc3
to
f41aa16
Compare
@bors r+ Thanks! |
📌 Commit f41aa16 has been approved by |
Rollup of 6 pull requests Successful merges: - rust-lang#71029 (Partial work on building with Cargo) - rust-lang#71034 (Clean up E0515 explanation) - rust-lang#71041 (Update links of `rustc guide`) - rust-lang#71048 (Normalize source when loading external foreign source into SourceMap) - rust-lang#71053 (Add some basic docs to `sym` and `kw` modules) - rust-lang#71057 (Clean up E0516 explanation) Failed merges: r? @ghost
The compiler normalizes source when reading files initially (removes BOMs, etc), but not when loading external sources.
This leads to the external source matching according to the
src_hash
, but differing internally because it was not normalized.Fixes #70874.