Rework suggestion generation of unit_arg lint

[flip1995]

Found this bug while running cargo fix --clippy on quite a big codebase:

This would replace something like:

Some(fn_that_actually_does_something(&a, b))

with

Some(())

which obviously suppresses side effects.

Since pretty much every expression could have side effects, I think making this suggestion MaybeIncorrect is the best thing to do here.

A correct suggestion would be:

fn_that_actually_does_something(&a, b);
Some(())

Somehow the suggestion is not correctly applied to the arguments, when more than one argument is a unit value. I have to look into this a little more, though.

changelog: Fixes suggestion of unit_arg lint, so that it suggests semantic equivalent code

Fixes #4741

[flip1995]

Building a well formatted suggestion is nearly impossible (at least I don't know how to do it), so this part has to be done by a rustfmt run afterwards.

If the expression that got moved out of the function call has no effect, the clippy::no_effect lint should handle this.

[flip1995]

Well multipart suggestions are not autofixable, with more than one suggestion rust-lang/rustfix#162, rust-lang/rust#53934

Let's try something else.

[flip1995]

Well that for every arg the help message is displayed once is not optimal. But it is still an improvement to before, where for every arg the lint message was emitted once.

[flip1995]

On rewriting the suggestion of this lint, I realized a few things:

1. When you write an expression, that returns the unit value, it rarely can be replaced with just () with the same semantic meaning. Most of the time it is probably a function or something similar, that has side effects in one way or another.
2. This lint is in the complexity group. This would make sense if you could just replace the expression with (), but this is rarely the case IMO (see 1.). I see this lint more in the style group.
3. From a code style perspective, I'm not really sure, if this

   foo(a, b, c);
   bar(());

   is better than

   bar(foo(a, b, c));

   So I have doubts, if this lint even makes sense.
4. The lint documentation talks about expressions like

   foo({
       bar();
       baz();
   });

   And that the last semicolon in the block was inserted by accident. If that's the case, I think that the Rust type system will catch most of these cases. The cases where you can pass a () instead of any other type could be caught by Clippy though. We would need to check for block expressions in function arguments and if the last expression would return something other than () if the semicolon was removed. This suggestion would be at most MaybeIncorrect though.

Thoughts?

[bors]

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

[bors]

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

[basil-cow]

Correct me if I am wrong, the only context where you reasonably have a () as an input type is generics, but as you said, most of errors sending () as an input would be caught by the type system.
I would argue that

foo(a, b, c);
bar(());

is much better from the "understanding what's going on" standpoint. When I see f(g()) in code I really expect output of g to determine effects of f.
Overall I think we should make this a style lint and make it MachineApplicable in case it's not a block (we can possibly make it it MachineApplicable if its a block not in a generic argument position, but that might be too complicated), as you suggested.

[flip1995]

Overall I think we should make this a style lint and make it MachineApplicable in case it's not a block

I think we can make this MachineApplicable in any case (as it is implemented in this PR), since swapping out an unit type Expr with the unit type itself is semantically the same.

[basil-cow]

I can easily see someone calling a log function with an extra semicolon in a block argument. I wouldn't want rustfix to silently move it out, I would rather leave it as a warning for a programmer to deal with.

[flip1995]

Hm, good point. But if that would be the case it would've logged () before.

What do you think of a second suggestion to remove the semicolon from the last statement in a block, in addition to moving the block out? That way, this wouldn't be applied by rustfix, since there are 2 different suggestions.

[basil-cow]

That sounds good if that is how rustfix works. So in case you have more than one suggestion (even if only one of them is machine applicable), it does nothing?

[flip1995]

even if only one of them is machine applicable

Yeah, since rustfix won't decide which suggestion to apply, if multiple fixes are possible.

[bors]

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

[phansch]

Hmm, so the {} is an expression (a block) that returns ()? Shouldn't the lint still provide the old suggestion here, instead of the two new ones? I don't think there's any point in moving an empty block expression out of the function args?

[flip1995]

This would be handled by the no_effect lint afterwards I think. But special casing empty blocks would definitely make sense here. I'll add this to the suggestion generation.

[flip1995]

@bors treeclosed-

[flip1995]

r? @phansch I finally applied your review comment about empty blocks, see a9cde3a

[phansch]

LGTM!

@bors r+

[bors]

📌 Commit 77dd0ea has been approved by phansch

[bors]

⌛ Testing commit 77dd0ea with merge 9fdcb13...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing 9fdcb13 to master...