fix false positive in Issue/12098 because lack of consideration of mutable caller #12650
Conversation
rustbot
added
the
S-waiting-on-review
|
Hey @GuillaumeGomez if you have the time, would you mind taking a look at this PR? :) @cocodery the CI is currently still red, can you take a look at it? :D |
| // skip lint | ||
| return true; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I'm surprised rustfmt isn't complaining about this colon. Oh well, if it's not complaining... :)
There was a problem hiding this comment.
The semicolon might be needed, if the if statement has a type other than (). rustfmt doesn't use type information for formatting, AFAIK. Therefore, it cannot safely remove this one.
We could build a Clippy lint for this. We already have the counter part (and one of my favorite lints): clippy::semicolon_if_nothing_returned
|
This PR is on my todo list for this week. Sorry for the delay, I was sick last week 😅 |
| fn is_caller_or_fields_changed(cx: &LateContext<'_>, body: &Expr<'_>, caller: &Expr<'_>) -> bool { | ||
| if let ExprKind::Block(block, ..) = body.kind { | ||
| for stmt in block.stmts { | ||
| if let StmtKind::Semi(e) = stmt.kind { |
There was a problem hiding this comment.
It would be better to use a visitor instead, as this might miss nested expressions. That would also check the block expression at the end.
As an example, this if expression is not a statement, but stored in block.expr. Even if it was a statement, it might be missed due to the nesting. A visitor would ensure nested and complete traversal :)
{
if <cond> {
test.mut_this = false;
}
}Maybe for_each_local_assignment might be exactly what we need here
There was a problem hiding this comment.
That's one exact case. However, the for_each_local_assignment is not easy to use in this context, so I write a recursive function to detect each assignment in the block and subblocks, and the test file is modified.
|
Sorry for the delay, this PR is on my todo list for this weekend :) |
| fn walk_blocks(cx: &LateContext<'_>, block: &Block<'_>, caller: &Expr<'_>) -> bool { | ||
| for stmt in block.stmts { | ||
| let changed = match stmt.kind { | ||
| StmtKind::Expr(e) | StmtKind::Semi(e) => match e.kind { | ||
| ExprKind::Assign(assignee, _, _) | ExprKind::AssignOp(_, assignee, _) => { | ||
| !can_mut_borrow_both(cx, caller, assignee) | ||
| }, | ||
| ExprKind::Block(block, ..) => walk_blocks(cx, block, caller), | ||
| _ => false, | ||
| }, | ||
| _ => false, | ||
| }; | ||
| if changed { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm pretty sure, that this function will not work for expressions that nest blocks, like if expressions loops etc. I would highly recommend to use for_each_expr over implmeneting a custom recursion.
cocodery
force-pushed
the
issue/12098
branch
2 times, most recently
from
0dfd85a to
c8622dd
Compare
if immutbale -> lint delete redudant clone if mutable -> lint check whether clone is needed
|
Looks good to me, thank you for fixing this =^.^= Roses are Red, |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes Issue#12098
In issue#12098, the former code doesn't consider the caller for clone is mutable, and suggests to delete clone function.
In this change, we first get the inner caller requests for clone,
and if it's immutable, the following code will suggest deleting clone.
If it's mutable, the loop will check whether a borrow check violation exists,
if exists, the lint should not execute, and the function will directly return;
otherwise, the following code will handle this.
changelog: [
clippy::unnecessary_to_owned]: fix false positive