result_large_err should be relative to the Ok variant

[dridi]

Summary

The current description for the lint is:

A Result is at least as large as the Err-variant. While we expect that variant to be seldomly used, the compiler needs to reserve and move that much memory every single time.

This should not be a problem if the Ok variant size is larger or not that smaller.

Lint Name

result_large_err

Reproducer

My code looks like this:

struct SomeContext<T> {
    outcome: T,
    // more fields
}

type SomeResult = Result<SomeContext<Good>, SomeContext<Bad>>;

In my case I believe the Good type is actually larger than the Bad one, and it is more convenient to embed the outcome in its surrounding context than embedding the context in both Good and Bad types.

It also doesn't bring any value to reference/box the context to avoid embedding it for this specific use case, it just adds one more indirection.

Version

rustc 1.66.1 (90743e729 2023-01-10) (Fedora 1.66.1-1.fc37)
binary: rustc
commit-hash: 90743e7298aca107ddaa0c202a4d3604e29bfeb6
commit-date: 2023-01-10
host: x86_64-unknown-linux-gnu
release: 1.66.1
LLVM version: 15.0.6

Additional Labels

No response

[lukaslueg]

The reason result_large_err does not take into account a ratio to the Ok-type is that the Err-value, if there is any, usually gets bubbled up the call stack. It is common the pass an Err-variant by means of the ?-operator, and it is also common that errors originating in library code end up in some kind of

enum FooError {
    ParserLibError(parselib::Error),
    IoError(io::Error),
    Synchrotron9000Error(synchroton9000::Error),
}

A large Err variant in any of the above would "infect" FooError, leading to the problem described in the lint in downstream code like fn() -> Result<(), FooError>

[dridi]

I understand the rationale, but if you apply my reasoning to your example Result<(), FooError> is precisely a case where there is a significant gap between the Ok and Err variants.

I still see it as a false positive: until you create a situation where an Err variant is solely responsible for a significant size increase of its Result type, it should not trigger.

[lukaslueg]

@Dridi That's a fair point, and I'm just re-iterating for clarification purposes:

// In `parselib`
pub fn parse() -> Result<LargeSuccess, Error> {
    let res = bar()?;
    do_the_work(&mut res);
    Ok(res)
}

// In `downstreamlib`
impl From<parselib::Error> for DownstreamError {
    // ...
}

fn thing() -> Result<(), DownstreamError> {
    let parsed = parselib::parse()?;
    Ok(())
}

result_large_err as currently implemented will warn the developer of parselib (!) that an unusually large error is returned to its consumers, excluding external ones. Within parselib, the ratio of LargeSuccess to Error is balanced, but we can't know how this plays out with consumers.

[dridi]

I would also like to reiterate that I believe I understand your point of view, I think we simply fundamentally disagree.

What you are essentially saying is that the lint will prevent library authors from laying traps where upstream libraries have "balanced" Ok/Err variants sizes, that silently (relatively) blow memory usage up for downstream Err variants when they are embedded in downstream Result types.

What I would argue is that the moment you add a dependency to a project you become responsible for the effects of that dependency in your project. So being careful about its API, behavior and potentially transitive dependencies. If anything, I would find this lint very useful if it triggered for the effect of a dependency on my project. That would signal that maybe I didn't pay enough attention to certain details when I adopted the dependency.

Hopefully I'm not misrepresenting you but in other words, you might see a relative take on the lint as enabling crates to silently lay traps for downstream consumers, and I see it as a safety net to catch when upstream types give my own results disproportionate Err variants.

[jacobbramley]

I'm jumping in a bit late, but (at the risk of stating the obvious) I think it would be useful to add the reasoning above to the documentation. My first thought, after hitting the lint and reading what is there, was very similar to this issue, and I hadn't considered the call stack bubbling concern.

I'm not trying to argue one way or another, but I'd like to offer another example. I have code that implements something a bit like try_into(self):

impl MyStruct {
    fn try_into(self) -> Result<MyOtherStruct, Self> { ... }
}

I'm taking self by value because it's expensive to construct, and most of it can be reused in MyOtherStruct. The function returns Err(self) if the conversion failed, because otherwise the object would be dropped. My Ok() is actually the same size as my Err(), which is why I was led to this issue. An example of the pattern (but not my code) is JsCast::dyn_into.

[utkarshgupta137]

Perhaps we can add lint configuration for this? Two parameters:

• min_err_to_ok_ratio: min ratio to report. 0 (default) means report every time, 1 means report when error is larger, 5 means report when error is 5x larger.
• min_err_size: min size to report. 0 means report every time, 128 (default) means report if err is bigger than 128B. This will preserve the current behavior & also allow users to choose which parameters suit them best. For example, I would choose 64B min size, but only if Err is larger than Ok.

[de-vri-es]

To add to the discussion about trying to steer library authors:

The lint does not take into account if a function is publicly exported or not. When writing a HTTP server, I have a bunch of private functions like this: async fn handle_request(...) -> Result<Response, Response>. They all trigger this lint, which in my opinion is wrong here. It's a useful pattern, there's no performance hit, and nobody outside the developers working on that particular code will ever notice it.

If clippy actually wants to steer library authors away from returning large errors, I think this lint should be split into two:

• One lint to check in any function if the Err is significantly larger than the Ok variant. This is usually undesirable, even in private internal functions. Even better would be to make this work for all generic enums.
• One lint that checks the public API for large error types, regardless of the size of the Ok variant and warns why this is considered bad (and personally I would put this one in the pedantic group).

Nevertheless, I agree with @dridi that the library consumer should be made responsible for how they consume errors. For example, it's quite simple to wrap the large error from the library in a box in your own error wrapper, especially since clippy will point out the problem for you.