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

clippy doesn't generate large_enum_variant warning when using bytes::Bytes #11915

Open
swanandx opened this issue Dec 3, 2023 · 8 comments · May be fixed by #12550
Open

clippy doesn't generate large_enum_variant warning when using bytes::Bytes #11915

swanandx opened this issue Dec 3, 2023 · 8 comments · May be fixed by #12550
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't

Comments

@swanandx
Copy link

swanandx commented Dec 3, 2023

Summary

If we are using Vec, clippy generates warning for large size difference in enum variants as expected, but if we replace Vec with bytes::Bytes, clippy doesn't generate any warning! Even when the size difference of enum variants is much bigger in later case!

Lint Name

large_enum_variant

Reproducer

I tried this code:

use bytes::Bytes;

// size = 296
enum NoWarnings {
    BigBoi(PublishWithBytes),
    _SmallBoi(u8),
}

//  size = 224
enum MakesClippyAngry {
    BigBoi(PublishWithVec),
    _SmallBoi(u8),
}

fn main() {
    let v = MakesClippyAngry::BigBoi(PublishWithVec::default());
    dbg!(std::mem::size_of_val(&v));
    let u = NoWarnings::BigBoi(PublishWithBytes::default());
    dbg!(std::mem::size_of_val(&u));
}

// can't find better way to generate structs with big size fast lol
#[derive(Default, Debug)]
struct PublishWithBytes {
    _dup: bool,
    _retain: bool,
    _topic: Bytes,
    __topic: Bytes,
    ___topic: Bytes,
    ____topic: Bytes,
    _pkid: u16,
    _payload: Bytes,
    __payload: Bytes,
    ___payload: Bytes,
    ____payload: Bytes,
    _____payload: Bytes,
}

#[derive(Default, Debug)]
struct PublishWithVec {
    _dup: bool,
    _retain: bool,
    _topic: Vec<u8>,
    __topic: Vec<u8>,
    ___topic: Vec<u8>,
    ____topic: Vec<u8>,
    _pkid: u16,
    _payload: Vec<u8>,
    __payload: Vec<u8>,
    ___payload: Vec<u8>,
    ____payload: Vec<u8>,
    _____payload: Vec<u8>,
}

I expected to see this happen:

clippy should have generated large_enum_variant warning for both enums NoWarnings ( uses Bytes ) & MakesClippyAngry ( uses Vec ).

Instead, this happened:

Clippy generates warning only for MakesClippyAngry, but doesn't for NoWarnings.

warning: large size difference between variants
  --> src/main.rs:10:1
   |
10 | / enum MakesClippyAngry {
11 | |     BigBoi(PublishWithVec),
   | |     ---------------------- the largest variant contains at least 224 bytes
12 | |     _SmallBoi(u8),
   | |     ------------- the second-largest variant contains at least 1 bytes
13 | | }
   | |_^ the entire enum is at least 224 bytes
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
   = note: `#[warn(clippy::large_enum_variant)]` on by default
help: consider boxing the large fields to reduce the total size of the enum
   |
11 |     BigBoi(Box<PublishWithVec>),
   |            ~~~~~~~~~~~~~~~~~~~

Version

rustc 1.74.0 (79e9716c9 2023-11-13)
binary: rustc
commit-hash: 79e9716c980570bfd1f666e3b16ac583f0168962
commit-date: 2023-11-13
host: x86_64-unknown-linux-gnu
release: 1.74.0
LLVM version: 17.0.4
@swanandx swanandx added C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't labels Dec 3, 2023
@TennyZhuang
Copy link
Contributor

@rustbot claim

@TennyZhuang
Copy link
Contributor

It seems that Bytes can't pass is_normalizable, and be treated as a false negative.

@TennyZhuang
Copy link
Contributor

pub struct Bytes {
    ptr: *const u8,
    len: usize,
    // inlined "trait object"
    data: AtomicPtr<()>,
    vtable: &'static Vtable,
}
pub(crate) struct Vtable {
    /// fn(data, ptr, len)
    pub clone: unsafe fn(&AtomicPtr<()>, *const u8, usize) -> Bytes,
    /// fn(data, ptr, len)
    ///
    /// takes `Bytes` to value
    pub to_vec: unsafe fn(&AtomicPtr<()>, *const u8, usize) -> Vec<u8>,
    /// fn(data, ptr, len)
    pub drop: unsafe fn(&mut AtomicPtr<()>, *const u8, usize),
}

Seems that the Vtable can't pass is_normalizable.

@TennyZhuang
Copy link
Contributor

I'm unsure since I'm not familiar with normalize, but I guess the field pub clone: unsafe fn(&AtomicPtr<()>, *const u8, usize) -> Bytes is treated as a recursive usage to Bytes.

@TennyZhuang
Copy link
Contributor

TennyZhuang commented Dec 16, 2023

I add a simple match arm ty::FnPtr(_) => true, at the position, then everything works well.

_ => ty.walk().all(|generic_arg| match generic_arg.unpack() {

Generally I believe FnPtr will not cause normalize panic, but I can't give a confidence.

I need some help :)

@TennyZhuang
Copy link
Contributor

TennyZhuang commented Dec 16, 2023

@anall @flip1995 It seems that you contribute a lot at this function. It'll be a great help if you can give some suggestion. Thanks!

The function is introduced 3 years ago with a FIXME.

// FIXME: Per https://doc.rust-lang.org/nightly/nightly-rustc/rustc_trait_selection/infer/at/struct.At.html#method.normalize

Is the function still needed?

@flip1995
Copy link
Member

I don't really contribute there. My only contributions are syncs between the rust and Clippy repo. Fastest way to get help here is to post on Zulip about this.

@y21 y21 linked a pull request Mar 25, 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`.
@Arjentix
Copy link

Arjentix commented Oct 1, 2024

+1 here.

I start getting weird false-positive with this lint, when I replaced some Vec<u8> with bytes::Bytes in my code. However an enum, where I get this warning, does not even have variant containing bytes::Bytes on the stack. Variant contains Vec<bytes::Bytes>. Looks strange to me that clippy somehow counts sizes for generics even if generic will never be on the stack.

Also interesting that clippy completely ignores middle-size variant and even biggest-size variant. It warns me about variant containing Vec<bytes::Bytes> in comparison to the smallest variant.

Simplified example of my code and the warning I get:

enum MyEnum {
    Smallest(...), // the second-largest variant contains at least 96 bytes
    NotSoSmall(...),
    JustOk(...),
    ContainsBytes(..., Vec<bytes::Bytes>), // the largest variant contains at least 440 bytes
    Biggest(...),
}
// ^ the entire enum is at least 0 bytes

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-negative Issue: The lint should have been triggered on code, but wasn't
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants