Skip to content
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

rustc_span: improve bounds checks in byte_pos_to_line_and_col #78423

Merged
merged 2 commits into from
Oct 30, 2020

Conversation

tgnottingham
Copy link
Contributor

The effect of this change is to consider edge-case spans that start or
end at the position one past the end of a file to be valid during span
hashing and encoding. This change means that these spans will be
preserved across incremental compilation sessions when they are part of
a serialized query result, instead of causing the dummy span to be used.

The effect of this change is to consider edge-case spans that start or
end at the position one past the end of a file to be valid during span
hashing and encoding. This change means that these spans will be
preserved across incremental compilation sessions when they are part of
a serialized query result, instead of causing the dummy span to be used.
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2020
@tgnottingham
Copy link
Contributor Author

The primary change is to go from an end-exclusive to an end-inclusive range check (by using SourceFile::contains). The line_contains part isn't a functional change, but just an anchor for a comment that points out edge-case subtleties.

To demonstrate the issue:

fn main() {
    println!("hello");
}
 ^ We generate spans that end here.

If the } is the last character in the file (which implies the file doesn't end in a newline), the above byte position is one past the end of the file. AFAIK, we consider such spans to be valid during most of compilation. But when it comes to hashing and serializing them, they get rejected. During hashing, we hash them as an invalid span, and during serialization, we encode them as a dummy span. Both actions cause us to lose information across incremental compilation sessions. This change rectifies that.

Honestly, I'm unsure if this fixes any bugs or improves incremental reuse in edge-cases, but I noticed the issue, and it seems like the correct thing to do. Apologies for the lack of hard data, but I thought I should put this out there while it is fresh in my mind.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 27, 2020

It would indeed be hard to write a test for this, as we'd have to commit test files without trailing newlines.

One thing I noticed while reviewing your change is that the CacheEntry and SourceFile ranges are split into two separate values, when they could be using std::ops::Range and std::ops::RangeInclusive. This would give use functions like contains for free and would allow the new big comment you wrote to be placed on the range field.

What do you think about the above idea?

@tgnottingham
Copy link
Contributor Author

I think it's a good idea, and I've made the change for CacheEntry. For SourceFile, I'd rather not make the change because the start_pos and end_pos fields are public, and the scope of the change is a bit larger than I'd like to make here.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 28, 2020

Great! Thanks

@bors r+

@bors
Copy link
Contributor

bors commented Oct 28, 2020

📌 Commit 47dad31 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 29, 2020
…as-schievink

Rollup of 11 pull requests

Successful merges:

 - rust-lang#75078 (Improve documentation for slice strip_* functions)
 - rust-lang#76138 (Explain fully qualified syntax for `Rc` and `Arc`)
 - rust-lang#78244 (Dogfood {exclusive,half-open} ranges in compiler (nfc))
 - rust-lang#78422 (Do not ICE on invalid input)
 - rust-lang#78423 (rustc_span: improve bounds checks in byte_pos_to_line_and_col)
 - rust-lang#78431 (Prefer new associated numeric consts in float error messages)
 - rust-lang#78462 (Use unwrapDIPtr because the Scope may be null.)
 - rust-lang#78493 (Update cargo)
 - rust-lang#78499 (Prevent String::retain from creating non-utf8 strings when abusing panic)
 - rust-lang#78505 (Update Clippy - temporary_cstring_as_ptr deprecation)
 - rust-lang#78527 (Fix some more typos)

Failed merges:

r? `@ghost`
@bors bors merged commit 151db25 into rust-lang:master Oct 30, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 30, 2020
@tgnottingham tgnottingham deleted the caching_source_map_bounds_check branch January 20, 2021 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants