Detect Expressions that evaluate to Unit and warn the user #2016

zmanian · 2017-09-03T03:39:29Z

Work in progress to resolve #2011

Feedback requested.

Manishearth · 2017-09-03T04:31:33Z

clippy_lints/src/is_unit_expr.rs

+}
+}
+}
+fn is_unit_expr(expr: &Expr)->Option<Span>{


probably rustfmt the file

Manishearth

looking good so far. Probably should add a test.

Manishearth · 2017-09-03T04:32:59Z

clippy_lints/src/is_unit_expr.rs

+        },
+        ExprKind::If(_, ref then, ref else_)=>{
+            let check_then = check_last_stmt_in_block(then);
+            if let Some(ref else_) = *else_{


If there is no else block then it's always going to evaluate to ()

Manishearth · 2017-09-03T04:33:50Z

clippy_lints/src/is_unit_expr.rs

+        ExprKind::If(_, ref then, ref else_)=>{
+            let check_then = check_last_stmt_in_block(then);
+            if let Some(ref else_) = *else_{
+                let check_else = is_unit_expr(else_.deref());


you shouldn't need to call deref directly. &else_ should work here.

Manishearth · 2017-09-03T04:35:21Z

clippy_lints/src/is_unit_expr.rs

+                return Some(expr.span.clone());
+
+            } else{
+                return Some(expr.span.clone());


This else block shouldn't be there I think? return None?

Manishearth · 2017-09-03T04:35:43Z

clippy_lints/src/is_unit_expr.rs

+                if let Some(ref expr_else) = check_else{
+                    return Some(expr_else.clone());
+                }else{
+                    return Some(expr.span.clone());


return None?

Manishearth · 2017-09-03T04:35:55Z

clippy_lints/src/is_unit_expr.rs

+            else{
+              return true; 
+            }
+}


nit: newline at EOF

Manishearth · 2017-09-03T16:15:54Z

tests/ui/is_unit_expr.rs

@@ -0,0 +1,14 @@
+
+#![feature(plugin)]
+#[plugin(clippy)]


Needs an exclamation mark. The tests didn't run correctly (see the output)

Manishearth

looking good, needs more tests for the call case and also tests for false positives.

Manishearth · 2017-09-03T19:34:13Z

clippy_lints/src/is_unit_expr.rs

+                        cx,
+                        UNIT_EXPR,
+                        expr.span,
+                        "This expression assigns the Unit type ()",


"assigns" probably isn't the right word to use here for a call

Manishearth · 2017-09-03T19:34:19Z

clippy_lints/src/is_unit_expr.rs

+                        cx,
+                        UNIT_EXPR,
+                        expr.span,
+                        "This expression assigns the Unit type ()",


Remove "assigns" from the lint

Add false positive tests

Manishearth · 2017-09-03T20:45:19Z

Needs rustfmt on the tests, otherwise it's good to go.

It occurs to me that we have chances for false positives on if blocks that diverge via panics too.

Perhaps we should only lint if both the if and else are semicolon'd?

Manishearth · 2017-09-03T20:45:49Z

or, well, at worst it will warn about a needless semicolon after a diverging statement. That's fine.

Manishearth · 2017-09-03T21:01:54Z

retriggering travis

Arnavion · 2017-09-05T07:18:34Z

or, well, at worst it will warn about a needless semicolon after a diverging statement. That's fine.

Does "That's fine." mean it's fine for now and that it's planned to be fixed? Or it's by design? I like putting a semi-colon after my continues and breaks and returns :\

oli-obk · 2017-09-05T09:04:36Z

We can detect continue, break and return without troubles. It's foo() where fn foo() -> ! that's problematic.

oli-obk · 2017-09-05T09:23:00Z

retutrn and break actually don't trigger this, just continue, opening an issue

Arnavion · 2017-09-05T10:30:05Z

Ah cool. I did only hit the false positive with continue

zmanian · 2017-09-05T15:45:12Z

Oh continue is an easy fix.

oli-obk · 2017-09-05T15:47:03Z

Fixed in #2022

Manishearth · 2017-09-05T16:05:59Z

Does "That's fine." mean it's fine for now and that it's planned to be fixed?

I meant that it's fine forever.

continue was an omission, that comment was about diverging functions.

zmanian added 9 commits September 2, 2017 11:25

Initial commit of unit expr

f581aa7

Use the type from the macro

a25a172

Introduce check_stmt

9e3be6a

More initial work

2a97aad

Check method calls

d6d78cd

Fix brace indentation

e7c5825

Handle method calls

34edc3f

Improved spans for lints and support match expressions

789e78e

Switch back to manual deref

27e55c9

Manishearth reviewed Sep 3, 2017

View reviewed changes

clippy_lints/src/is_unit_expr.rs Outdated

}

}

}

fn is_unit_expr(expr: &Expr)->Option<Span>{

Copy link

Member

Manishearth Sep 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably rustfmt the file

RustFmt changes

93e78c8

Manishearth reviewed Sep 3, 2017

View reviewed changes

zmanian added 3 commits September 3, 2017 08:31

Fix false positives in assignment inside the else condition

0233d9b

early tests

33e8640

Update unit tests

436d838

Manishearth reviewed Sep 3, 2017

View reviewed changes

zmanian added 4 commits September 3, 2017 09:52

Improve the lint message

98ec865

More relevant tests

eb7955b

Don't trigger lint on break or return

1c75837

RustFmt file and tests

e0caf26

Manishearth approved these changes Sep 3, 2017

View reviewed changes

zmanian added 2 commits September 3, 2017 12:50

Remove direct call for Deref

6657d4e

Remove "assigns" from the lint

Fix all the clippy lints

8b53f22

Add false positive tests

zmanian added 2 commits September 3, 2017 13:46

Rustfmt tests

44f694d

Mention the false positive

0d9f566

Manishearth closed this Sep 3, 2017

Manishearth reopened this Sep 3, 2017

Manishearth merged commit 0d9f566 into rust-lang:master Sep 3, 2017

zmanian deleted the unit_expr branch September 3, 2017 21:15

		@@ -0,0 +1,14 @@

		#![feature(plugin)]
		#[plugin(clippy)]

Detect Expressions that evaluate to Unit and warn the user #2016

Detect Expressions that evaluate to Unit and warn the user #2016

Uh oh!

Conversation

zmanian commented Sep 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Sep 3, 2017

Uh oh!

Manishearth commented Sep 3, 2017

Uh oh!

Manishearth commented Sep 3, 2017

Uh oh!

Arnavion commented Sep 5, 2017

Uh oh!

oli-obk commented Sep 5, 2017

Uh oh!

oli-obk commented Sep 5, 2017

Uh oh!

Arnavion commented Sep 5, 2017

Uh oh!

zmanian commented Sep 5, 2017

Uh oh!

oli-obk commented Sep 5, 2017

Uh oh!

Manishearth commented Sep 5, 2017

Uh oh!

Uh oh!

zmanian commented Sep 3, 2017 •

edited

Loading