Add drop_result lint

[GuillaumeGomez]

Fixes #9876.

I don't think there is any useful suggestion we could give to the user for this lint so I didn't add one.

r? @flip1995

changelog: Add drop_result lint.

[GuillaumeGomez]

CI finally passed! \o/

[oconnor663]

As far as useful suggestions go, I'm in favor of suggesting _ = my_result; for cases where the user really wants to ignore the error.

[flip1995]

What about a case like this:

let res = get_result();

match res {
    Ok(yay) => yay.do_something();
    Err(nay) => nay.report_but_continue();
}

drop(res); // dropping the entire result, because both Err and Ok case 
           // take resources that have to be freed before continuing.

Not sure how often something like this happens. Let's run lintcheck for this lint:

Reports

jobserver-0.1.27/src/lib.rs:511:13
jobserver-0.1.27/src/unix.rs:152:9
jobserver-0.1.27/src/unix.rs:153:9
jobserver-0.1.27/src/unix.rs:398:13
jobserver-0.1.27/src/unix.rs:80:9
jobserver-0.1.27/src/unix.rs:81:9
openssl-0.10.59/src/hash.rs:369:17
tracing-core-0.1.32/src/dispatcher.rs:904:9

[flip1995]

I would argue that the first one in jobserver and the ones in openssl and tracing-core are FPs, as they are called in a drop implementation on a finish() or similar functions on a field, where one just wants to ignore the error, and if there is actually something returned, drop it immediately.

The others in jobserver I would argue are style preferences, where they use drop(fn_call_returning_result()) instead of let _ = fn_call_returning_result(). There's nothing really wrong with doing this IMO. At least not on the level of a suspicious lint.

[GuillaumeGomez]

Would it be accepted in style or should I just close this PR?

[flip1995]

I think the alternative to the above cases is let _ = result_thing. And then the question is: Is let _ better style than drop()? I can come up for arguments for both cases:

1. let _ = is the idiomatic syntax to ignore a Result (or must_use type)
2. drop signals better that you actually want to drop the value.

So maybe rather restriction/pedantic? WDYT?

[GuillaumeGomez]

I'm fine with removing it too but pedantic sounds good as well. Updating to reflect that.

[GuillaumeGomez]

Moved the lint to the pedantic level.

[bors]

☔ The latest upstream changes (presumably #11597) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

What do you think about adding a MaybeIncorrect suggestion of changing this to let _ =?

[GuillaumeGomez]

As you prefer. If you want a suggestion I can add it.

[oconnor663]

What do you think about adding a MaybeIncorrect suggestion of changing this to let _ =?

I can't remember what Rust version shipped this, but now we can just write _ = without the let.

[flip1995]

Just Playgrounded a bit: let _ instead of drop changes the behavior. So I'm hesitant suggesting this.

I think the lint is way more complex than I initially thought. If you use drop(res) on a result and actually want to drop it in that position, there's no alternative. So to avoid FPs, this lint would have to check, whether changing this to let _ will still drop the value when removing the drop. This is probably hard to do, but with that I'm not sure.

[GuillaumeGomez]

Again if we're not sure about this lint, we can close this PR and the related issue. It's fine. :)

[oconnor663]

Oh good point. I'd actually argue that the this code is still worth linting on:

    let s: Result<S, ()> = Ok(S);
    drop(s);

The problem there is the same with dropping a temporary result. In practice, the caller probably isn't explicitly spelling out the Result type, so this runs into the same problem where a function that used to return T was refactored to return Result<T, E> without breaking the old callsite. If the caller really truly wants this "retain the Ok value for a while but ignore any errors" behavior, I'd recommend explicitly converting Result to Option with .ok(). But I think the overwhelming majority of callers would probably prefer to either handle the error immediately with ? or .unwrap(), or ignore the result completely instead of binding it to a local in the first place.

It does seem tricky to know what suggestion is best here, so maybe it's best not to include a suggestion. But overall the lint still seems reliably useful to me.

[flip1995]

Coming back to this after a long time:

Catching the refactoring case is a strong argument for the lint. However Clippy can't know if a refactoring is happening, so we could only catch this by accepting FPs in other cases.

But as alternatives to dropping a Result change semantics in subtle ways, I don't feel like we should lint this, without a good suggestion. And every suggestion we come up with might change semantics. So we would have to explain this in the lint documentation, but then users have to read that documentation and understand the subtleties.

So the only option would be to put this lint into restriction. The same problems described above still exist there, but at least people would have to opt-in and understand the lint.

I would accept this lint as a restriction lint, but the subtle differences of the let _ alternative should be documented.

[GuillaumeGomez]

Let's close it, I don't think this is really worth it. I think the related issue should also be closed.