Fix false positive for unit_arg lint #6601
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
|
I saw that the check for the size of the .stderr file failed. What would be the best way to resolve this? |
|
How about splitting the test file into two? |
| @@ -955,7 +955,16 @@ impl<'tcx> LateLintPass<'tcx> for UnitArg { | |||
| .iter() | |||
| .filter(|arg| { | |||
| if is_unit(cx.typeck_results().expr_ty(arg)) && !is_unit_literal(arg) { | |||
There was a problem hiding this comment.
Can you check the TypeFlags to see if the type was resolved from a variable? Maybe expr_ty(arg).needs_subst().
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Ok, I changed my implementation to check for ExprKind::Path.
There was a problem hiding this comment.
Sorry to send you for a loop there.
|
@camsteffen do you wanna handle review for this? I forgot this was in my queue. @bors delegate=camsteffen |
|
✌️ @camsteffen can now approve this pull request |
|
okay |
|
The fix looks good. Please add a test like the code in the bug report to give us better security for future enhancements. |
|
I added the function |
|
The test you added is good. Another test like the one in the bug report would be help us make sure we continue to handle that case properly in the future. |
|
Thanks! @bors r+ |
|
📌 Commit cbe6eec has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #6447
To avoid false positives don't complain about unit args when they come from a path expression, e.g. a local variable.
Note: This is my first contribution to Clippy, so I might have messed up somewhere. Any feedback is welcome and I'm happy to work out any kinks.
changelog: Do not lint unit arguments when they come from a path expression.