Fixes #16109: For non-sized types, use reborrow in suggestions to remove while-let loops #16100
Conversation
rustbot
added
the
S-waiting-on-review
|
No changes for bbadb72 |
|
The suggestion used to be to borrow/reborrow before it was changed to |
Great, thanks, I can rework the PR to do a reborrow only when needed. |
sergiogiro
force-pushed
the
sergio-clippy/avoid-by-ref-in-suggestion-for-while-replacement
branch
from
9638f9c to
1c8b099
Compare
sergiogiro
force-pushed
the
sergio-clippy/avoid-by-ref-in-suggestion-for-while-replacement
branch
from
1c8b099 to
f4496bf
Compare
|
Thanks for the advice. Now the PR just changes the behaviour for non-sized types, and adds a couple tests accordingly. |
sergiogiro
changed the title
| fn make_iterator_snippet<'tcx>(cx: &LateContext<'tcx>, iter_expr: &Expr<'tcx>, iterator: impl Into<String>) -> String { | ||
| let iterator = iterator.into(); | ||
| let ty = cx.typeck_results().expr_ty(iter_expr); | ||
|
|
||
| if let ty::Ref(_, inner_ty, _) = ty.kind() | ||
| && !inner_ty.is_sized(cx.tcx, cx.typing_env()) | ||
| { | ||
| return format!("&mut *{iterator}"); | ||
| } | ||
|
|
||
| format!("{iterator}.by_ref()") | ||
| } |
There was a problem hiding this comment.
This won't work if auto-deref is used for the next call. A simple example is:
struct S<T>(T);
impl<T> core::ops::Deref for S<T> {
type Target = T;
fn deref(&self) -> &Self::Target { &self.0 }
}
impl<T> core::ops::DerefMut for S<T> {
fn deref_mut(&mut self) -> &mut Self::Target { &mut self.0 }
}
fn f(mut x: S<S<&mut dyn Iterator<Item = u32>>>) {
while let Some(_) = x.next() {}
// suggestion: `for _ in &mut **x {}`
}What you'll need to do is walk the adjustments to find the last deref and use that type for the sized check and suggest applying all the derefs manually.
if let Some((n, adjust)) = cx
.typeck_results()
.expr_adjustments(e)
.iter()
.take_while(|x| matches!(x.kind, Adjust::Deref(_)))
.enumerate()
.last()
&& !adjust.target.is_sized(cx.tcx, cx.typing_env())
{
format!("&mut {:*<n$}{iterator}", '*', n = n + 1)
} else {
// by_ref
}There will always be at least one deref adjustment on the expression since the receiver type on Iterator::next is a reference.
There was a problem hiding this comment.
Thank you so much for the help, doing it this way now.
| make_iterator_snippet(cx, iter_expr, iterator) | ||
| } else { | ||
| "" | ||
| iterator.to_string() |
There was a problem hiding this comment.
Yes, thank you, changed it now.
| /// Constructs the transformed iterator expression for the suggestion. | ||
| /// Returns `iterator.by_ref()` unless the iterator type is a reference to an unsized type, | ||
| /// in which case it returns `&mut *iterator`. | ||
| fn make_iterator_snippet<'tcx>(cx: &LateContext<'tcx>, iter_expr: &Expr<'tcx>, iterator: impl Into<String>) -> String { |
There was a problem hiding this comment.
Just take &str rather than Into<String>.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
Thank you so much for the guidance, it's been educative and a lot of fun to work on this issue. I added the case suggested in the PR comment as test, plus some other tests to check the intended behavior in detail. Happy to delete tests if you think they're too many. @rustbot ready |
rustbot
added
S-waiting-on-review
3 participants
Fixes #16019 by reborrowing, instead of using by_ref, when the iterator expression is not
Sized.