Add suggest mut method for loop#81466
Merged
bors merged 2 commits intorust-lang:masterfrom Feb 10, 2021
Merged
Conversation
Contributor
|
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
Collaborator
|
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
Contributor
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 😆
Contributor
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
Contributor
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?
Contributor
Author
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.
Contributor
There was a problem hiding this comment.
ok, that makes sense. Please leave a comment explaining this in the source
Collaborator
|
☔ The latest upstream changes (presumably #81493) made this pull request unmergeable. Please resolve the merge conflicts. |
pickfire
reviewed
Jan 30, 2021
Contributor
There was a problem hiding this comment.
How come this help is empty? This is also the first time I came across SUGGESTION.
Contributor
Author
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
Contributor
There was a problem hiding this comment.
Maybe we should have some key message here?
davidtwco
requested changes
Jan 30, 2021
Member
davidtwco
left a comment
davidtwco
left a comment
There was a problem hiding this comment.
Implementation looks appropriate, I'd like to see the comments from other reviewers addressed, and, if feasible, an attempt to check that the method we're suggesting does produce mutable references. r=me after that.
Member
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?
Contributor
Author
There was a problem hiding this comment.
This method called only here.
It seems we don't need to check.
Member
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
Contributor
|
@bors r+ rollup |
Collaborator
|
📌 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
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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