-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix the start/end byte positions in the compiler JSON output #65074
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
I added the This got a bit more confusing when implementing (I also managed to sneak in a typo with my last refactoring that fails the build, I'll be rebasing this |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
994bda0
to
d0dc2a8
Compare
Fixed couple of naming issues and one spaces-within-parens violation. I think those were all of them. As far as I'm concerned, I think the code could be good to go. The compiler might disagree, my test run is still in progress - wanted to get the style issues pushed early so they wouldn't distract too much in the review. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
r? @matklad |
I'll also try to remember run the libsyntax_pos unit tests this time. Unfortunately UDP tests are failing on my machine so running the full test suite is not possible for me. :| |
d0dc2a8
to
c80d54b
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
c80d54b
to
e492ca8
Compare
Also found a way to run the tidy tests. All the relevant tests passed on my machine, so I think this is good to go now from my end as long as I didn't screw things over in rebase and it's okay to leave the |
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.
LGTM, just a couple of nits left
@bors r+ Thanks! |
📌 Commit bbf262d has been approved by |
Fix the start/end byte positions in the compiler JSON output Track the changes made during normalization in the `SourceFile` and use this information to correct the `start_byte` and `end_byte` fields in the JSON output. This should ensure the start/end byte fields can be used to index the original file, even if Rust normalized the source code for parsing purposes. Both CRLF to LF and BOM removal are handled with this one. The rough plan was discussed with @matklad in rust-lang/rustfix#176 - although I ended up going with `u32` offset tracking so I wouldn't need to deal with `u32 + i32` arithmetics when applying the offset to the span byte positions. Fixes rust-lang#65029
Fix the start/end byte positions in the compiler JSON output Track the changes made during normalization in the `SourceFile` and use this information to correct the `start_byte` and `end_byte` fields in the JSON output. This should ensure the start/end byte fields can be used to index the original file, even if Rust normalized the source code for parsing purposes. Both CRLF to LF and BOM removal are handled with this one. The rough plan was discussed with @matklad in rust-lang/rustfix#176 - although I ended up going with `u32` offset tracking so I wouldn't need to deal with `u32 + i32` arithmetics when applying the offset to the span byte positions. Fixes rust-lang#65029
Failed in #65199 (comment), @bors r- |
…, r=petrochenkov" This reverts commit ef1ecbe, reversing changes made to fc8765d. That changed unfortunately broke rustfix on windows: rust-lang/rustfix#176 Specifically, what ef1ecbe did was to enforce normalization of \r\n to \n at file loading time, similarly to how we deal with Byte Order Mark. Normalization changes raw offsets in files, which are exposed via `--error-format=json`, and used by rusfix. The proper solution here (which also handles the latent case with BOM) is rust-lang#65074 However, since it's somewhat involved, and we are time sensitive, we prefer to revert the original change on beta.
Submitted #65273 which should relieve time pressure here, and increase our confidence that the backport is correct. |
Ping from Triage: Hi @Rantanen, any updates? |
89318cf
to
ff1860a
Compare
Oh, sorry. I was under the impression this was good to go and people were just busy with other stuff that the review was taking a while.
I squashed the "Fix implementation" commits into the "Fix the start/end byte positions in the compiler JSON output", but still left the logically separate "Add unit tests" commits alone. Didn't know if the wish was to squash them all into one. I've now done so. Do let me know if there's still something missing. |
📌 Commit ff1860a has been approved by |
⌛ Testing commit ff1860a with merge 93193b822279b7c5a943dc56de5e4296934a9ea0... |
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-azure |
I'm assuming this is a transient error. If it isn't and I need to do something about it, I'll need a bit more information than that. :) |
hm, indeed seems spurious @bors retry |
Fix the start/end byte positions in the compiler JSON output Track the changes made during normalization in the `SourceFile` and use this information to correct the `start_byte` and `end_byte` fields in the JSON output. This should ensure the start/end byte fields can be used to index the original file, even if Rust normalized the source code for parsing purposes. Both CRLF to LF and BOM removal are handled with this one. The rough plan was discussed with @matklad in rust-lang/rustfix#176 - although I ended up going with `u32` offset tracking so I wouldn't need to deal with `u32 + i32` arithmetics when applying the offset to the span byte positions. Fixes rust-lang#65029
Fix the start/end byte positions in the compiler JSON output Track the changes made during normalization in the `SourceFile` and use this information to correct the `start_byte` and `end_byte` fields in the JSON output. This should ensure the start/end byte fields can be used to index the original file, even if Rust normalized the source code for parsing purposes. Both CRLF to LF and BOM removal are handled with this one. The rough plan was discussed with @matklad in rust-lang/rustfix#176 - although I ended up going with `u32` offset tracking so I wouldn't need to deal with `u32 + i32` arithmetics when applying the offset to the span byte positions. Fixes rust-lang#65029
Rollup of 9 pull requests Successful merges: - #64639 (Stabilize `#[non_exhaustive]` (RFC 2008)) - #65074 (Fix the start/end byte positions in the compiler JSON output) - #65315 (Intern place projection) - #65685 (Fix check of `statx` and handle EPERM) - #65731 (Prevent unnecessary allocation in PathBuf::set_extension.) - #65740 (Fix default "disable-shortcuts" feature value) - #65787 (move panictry! to where it is used.) - #65789 (move Attribute::with_desugared_doc to librustdoc) - #65790 (move report_invalid_macro_expansion_item to item.rs) Failed merges: r? @ghost
…, r=petrochenkov" This reverts commit ef1ecbe, reversing changes made to fc8765d. That changed unfortunately broke rustfix on windows: rust-lang/rustfix#176 Specifically, what ef1ecbe did was to enforce normalization of \r\n to \n at file loading time, similarly to how we deal with Byte Order Mark. Normalization changes raw offsets in files, which are exposed via `--error-format=json`, and used by rusfix. The proper solution here (which also handles the latent case with BOM) is rust-lang#65074 However, since it's somewhat involved, and we are time sensitive, we prefer to revert the original change on beta.
Track the changes made during normalization in the
SourceFile
and use this information to correct thestart_byte
andend_byte
fields in the JSON output.This should ensure the start/end byte fields can be used to index the original file, even if Rust normalized the source code for parsing purposes. Both CRLF to LF and BOM removal are handled with this one.
The rough plan was discussed with @matklad in rust-lang/rustfix#176 - although I ended up going with
u32
offset tracking so I wouldn't need to deal withu32 + i32
arithmetics when applying the offset to the span byte positions.Fixes #65029