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

Don't warn empty branches unreachable for now #129103

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Aug 14, 2024

The stabilization of min_exhaustive_patterns updated the unreachable_pattern lint to trigger on empty arms too. This has caused some amount of churn, and imposes an unjoyful #[allow(unreachable_patterns)] onto library authors who want to stay backwards-compatible.

While I think the lint should eventually cover these cases, for transition's sake I'd prefer to revert linting to what it was prior to stabilization, at least for now.

Fixes #129031.

r? @compiler-errors

@Nadrieril Nadrieril added the F-min_exhaustive_patterns `#![feature(min_exhaustive_patterns)]` label Aug 14, 2024
@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 Aug 14, 2024
@compiler-errors
Copy link
Member

Hm... I find it strange to be associating this with an edition, rather than associating it with a new lint and a lint group. I wonder if @traviscross has any thoughts about this.

@kpreid
Copy link
Contributor

kpreid commented Aug 14, 2024

As I see it, the logic of associating it with an edition is that people who are doing an edition migration are opting into changing their source code in "syntax cleanup" kinds of ways. It's not necessarily that the lint should be permanently associated only with the edition, but that looking for 2024 edition is a cheap way to identify authors who are opting in now. As time passes (at a minimum, after 1.82 is stable; maximum, after 1.81 is no longer a reasonable MSRV to maintain), it will become less important that only opt-ins get this warning, and it could become edition independent.

@compiler-errors
Copy link
Member

compiler-errors commented Aug 14, 2024

Lints can have edition-specific warning levels though -- e.g.

    @edition Edition2024 => Warn;

in the lint definition. I'd still much rather we still split the lint out into two and apply an edition-2024-specific Warn level to it, so that users can opt into this on earlier editions, and we can attach lint-specific wording and documentation to the case where patterns are redundant due to being uninhabited.

@traviscross
Copy link
Contributor

@rustbot labels +T-lang

As described here, I don't think there's going to be lang appetite for this.

@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 14, 2024
@compiler-errors
Copy link
Member

@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 Aug 19, 2024
@compiler-errors
Copy link
Member

I'd prefer if this were implemented as splitting the lint into two as I've mentioned several times before.

@Nadrieril
Copy link
Member Author

Nadrieril commented Aug 19, 2024

Well, since T-lang doesn't seem to want to mitigate the fallout there's no need to do that either

@Nadrieril Nadrieril closed this Aug 19, 2024
@Nadrieril Nadrieril reopened this Aug 21, 2024
@bors
Copy link
Contributor

bors commented Aug 21, 2024

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

@joshtriplett joshtriplett removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 21, 2024
@joshtriplett
Copy link
Member

Let's see if we have consensus to go ahead with this:

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Aug 21, 2024

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

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 Aug 21, 2024
@joshtriplett joshtriplett added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 21, 2024
@traviscross
Copy link
Contributor

CE said:

I'd prefer if this were implemented as splitting the lint into two as I've mentioned several times before.

@rfcbot concern split-lint

I'd like to see this change done by making unreachable_patterns into a lint group and then adding an impossible_patterns lint to it that's allow-by-default (and fires in all editions) and then make that lint warn-by-default in Rust 2024. This is closer to what we decided earlier here.

This concern is to confirm that's what we're deciding here, as that's not what this PR implements.

@rfcbot rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Aug 21, 2024
@joshtriplett
Copy link
Member

@traviscross My intent was to build a consensus on "the lint should not fire on these cases", without specifying the exact mechanism. No objection to doing that by splitting the lint.

@tmandry
Copy link
Member

tmandry commented Aug 21, 2024

@rfcbot reviewed

I support splitting the lint, or reverting to its old behavior and reintroducing as a separate lint later. I particularly agree with what @compiler-errors said in #129031 (comment):

I find the unreachable patterns lint to be very high signal towards there being an underlying problem with the code, and impossible patterns to be useful for signaling "hey, this code is dead" but not necessarily at the same severity level as the former, especially because we required users to include these dead arms previously.

Going forward, I would like to separate such cases whenever possible. I think it is tempting to collapse the cases into one because they fit nicely under a succinct description. But the combination of

  • aggressive churn (going from enforcing one way to linting against that way) and
  • a different risk profile for churned code

is a clear signal to separate them. (cc #122759)

edit: Changing the default from allow to warn on the new lint in edition 2024 seems like a good rollout strategy; we can always move to warn in all editions later.

@Nadrieril
Copy link
Member Author

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Sep 11, 2024

📌 Commit 5b7be14 has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 11, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 11, 2024
…ble, r=compiler-errors

Don't warn empty branches unreachable for now

The [stabilization](rust-lang#122792) of `min_exhaustive_patterns` updated the `unreachable_pattern` lint to trigger on empty arms too. This has caused some amount of churn, and imposes an unjoyful `#[allow(unreachable_patterns)]` onto library authors who want to stay backwards-compatible.

While I think the lint should eventually cover these cases, for transition's sake I'd prefer to revert linting to what it was prior to stabilization, at least for now.

Fixes rust-lang#129031.

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#119286 (show linker output even if the linker succeeds)
 - rust-lang#129103 (Don't warn empty branches unreachable for now)
 - rust-lang#129696 (update stdarch)
 - rust-lang#129835 (enable const-float-classify test, and test_next_up/down on 32bit x86)
 - rust-lang#129992 (Update compiler-builtins to 0.1.125)
 - rust-lang#130052 (Don't leave debug locations for constants sitting on the builder indefinitely)
 - rust-lang#130077 (Fix linking error when compiling for 32-bit watchOS)
 - rust-lang#130114 (Remove needless returns detected by clippy in the compiler)
 - rust-lang#130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd)
 - rust-lang#130168 (maint: update docs for change_time ext and doc links)
 - rust-lang#130239 (miri: fix overflow detection for unsigned pointer offset)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#129103 (Don't warn empty branches unreachable for now)
 - rust-lang#129696 (update stdarch)
 - rust-lang#129835 (enable const-float-classify test, and test_next_up/down on 32bit x86)
 - rust-lang#130077 (Fix linking error when compiling for 32-bit watchOS)
 - rust-lang#130114 (Remove needless returns detected by clippy in the compiler)
 - rust-lang#130168 (maint: update docs for change_time ext and doc links)
 - rust-lang#130228 (notify Miri when intrinsics are changed)
 - rust-lang#130239 (miri: fix overflow detection for unsigned pointer offset)
 - rust-lang#130244 (Use the same span for attributes and Try expansion of ?)
 - rust-lang#130248 (Limit `libc::link` usage to `nto70` target only, not NTO OS)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6879ee6 into rust-lang:master Sep 12, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
Rollup merge of rust-lang#129103 - Nadrieril:dont-warn-empty-unreachable, r=compiler-errors

Don't warn empty branches unreachable for now

The [stabilization](rust-lang#122792) of `min_exhaustive_patterns` updated the `unreachable_pattern` lint to trigger on empty arms too. This has caused some amount of churn, and imposes an unjoyful `#[allow(unreachable_patterns)]` onto library authors who want to stay backwards-compatible.

While I think the lint should eventually cover these cases, for transition's sake I'd prefer to revert linting to what it was prior to stabilization, at least for now.

Fixes rust-lang#129031.

r? ``@compiler-errors``
@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 12, 2024
@Nadrieril Nadrieril deleted the dont-warn-empty-unreachable branch September 12, 2024 19:42
@cuviper cuviper mentioned this pull request Sep 19, 2024
@cuviper cuviper modified the milestones: 1.83.0, 1.82.0 Sep 19, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2024
[beta] backports

- Don't warn empty branches unreachable for now rust-lang#129103
- Win: Add dbghelp to the list of import libraries rust-lang#130047
- `RepeatN`: use MaybeUninit rust-lang#130145
- Update LLVM to 19 327ca6c rust-lang#130212
- Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477
- Check params for unsafety in THIR rust-lang#130531

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2024
[beta] backports

- Don't warn empty branches unreachable for now rust-lang#129103
- Win: Add dbghelp to the list of import libraries rust-lang#130047
- `RepeatN`: use MaybeUninit rust-lang#130145
- Update LLVM to 19 327ca6c rust-lang#130212
- Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477
- Check params for unsafety in THIR rust-lang#130531

r? cuviper
try-job: dist-various-1
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2024
[beta] backports

- Don't warn empty branches unreachable for now rust-lang#129103
- Win: Add dbghelp to the list of import libraries rust-lang#130047
- `RepeatN`: use MaybeUninit rust-lang#130145
- Update LLVM to 19 327ca6c rust-lang#130212
- Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477
- Check params for unsafety in THIR rust-lang#130531

r? cuviper
try-job: dist-various-1
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2024
[beta] backports

- Don't warn empty branches unreachable for now rust-lang#129103
- Win: Add dbghelp to the list of import libraries rust-lang#130047
- `RepeatN`: use MaybeUninit rust-lang#130145
- Update LLVM to 19 327ca6c rust-lang#130212
- Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477
- Check params for unsafety in THIR rust-lang#130531

r? cuviper
try-job: dist-various-1
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2024
[beta] backports

- Don't warn empty branches unreachable for now rust-lang#129103
- Win: Add dbghelp to the list of import libraries rust-lang#130047
- `RepeatN`: use MaybeUninit rust-lang#130145
- Update LLVM to 19 327ca6c rust-lang#130212
- Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477
- Check params for unsafety in THIR rust-lang#130531

r? cuviper
try-job: dist-various-1
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2024
[beta] backports

- Don't warn empty branches unreachable for now rust-lang#129103
- Win: Add dbghelp to the list of import libraries rust-lang#130047
- `RepeatN`: use MaybeUninit rust-lang#130145
- Update LLVM to 19 327ca6c rust-lang#130212
- Revert rust-lang#129749 to fix segfault in LLVM rust-lang#130477
- Check params for unsafety in THIR rust-lang#130531

r? cuviper
try-job: dist-various-1
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 26, 2024
bengsparks pushed a commit to bengsparks/axum that referenced this pull request Oct 17, 2024
Backport to beta occurred in rust-lang/rust#129103
and stable never implemented this warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-min_exhaustive_patterns `#![feature(min_exhaustive_patterns)]` 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unreachable_patterns lint due to min_exhaustive_patterns