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

indirect_structural_match lint misfiring on enum variant unused in const item #62614

Closed
pnkfelix opened this issue Jul 12, 2019 · 3 comments · Fixed by #67343
Closed

indirect_structural_match lint misfiring on enum variant unused in const item #62614

pnkfelix opened this issue Jul 12, 2019 · 3 comments · Fixed by #67343
Assignees
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Jul 12, 2019

(Spawned off of #62411 (comment))

Consider this code (play):

struct Sum(u32, u32);

impl PartialEq for Sum {
    fn eq(&self, other: &Self) -> bool { self.0 + self.1 == other.0 + other.1 }
}

impl Eq for Sum { }

#[derive(PartialEq, Eq)]
enum Eek {
    TheConst,
    UnusedByTheConst(Sum)
}

const THE_CONST: Eek = Eek::TheConst;

pub fn main() {
    match Eek::UnusedByTheConst(Sum(1,2)) {
        THE_CONST => { println!("Hello"); }
        _ => { println!("Gbye"); }
    }
}

In stable and beta, it compiles without warning.

In nightly, it compiles with the following diagnostic warning (#62411):

warning: to use a constant of type `Sum` in a pattern, `Sum` must be annotated with `#[derive(PartialEq, Eq)]`
  --> src/main.rs:19:9
   |
19 |         THE_CONST => { println!("Hello"); }
   |         ^^^^^^^^^
   |
   = note: #[warn(indirect_structural_match)] on by default
   = warning: 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 #62411 <https://github.com/rust-lang/rust/issues/62411>

This is probably not what we want for this lint: It is entirely reasonable for the user to assume that the structural_match check is based solely on the ADT's that one encounters when traversing the const expression itself, and one should ignore any unused enum variants that are only tangentially related to the const.

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jul 12, 2019
@pnkfelix
Copy link
Member Author

pnkfelix commented Jul 12, 2019

My current thinking is that maybe we should revert (part of) PR #62339.

  • The "right way" to do this, I think, isn't going to use a type-visitor at all, so the code there is going to be thrown away anyway.

  • I'm not going to have time to fix it properly today, and after today I'm going to be gone for 8 weeks, which means if nothing is done, the current behavior will propagate to beta.

  • So the only question is whether the current lint, with its false positives, provides significantly more value to our users than the negative effects that its false positives have on our users.

@pnkfelix
Copy link
Member Author

New thought (since PR #62339 did include some useful fixes to e.g. ICEs): don't revert, "just" change the lint to default to Allow for now. We can move its default to Warn after I fix #62614

@pnkfelix pnkfelix self-assigned this Jul 12, 2019
@jonas-schievink jonas-schievink added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Jul 12, 2019
pnkfelix added a commit to pnkfelix/rust that referenced this issue Jul 12, 2019
This is a way to address the regression aspect of rust-lang#62614 in the
short term without actually fixing the bug. (My thinking is that the bug that
this lint detects has gone undetected for this long, it can wait a bit longer
until I or someone else has a chance to put in a proper fix that accounts for
rust-lang#62614.)
@pnkfelix
Copy link
Member Author

when/if PR #62623 lands, the stable-to-nightly regression aspect of this issue can be removed.

Centril added a commit to Centril/rust that referenced this issue Jul 12, 2019
…rect-structural-match-lint-to-allow, r=zackmdavis

downgrade indirect_structural_match lint to allow

This is a short-term band-aid for the regression aspect of rust-lang#62614.
@nikomatsakis nikomatsakis added the P-medium Medium priority label Jul 25, 2019
@jonas-schievink jonas-schievink removed the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Aug 15, 2019
@bors bors closed this as completed in 36d13cb Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
3 participants