New lint: large Err(E) variant, as compared with Ok(T), for Result<T, E>

[dekellum]

I've found the large_enum_variant lint from clippy quite educational and was hoping it would be able to lint on what I'll call unbalanced Result<T, E> specified types, in particular when the E (Error) type is really large but the T type is quite small. From my reading, this is often a performance concern, e.g. wasting lots of stack space for an infrequently used error path. On the other hand if the T (Ok, happy path) is larger, then the cost of E should be negligible. Make sense?

After not being able to trigger clippy with Result<(), BigError> (where BigError is 3,000 bytes) I tried using my own MyResult, along the lines of:

pub enum MyResult<T, E> {
    Ok(T),
    Err(E),
}

/// MyResult concrete type with zero-bytes `Ok` and 3K bytes `Err` variant,
/// for a fixed stack size of 3001 bytes (asserted).
type Unbalanced = MyResult<(), BigError>;

fn f() -> Unbalanced {
    MyResult::Err(BigError::Large(error::Biggie { blob: [0u8; 3000] }))
}

But no clippy lint at present on either the fully specified type Unbalanced alias or the f() function definition.

Have/would you consider extending this lint to all generic enums, where they are fully specified, or perhaps more practically, adding a new lint specific to core::result::Result? The suggestion to use Box<E> would be applicable in the later case.

I don't claim to well understand the clippy code base, but this was the only hint I could find (an @oli-obk comment) that generic enum's were considered (and are likely ignored) for the original lint:

rust-clippy/clippy_lints/src/large_enum_variant.rs

Line 71 in bb41b16

// don't count generics by filtering out everything

% cargo clippy -V
clippy 0.0.212 (016d92d 2019-03-10)

[oli-obk]

My comment refers generic enums that are still too generic to compute the size. Your case completely monomorphizes the generic enum to something where we can compute the size, and I agree with you that we should be doing this.

I think we should start small and only look at Result types to begin with, since most of the time one does not have control over arbitrary enums from other crates, so there might be no way to change it.

I think we should start with a MIR lint that goes over all arguments and user defined local variables and the return type. For each of those types, use the walk() method that gives you an iterator over all the types present inside that type. Then, check for occurrences of ty::Adt whose DefId is the same as Result's DefId. Then run the same variant size checking code that we are running for enums in the large_enum_variant lint.

[dekellum]

Good to hear this is possible as a MIR lint, thanks @oli-obk!

Do you think this Result case deserves its own new top level lint? A reason I think it might is that the opposite case, where the Ok(T) is large and the Err(E) is small, there is likely no reason to raise a lint. Also in the large Err(E) case it would be helpful to further elaborate (at least on the ALL the Clippy Lints page) why this can be bad in particular, and perhaps independently adjust the threshold size difference at which it is reported?

[oli-obk]

Yea, making it its own lint just for Result makes sense.

[lukaslueg]

@oli-obk Could you point out wrt #3884 (comment) why we need to walk locals and arguments at all - I'm missing the point. Isn't it enough to walk the E in a Result<_, E> as far as we can, and come up with an at least size, e.g. for Result<(), (T, [u8; 512])>, by simply summing up fields as far as we can determine? The size will be wrong due to generics, but it's the best we can do for library code, and the final E will certainly not be smaller.

[oli-obk]

I don't remember and can't make much sense of my own comment. Your explanation for a plan sounds good