Skip to content

Fix incorrect span when using byte-escaped rbrace#103828

Merged
bors merged 1 commit intorust-lang:masterfrom
cassaundra:fix-format-args-span2
Dec 26, 2022
Merged

Fix incorrect span when using byte-escaped rbrace#103828
bors merged 1 commit intorust-lang:masterfrom
cassaundra:fix-format-args-span2

Conversation

@cassaundra
Copy link
Contributor

@cassaundra cassaundra commented Nov 1, 2022 •
edited
Loading

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.

@rustbot
Copy link
Collaborator

rustbot commented Nov 1, 2022

r? @oli-obk

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 1, 2022
cassaundra
cassaundra commented Nov 1, 2022
View reviewed changes
compiler/rustc_parse_format/src/lib.rs Outdated
Comment on lines 847 to 852
Copy link
Contributor Author

@cassaundra cassaundra Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: a small check removed here. The lexer will raise "numeric character escape is too short" before we ever get to this code if there aren't three characters following the \.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 8, 2022

r? @cjgillot since you also reviewed #102214

@rustbot rustbot assigned cjgillot and unassigned oli-obk Nov 8, 2022
@bors
Copy link
Collaborator

bors commented Nov 8, 2022

☔ The latest upstream changes (presumably #104138) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot
cjgillot reviewed Nov 12, 2022
View reviewed changes
compiler/rustc_parse_format/src/lib.rs Outdated
Copy link
Contributor

@cjgillot cjgillot Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be changed by #104193.
cc @TaKO8Ki

Copy link
Contributor Author

@cassaundra cassaundra Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there are conflicts from both #103651 and #104193, I'll go ahead and resolve the other issues before I try sort those out.

compiler/rustc_parse_format/src/lib.rs Outdated
Copy link
Contributor

@cjgillot cjgillot Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate the comment? Notably: what is the index of the Vec?

compiler/rustc_parse_format/src/lib.rs Outdated
Copy link
Contributor

@cjgillot cjgillot Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you comment the individual fields?
What are they: positions? Starting at which point of reference?

Copy link
Contributor Author

@cassaundra cassaundra Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments.

For what it's worth, I think this could also be refactored to instead have a InnerSpan and a usize, which might make some of the intent more clear.

compiler/rustc_parse_format/src/lib.rs Outdated
Copy link
Contributor

@cjgillot cjgillot Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we wrap all creations of InnerOffset?

Copy link
Contributor Author

@cassaundra cassaundra Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not exactly sure what you mean.

Maybe related: a function that gives you both position and width could clean this up

compiler/rustc_parse_format/src/lib.rs Outdated
Copy link
Contributor

@cjgillot cjgillot Nov 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment: what are the input and output usizes?
Should we introduce some type to differentiate "raw" from "remapped" positions?

Copy link
Contributor Author

@cassaundra cassaundra Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should be InnerOffset, my bad.

@cassaundra cassaundra force-pushed the fix-format-args-span2 branch from af0d14c to 4ccc7f3 Compare November 18, 2022 05:27
@cassaundra cassaundra force-pushed the fix-format-args-span2 branch from 4ccc7f3 to 35c7939 Compare November 22, 2022 04:37
@cassaundra
Copy link
Contributor Author

cassaundra commented Nov 22, 2022

Rebased against master.

@cjgillot
Copy link
Contributor

cjgillot commented Dec 26, 2022

Thanks @cassaundra. Sorry for the slow review.
@bors r+

@bors
Copy link
Collaborator

bors commented Dec 26, 2022

📌 Commit 35c7939 has been approved by cjgillot

It is now in the queue for this repository.

@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 Dec 26, 2022
@bors
Copy link
Collaborator

bors commented Dec 26, 2022

⌛ Testing commit 35c7939 with merge 731e0bf...

@bors
Copy link
Collaborator

bors commented Dec 26, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 731e0bf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 26, 2022
@bors bors merged commit 731e0bf into rust-lang:master Dec 26, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 26, 2022
@rust-timer
Copy link
Collaborator

rust-timer commented Dec 26, 2022

Finished benchmarking commit (731e0bf): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#103828 - cassaundra:fix-format-args-span2, r=…
ca03b20 
…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)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.68.0

Development

Successfully merging this pull request may close these issues.

Incorrect span produced when using encoded }

6 participants

@cassaundra @rustbot @oli-obk @bors @cjgillot @rust-timer