redundant_closure fix FP on coerced closure
#8431
Conversation
|
r? @flip1995 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| | | ||
| LL | let e = Some(1u8).map(|a| divergent(a)); | ||
| | ^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `divergent` | ||
|
|
There was a problem hiding this comment.
This seems like a false negative because map allows a function with any return type but the bug in the example expects a function that returns nothing (or unit). It seems like it should be possible to detect the difference. Maybe there is an expr type adjustment to coerce ! to () on the closure or the function call within the closure. Or maybe you can detect this scenario by comparing call_ty and closure_ty (one of them has -> () and the other has -> !?).
There was a problem hiding this comment.
Ah, I definitely misunderstood the bug in the example. I will take a further look. Thanks!
There was a problem hiding this comment.
Seems like checking for coerced closure is not enough. In the case above, it seems like the closure is coerced to () before being passed on to map (not sure why this is the case, though). AFAIU, in the example above, map accepts divergent nonetheless, probably because of generics. For non-generics, this is not the case and type check wouldn't resolve (hence the problem with issue #8416 and similar, see playground).
Ignoring coerced closures is not hard, but it does produce false negatives such as the case above. Although I'd say that this is still an improvement over all the false positives.
A full solution would be to check if type-check is resolvable for the call/method-call taking Fn(..) -> T without the coerced closure, but I'm not sure that this is trivial to do.
flip1995
added
S-waiting-on-author
dswij
force-pushed
the
8416
branch
3 times, most recently
from
929c5f3
to
4f5e96e
Compare
dswij
changed the title
redundant_closure fix FP on Never typeredundant_closure fix FP on coerced closure
flip1995
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #8676) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Hey @flip1995, I'd love to fix the merge conflicts, but this PR has not seen any activities for some time now. |
|
We currently sadly have a long reviewing queue and only a few reviewers. That's why stale PRs are sadly not always picked up. That being said, I'll take over the review, my queue is still manageable (If I ignore some university and works stuff xD). The linked issue contains two references to duplicate issues. Could you add tests and check that they are also fixed? If so, you can add them to the r? @xFrednet |
| @@ -145,6 +146,7 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction { | |||
| ); | |||
|
|
|||
| if_chain!( | |||
| if !is_adjusted(cx, &body.value); | |||
There was a problem hiding this comment.
I think that is_adjusted is a bit too restrictive. The example cases all are related to the never type. Therefore, I would only suppress the lint if the wrapped function returns the never type, in all other cases it seems like the normal adjustment has worked its magic.
Have you tried the following two options?
- Check the return type of the body expression of the closure. The expression might still return the never type if it gets adjusted, but I'm not sure about that.
- Otherwise we might have to resolve the referenced function and check what that returns.
There was a problem hiding this comment.
The example cases all are related to the never type.
The problem is not never type, but it's when the wrapped function can only be resolved by typechecks by adjusting it to return () type.
Check the return type of the body expression of the closure. The expression might still return the never type if it gets adjusted, but I'm not sure about that.
It gets adjusted to () type.
Otherwise we might have to resolve the referenced function and check what that returns.
Yes, the only way that I see for a complete solution is to check if the function typecheck can be resolved to accept the original return type of the closure. I'm not sure this is trivial to do.
There was a problem hiding this comment.
The example cases all are related to the never type.
The problem is not
nevertype, but it's when the wrapped function can only be resolved by typechecks by adjusting it to return()type.
Looks like I also misunderstood the issue at first. The type adjustment to () sounds like a quirk of the type checking system.
In that case, I think that your fix is the correct one. It will have some FNs but those are better than FPs IMO. 🙃
Hope the team can get enough bandwidth soon 🙂 |
Btw and slightly off topic, I've pinged you on Zulip. Not sure, if that's your preferred method of communication. I'll put the next review on my todo list 🙃 |
|
FYI, for anyone reading this in the future. This fix introduces some false negatives, but we believe that the trade of is worth it. Avoiding these false negatives would take several complicated checks, which also speaks for this solution. @dswij Thank you for this change and your patience. It's highly appreciated! Could you rebase one last time? Then you can use bors with @bors delegate+ |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
✌️ @dswij can now approve this pull request |
|
@dswij Can you resolve the conflicts? |
|
@bors r=xFrednet |
|
📌 Commit 33bf9e9 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Closes #8416,
Closes #7812
Closes #8091
Seems like this is fixed in #817 and regressed.Ignore coerced closure false positives, but this will lead to some false negatives on resolvable generics. IMO, this is still an overall improvement
changelog: [
redundant_closure] ignores coerced closure