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 · 7 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 · 7 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
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.

3 participants