Skip to content
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

[crater] Make missing_fragment_specifier an unconditional error #128425

Open
wants to merge 3 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.

Experiment with turning missing fragment specifiers an unconditional error again.

More context: #128006

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
Contributor

bors commented Jul 31, 2024

⌛ Trying commit c2492ec with merge 063c08d...

@bors
Copy link
Contributor

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
@apiraino apiraino added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 31, 2024
@workingjubilee workingjubilee added the C-crater Category: Issue for tracking crater runs label Feb 14, 2025
@petrochenkov
Copy link
Contributor

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 Do you plan to return to this? The mentioned beta branch was released long time ago.

@tgross35
Copy link
Contributor Author

Yeah, that came and went. I'll get this rebased soon, should probably rerun crater to see if anything improved.

@tgross35 tgross35 force-pushed the missing-fragment-specifier-unconditional branch from c2492ec to 989d152 Compare February 20, 2025 06:59
@rustbot rustbot added the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label Feb 20, 2025
@tgross35 tgross35 force-pushed the missing-fragment-specifier-unconditional branch from 989d152 to 191a514 Compare February 20, 2025 07:02
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the missing-fragment-specifier-unconditional branch from 191a514 to 41f9c75 Compare February 20, 2025 17:52
@rust-log-analyzer

This comment has been minimized.

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.

[1]: rust-lang#75516
[2]: rust-lang#80210
[3]: rust-lang#128006
@tgross35 tgross35 force-pushed the missing-fragment-specifier-unconditional branch from 41f9c75 to f65c533 Compare February 26, 2025 05:44
The edition guide doc linkcheck fails, this is in a submodule so not
trivial to fix.
@tgross35
Copy link
Contributor Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2025
…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
Contributor

bors commented Feb 26, 2025

⌛ Trying commit d1ce28d with merge d124845...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#21 exporting to docker image format
#21 sending tarball 28.1s done
#21 DONE 41.2s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Listening on address 127.0.0.1:4226
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 4.19s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
warning: target feature `x87` must be enabled to ensure that the ABI of the current target can be implemented correctly
  |
  = note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #116344 <https://github.com/rust-lang/rust/issues/116344>
---
failures:

---- broken_fragment_local stdout ----

thread 'broken_fragment_local' panicked at src/tools/linkchecker/tests/checks.rs:17:5:
assertion failed: !status.success()

---- broken_fragment_remote stdout ----


thread 'broken_fragment_remote' panicked at src/tools/linkchecker/tests/checks.rs:17:5:
assertion failed: !status.success()

failures:
    broken_fragment_local
    broken_fragment_remote

@bors
Copy link
Contributor

bors commented Feb 26, 2025

☀️ Try build successful - checks-actions
Build commit: d124845 (d12484522bcab8d07e325c6570f226a2faace3f7)

@tgross35
Copy link
Contributor Author

The PR CI is just stuck on linkcheck. Cratering again to see if anything has changed since the error upgrades.

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-128425-1 created and queued.
🤖 Automatically detected try build d124845
🔍 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-128425-1 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-1 is completed!
📊 140 regressed and 0 fixed (589170 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ 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 Feb 27, 2025
@petrochenkov
Copy link
Contributor

Waiting on author to analyze the regressions and fix CI.
@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 Feb 27, 2025
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 S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for future-incompatibility lint missing_fragment_specifier
9 participants