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

large_enum_variant false positive (incorrect size calculation?) #9798

Open
djc opened this issue Nov 4, 2022 · 5 comments · May be fixed by #12550 or #13833
Open

large_enum_variant false positive (incorrect size calculation?) #9798

djc opened this issue Nov 4, 2022 · 5 comments · May be fixed by #12550 or #13833
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

@djc
Copy link
Contributor

djc commented Nov 4, 2022

Summary

There seems to be a regression in calculating the size for the type contained in a variant here.

https://github.com/tokio-rs/tls/actions/runs/3391306304/jobs/5636351880

Lint Name

large_enum_variant

Reproducer

I tried this code:

pub enum TlsStream<T> {
    Client(client::TlsStream<T>),
    Server(server::TlsStream<T>),
}

I saw this happen:

Error:    --> tokio-rustls/src/lib.rs:393:1
    |
393 | / pub enum TlsStream<T> {
394 | |     Client(client::TlsStream<T>),
    | |     ---------------------------- the second-largest variant contains at least 0 bytes
395 | |     Server(server::TlsStream<T>),
    | |     ---------------------------- the largest variant contains at least 608 bytes
396 | | }
    | |_^ the entire enum is at least 0 bytes
    |
    = note: `-D clippy::large-enum-variant` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
    |
395 |     Server(Box<server::TlsStream<T>>),
    |            ~~~~~~~~~~~~~~~~~~~~~~~~~

I expected to see this happen:

Smaller size difference, or maybe no lint at all.

Version

rustc 1.65.0 (897e37553 2022-11-02)
binary: rustc
commit-hash: 897e37553bba8b42751c67658967889d11ecd120
commit-date: 2022-11-02
host: aarch64-apple-darwin
release: 1.65.0
LLVM version: 15.0.0

Additional Labels

Seems to be a regression in 1.65.

@djc djc 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 Nov 4, 2022
@lukaslueg
Copy link
Contributor

lukaslueg commented Nov 5, 2022

Before 1.65 large_enum_variant could not determine the size of enums that have type parameters on them. Since 1.65 it will de-facto assume a ZST for any type parameter. In the example above client::TlsStream<T> will be a ZST/indeterminate, but server::TlsStream<T> will be at least 608 bytes for any T, and only grow from there, triggering the lint-threshold of 200 bytes. That is, no matter what T is, the enum would lint.

@djc
Copy link
Contributor Author

djc commented Nov 7, 2022

Why do you think client::TlsStream<T> will be a ZST/indeterminate? This is client::TlsStream:

pub struct TlsStream<IO> {
    pub(crate) io: IO,
    pub(crate) session: ClientConnection,
    pub(crate) state: TlsState,

    #[cfg(feature = "early-data")]
    pub(crate) early_waker: Option<std::task::Waker>,
}

While this is server::TlsStream<T>:

pub struct TlsStream<IO> {
    pub(crate) io: IO,
    pub(crate) session: ServerConnection,
    pub(crate) state: TlsState,
}

If it is able to deduce that server::TlsStream is at least 608 bytes, why does it think client::TlsStream is at least 0 bytes, instead of something more? (ServerConnection and ClientConnection should be similar in size.)

@kangalio
Copy link

kangalio commented Nov 9, 2022

In serenity, this bug is happening even without any generics.

/// A container for any channel.
#[derive(Clone, Debug, Serialize)]
#[serde(untagged)]
#[non_exhaustive]
pub enum Channel {
    /// A channel within a [`Guild`].
    Guild(GuildChannel),
    /// A private channel to another [`User`] (Direct Message). No other users may access the
    /// channel. For multi-user "private channels", use a group.
    Private(PrivateChannel),
}
warning: large size difference between variants
  --> src/model/channel/mod.rs:47:1
   |
47 | / pub enum Channel {
48 | |     /// A channel within a [`Guild`].
49 | |     Guild(GuildChannel),
   | |     ------------------- the largest variant contains at least 312 bytes
50 | |     /// A private channel to another [`User`] (Direct Message). No other users may access the
51 | |     /// channel. For multi-user "private channels", use a group.
52 | |     Private(PrivateChannel),
   | |     ----------------------- the second-largest variant contains at least 0 bytes
53 | | }
   | |_^ the entire enum is at least 0 bytes
   |
   = note: `#[warn(clippy::large_enum_variant)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
   |
49 |     Guild(Box<GuildChannel>),

@kangalio
Copy link

kangalio commented Nov 10, 2022

Apparently, clippy's size calculation chokes on recursive types:

pub struct Recursive(Box<Recursive>);

pub enum Foo {
    A([u8; 201]),
    B(Recursive),
}

// 5 | |     B(Recursive),
//   | |     ------------ the second-largest variant contains at least 0 bytes

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=43e0a41a5cc088d5aaba28fcada26ed9

(In serenity's case, PrivateChannel contained a User, which contained a Box<PartialMember>, which contained a User)

kangalio added a commit to kangalio/serenity that referenced this issue Nov 10, 2022
arqunis pushed a commit to serenity-rs/serenity that referenced this issue Nov 10, 2022
arqunis pushed a commit to serenity-rs/serenity that referenced this issue Nov 13, 2022
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this issue Feb 28, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this issue May 18, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this issue May 30, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this issue Sep 21, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this issue Oct 17, 2023
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this issue Oct 24, 2023
arqunis pushed a commit to arqunis/serenity that referenced this issue Oct 24, 2023
arqunis pushed a commit to arqunis/serenity that referenced this issue Oct 24, 2023
@Jarcho Jarcho linked a pull request Aug 10, 2024 that will close this issue
bors added a commit that referenced this issue Aug 14, 2024
Remove `is_normalizable`

fixes #11915
fixes #9798
Fixes only the first ICE in #10508

`is_normalizable` is used in a few places to avoid an ICE due to a delayed bug in normalization which occurs when a projection type is used on a type which doesn't implement the correct trait. The only part of clippy that actually needed this check is `zero_sized_map_values` due to a quirk of how type aliases work (they don't a real `ParamEnv`). This fixes the need for the check there by manually walking the type to determine if it's zero sized, rather than attempting to compute the type's layout thereby avoid the normalization that triggers the delayed bug.

For an example of the issue with type aliases:
```rust
trait Foo { type Foo; }
struct Bar<T: Foo>(T::Foo);

// The `ParamEnv` here just has `T: Sized`, not `T: Sized + Foo`.
type Baz<T> = &'static Bar<T>;
```

When trying to compute the layout of `&'static Bar<T>` we need to determine if what type `<Bar<T> as Pointee>::Metadata` is. Doing this requires knowing if `T::Foo: Sized`, but since `T` doesn't have an associated type `Foo` in the context of the type alias a delayed bug is issued.

changelog: [`large_enum_variant`]: correctly get the size of `bytes::Bytes`.
@Wilfred
Copy link
Contributor

Wilfred commented Nov 8, 2024

I hit this today, and the text "the second-largest variant contains at least 0 bytes" makes it hard to trust this specific lint.

I believe this is indicative of a genuine issue with my code, but just that the reporting is confused by a recursive type.

@samueltardieu samueltardieu linked a pull request Dec 15, 2024 that will close this issue
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
Development

Successfully merging a pull request may close this issue.

4 participants