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 now fires for arcstr::ArcStr #12951

Closed
kpreid opened this issue Jun 18, 2024 · 5 comments · Fixed by #13290 · May be fixed by #13207
Closed

declare_interior_mutable_const now fires for arcstr::ArcStr #12951

kpreid opened this issue Jun 18, 2024 · 5 comments · Fixed by #13290 · May be fixed by #13207
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@kpreid
Copy link
Contributor

kpreid commented Jun 18, 2024

Summary

The arcstr::ArcStr type is interior mutable for its reference count, but provides a macro which can safely construct const instances. As of 1.81.0-nightly (59e2c01 2024-06-17), presumably due to #12691, there is now a warning emitted even though the macro tries to suppress it — all relevant components of the macro expansion have #[allow(clippy::declare_interior_mutable_const)].

I imagine that this might be considered “working as intended”, with the solution being to configure ignore-interior-mutability much as is done with bytes. Still, this is a regression in out-of-the-box usability, so I figured I'd report it for consideration.

Lint Name

declare_interior_mutable_const

Reproducer

I tried this code:

// arcstr = { version = "1.2.0" }

const FOO: arcstr::ArcStr = arcstr::literal!("hello");

I saw this happen:

warning: a `const` item should not be interior mutable
 --> src/lib.rs:1:1
  |
1 | const FOO: arcstr::ArcStr = arcstr::literal!("hello");
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I expected to see this happen: No warning

Version

rustc 1.81.0-nightly (59e2c01c2 2024-06-17)
binary: rustc
commit-hash: 59e2c01c2217a01546222e4d9ff4e6695ee8a1db
commit-date: 2024-06-17
host: x86_64-apple-darwin
release: 1.81.0-nightly
LLVM version: 18.1.7

Additional Labels

No response

@kpreid kpreid 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 Jun 18, 2024
@kpreid kpreid changed the title declare_interior_mutable_const can no longer be allowed in a macro declare_interior_mutable_const now fires in cases where it is meant to be allowed Jun 18, 2024
@kpreid kpreid changed the title declare_interior_mutable_const now fires in cases where it is meant to be allowed declare_interior_mutable_const now fires in macros where it is meant to be allowed Jun 18, 2024
@kpreid
Copy link
Contributor Author

kpreid commented Jul 22, 2024

This bug has now reached the 1.80.0 stable pre-release :(

@Alexendoo Alexendoo changed the title declare_interior_mutable_const now fires in macros where it is meant to be allowed declare_interior_mutable_const now fires for arcstr::ArcStr Jul 23, 2024
@Alexendoo Alexendoo added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jul 23, 2024
@Alexendoo
Copy link
Member

ArcStr stores a NonNull pointer to an interior mutable type, previously these weren't linted but after #12691 the lint now follows references/pointers to interior mutable types

Since it's linting based on the type this also triggers without macros being used:

const S: ArcStr = ArcStr::new();

Nominating for discussion for how we want to handle this in an ongoing fashion, we could continue adding exceptions to the list as they pop up or make the lint more conservative

@flip1995
Copy link
Member

flip1995 commented Aug 6, 2024

#13207 This PR might fix this issue, without having to add arcstr to our default config. So we decided to wait for that and then re-evaluate.

@flip1995 flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Aug 6, 2024
@thomcc
Copy link
Member

thomcc commented Aug 9, 2024

(author of arcstr here) I tried #[allow]ing the lint inside the macros, and because the user's const is outside the macro it didn't do the trick. That said, it might help some cases (like macro use that's not directly assigning a const) so I might publish an update with it.

@Alexendoo
Copy link
Member

We spoke about the issue in the Clippy meeting and decided to backport a small fix (#13290) while waiting for the full #13207

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 I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
4 participants