[redundant_closure_call]: handle nested closures
#10930
Conversation
|
r? @Manishearth (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
Failed to set assignee to
|
|
Yep, will review it in a few hours 👍 |
|
I haven't reviewed this yet. But I find that... maybe... Instead of reworking the lint to be a late one. We could just add the parenthesis and run the lint logic again after the fix? I'm not sure if that's possible. Just a thought. If changing it to late will fix another issue, I guess it's still a worth it conversion. |
We might be able to do that. It's okay, if the suggestion of a lint, triggers a different lint (Even though it should be avoided, if that's reasonable). Though, late lints are generally speaking preferable. Clippy has more utilities for them, they provide important type information etc. It's not worth going back and rewriting all early lints, but if we have a late lint implementation, that passes all tests, I'd say that's preferable. This can, of course, depend on the lint, but if the logic is similar, I think it would be better to do this refactoring :) |
|
I originally had it suggest just wrapping it in parens, but I found it sad that it suggest something that is going to be linted again by itself (e.g. rewrite But, if this logic is unnecessary, I suppose I could also change it back to just suggesting parens. |
Not really, early lint passes can only go down the AST and not up.
With that comment, I was more referencing a potential case, where the reimplementation is 2x the original size with no benefit. This is not the case here, so don't worry. I think ranges are the few things which are easier to handle on the AST level. Almost everything else is better during a late lint pass. |
|
I find this use case very funny. If an user writes |
|
LGTM to me as well. Thank you for the PR. Dear bors, I humbly ask you to merge this pull request into master, in the name of princess @blyxyas and me as her shadow reviewer ✨ |
[`redundant_closure_call`]: handle nested closures Fixes #9956. This ended up being a much larger change than I'd thought, and I ended up having to pretty much rewrite it as a late lint pass, because it needs access to certain things that I don't think are available in early lint passes (e.g. getting the parent expr). I think this'll be required to fi-x #10922 anyway, so this is probably fine. (edit: had to write "fi-x" because "fix" makes github think that this PR fixes it, which it doesn't 😅 ) Previously, it would suggest changing `(|| || 42)()()` to `|| 42()`, which is a type error (it needs parens: `(|| 42)()`). In my opinion, though, the suggested fix should have really been `42`, so that's what this PR changes. changelog: [`redundant_closure_call`]: handle nested closures and rewrite as a late lint pass
|
💔 Test failed - checks-action_test |
what happened there? |
|
I'm guessing just some CI funkiness? Bors, please, we even asked nicely in the last comment |
|
Woops, wrong button 😅 Now let's try this properly again: @bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #9956.
This ended up being a much larger change than I'd thought, and I ended up having to pretty much rewrite it as a late lint pass, because it needs access to certain things that I don't think are available in early lint passes (e.g. getting the parent expr). I think this'll be required to fi-x #10922 anyway, so this is probably fine.
(edit: had to write "fi-x" because "fix" makes github think that this PR fixes it, which it doesn't 😅 )
Previously, it would suggest changing
(|| || 42)()()to|| 42(), which is a type error (it needs parens:(|| 42)()). In my opinion, though, the suggested fix should have really been42, so that's what this PR changes.changelog: [
redundant_closure_call]: handle nested closures and rewrite as a late lint pass