add extra byref checking for the guard's local
#11468
Conversation
|
r? @blyxyas (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
|
||
| for pat in *pats { | ||
| if let PatKind::Binding(annot, pat_id, ..) = &pat.kind | ||
| && matches!(annot.0, rustc_ast::ByRef::No) |
There was a problem hiding this comment.
Would it make sense to have this ByRef check in the get_pat_binding function, instead of duplicating the code for finding the right binding in a new function? I.e. having it here:
rust-clippy/clippy_lints/src/matches/redundant_guards.rs
Lines 89 to 90 in 415ba21
There was a problem hiding this comment.
Thanks. This is better.
| @@ -80,6 +81,37 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) { | |||
| } | |||
| } | |||
|
|
|||
| fn is_the_local_binded_by_value_in_pat(local: &Expr<'_>, arm: &Arm<'_>) -> bool { | |||
There was a problem hiding this comment.
This name probably should be more illustrative.
There was a problem hiding this comment.
Thanks, this function is removed now.
tests/ui/redundant_guards.rs
Outdated
There was a problem hiding this comment.
Could you add some more tests? Like 2 simpler tests that are not specifically from #11465, checking that everything still works.
There was a problem hiding this comment.
Thanks, a few tests are added.
kiscad
changed the title
kiscad
changed the title
byref checking for the guard's local
|
Instead of just not linting any pattern with a Some(ref x) if let &1 = x => {},Clippy could remove the reference from this I'm pretty sure this is the case for all arguments where |
| multiple_bindings = true; | ||
| return false; | ||
| is_byref = matches!(bind_annot.0, rustc_ast::ByRef::Yes); | ||
| if span.replace(pat.span).is_some() { |
There was a problem hiding this comment.
It seems like span is set to None and not modified anytime before this if statement. Could this return true? Seeing the replace documentation, that .is_some would evaluate if before replacing, it had any value. But I don't see it having any other value than None. 🤔 It has been here since the very start.
@Centri3, is there a reason for this if statement?
There was a problem hiding this comment.
It gets set to Some(_) by the replace call and since it returns the old option, the condition will be true when it reaches the replace call for the 2nd time or any other time after the first replace call (ie when the pattern is found twice, for stuff like let Ok(v) | Err(v) I imagine?).
I got confused by this before when I looked at the code, maybe it could be written clearer
There was a problem hiding this comment.
How about the code below?
if span.is_some() {
multiple_bindings = true;
return false;
} else {
span.insert(pat.span);
}There was a problem hiding this comment.
No, but a comment linking to @y21's comment would be really helpful for future reference would help future reviewers / contributors?
| cx: &LateContext<'tcx>, | ||
| guard_expr: &Expr<'_>, | ||
| outer_arm: &Arm<'tcx>, | ||
| ) -> Option<(Span, bool, bool)> { |
There was a problem hiding this comment.
Probably a good idea to write a comment describing what are these 2 bools / what are their function
There was a problem hiding this comment.
I just made a custom type PatBindingInfo for the return type.
tests/ui/redundant_guards.fixed
Outdated
| B { ref c, .. } if c == &1 => {}, | ||
| B { ref c, .. } if let &1 = c => {}, | ||
| B { ref c, .. } if matches!(c, &1) => {}, |
There was a problem hiding this comment.
Shouldn't these have the same treatment as the first match c? Is this intended because it's from a struct?
There was a problem hiding this comment.
Yes. These cases are handled now.
| cx: &LateContext<'tcx>, | ||
| guard_expr: &Expr<'_>, | ||
| outer_arm: &Arm<'tcx>, | ||
| ) -> Option<PatBindingInfo> { |
There was a problem hiding this comment.
Good idea to abstract this return type! 🤠
fix typo Co-authored-by: Alejandra González <blyxyas@gmail.com>
|
@bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
changelog: [
redundant_guards]: Now checks if the variable is bound usingrefbefore linting.The lint should not be emitted, when the local variable is bind by-ref in the pattern.
fixes #11465