Reprise: new lint: Unintentional return of unit from closures expecting Ord #5737
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
r? @flip1995 since he worked on the previous pr |
|
(Looks like the CI doesn't work but it seems unrelated to the changes ?) |
|
Yes this is unrelated and will be fixed tomorrow (if no other unexpected breakage occurs). In regard of reviewing this: I will do that after we get Clippy working again in the Clippy repo and in the rustc repo. (hopefully in one or two days) |
|
What you can do in the meantime: Please squash the commit b8d55fc in your rebase commit, since the rebase commit reverts the former. |
|
Squashed |
|
Looks like there are unrelated errors again, and a related one which I fixed. EDIT: caused by rust-lang/rust#73578 |
|
rebased |
|
Ok now I have actual test fails however I have no idea where it comes from :( |
|
(I'm sorry for wasting the CI a bit, I never contributed to clippy and can't run dogfood test since I'm on windows) |
|
🎉 |
|
Also addressing original author's remark:
This is true for lots of lints, but they can be disabled selectively using |
flip1995
added
S-waiting-on-author
|
Added some tests and changed the way the lint looks. Now prints something like error: This closure returns the unit type which also implements PartialOrd.
--> $DIR/unintentional_unit_return.rs:20:30
|
LL | structs.is_sorted_by_key(|s| {
| ^^^
|
help: Probably caused by this trailing semicolon.
--> $DIR/unintentional_unit_return.rs:21:24
|
LL | double(s.field);
| ^ |
|
Can you squash some of the commits, please? After that, this should be good to go 🚀 |
|
Just, should this be in the nursery (as it is right now) ? Or what's the best category ? i would guess correctness (akin to |
| /// twins.sort_by_key(|x| { x.1; }); | ||
| /// ``` | ||
| pub UNINTENTIONAL_UNIT_RETURN, | ||
| nursery, |
There was a problem hiding this comment.
I think we can move this to correctness since it should be finished now. Or do you know of any problems you encountered with this lint, while implementing/testing it?
There was a problem hiding this comment.
Nope, I think it serves the exact purpose it was made for.
|
Moved from nursery to correctness and rewritten to description to match current (updated) behavior |
|
Thanks! @bors r+ |
|
📌 Commit 0f7cbf4 has been approved by |
flip1995
added
S-waiting-on-bors
Reprise: new lint: Unintentional return of unit from closures expecting Ord This lint catches cases where the last statement of a closure expecting an instance of Ord has a trailing semi-colon. It compiles since the closure ends up return () which also implements Ord but causes unexpected results in cases such as sort_by_key. Fixes #5080 Reprise of #5348 where I addressed all the comments there
|
💔 Test failed - checks-action_test |
This lint catches cases where the last statement of a closure expecting an instance of Ord has a trailing semi-colon. It compiles since the closure ends up return () which also implements Ord but causes unexpected results in cases such as sort_by_key. Fixes #5080 reprise: rebase, update and address all concerns
|
Renamed, rebased and rustuped |
|
@bors r+ Thanks! |
|
📌 Commit 1267909 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
|
Nice ! |
This lint catches cases where the last statement of a closure expecting
an instance of Ord has a trailing semi-colon. It compiles since the
closure ends up return () which also implements Ord but causes
unexpected results in cases such as sort_by_key.
Fixes #5080
Reprise of #5348 where I addressed all the comments there
changelog: add lint [
unit_return_expecting_ord]