Skip to content

Fix false positive for unit_arg lint #6601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 25, 2021
Merged

Fix false positive for unit_arg lint #6601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c347d3
Fix false positive for unit_arg lint
mdm Jan 17, 2021
eb476c6
Split up tests for unit arg expressions
mdm Jan 18, 2021
9fe9d94
Don't lint unit args if expression kind is path
mdm Feb 24, 2021
cbe6eec
Add original test case from issue
mdm Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion clippy_lints/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,7 +955,10 @@ impl<'tcx> LateLintPass<'tcx> for UnitArg {
.iter()
.filter(|arg| {
if is_unit(cx.typeck_results().expr_ty(arg)) && !is_unit_literal(arg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check the TypeFlags to see if the type was resolved from a variable? Maybe expr_ty(arg).needs_subst().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, for the slow response. I missed your comment regarding the type flags. Checking needs_subst() does not seem to have the desired effect. The following example passes linting with my implemention, but running needs_subst() on the type for the argument of foo returns false:

    let arg = if true {
        1;
    };
    foo(arg);

I'm not familiar with the compiler internals. I looked at the definition of the various type flags, but did not really understand many of them. Do you have any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it some more, I guess you meant type variables (as in the example in issue #6447). I will test that again later today.

Copy link
Contributor

@camsteffen camsteffen Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes I'm hoping we can detect type variables by looking at the TypeFlags which is readable through TypeFoldable (that's where needs_subst is). Another method in there that might be useful is has_projections which means a type with <X as Y>::Z. I am guessing a bit here as I don't completely understand when those different flags apply.

I'm afraid this might be even more complicated - we may have to detect "does the type variable reference a type that is defined outside of the impl block?".

Copy link
Contributor Author

@mdm mdm Feb 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need a different approach here. Using the example from issue #6447 I checked all the type flags and they are all false. So we might not get any help there.
One problem might be that the original lint looks at the type of the expression passed as the argument and not at the type of the argument from the function definition.
I'll investigate this further, but if you have additional ideas I would be grateful.

Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking on this again, I think we should simply not lint if matches(arg.kind, ExprKind::Path). This will not lint if arg is a local variable.

It may be bad style to have a variable with unit as the type, but I think that should be linted upon declaration of the variable (not in this lint).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I changed my implementation to check for ExprKind::Path.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to send you for a loop there.

!matches!(&arg.kind, ExprKind::Match(.., MatchSource::TryDesugar))
!matches!(
&arg.kind,
ExprKind::Match(.., MatchSource::TryDesugar) | ExprKind::Path(..)
)
} else {
false
}
Expand Down
30 changes: 29 additions & 1 deletion tests/ui/unit_arg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,30 @@ impl Bar {
}
}

fn baz<T: Debug>(t: T) {
foo(t);
}

trait Tr {
type Args;
fn do_it(args: Self::Args);
}

struct A;
impl Tr for A {
type Args = ();
fn do_it(_: Self::Args) {}
}

struct B;
impl Tr for B {
type Args = <A as Tr>::Args;

fn do_it(args: Self::Args) {
A::do_it(args)
}
}

fn bad() {
foo({
1;
Expand Down Expand Up @@ -59,7 +83,7 @@ fn bad() {
None.or(Some(foo(2)));
// in this case, the suggestion can be inlined, no need for a surrounding block
// foo(()); foo(()) instead of { foo(()); foo(()) }
foo(foo(()))
foo(foo(()));
}

fn ok() {
Expand All @@ -71,6 +95,10 @@ fn ok() {
b.bar({ 1 });
b.bar(());
question_mark();
let named_unit_arg = ();
foo(named_unit_arg);
baz(());
B::do_it(());
}

fn question_mark() -> Result<(), ()> {
Expand Down
24 changes: 12 additions & 12 deletions tests/ui/unit_arg.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: passing a unit value to a function
--> $DIR/unit_arg.rs:31:5
--> $DIR/unit_arg.rs:55:5
|
LL | / foo({
LL | | 1;
Expand All @@ -20,7 +20,7 @@ LL | foo(());
|

error: passing a unit value to a function
--> $DIR/unit_arg.rs:34:5
--> $DIR/unit_arg.rs:58:5
|
LL | foo(foo(1));
| ^^^^^^^^^^^
Expand All @@ -32,7 +32,7 @@ LL | foo(());
|

error: passing a unit value to a function
--> $DIR/unit_arg.rs:35:5
--> $DIR/unit_arg.rs:59:5
|
LL | / foo({
LL | | foo(1);
Expand All @@ -54,7 +54,7 @@ LL | foo(());
|

error: passing a unit value to a function
--> $DIR/unit_arg.rs:40:5
--> $DIR/unit_arg.rs:64:5
|
LL | / b.bar({
LL | | 1;
Expand All @@ -74,7 +74,7 @@ LL | b.bar(());
|

error: passing unit values to a function
--> $DIR/unit_arg.rs:43:5
--> $DIR/unit_arg.rs:67:5
|
LL | taking_multiple_units(foo(0), foo(1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -87,7 +87,7 @@ LL | taking_multiple_units((), ());
|

error: passing unit values to a function
--> $DIR/unit_arg.rs:44:5
--> $DIR/unit_arg.rs:68:5
|
LL | / taking_multiple_units(foo(0), {
LL | | foo(1);
Expand All @@ -110,7 +110,7 @@ LL | taking_multiple_units((), ());
|

error: passing unit values to a function
--> $DIR/unit_arg.rs:48:5
--> $DIR/unit_arg.rs:72:5
|
LL | / taking_multiple_units(
LL | | {
Expand Down Expand Up @@ -140,7 +140,7 @@ LL | foo(2);
...

error: passing a unit value to a function
--> $DIR/unit_arg.rs:59:13
--> $DIR/unit_arg.rs:83:13
|
LL | None.or(Some(foo(2)));
| ^^^^^^^^^^^^
Expand All @@ -154,19 +154,19 @@ LL | });
|

error: passing a unit value to a function
--> $DIR/unit_arg.rs:62:5
--> $DIR/unit_arg.rs:86:5
|
LL | foo(foo(()))
LL | foo(foo(()));
| ^^^^^^^^^^^^
|
help: move the expression in front of the call and replace it with the unit literal `()`
|
LL | foo(());
LL | foo(())
LL | foo(());
|

error: passing a unit value to a function
--> $DIR/unit_arg.rs:95:5
--> $DIR/unit_arg.rs:123:5
|
LL | Some(foo(1))
| ^^^^^^^^^^^^
Expand Down