Skip to content

Make missing_fragment_specifier an unconditional error #128425

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jul 31, 2024

This was attempted in 1 then reverted in 2 because of fallout. Recently, this was made an edition-dependent error in 3.

Make missing fragment specifiers an unconditional error again, across all editions.

More context: #128006
Most recent crater: #128425 (comment)
Fixes: #40107

r? @petrochenkov

@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 Jul 31, 2024
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the missing-fragment-specifier-unconditional branch 2 times, most recently from 392a2c7 to c2492ec Compare July 31, 2024 06:14
@tgross35
Copy link
Contributor Author

This is just to test the diagnostics, none of the possible code cleanup is included.

@petrochenkov please take a look when you get the chance and start crater if the change seems correct.

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 31, 2024
…unconditional, r=<try>

[crater] Make `missing_fragment_specifier` an unconditional error

This was attempted in [1] then reverted in [2] because of fallout. Recently, this was made an edition-dependent error in [3].

Experiment with turning missing fragment specifiers an unconditional error again.

More context: rust-lang#128006

[1]: rust-lang#75516
[2]: rust-lang#80210
[3]: rust-lang#128006
@bors
Copy link
Collaborator

bors commented Jul 31, 2024

⌛ Trying commit c2492ec with merge 063c08d...

@bors
Copy link
Collaborator

bors commented Jul 31, 2024

☀️ Try build successful - checks-actions
Build commit: 063c08d (063c08dd8db6ff113bb809c130456f1781abe72c)

@petrochenkov
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-128425 created and queued.
🤖 Automatically detected try build 063c08d
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-128425 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-128425 is completed!
📊 156 regressed and 2 fixed (487733 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 11, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 14, 2024

rg 'missing fragment specifier' -l --stats in the logs reports 373 matches. The previous attempt in 2020 had 475 regressions per #76605, so that is slight improvement but not much.

Like the previous run, almost all of these (361) come from clap <= 2.20, based on a crude rg 'missing fragment specifier.*\n.*clap' -l --stats -U. Remaining 12:

build-fail/reg/sem/0.1.0/master#83dcdb3a5dad0ed1e3e1fadc848d3f7727b41aa5.txt-[INFO] [stdout]    --> src/macros.rs:121:17
build-fail/reg/sem/0.1.0/master#83dcdb3a5dad0ed1e3e1fadc848d3f7727b41aa5.txt-[INFO] [stdout]    --> src/macros.rs:136:17
build-fail/reg/sem/0.1.0/master#83dcdb3a5dad0ed1e3e1fadc848d3f7727b41aa5.txt-[INFO] [stdout]    --> src/macros.rs:121:17
build-fail/reg/sem/0.1.0/master#83dcdb3a5dad0ed1e3e1fadc848d3f7727b41aa5.txt-[INFO] [stdout]    --> src/macros.rs:136:17
build-fail/reg/sem/0.1.0/try#063c08dd8db6ff113bb809c130456f1781abe72c.txt-[INFO] [stdout]    --> src/macros.rs:121:17
build-fail/reg/sem/0.1.0/try#063c08dd8db6ff113bb809c130456f1781abe72c.txt-[INFO] [stdout]    --> src/macros.rs:136:17
build-fail/reg/sem/0.1.0/try#063c08dd8db6ff113bb809c130456f1781abe72c.txt-[INFO] [stdout]    --> src/macros.rs:121:17
build-fail/reg/sem/0.1.0/try#063c08dd8db6ff113bb809c130456f1781abe72c.txt-[INFO] [stdout]    --> src/macros.rs:136:17
build-fail/reg/sem/0.1.0/try#063c08dd8db6ff113bb809c130456f1781abe72c.txt-[INFO] [stdout]    --> src/macros.rs:121:17
build-fail/reg/sem/0.1.0/try#063c08dd8db6ff113bb809c130456f1781abe72c.txt-[INFO] [stdout]    --> src/macros.rs:121:17
build-fail/reg/sem/0.1.0/try#063c08dd8db6ff113bb809c130456f1781abe72c.txt-[INFO] [stdout]    --> src/macros.rs:136:17
build-fail/reg/sem/0.1.0/try#063c08dd8db6ff113bb809c130456f1781abe72c.txt-[INFO] [stdout]    --> src/macros.rs:136:17

We could let this simmer for a while with the new error in deps level (#128122) and/or the edition-dependent lint (#128006). I don't really know how to feel about these results because all of the relevant clap versions have been yanked for ~4 years, per clap-rs/clap#2076.

Requesting compiler feedback for how to proceed. Context: this was made an e2024 error in #128006, making it an error in all editions is being considered here.

@rustbot label +I-compiler-nominated

@rustbot rustbot added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Aug 14, 2024
@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2024
@wesleywiser
Copy link
Member

Discussed briefly in the compiler team triage meeting. Given that nearly all regressions are on very old versions of clap and this has been a compatibility warning since 2017, we think it's reasonable to make this an unconditional error.

@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Aug 15, 2024
@tgross35
Copy link
Contributor Author

Thanks for discussing this. I'll wait for the next beta branch so #128122 is on stable for at least a cycle, then continue pushing this forward.

@tgross35
Copy link
Contributor Author

To reflect my above comment

@rustbot blocked

@rustbot rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Aug 19, 2024
@petrochenkov petrochenkov added I-lang-nominated Nominated for discussion during a lang team meeting. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2025
@traviscross traviscross added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Apr 29, 2025
@traviscross
Copy link
Contributor

Question. Is there any particular reason to do this sooner rather than later? E.g., is it in the way of anything else?

@tgross35
Copy link
Contributor Author

Not especially. The upsides are code simplification, and if we decide to do something like rust-lang/rfcs#3649 then it would allow us to make that available in all editions.

@traviscross traviscross removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Apr 30, 2025
@tmandry
Copy link
Member

tmandry commented Apr 30, 2025

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 30, 2025

Team member @tmandry 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.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 30, 2025
@traviscross
Copy link
Contributor

@rfcbot reviewed

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

It'd be good for us to setup some clearer criteria. here, but thanks @tgross35 for your persistence.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 30, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 30, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@traviscross traviscross added I-lang-radar Items that are on lang's radar and will need eventual work or consideration. and removed I-lang-nominated Nominated for discussion during a lang team meeting. labels May 7, 2025
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 10, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented May 10, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Copy link
Contributor

@traviscross traviscross left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@traviscross traviscross added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 11, 2025
@traviscross
Copy link
Contributor

traviscross commented May 11, 2025

Also, I checked the Reference, and it doesn't seem anything needs to happen there. It's already correct.

On the edition guide, we'll probably want to keep that page but add a note at the top that mentions that after shipping Rust 2024, we later made this lint into a hard error in all editions. If you can add that to

then we'll get that merged.

Also, it looks like there are conflicts here to resolve.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 11, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 11, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-crater Category: Issue for tracking crater runs disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for future-incompatibility lint missing_fragment_specifier