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.

Sign up for GitHub

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

Jump to bottom

Denote ControlFlow as #[must_use] #137449

Merged
merged 3 commits into from
Mar 17, 2025
Merged

Denote ControlFlow as #[must_use] #137449

merged 3 commits into from
Mar 17, 2025

Conversation

compiler-errors
Copy link
Member

I've repeatedly hit bugs in the compiler due to ControlFlow not being marked #[must_use]. There seems to be an accepted ACP to make the type #[must_use] (rust-lang/libs-team#444), so this PR implements that part of it.

Most of the usages in the compiler that trigger this new warning are "root" usages (calling into an API that uses control-flow internally, but for which the callee doesn't really care) and have been suppressed by let _ = ..., but I did legitimately find one instance of a missing ? and one for a never-used ControlFlow value in #137448.

Presumably this needs an FCP too, so I'm opening this and nominating it for T-libs-api.

This PR also touches the tools (incl. rust-analyzer), but if this went into FCP, I'd split those out into separate PRs which can land before this one does.

r? libs-api
@rustbot label: T-libs-api I-libs-api-nominated

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc 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 Feb 23, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 24, 2025
@compiler-errors compiler-errors added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library 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 Feb 24, 2025
@Amanieu
Copy link
Member

Amanieu commented Feb 25, 2025

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Feb 25, 2025 •
edited by m-ou-se
Loading

Team member @Amanieu 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!

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 Feb 25, 2025
@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 25, 2025
@joshtriplett
Copy link
Member

I've checked my box. But also, as a procedural matter, #[must_use] is a two-way door, so I don't think it needs an FCP.

@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 Feb 25, 2025
@rfcbot
Copy link

rfcbot commented Feb 25, 2025

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

@Amanieu
Copy link
Member

Amanieu commented Feb 26, 2025

I've checked my box. But also, as a procedural matter, #[must_use] is a two-way door, so I don't think it needs an FCP.

True, but we only had 3 people in the libs-api meeting when this came up (of which I was the only libs-api member). It was easier to just start an FCP.

@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 Mar 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 4, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 7, 2025
@rfcbot
Copy link

rfcbot commented Mar 7, 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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Mar 7, 2025
@rustbot rustbot mentioned this pull request Mar 7, 2025
Open
@bors
Copy link
Contributor

bors commented Mar 13, 2025

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

@Amanieu
Copy link
Member

Amanieu commented Mar 16, 2025

r=me once conflicts are resolved.

@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@compiler-errors
Copy link
Member Author

hey @rust-lang/rust-analyzer, just pinging that while I said that I'd split this into a separate PR in the description:

This PR also touches the tools (incl. rust-analyzer), but if this went into FCP, I'd split those out into separate PRs which can land before this one does.

I've decided that's probably just a lot of work for no benefit at this point. Since we enforce no warnings in CI, and since this makes a new warning in CI that I need to fix for all tools we build in-tree, it's definitely not worth fixing this (for example) as a separate PR on the r-a repo and waiting for a subtree sync rather than just fixing it here.

If y'all disagree, I'm happy to coordinate this somehow, but I'd prefer if we didn't unnecessarily block landing this PR.

@Veykril
Copy link
Member

Veykril commented Mar 16, 2025

I don't think that should be an issue though I delegate that decision to @lnicola given I am not the one doing the syncs

@compiler-errors
Copy link
Member Author

@bors r=Amanieu,lnicola

@bors
Copy link
Contributor

bors commented Mar 17, 2025

📌 Commit e250bd1 has been approved by Amanieu,lnicola

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 Mar 17, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 17, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2025
@bors
Auto merge of rust-lang#138611 - matthiaskrgr:rollup-hmjbqva, r=matth…
43a2e9d 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133870 (Stabilize `asm_goto` feature gate)
 - rust-lang#137449 (Denote `ControlFlow` as `#[must_use]`)
 - rust-lang#137465 (mir_build: Avoid some useless work when visiting "primary" bindings)
 - rust-lang#138349 (Emit function declarations for functions with `#[linkage="extern_weak"]`)
 - rust-lang#138412 (Install licenses into `share/doc/rust/licenses`)
 - rust-lang#138577 (rustdoc-json: Don't also include `#[deprecated]` in `Item::attrs`)
 - rust-lang#138588 (Avoid double lowering of idents)

Failed merges:

 - rust-lang#138321 ([bootstrap] Distribute split debuginfo if present)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9adf218 into rust-lang:master Mar 17, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 17, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2025
@rust-timer
Unrolled build for rust-lang#137449
7a36a77 
Rollup merge of rust-lang#137449 - compiler-errors:control-flow, r=Amanieu,lnicola

Denote `ControlFlow` as `#[must_use]`

I've repeatedly hit bugs in the compiler due to `ControlFlow` not being marked `#[must_use]`. There seems to be an accepted ACP to make the type `#[must_use]` (rust-lang/libs-team#444), so this PR implements that part of it.

Most of the usages in the compiler that trigger this new warning are "root" usages (calling into an API that uses control-flow internally, but for which the callee doesn't really care) and have been suppressed by `let _ = ...`, but I did legitimately find one instance of a missing `?` and one for a never-used `ControlFlow` value in rust-lang#137448.

Presumably this needs an FCP too, so I'm opening this and nominating it for T-libs-api.

This PR also touches the tools (incl. rust-analyzer), but if this went into FCP, I'd split those out into separate PRs which can land before this one does.

r? libs-api
`@rustbot` label: T-libs-api I-libs-api-nominated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lnicola lnicola lnicola approved these changes

Assignees

@Amanieu Amanieu

Labels
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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API 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. to-announce Announce this issue on triage meeting
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
9 participants
@compiler-errors @rust-log-analyzer @Amanieu @rfcbot @joshtriplett @rustbot @bors @Veykril @lnicola