-
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 span of byte-escaped left format args brace #102214
Conversation
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
@@ -224,7 +224,7 @@ impl<'a> Iterator for Parser<'a> { | |||
'{' => { | |||
let curr_last_brace = self.last_opening_brace; | |||
let byte_pos = self.to_span_index(pos); | |||
let lbrace_end = InnerOffset(byte_pos.0 + 1); | |||
let lbrace_end = self.to_span_index(pos + 1); |
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 is a manual creation of an InnerOffset
just below (github won't let me comment on it). Could it trigger a similar bug? Should it be changed in a similar 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.
That's a good point. Originally I had assumed not since I wasn't able to find any breaking examples, but I just found that following:
println!("{\x7D");
gives the following error:
error: 1 positional argument in format string, but no arguments were given
--> src/main.rs:2:15
|
2 | println!("{\x7D");
| ^^
I can go ahead and apply the fix to that line as well, and add some more 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.
Yes, please, with the test if possible. That's a good opportunity to fix several identical bugs at once. I'll approve once this is done.
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.
Fixed in efc6df2 (edit: this resulted in a regression, see follow-up comment).
ebc0a52
to
efc6df2
Compare
Looking more into this issue, it seems like the way that format strings handle skipped characters erases some necessary information about the source string. Consider these two statements: println!("{\x7D"};
println!("{}\
"); In both cases, the resultant interned string looks like I'm thinking that it would be best to refactor this skipping mechanism to instead deal with a sequence of character widths (essentially squashing together some of the skips). It would make this PR larger than it initially was, but that's fine with me. Thoughts? |
This comment has been minimized.
This comment has been minimized.
@cassaundra a follow-up PR sounds good. If you don't mind, let's go back to the previous version of the patch, so we can get it merged in the mean time. |
efc6df2
to
e5096d4
Compare
Done. |
@bors r+ rollup |
…=cjgillot Fix span of byte-escaped left format args brace Fix rust-lang#102057 (see issue for example). Previously, the use of escaped left braces (`\x7B`) in format args resulted in an incorrectly offset span. This patch fixes that by considering any escaped characters within the string instead of using a constant offset.
Rollup of 7 pull requests Successful merges: - rust-lang#102214 (Fix span of byte-escaped left format args brace) - rust-lang#102426 (Don't export `__wasm_init_memory` on WebAssembly.) - rust-lang#102437 (rustdoc: cut margin-top from first header in docblock) - rust-lang#102442 (rustdoc: remove bad CSS font-weight on `.impl`, `.method`, etc) - rust-lang#102447 (rustdoc: add method spacing to trait methods) - rust-lang#102468 (tidy: make rustc dependency error less confusing) - rust-lang#102476 (Split out the error reporting logic into a separate function) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…cjgillot Fix incorrect span when using byte-escaped rbrace Fix rust-lang#103826, a format args span issue introduced in rust-lang#102214. The current solution for tracking skipped characters made it so that certain situations were ambiguous enough that the original span couldn't be worked out later. This PR improves on the original solution by keeping track of groups of skipped characters using a map, and fixes the previous bug. See an example of this ambiguity in the [previous PR's discussion](rust-lang#102214 (comment)).
Fix incorrect span when using byte-escaped rbrace Fix #103826, a format args span issue introduced in #102214. The current solution for tracking skipped characters made it so that certain situations were ambiguous enough that the original span couldn't be worked out later. This PR improves on the original solution by keeping track of groups of skipped characters using a map, and fixes the previous bug. See an example of this ambiguity in the [previous PR's discussion](rust-lang/rust#102214 (comment)).
Fix incorrect span when using byte-escaped rbrace Fix #103826, a format args span issue introduced in #102214. The current solution for tracking skipped characters made it so that certain situations were ambiguous enough that the original span couldn't be worked out later. This PR improves on the original solution by keeping track of groups of skipped characters using a map, and fixes the previous bug. See an example of this ambiguity in the [previous PR's discussion](rust-lang/rust#102214 (comment)).
Fix incorrect span when using byte-escaped rbrace Fix #103826, a format args span issue introduced in #102214. The current solution for tracking skipped characters made it so that certain situations were ambiguous enough that the original span couldn't be worked out later. This PR improves on the original solution by keeping track of groups of skipped characters using a map, and fixes the previous bug. See an example of this ambiguity in the [previous PR's discussion](rust-lang/rust#102214 (comment)).
Fix #102057 (see issue for example).
Previously, the use of escaped left braces (
\x7B
) in format args resulted in an incorrectly offset span. This patch fixes that by considering any escaped characters within the string instead of using a constant offset.