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 triggers on array initializer #7665

Open
quaternic opened this issue Sep 11, 2021 · 5 comments
Open

declare_interior_mutable_const triggers on array initializer #7665

quaternic opened this issue Sep 11, 2021 · 5 comments
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages

Comments

@quaternic
Copy link

quaternic commented Sep 11, 2021

Lint name:
declare_interior_mutable_const

I tried this code:

let _: [Cell<bool>; 7] = [Cell::new(true); 7];

Since the array initialization syntax requires T: Copy, rustc errors and suggests this fix that works correctly:

const TRUE_CELL: Cell<bool> = Cell::new(true);
let _: [Cell<bool>; 7] = [TRUE_CELL; 7];

playground

To my knowledge this is the best way to initialize an array with a const but !Copy value.
(stackoverflow answer)

However, declare_interior_mutable_const triggers and incorrectly suggests "make this a static item (maybe with lazy_static)", which doesn't actually work at all. The lint documentation claims the code is bad because "Consts are copied everywhere they are referenced, ... , which defeats the whole purpose of using these types in the first place.", but in this case that is the entire reason for using a const item.

The known issues mention "a legacy way to supply an initialized value to downstream static items" as a legit use, but it is not immediately clear if this case is covered by that.

Meta

Rust version (rustc -Vv):

rustc 1.54.0 (a178d0322 2021-07-26)
binary: rustc
commit-hash: a178d0322ce20e33eac124758e837cbd80a6f633
commit-date: 2021-07-26
host: x86_64-unknown-linux-gnu
release: 1.54.0
LLVM version: 12.0.1
@quaternic quaternic added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Sep 11, 2021
@xFrednet
Copy link
Member

Hey, thank you for the report 🙃. This is actually a difficult case, where this way of initializing the array uses the fact that each const value is copied. This is copying is the exact thing that the lint, tries to warn for as is can be surprising to newcomers if this is not intended.

For cases like this, I would suggest adding a #[allow(clippy::declare_interior_mutable_const)] attribute to the const value as you intentionally use this behavior. Currently, I can sadly not think of a simple way to fix this, as we would need to check that the const value is only used for array initialization. It is possible to check for the usage of a value, but we would need to check the entire crate for global constant values. A good compromise could be to only check if a const value is used to initialize an array if the code is inside a function body. That would limit the scope of the search.

As this is theoretically working as intended, I'll mark this as a possible enhancement of the lint 🙃

@rustbot label -C-Bug -I-false-positive +C-enhancement

@rustbot rustbot added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages and removed I-false-positive Issue: The lint was triggered on code it shouldn't have labels Sep 12, 2021
@xFrednet xFrednet removed the C-bug Category: Clippy is not doing the correct thing label Sep 12, 2021
@quaternic
Copy link
Author

This is copying is the exact thing that the lint, tries to warn for as is can be surprising to newcomers if this is not intended.

Wouldn't that be just as surprising if the const is not interior-mutable?

The example in the documentation also triggers borrow_interior_mutable_const, so it's not clear why this lint is needed in addition. In fact, it seems to me that any attempt to mutate the const requires a shared reference and triggers borrow_interior_mutable_const. If there is no such attempt, the interior mutability is inconsequential.

@xFrednet
Copy link
Member

xFrednet commented Sep 13, 2021

This is copying is the exact thing that the lint, tries to warn for as is can be surprising to newcomers if this is not intended.

Wouldn't that be just as surprising if the const is not interior-mutable?

It shouldn't, because you can't modify constant values. Here is an example, what the lint is actually about
Playground:

const GLOBAL_CELL: Cell<bool> = Cell::new(false);
fn main() {
    GLOBAL_CELL.set(true);
    println!("{:?}", GLOBAL_CELL)
}

This will print out "Cell { value: false }" which might be counterintuitive. The lint basically tries to warn if a user tries to use const to create global values, as they are not truly global and instead placed in the code.

The example in the documentation also triggers borrow_interior_mutable_const, so it's not clear why this lint is needed in addition. In fact, it seems to me that any attempt to mutate the const requires a shared reference and triggers borrow_interior_mutable_const. If there is no such attempt, the interior mutability is inconsequential.

You're right, those lints have some overlaps. I'm not sure why, my guess is that declare_interior_mutable_const is for constant values in general and borrow_interior_mutable_const is an additional lint in case you use such a const value from an external crate. But I'm not sure on this one. It might also be so that you can allow declare_interior_mutable_const for these cases.

hawkw added a commit to hawkw/mycelium that referenced this issue Jul 28, 2022
clippy doesn't like interior mutable items in `const`s, because
mutating an instance of the `const` value will not mutate the const.
that is the *correct* behavior here, as the const is used just as an
array initializer; every time it's referenced, it *should* produce a
new value. therefore, this warning is incorrect in this case.

see rust-lang/rust-clippy#7665
hawkw added a commit to hawkw/mycelium that referenced this issue Jul 28, 2022
clippy doesn't like interior mutable items in `const`s, because
mutating an instance of the `const` value will not mutate the const.
that is the *correct* behavior here, as the const is used just as an
array initializer; every time it's referenced, it *should* produce a
new value. therefore, this warning is incorrect in this case.

see rust-lang/rust-clippy#7665
hawkw added a commit to hawkw/mycelium that referenced this issue Jul 28, 2022
clippy doesn't like interior mutable items in `const`s, because
mutating an instance of the `const` value will not mutate the const.
that is the *correct* behavior here, as the const is used just as an
array initializer; every time it's referenced, it *should* produce a
new value. therefore, this warning is incorrect in this case.

see rust-lang/rust-clippy#7665
hawkw added a commit to hawkw/mycelium that referenced this issue Jul 28, 2022
clippy doesn't like interior mutable items in `const`s, because
mutating an instance of the `const` value will not mutate the const.
that is the *correct* behavior here, as the const is used just as an
array initializer; every time it's referenced, it *should* produce a
new value. therefore, this warning is incorrect in this case.

see rust-lang/rust-clippy#7665
hawkw added a commit to hawkw/mycelium that referenced this issue Jul 29, 2022
clippy doesn't like interior mutable items in `const`s, because
mutating an instance of the `const` value will not mutate the const.
that is the *correct* behavior here, as the const is used just as an
array initializer; every time it's referenced, it *should* produce a
new value. therefore, this warning is incorrect in this case.

see rust-lang/rust-clippy#7665
@hniksic
Copy link

hniksic commented Nov 6, 2023

I stumbled upon this issue. It was in code like this:

static REGISTERED_NAMES: [OnceLock<String>; 256] = {
    const INIT_HELPER: OnceLock<String> = OnceLock::new();
    [INIT_HELPER; 256]
};

This const is the only way to initialize the static that I'm aware of, advertised e.g. in answers to this StackOverflow question.

@Jarcho
Copy link
Contributor

Jarcho commented Aug 6, 2024

As of 1.79 you can use inline const blocks (e.g. [const { Cell::new(0) }; 5]).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages
Projects
None yet
Development

No branches or pull requests

5 participants