Add suggest mut method for loop #81466
Merged
+117
−9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
sasurau4
force-pushed
the
fix/enhance-sugget-mut-method-for-loop
branch
from
3024a14
to
f66f94a
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
sasurau4
force-pushed
the
fix/enhance-sugget-mut-method-for-loop
branch
from
f66f94a
to
9567192
Compare
oli-obk
reviewed
Jan 28, 2021
Comment on lines
473
to
481
| if let hir::ItemKind::Fn(_, _, body_id) = node.kind { | ||
| let body = hir.body(body_id); | ||
| if let hir::ExprKind::Block(block, _) = body.value.kind { | ||
| if let Some(expr) = block.expr { | ||
| if let hir::ExprKind::DropTemps(expr) = expr.kind { | ||
| if let hir::ExprKind::Match(expr, ..) = expr.kind { | ||
| if let hir::ExprKind::Call(_, [expr, ..]) = expr.kind { | ||
| if let hir::ExprKind::MethodCall(path_segment, ..) = expr.kind { | ||
| if let Some(hir_id) = path_segment.hir_id { |
There was a problem hiding this comment.
I think it's time to use the if_chain crate in the compiler instead of just in clippy 😆
There was a problem hiding this comment.
Heh, I did think about CCing you when I saw that xD
When playing around with it I came up with this. Still gnarly but at least it separates the actual code from taking the structure apart.
fn suggest_similar_mut_method_for_for_loop(&self, err: &mut DiagnosticBuilder<'_>) {
let hir = self.infcx.tcx.hir();
let node = hir.item(self.mir_hir_id());
use hir::{
Expr,
ExprKind::{Block, Call, DropTemps, Match, MethodCall},
};
if let hir::ItemKind::Fn(_, _, body_id) = node.kind {
if let Block(
hir::Block {
expr:
Some(Expr {
kind:
DropTemps(Expr {
kind:
Match(
Expr {
kind:
Call(
_,
[Expr {
kind: MethodCall(path_segment, ..),
hir_id,
..
}, ..],
),
..
},
..,
),
..
}),
..
}),
..
},
_,
) = hir.body(body_id).value.kind
{
let opt_suggestions = path_segment
.hir_id
.map(|path_hir_id| self.infcx.tcx.typeck(path_hir_id.owner))
.and_then(|typeck| typeck.type_dependent_def_id(*hir_id))
.and_then(|def_id| self.infcx.tcx.impl_of_method(def_id))
.map(|def_id| self.infcx.tcx.associated_items(def_id))
.map(|assoc_items| {
assoc_items
.in_definition_order()
.map(|assoc_item_def| assoc_item_def.ident)
.filter(|&ident| {
let original_method_ident = path_segment.ident;
original_method_ident != ident
&& ident
.as_str()
.starts_with(&original_method_ident.name.to_string())
})
.map(|ident| format!("{}()", ident))
});
if let Some(suggestions) = opt_suggestions {
err.span_suggestions(
path_segment.ident.span,
&format!("use mutable method"),
suggestions,
Applicability::MaybeIncorrect,
);
}
}
}
}
oli-obk
reviewed
Jan 28, 2021
Comment on lines
473
to
478
| if let hir::ItemKind::Fn(_, _, body_id) = node.kind { | ||
| let body = hir.body(body_id); | ||
| if let hir::ExprKind::Block(block, _) = body.value.kind { | ||
| if let Some(expr) = block.expr { | ||
| if let hir::ExprKind::DropTemps(expr) = expr.kind { | ||
| if let hir::ExprKind::Match(expr, ..) = expr.kind { |
There was a problem hiding this comment.
I don't understand this part. Do we need to special case this suggestion to for loops? Or do other cases already work well?
There was a problem hiding this comment.
No, we don't need to special case this suggestion in the future. But initially, this PR focuses on to suggest mutable method only for loop rhs method call.
There was a problem hiding this comment.
ok, that makes sense. Please leave a comment explaining this in the source
|
☔ The latest upstream changes (presumably #81493) made this pull request unmergeable. Please resolve the merge conflicts. |
pickfire
reviewed
Jan 30, 2021
| buzz.insert("a", Y { v: 0 }); | ||
|
|
||
| for mut t in buzz.values() { | ||
| //~^ HELP |
There was a problem hiding this comment.
How come this help is empty? This is also the first time I came across SUGGESTION.
There was a problem hiding this comment.
I use this same as src/test/ui/suggestions/suggest-ref-mut.rs. In suggest-ref-mut, the help seems available to be empty.
The SUGGESTION is introduced by https://rustc-dev-guide.rust-lang.org/tests/adding.html#error-levels.
pickfire
reviewed
Jan 30, 2021
| //~^ HELP | ||
| //~| SUGGESTION values_mut() | ||
| t.v += 1; | ||
| //~^ ERROR |
There was a problem hiding this comment.
Maybe we should have some key message here?
davidtwco
requested changes
Jan 30, 2021
| }); | ||
| err.span_suggestions( | ||
| path_segment.ident.span, | ||
| &format!("use mutable method"), |
There was a problem hiding this comment.
I'm surprised that we don't check that the methods we're suggesting are actually yielding mutable references. Is that something which is just too tricky to check here?
There was a problem hiding this comment.
This method called only here.
It seems we don't need to check.
There was a problem hiding this comment.
Could you add a comment that documents this assumption?
davidtwco
added
S-waiting-on-author
sasurau4
force-pushed
the
fix/enhance-sugget-mut-method-for-loop
branch
from
9567192
to
90c9b57
Compare
|
@bors r+ rollup |
|
📌 Commit 90c9b57 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 10, 2021
Rollup of 11 pull requests Successful merges: - rust-lang#79849 (Clarify docs regarding sleep of zero duration) - rust-lang#80438 (Add `Box::into_inner`.) - rust-lang#81466 (Add suggest mut method for loop) - rust-lang#81687 (Make Vec::split_at_spare_mut public) - rust-lang#81904 (Bump stabilization version for const int methods) - rust-lang#81909 ([compiler/rustc_typeck/src/check/expr.rs] Remove unnecessary refs in pattern matching) - rust-lang#81910 (Use format string in bootstrap panic instead of a string directly) - rust-lang#81913 (Rename HIR UnOp variants) - rust-lang#81925 (Add long explanation for E0547) - rust-lang#81926 (add suggestion to use the `async_recursion` crate) - rust-lang#81951 (Update cargo) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #49839
This PR focus on the comment case