new lint: Unintentional return of unit from closures expecting Ord #5348

rabisg0 · 2020-03-21T16:32:52Z

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

changelog: none

bors · 2020-03-23T20:56:45Z

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

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 rust-lang#5080

bors · 2020-04-08T14:34:07Z

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

rabisg0 · 2020-04-13T05:08:06Z

gentle reminder for review

flip1995

Can you add some documentation what the get_* functions are trying to get. You could just copy the debug output of these types.

flip1995 · 2020-04-15T16:29:49Z

clippy_lints/src/unintentional_unit_return.rs

+        if_chain! {
+            if let ExprKind::MethodCall(_, _, ref args) = expr.kind;


No need for if_chain if there is only one if

flip1995 · 2020-04-15T16:35:18Z

clippy_lints/src/unintentional_unit_return.rs

+        // Trying to call erase_late_bound_regions on fn_sig.inputs() gives the following error
+        // The trait `rustc::ty::TypeFoldable<'_>` is not implemented for `&[&rustc::ty::TyS<'_>]`
+        let inputs_output = cx.tcx.erase_late_bound_regions(&fn_sig.inputs_and_output());
+        inputs_output.iter().enumerate().for_each(|(i, inp)| {


This can be done with

Suggested change

inputs_output.iter().enumerate().for_each(|(i, inp)| {

inputs_output.iter().rev().skip(1).rev().enumerate().for_each(|(i, inp)| {

flip1995 · 2020-04-15T16:36:33Z

clippy_lints/src/unintentional_unit_return.rs

+            if i == inputs_output.len() - 1 {
+                return;
+            }
+            fn_mut_preds.iter().for_each(|pred| {


I'd prefer a normal for loop here, since this is not in an iterator chain and for_each is less readable IMO.

Suggested change

fn_mut_preds.iter().for_each(|pred| {

for pred in &fn_mut_preds {

flip1995 · 2020-04-15T16:41:36Z

clippy_lints/src/utils/paths.rs

@@ -71,6 +72,7 @@ pub const ORD: [&str; 3] = ["core", "cmp", "Ord"];
 pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"];
 pub const OS_STRING_AS_OS_STR: [&str; 5] = ["std", "ffi", "os_str", "OsString", "as_os_str"];
 pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"];
+pub const PARTIAL_ORD: [&str; 3] = ["std", "cmp", "PartialOrd"];


You don't need this, since PartialOrd is a LangItem, which you can get with cx.tcx.lang_items().partial_ord_trait()

flip1995 · 2020-04-15T16:43:22Z

clippy_lints/src/unintentional_unit_return.rs

+    if let ExprKind::Closure(_, _fn_decl, body_id, _span, _) = arg.kind {
+        if_chain! {
+            let body = cx.tcx.hir().body(body_id);
+            if let ExprKind::Block(block, _) = body.value.kind;
+            if let Some(stmt) = block.stmts.last();
+            if let StmtKind::Semi(_) = stmt.kind;
+            then { return Some(stmt) }
+        }
+    }
+    return None;


Can be simplified to

Suggested change

if let ExprKind::Closure(_, _fn_decl, body_id, _span, _) = arg.kind {

if_chain! {

let body = cx.tcx.hir().body(body_id);

if let ExprKind::Block(block, _) = body.value.kind;

if let Some(stmt) = block.stmts.last();

if let StmtKind::Semi(_) = stmt.kind;

then { return Some(stmt) }

}

}

return None;

if_chain! {

if let ExprKind::Closure(_, _fn_decl, body_id, _span, _) = arg.kind;

let body = cx.tcx.hir().body(body_id);

if let ExprKind::Block(block, _) = body.value.kind;

if let Some(stmt) = block.stmts.last();

if let StmtKind::Semi(_) = stmt.kind;

then {

Some(stmt)

} else {

None

}

}

flip1995 · 2020-04-15T16:44:33Z

clippy_lints/src/unintentional_unit_return.rs

+        if_chain! {
+            let body = cx.tcx.hir().body(body_id);
+            if let ExprKind::Block(block, _) = body.value.kind;
+            if let Some(stmt) = block.stmts.last();


You also have to check for block.expr.is_none()

flip1995 · 2020-04-15T16:46:39Z

clippy_lints/src/unintentional_unit_return.rs

+    trait_path: &[&str],
+) -> Vec<&'tcx Predicate<'tcx>> {
+    let mut preds = Vec::new();
+    generics.predicates.iter().for_each(|(pred, _)| {


Also here, I'd prefer a for loop

Suggested change

generics.predicates.iter().for_each(|(pred, _)| {

for (pred, _) in &generics.predicates {

flip1995 · 2020-04-15T16:47:12Z

clippy_lints/src/unintentional_unit_return.rs

+                    preds.push(pred);
+                }


Formatting

Suggested change

preds.push(pred);

}

preds.push(pred);

}

flip1995 · 2020-04-15T16:52:06Z

clippy_lints/src/utils/paths.rs

@@ -37,6 +37,7 @@ pub const FILE_TYPE: [&str; 3] = ["std", "fs", "FileType"];
 pub const FMT_ARGUMENTS_NEW_V1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
 pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"];
 pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"];
+pub const FN_MUT: [&str; 3] = ["std", "ops", "FnMut"];


This is also a LangItem

flip1995 · 2020-04-15T16:57:58Z

Sorry for taking so long for the review. I don't have an excuse, just wasn't really motivated to review PRs recently.

flip1995 · 2020-05-25T15:42:32Z

Thanks for contributing to Clippy! Sadly this PR was not updated in quite some time. If you waited on input from a reviewer, we're sorry that this fell under the radar. If you want to continue to work on this, just reopen the PR and/or ping a reviewer.

Uriopass · 2020-06-22T18:58:10Z

Since this PR is inactive and was open because of a problem I had initially I'm going to try and finish it :-)

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

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 changelog: add lint [`unit_return_expecting_ord`]

rabisg0 force-pushed the unit-for-ord branch 3 times, most recently from 5a3624b to 418179d Compare March 21, 2020 18:21

rabisg0 force-pushed the unit-for-ord branch from 418179d to e7d54ce Compare March 30, 2020 13:29

flip1995 self-requested a review March 30, 2020 17:41

flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 30, 2020

rabisg0 force-pushed the unit-for-ord branch from e7d54ce to b3c2cd0 Compare April 1, 2020 10:01

flip1995 requested changes Apr 15, 2020

View reviewed changes

flip1995 closed this May 25, 2020

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 25, 2020

Uriopass mentioned this pull request Jun 22, 2020

Reprise: new lint: Unintentional return of unit from closures expecting Ord #5737

Merged

blyxyas removed the S-inactive-closed Status: Closed due to inactivity label Oct 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new lint: Unintentional return of unit from closures expecting Ord #5348

new lint: Unintentional return of unit from closures expecting Ord #5348

rabisg0 commented Mar 21, 2020

bors commented Mar 23, 2020

bors commented Apr 8, 2020

rabisg0 commented Apr 13, 2020

flip1995 left a comment

flip1995 Apr 15, 2020

flip1995 Apr 15, 2020

flip1995 Apr 15, 2020

flip1995 Apr 15, 2020

flip1995 Apr 15, 2020

flip1995 Apr 15, 2020

flip1995 Apr 15, 2020

flip1995 Apr 15, 2020

flip1995 Apr 15, 2020

flip1995 commented Apr 15, 2020

flip1995 commented May 25, 2020

Uriopass commented Jun 22, 2020

		if_chain! {
		if let ExprKind::MethodCall(_, _, ref args) = expr.kind;

	inputs_output.iter().enumerate().for_each(\|(i, inp)\| {
	inputs_output.iter().rev().skip(1).rev().enumerate().for_each(\|(i, inp)\| {

	fn_mut_preds.iter().for_each(\|pred\| {
	for pred in &fn_mut_preds {

	generics.predicates.iter().for_each(\|(pred, _)\| {
	for (pred, _) in &generics.predicates {

new lint: Unintentional return of unit from closures expecting Ord #5348

new lint: Unintentional return of unit from closures expecting Ord #5348

Conversation

rabisg0 commented Mar 21, 2020

bors commented Mar 23, 2020

bors commented Apr 8, 2020

rabisg0 commented Apr 13, 2020

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 commented Apr 15, 2020

flip1995 commented May 25, 2020

Uriopass commented Jun 22, 2020