add hint to fix error for immutable ref in arg #37863
Conversation
|
error: the lock file needs to be updated but --locked was passed to prevent this |
|
@jonathandturner How to rerun checks? |
|
The errors look pretty good. r? @nikomatsakis to take a look at the implementation side I don't think you need to worry about travis, as long as it passes bors. |
There was a problem hiding this comment.
This is an awesome error message! I left some comments -- I'm particularly interested in whether we can wind up producing multiple errors for a single input with the code structured the way it is, as well as improving the message for the case where a named lifetime is used.
| @@ -970,52 +970,10 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { | |||
|
|
|||
| pub fn note_and_explain_bckerr(&self, db: &mut DiagnosticBuilder, err: BckError<'tcx>, | |||
| error_span: Span) { | |||
| let code = err.code; | |||
| let code = &err.code; | |||
| match code { | |||
There was a problem hiding this comment.
Nit: you could also do match err.code { and forego the & in the &err_foo patterns below; this would be a bit more typical style
| if let Ok(lifetime_snippet) = self.tcx.sess.codemap() | ||
| .span_to_snippet(lifetime.span) { | ||
| db.span_label(arg.ty.span, | ||
| &format!("use `{} mut {}`", |
There was a problem hiding this comment.
Nit: I think this should be &{} mut {}, not {} mut {}, so that the final message is "use &'a mut Foo, not "use 'a mut Foo". The latter seems confusing. Also, the message seems incomplete: maybe "use &{} mut {} here to make mutable"?
| } | ||
| if let Categorization::Local(local_id) = cmt.cat { | ||
| let parent = self.tcx.map.get_parent_node(local_id); | ||
| let may_be_function = match self.tcx.map.get(parent) { |
There was a problem hiding this comment.
I think you can use this funky FnLikeNode thing. Something like this instead of that big match:
use rustc::hir::map::blocks::FnLikeNode;
let opt_function =
FnLikeNode::from_node(self.tcx.map.get(parent))
.map(|fn_like| fn_like.decl());I also changed the name of may_be_function to opt_function, which I find a bit more obvious (since in Rust the type is Option, not Maybe).
| _ => None | ||
| }; | ||
|
|
||
| if let Some(function) = may_be_function { |
There was a problem hiding this comment.
Nit: variables of this type are usually called fn_decl or decl, not function, at least by me (since they are a FnDecl)
| } | ||
| } | ||
| } | ||
| if let Categorization::Local(local_id) = err.cmt.cat { |
There was a problem hiding this comment.
It seems to me that we only want to execute one of these two ifs -- but I'm not clear on why we never wind up executing both of them. For example, what would this test do?
fn foo(x: &'static i32, y: &'static i32) {
x = y;
}There was a problem hiding this comment.
Basically the first if says "if you are mutating something like x, *x, or *****x where x is a varibale of type &T" and the second if says "if you are mutating somthing like x", but since the first does not require at least one * these are not mutually exclusive, right?
There was a problem hiding this comment.
As an aside, I think it would be to pull the bodies of these ifs into helper methods with a nice name, like fn give_suggestion_to_make_referent_mutable() (for the first one) and fn give_suggestion_to_make_binding_mutable() (for the second one). Also some comments, ideally with code examples, is also helpful in these kinds of situations (or at least links to test cases) -- basically explaining "if the input program is this, we want this error"
There was a problem hiding this comment.
for x=y other error produced and note_and_explain_mutbl_error is not called, output is
1 | fn foo(x: &'static i32, y: &'static i32) {
| - first assignment to `x`
2 | x = y;
| ^^^^^ re-assignment of immutable variable
You right the ifs are not mutually exclusive, but I cannot find example executes both of them. May be I should prevent match first of them with x. Actually I'm going to support cases with local variables by first case too, but it's more complicated and now matching with *x is enough.
There was a problem hiding this comment.
example for **x
fn main() {
let mut foo = &"a".to_string();
let mut bar = &mut foo;
bar.push_str("foo2");
}
|
☔ The latest upstream changes (presumably #37676) made this pull request unmergeable. Please resolve the merge conflicts. |
mikhail-m1
force-pushed
the
mut_error
branch
from
266a348 to
67a24c2
Compare
|
@nikomatsakis I fixed nits and leave only *x case, you can find some comments in review. I don't create separate functions for now, and made cases mutually exclusive |
|
@bors r+ |
|
📌 Commit 67a24c2 has been approved by |
|
@mikhail-m1 looks great :) |
add hint to fix error for immutable ref in arg fix #36412 part of #35233 r? @jonathandturner
fix #36412 part of #35233
r? @jonathandturner