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

declare_interior_mutable_const and borrow_interior_mutable_const need to respect enum variants #3962

Closed
devsnek opened this issue Apr 15, 2019 · 3 comments · Fixed by #6110
Closed
Labels
C-bug Category: Clippy is not doing the correct thing C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages

Comments

@devsnek
Copy link

devsnek commented Apr 15, 2019

#[derive(Debug)]
enum Thing {
	// variant A has interior mutability
    A(std::cell::Cell<bool>),
	// variant B clearly does not
    B,
}

impl Thing {
    // declare_interior_mutable_const triggers here
    pub const BB: Thing = Thing::B;
}

fn main() {
    // borrow_interior_mutable_const triggers here
    println!("Thing::BB = {:?}", Thing::BB);
}

I realize that it isn't possible to know if the thing on the rhs of a const declaration is interior mutable or not without full const evaluation, but trivial cases such as the example shouldn't be too difficult.

@flip1995 flip1995 added C-bug Category: Clippy is not doing the correct thing C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages labels Apr 15, 2019
@ijackson
Copy link

ijackson commented Aug 3, 2020

I just tripped over this. In my case, it occurs with a trait constant:

use std::io;
use std::error::Error;
use thiserror::Error;

#[derive(Debug,Error)]
#[error("my error {:?}",self)]
pub enum MyError {
    Simple,
    Complicated(io::Error), // <- so MyError cannot be Copy
}

pub trait Trait {
    type Error : Error;
    #[allow(clippy::declare_interior_mutable_const)]
    const ERROR : Self::Error;
}

pub struct Object;

impl Trait for Object {
    type Error = MyError;
    const ERROR : MyError = MyError::Simple;
}

The playground doesn't have thiserror, so here is my Cargo.toml:

[package]
name="junk"
version="0.0.0"
edition="2018"
[dependencies]
thiserror="1"

@ijackson
Copy link

ijackson commented Aug 3, 2020

It's probably also possible to get a false positive from this without using an enum, if there is some other kind of type that can be constructed const but which is not Copy.

I think the the root of this particular problem is that the lint ought to depend on the value of the constant, rather than the type.

@rail-rain
Copy link
Contributor

rail-rain commented Aug 25, 2020

@ijackson I think what you found is actually something related to #5050 because I believe the below is the only error message you'd get if you removed #[allow(...)] from the example; and it says the type of the constant ERROR - Self::Error associated type - is not guaranteed to be interior mutability free; therefore it has nothing to do with the MyError enum and the Object struct. Is that right?

error: a `const` item should never be interior mutable
  --> src/main.rs:15:5
   |
15 |     const ERROR : Self::Error;
   |     ^^^^^^^^^^^^^^-----------^
   |                   |
   |                   consider requiring `<Self as Trait>::Error` to be `Copy`
   |
   = note: `#[deny(clippy::declare_interior_mutable_const)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const

The minimum example on the playground

rail-rain added a commit to rail-rain/rust-clippy that referenced this issue Oct 4, 2020
fix a false positive in two `interior_mutable_const` lints where a constant with enums gets linted
even if it uses a clearly unfrozen variant. Note that the code uses the MIR interpreter, which
the author of rust-lang#3962 thought unlikely to be a solution. This might be over-engineering;
but, I think it's important to be able to work with the 'http' crate (rust-lang#3825).
rail-rain added a commit to rail-rain/rust-clippy that referenced this issue Oct 4, 2020
fix a false positive in two `interior_mutable_const` lints where a constant with enums gets linted
even if it uses a clearly unfrozen variant. Note that the code uses the MIR interpreter, which
the author of rust-lang#3962 thought unlikely to be a solution. This might be over-engineering;
but, I think it's important to be able to work with the 'http' crate (rust-lang#3825).
@bors bors closed this as completed in 694cec1 Nov 7, 2020
Ryan1729 pushed a commit to Ryan1729/rust-clippy that referenced this issue Nov 7, 2020
fix a false positive in two `interior_mutable_const` lints where a constant with enums gets linted
even if it uses a clearly unfrozen variant. Note that the code uses the MIR interpreter, which
the author of rust-lang#3962 thought unlikely to be a solution. This might be over-engineering;
but, I think it's important to be able to work with the 'http' crate (rust-lang#3825).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants