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

Allow disabling const_item_mutation lint for all uses of a specific const item #77522

Closed
wants to merge 2 commits into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Oct 4, 2020

Closes #77425.

The const_item_mutation lint will look at whether allow(const_item_mutation) is present on the const being "mutated", and if so, respect that as suppressing const_item_mutation for all uses of that const.

#[allow(const_item_mutation)]
const MYCONST: MyConst = MyConst {...};


MYCONST.foo = ""; // does not trigger const_item_mutation

This supersedes the special case around consts having a Drop impl introduced in #77251, so it is removed in this PR.

r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2020
@Aaron1011
Copy link
Member

I thought about doing this - however, this has the unfortunate side effect of suppressing the lint within the initializer:

const OTHER: [bool; 1] = [true];

#[allow(const_item_mutation)]
const MYCONST: u8 = {
    OTHER[0] = false; // doesn't warn
    0
};

While this seems unlikely to come up in practice, I would prefer if we separated lints that reference a const definition from lints that apply to a const definition (i.e. the initializer).

@dtolnay
Copy link
Member Author

dtolnay commented Oct 4, 2020

Considering this in terms of false negatives, I think this approach is less likely to have problematic/surprising false negatives in practice than the one from #77251. The programmer already typed an allow(const_item_mutation) so it's on their mind and they are consciously doing something bizarre. If they need a super big initializer expression for some reason to the point that false negatives are concerning, the initializer can be moved to a separate const.

const OTHER: [bool; 1] = [true];

const MYCONST_HELPER: u8 = {
    OTHER[0] = false; //~ warning
    0
};

#[allow(const_item_mutation)]
const MYCONST: u8 = MYCONST_HELPER;

I would prefer if we separated lints that reference a const definition from lints that apply to a const definition (i.e. the initializer).

I agree, though having a "shallow" way to talk about an item without talking about nested pieces like the initializer is an extremely general problem. Some examples where I have seen the same thing come up with existing lints are:

  • In async-trait we expand from this:

    #[async_trait]
    trait Trait {
        async fn f() {...}
    }

    into effectively something like this:

    trait Trait {
        fn f() -> Pin<Box<dyn Future<Output = ()> + Send>> {
            #[allow(clippy::missing_docs_in_private_items)]
            async fn __f() {
                ... // original body
            }
            Box::pin(__f())
        }
    }

    Notice the problem? I can't apply the allow to __f without also applying it to everything I'm pasting into __f from the user's code.

  • This case from rustc_data_structures:

    #[macro_export]
    macro_rules! likely {
    ($e:expr) => {
    #[allow(unused_unsafe)]
    {
    unsafe { std::intrinsics::likely($e) }
    }
    };
    }

    We want likely! to be callable from inside safe code as well as from inside unsafe code, so it has an allow(unused_unsafe) around its unsafe call to std::intrinsics::likely, but that allow(unused_unsafe) (and, frighteningly, the unsafe itself) also gets applied to the caller's $expr.

@jyn514 jyn514 added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 4, 2020
@dtolnay
Copy link
Member Author

dtolnay commented Oct 7, 2020

Will close to give a chance to design a way to distinguish lints that reference a const vs lints inside the const definition; we can follow up in #77425.

@dtolnay dtolnay closed this Oct 7, 2020
@dtolnay dtolnay deleted the drop branch October 7, 2020 23:58
@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Nov 14, 2020

FWIW, I agree with this idea / initiative; lints are good, and this one was direly needed, but "false positive" on lints can be quite annoying.

  • I have personally been intending to use StackBox::new_in(&mut Slot::EMPTY, …) (and to suggest downstream users of my crate do so too), only to realize this lint was firing. &mut None is an idiomatic alloc-in-caller-frame pattern, and my Slot type was a small wrapper around that pattern for some added type-safety, hence my adding a const to emulate the None variant. I'd have liked the possibility to make my type opt out of this lint.

    • In the meantime, I have defined a free function const fn slot(), so that the pattern has now become &mut slot() 🤷

Aside

This case from rustc_data_structures

FWIW, I have encountered this myself, and this is how I palliate it:

match $e { b => { #[allow(unused_unsafe)]] {
    unsafe { ::std::intrinsics::likely(b) }
}}}

or:

({
    #[inline]
    fn likely (b: bool) -> bool {
        unsafe { ::std::intrinsics::likely(b) }
    }
    likely
})($e)

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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Way to disable const_item_mutation lint for one specific const
5 participants