I wouldn't say it's "semantically different". It's just that the original code would compile even when a function signature is changed, while Clippy's suggested code would not compile.

Anyway concept ACK dropping the lint, I agree it's annoying and just adds fragility.

I agree, though in practice I doubt there's going to be lots of "fragility" introduced this way, while 5 characters less while marginal, keep (not) being there.

Just want to mention cargo clippy --fix is a quick way to take care of clippy nits. With LSPs applying the fix should be easy too.

Just want to mention cargo clippy --fix is a quick way to take care of clippy nits. With LSPs applying the fix should be easy too.

Unfortunately clippy ofter introduces multiple lints at once, and just --fixing them results in a big diff that makes it hard to review the result. (Yes, you can reproduce it locally, but if clippy thinks our code should be different, at least 10% of the time it's simply wrong.)

I personally convey the intention by writing foo.map_err(Into::into) but there's another lint about useless conversions.

Do we really not want to get compile error when the signature changes? It can sometimes uncover problems. E.g. if we want to tighten the error type it may be that we need to change multiple functions, not just one.

Do we really not want to get compile error when the signature changes? It can sometimes uncover problems

In theory, yes, but any use of ? would hide such a problem. So forcing users to avoid ? in this specific situation has limited value. And the cost is that 99% of the time, the compilation error is uninteresting and you just need to change the code back to Ok(x?) like you tried to write in the first place, before Clippy pointlessly made you change it.

@stevenroose BTW could you update this PR to change the comment? I think it's misleading to claim there's a "semantic difference" (which would be a clippy bug, even by the clippy authors' standards) when it's really more of a "difference in compiler behavior" if that.

I think "delegate the remainder of the method call to the following function" is semantically different from "return the value returned by this function after error handling", no? I admit it's a subtle difference. I guess I can just remove the comment altogether as well, as we don't really have other comments on module-level compiler indications anyway.

It seems like rustfmt doesn't allow me to leave a newline between different module-level compiler indications, so I added a pointless comment to separate the clippy line from the other module-level compiler indications. 👌

I think "delegate the remainder of the method call to the following function" is semantically different from "return the value returned by this function after error handling", no?

This lint only triggers on the last line of the function so both cases are "returning" a value. With your code you call From::from to convert an error type to itself (which is defined as the identity function by the From<T> for T impl in stdlib, and with the clippy code you don't.

@tcharding I'd like there to be some comment indicating why this was added. Even just referencing this issue would be fine.

Did you mean to mention me?

Oops, no, I meant to mention @stevenroose.

Changed the comment

@stevenroose please indicate why this specific lint is a problem. Otherwise we are going to accumulate a pile of clippy exceptions and not remember what we were thinking, whether they have stopped being relevant or if we've changed our minds or what. It also helps the clippy developers who periodically scan crates.io for ignored lints and try to understand why people are doing this.

It would be fine if the comment was a link to this PR and nothing else. (I can use git blame and some aux scripts to get to the PR link but in general that's a pain in the ass, especially for casual contributors.)

done, also put everything in the same commit for easier git blame