rustc_parse: More precise spans for tuple.0.0
#77774
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The rustfmt issue looks severe enough to beta-backport this (if this PR is confirmed to fix it). |
petrochenkov
added
beta-nominated
Thanks for the ping @petrochenkov. If this lands before Tuesday (08:00 UTC) then it'll be in the next release of the rustc-ap crates which we can pull into rustfmt to confirm it resolves rust-lang/rustfmt#4355. Would be tough to validate beforehand though because with the current model it's usually pretty painful to test rustc changes within rustfmt outside the auto publish process. |
|
Actually can confirm this will indeed resolve rust-lang/rustfmt#4355 We were already using a relatively up-to-date version of the rustc-ap crates so it wasn't too difficult to validate |
| let span = self.token.span; | ||
| let span_is_good = || self.span_to_snippet(span).as_deref() == Ok(float_str).as_deref(); |
There was a problem hiding this comment.
I worry that this span_to_snippet might be too expensive in the parser. Can't we instead look at self.token.kind? Of course, this would be called in an uncommon enough codepath that it wouldn't be too bad, but I would prefer to avoid any unnecessary perf expense.
There was a problem hiding this comment.
The span can point anywhere regardless of the self.token.kind, and for an arbitrary span span.lo + 1 can potentially point e.g. in the middle of a UTF-8 encoded code point.
So, I'm afraid that actually looking into the source map is the only reliable way to determine that you can do arithmetic operations on that span.
Of course, this would be called in an uncommon enough codepath
Yes, that's what I hope for, tuple.0.0 is a rare syntactic construction.
Hopefully we'll be able to migrate to more fine-grained tokens ~next year, then the whole parse_tuple_field_access_expr_float could be eliminated.
| match &*components { | ||
| // 1e2 | ||
| [IdentLike(i)] => { | ||
| self.parse_tuple_field_access_expr(lo, base, Symbol::intern(&i), suffix, None) | ||
| } | ||
| // 1. | ||
| [IdentLike(i), Punct('.')] => { | ||
| let (ident_span, dot_span) = if span_is_good() { |
There was a problem hiding this comment.
Can we rename span_is_good to span_is_not_through_macro or something else more descriptive? Even though the diff is easy to follow, it'll be hard to understand what this means on isolation, and the comment explaining why is not next to the closure.
There was a problem hiding this comment.
I've expanded the comment a bit.
(I don't think it's not about macros exactly, just about dangers of arithmetic with arbitrary spans.)
|
r=me, preferably with both nitpicks addressed. |
petrochenkov
force-pushed
the
floatuple
branch
from
b40e06e
to
dee7049
Compare
|
@bors r=estebank |
|
📌 Commit dee7049 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
rustbot
added
the
beta-accepted
pietroalbini
removed
the
beta-nominated
[beta] Rollup backports Cherry-picked: * Always use the Rust version in package names rust-lang#77336 * rustc_parse: More precise spans for `tuple.0.0` rust-lang#77774 * Update crossbeam-channel to avoid UB rust-lang#77819 * build-manifest: stop generating numbered channel names except for stable rust-lang#77854 * Dist build manifest rust-lang#77762 * bootstrap: set correct path for the build-manifest binary rust-lang#77909 r? `@ghost`
This should help with rust-lang/rustfmt#4355, but I haven't verified, cc @calebcartwright.