rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.43k stars 1.54k forks source link

large_enum_variant triggered in error-chain macro has unhelpful help message #2121

Open killercup opened 7 years ago

killercup commented 7 years ago

You know how I like to suggest E-hard new lints? Sorry, this time I only have an E-hard L-enhancement to report.

warning: large size difference between variants
  --> src/main.rs:66:1
   |
66 | / error_chain! {
67 | |     foreign_links {
68 | |         Io(::std::io::Error);
69 | |         Handlebars(::handlebars::TemplateRenderError);
70 | |         Yaml(::yaml::Error);
71 | |     }
72 | | }
   | |_^
   |
   = note: #[warn(large_enum_variant)] on by default
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.165/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
   |
113| err : Box<$ foreign_link_error_path> ) {
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: this error originates in a macro outside of the current crate
   = issue author's remark: this help message is not helpful

Is there a way to (easily) print the enum variants with their sizes? This might be helpful even in rather obvious cases.

oli-obk commented 7 years ago

is there a point in reporting anything inside that macro? We can just turn it off in macros.

killercup commented 7 years ago

Well, the concern is valid: It's a large enum. Question is, if the programmer can do anything about it (in this case: probably). It's just the quoted code that's not really useful here.

oli-obk commented 7 years ago

can you tell error_chain to box the args?

Something like

error_chain! {
    foreign_links {
        Io(Box<::std::io::Error>);
    }
}

?

oli-obk commented 7 years ago

Is there a way to (easily) print the enum variants with their sizes? This might be helpful even in rather obvious cases.

That is trivial.

Figuring out how to produce nice error spans is a little involved, but sound like "just some detective work"

killercup commented 7 years ago

can you tell error_chain to box the args?

Doesn't look like it: https://play.rust-lang.org/?gist=0007d7af1676d861d6cb534f4fa735c1&version=stable


Sorry, this time I only have an E-hard L-enhancement to report.

oli-obk added E-easy E-medium L-enhancement T-middle

😞

oli-obk commented 7 years ago

error-chain already seems to box its contents: https://play.rust-lang.org/?gist=232208cd5ee6f1fcf95c4eb52562dfde&version=stable

killercup commented 7 years ago

Hahaha, sorry, hadn't had enough coffee yet. That's basically the quoted macro line from above! πŸ˜†

113| err : Box<$ foreign_link_error_path> ) {
Arnavion commented 7 years ago

@killercup

can you tell error_chain to box the args?

Doesn't look like it: https://play.rust-lang.org/?gist=0007d7af1676d861d6cb534f4fa735c1&version=stable

error-chain is totally fine with letting you have a foreign link of Box<io::Error>, since Box<io::Error> does impl std::error::Error that foreign links require.

The error in your link is because you told error-chain that the foreign link is over Box<io::Error>, so the EK impls From<Box<io::Error>>. But your code is trying to use ? with an unboxed io::Error for which there is no From impl.


@oli-obk

error-chain already seems to box its contents: https://play.rust-lang.org/?gist=232208cd5ee6f1fcf95c4eb52562dfde&version=stable

No, error-chain's codegen does not box foreign links. The size of foo::Error and bar::Error in your playground is the same because of the effect of the implicit Msg(String) member that occupies 24 bytes, leading to 32 bytes total to include the discriminant. See:

mod foo {
    pub enum ErrorKind {
        Msg(String),
        Io(Box<::std::io::Error>),
        #[doc(hidden)]
        __Nonexhaustive { },
    }
}

mod bar {
    pub enum ErrorKind {
        Msg(String),
        Io(::std::io::Error),
        #[doc(hidden)]
        __Nonexhaustive { },
    }
}

fn main() {
    println!("{}", std::mem::size_of::<foo::ErrorKind>()); // 32
    println!("{}", std::mem::size_of::<bar::ErrorKind>()); // 32
    println!("{}", std::mem::size_of::<String>()); // 24
    println!("{}", std::mem::size_of::<::std::io::Error>()); // 16
    println!("{}", std::mem::size_of::<Box<::std::io::Error>>()); // 8
}