-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Reject invalid literal suffixes in tuple indexing, tuple struct indexing, and struct field name position #145463
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
base: master
Are you sure you want to change the base?
Conversation
@bors try |
This comment has been minimized.
This comment has been minimized.
[WIP] Reject invalid literal suffixes in tuple indexing, tuple struct indexing, and struct field name position
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
Nominating for T-lang FCP. Please refer to PR description for request. @rustbot label: +I-lang-nominated |
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.
r=me after nits and if/once T-lang approves with a fFCP-merge.
To make it more obvious what it's testing. This is its own commit to make git blame easier.
Actually, the accidentally accepted invalid suffixes also include tuple struct indexing positions, struct numeral field name positions. So before changing anything, first expand test coverage, so we can observe the effect of bumping the non-lint pseudo-FCW warning into a hard error.
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Addressed nits in (range-diff)1 Footnotes
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
Can we get a PR to the reference to update everything there?
|
Sure. I'll send one tmrw / Wednesday. Will need to double-check which bits of the grammar needs updating tho. |
Tracking issue: #60210
Closes #60210
Summary
Bump the "suffixes on a tuple index are invalid" non-lint pseudo future-incompatibility warning (#60210)1 to a hard error across all editions, rejecting the remaining carve outs from accidentally accepted invalid suffixes since Rust 1.27.
{i,u}{32,size}
in Temporarily accept [i|u][32|size] suffixes on a tuple index and warn #60186 (the "carve outs") to mitigate immediate ecosystem impact, but it came with an FCW warning indicating that we wanted to reject it after a few Rust releases.std::mem::offset_of!
stabilized in Rust 1.77.0 happens to use the same "don't expect suffix" code path which has the carve outs, so it also accepted the carve out suffixes. I'm proposing to reject this case as well.What specifically breaks?
Code that still relied on invalid
{i,u}{32,size}
suffixes being temporarily accepted by #60186 as an ecosystem impact mitigation measure (cf. #60138). Specifically, the following cases (particularly the construction of these forms in proc macros like reported in #60138):Position 1: Invalid
{i,u}{32,size}
suffixes in tuple indexingPosition 2: Invalid
{i,u}{32,size}
suffixes in tuple struct indexingPosition 3: Invalid
{i,u}{32,size}
suffixes in numeric struct field namesPosition 4: Invalid
{i,u}{32,size}
suffixes instd::mem::offset_of!
While investigating the warning, unfortunately I noticed
std::mem::offset_of!
also happens to use the "expect no suffix" code path which had the carve outs. So this was accepted since Rust 1.77.0 with the same FCW:The above forms in proc macros
For instance, constructions like (see tracking issue #60210):
where the user needs to actually write
Crater results
Conducted a crater run (#145463 (comment)).
usize
" in derive macro. Has a ton of other build warnings, last updated 6 years ago.quote
'sToTokens
on ausize
index (i.e. on tuple structTup(())
), the generated suffix becomes.0usize
(cf. Position 2).Review remarks
Tasks
Footnotes
The FCW was implemented as a non-lint warning (meaning it has no associated lint name, and you can't
#![deny(..)]
it) because spans coming from proc macros could not be distinguished from regular field access. This warning was also intentionally impossible to silence. See https://github.com/rust-lang/rust/pull/60186#issuecomment-485581694. ↩