-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve case with both anonymous lifetime parameters #43269 #43298
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! A couple of nitpicks inline.
@@ -0,0 +1,201 @@ | |||
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this file shouldn't be in the PR.
11 | fn foo(x: &mut Vec<&u8>, y: &u8) { | ||
| --- --- these references must have the same lifetime | ||
12 | x.push(y); | ||
| ^ data from `y` flows into `x` here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
--> $DIR/ex3-both-anon-regions.rs:12:12 | ||
| | ||
11 | fn foo(x: &mut Vec<&u8>, y: &u8) { | ||
| --- --- these references must have the same lifetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if having a label on only one of the arguments is good enough (but no strong opinion in any direction), given that these are also displayed on other tools, like VSCode (through RLS). I'm wondering if @nikomatsakis has an opinion about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying -- if some other tool were to hide one of these labels, would that be ok? It seems like it would be too bad, but I'm not sure what to do about it. I guess we could offer multiple labels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry is for example, a client that displays information for each span individually without the entire context of the error on hover. I feel that this is another case of conflicting requirements for the RLS and for rustc
visual output.
src/librustc/diagnostics.rs
Outdated
@@ -2026,4 +2026,5 @@ register_diagnostics! { | |||
E0495, // cannot infer an appropriate lifetime due to conflicting requirements | |||
E0566, // conflicting representation hints | |||
E0587, // conflicting packed and align representation hints | |||
E0623, // lifetime mismatch where both parameters are anonymous regions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great if you could add a full description of the new error code, but that could of course be done in a subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do it once I get in the anonymous region in Structs changes. Probably a separate PR
let def_id = self.is_suitable_anonymous_region(region).unwrap(); | ||
let node_id = self.tcx.hir.as_local_node_id(def_id).unwrap(); | ||
let ret_ty = self.tcx.type_of(def_id); | ||
match ret_ty.sty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that in all of these you're matching only one possible element on the enum, you can also do
if let (ty::TyFnDef(..), hir_map::NodeItem(it)) = (ret_ty.sty, self.tcx.hir.get(node_id)) {
if let hir::ItemFn(ref fndecl, ..) = it.node {
fndecl
.inputs
...
}
}; | ||
nested_visitor.visit_ty(&**arg); | ||
if nested_visitor.found_type.is_some() { | ||
return nested_visitor.found_type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can avoid the return
s in both branches of the if
(if you omit the ;
as well) as the if
is the last element of the closure.
} else { | ||
None | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of all the } else { None }
you can just return in the inner part of the chain (return fndecl.<...>.next()
) and have a single None
as the last line of the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if nested_visitor.found_type.is_some() {
return nested_visitor.found_type;
}
})
.next()
}
_ => None,
}
}
_ => None,
}
}
_ => None,
}
}
None
}
gives me a missing else clause error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following should be (at least close to) correct and equivalent to the code you wrote:
fn find_anon_type(&self, region: Region<'tcx>, br: &ty::BoundRegion) -> Option<&hir::Ty> {
if let Some(anon_reg) = self.is_suitable_anonymous_region(region) {
let (def_id, _) = anon_reg;
if let Some(node_id) = self.tcx.hir.as_local_node_id(def_id) {
let ret_ty = self.tcx.type_of(def_id);
if let ty::TyFnDef(_, _) = ret_ty.sty {
if let hir_map::NodeItem(it) = self.tcx.hir.get(node_id) {
if let hir::ItemFn(ref fndecl, _, _, _, _, _) = it.node {
return fndecl
.inputs
.iter()
.filter_map(|arg| {
let mut nested_visitor = FindNestedTypeVisitor {
infcx: &self,
hir_map: &self.tcx.hir,
bound_region: *br,
found_type: None,
};
nested_visitor.visit_ty(&**arg);
if nested_visitor.found_type.is_some() {
nested_visitor.found_type
} else {
None
}
}).next();
}
}
}
}
}
None
}
Notice the fall through for all the else conditions, with only one implicit return at the end.
_ => return false, // inapplicable | ||
}; | ||
|
||
// Determine whether the sub and sup consist of both anonymous (elided) regions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be written a bit more succinctly:
// Determine whether the sub and sup consist of both anonymous (elided) regions.
if let (Some(br1), Some(br2)) = (self.is_anonymous_region(sup),
self.is_anonymous_region(sub)) {
if let (Some(ty1), Some(t2)) = (self.find_anon_type(sup, &br1),
self.find_anon_type(sub, &br2)) {
if let (Some((anon_arg1, _, _, _)), Some((anon_arg2, _, _, _))) =
(self.find_arg_with_anonymous_region(sup, sup),
self.find_arg_with_anonymous_region(sub, sub))
{
let span_label = if let (Some(var1), Some(var2)) = (anon_arg1.pat.simple_name(),
anon_arg2.pat.simple_name()) {
format!("data from `{}` flows into `{}` here", var1, var2))
} else {
"data flows here".to_string()
};
struct_span_err!(self.tcx.sess, span, E0623, "lifetime mismatch")
.span_label(ty1.span,
format!("these references must have the same lifetime"))
.span_label(ty2.span, format!(""))
err.span_label(span, span_label)
.emit();
return true;
}
}
}
false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
sup_origin, sup_r); | ||
} | ||
} | ||
if !self.try_report_named_anon_conflict(&error) && !self.try_report_anon_anon_conflict(&error){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(&error) {
sub_origin, sub_r, | ||
sup_origin, sup_r) => { | ||
self.report_sub_sup_conflict(var_origin, | ||
sub_origin, sub_r, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation missing two spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix indentation above.
let span_label_var1 = if let Some(simple_name) = anon_arg1.pat.simple_name() { | ||
format!("from `{}`", simple_name) | ||
} else { | ||
format!("data flows here") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be "" so the error would be "data flows into x
here"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of a complex pattern, the error would not show any of the parameters but just data flows here
. Not sure if that's what you meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the code is right now, in that case the label's text would be data data flows here flows here
(also notice the double space between the last flows
and here
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make tests for the four cases:
- source and dest both have simple names
- source has a simple name
- destination has a simple name
- no simple names
match self.infcx.tcx.named_region_map.defs.get(&lifetime.id) { | ||
// the lifetime of the TyRptr! | ||
Some(&rl::Region::LateBoundAnon(debuijn_index, anon_index)) => { | ||
if debuijn_index.depth == 1 && anon_index == br_index { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo: should be debruijn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I generally like the approach here, but we might also want to come up with a strategy for structs with anonymous lifetimes, and the code could be prettier. |
gives the error
The @nikomatsakis I'm not sure whether the error message looks a bit too long. |
bc177a9
to
9f82c94
Compare
@arielb1 @estebank @nikomatsakis updated PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems close, couple of nitpicks.
} | ||
} | ||
if !self.try_report_named_anon_conflict(&error) && | ||
!self.try_report_anon_anon_conflict(&error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix indentation.
|
||
let id = free_region.scope; | ||
let node_id = self.tcx.hir.as_local_node_id(id).unwrap(); | ||
let body_id = self.tcx.hir.maybe_body_owned_by(node_id).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid unwrap()
in the compiler unless you are 110% sure that there's no way it will ever panic. On cases where you're 109% sure and below, either use ?
to bubble up or use match
/if let
to handle the error with a bug!()
message. If an error case is expected but doesn't need to be handled, just use if let
to guard your new code against it. You can even do it with multiple variables at the same time:
let hir = self.tcx.hir;
if let (Some(node_id), Some(body_id)) = (hir.maybe_body_owned_by(node_id),
hir.as_local_node_id(id)) {
...
}
} else { | ||
None | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment above.
9f82c94
to
b384a4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some nits. Once @estebank is happy, I am also happy (i.e., r=me modulo the nits here and @estebank's approval).
(I see now that GH says it was showing me outdated commits; grr.... hopefully my comments are still relevant.)
With respect to the comment from @arielb1, the plan that @gaurikholkar and I had discussed was to handle cases like Foo<'a, ...>
in a separate follow-up PR, and just focus here on the simpler &T
case.
|
||
// The visitor captures the corresponding `hir::Ty` of the | ||
// anonymous region. | ||
struct FindNestedTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to, in this comment, give an example of the error and show the role this visitor plays, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. "Consider a case where we have fn foo(x: &mut Vec<&u8>, y: &u8) { x.push(y); }
. This would lead to a conflict between the two anonymous lifetimes for &u8
in x
and y
respectively. This visitor would be invoked twice, once for each lifetime, and would walk the types like &mut Vec<&u8>
and &u8
looking for the HIR where that lifetime appears. This allows us to highlight the specific part of the type."
struct FindNestedTypeVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> { | ||
infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, | ||
hir_map: &'a hir::map::Map<'gcx>, | ||
bound_region: ty::BoundRegion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly, might be nice to add some (brief) comments to the fields, e.g. explaining that this is the bound-region we are looking for. These can reference your example snippet above.
infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, | ||
hir_map: &'a hir::map::Map<'gcx>, | ||
bound_region: ty::BoundRegion, | ||
found_type: Option<&'gcx hir::Ty>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and that this field (I presume) is an output of the visitor
// This function calls the `visit_ty` method for the parameters | ||
// corresponding to the anonymous regions. The `nested_visitor.found_type` | ||
// contains the anonymous type. | ||
pub fn find_anon_type(&self, region: Region<'tcx>, br: &ty::BoundRegion) -> Option<&hir::Ty> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this comment seems to describe the implementation. I'd make a doc comment (using ///
comments) and describe the interface to this function -- what does it take as input and return? What is it used for as part of the error reporting process?
The stuff in this comment can be moved inside the fn body, since the caller doesn't care about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, is this called from outside the file? If not, it need not be pub
.
// corresponding to the anonymous regions. The `nested_visitor.found_type` | ||
// contains the anonymous type. | ||
pub fn find_anon_type(&self, region: Region<'tcx>, br: &ty::BoundRegion) -> Option<&hir::Ty> { | ||
if self.is_suitable_anonymous_region(region).is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Use if let Some(def_id) = self.is_suitable_anonymous_region(region) {
or match
instead of calling is_some()
and then unwrap()
} | ||
} | ||
|
||
pub fn try_report_anon_anon_conflict(&self, error: &RegionResolutionError<'tcx>) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to have a comment explaining the kinds of errors this handles (give a concrete example). For that matter, I would try to move this function to the top of the file -- it's the "main entry point" for your reader. You can move the visitor struct etc, which are implementation details, down to the bottom of the file. Then you can also have the comments in those other stuff reference the example you introduce here.
.span_label(span, | ||
format!("data{}flows{}here", span_label_var1, span_label_var2)) | ||
.emit(); | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also try to "un-nest" here, so that you have return false
bailing early, and just focus on the main path.
false | ||
} | ||
|
||
pub fn is_anonymous_region(&self, region: Region<'tcx>) -> Option<ty::BoundRegion> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used from outside the file? If not, make it not be pub
.
match self.tcx.hir.find(node_id) { | ||
Some(hir_map::NodeItem(..)) | | ||
Some(hir_map::NodeTraitItem(..)) => { | ||
// proceed ahead // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change this to something like: "// Success -- proceed to return Some
below"
// since the signature must match the trait. | ||
// | ||
// FIXME(#42706) -- in some cases, we could do better here. | ||
return None; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this function feels familiar -- I think we discussed this already, can it be combined with code from your other PR?
@nikomatsakis @estebank updated PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. I'm happy with the overall state of the PR, but have some minor comments still. As soon as you're done with the style changes, I'l r+.
} | ||
} | ||
/* | ||
fn is_suitable_anonymous_region(&self, region: Region<'tcx>) -> Option<ty::BoundRegion> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented out code.
} else { | ||
None | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following should be (at least close to) correct and equivalent to the code you wrote:
fn find_anon_type(&self, region: Region<'tcx>, br: &ty::BoundRegion) -> Option<&hir::Ty> {
if let Some(anon_reg) = self.is_suitable_anonymous_region(region) {
let (def_id, _) = anon_reg;
if let Some(node_id) = self.tcx.hir.as_local_node_id(def_id) {
let ret_ty = self.tcx.type_of(def_id);
if let ty::TyFnDef(_, _) = ret_ty.sty {
if let hir_map::NodeItem(it) = self.tcx.hir.get(node_id) {
if let hir::ItemFn(ref fndecl, _, _, _, _, _) = it.node {
return fndecl
.inputs
.iter()
.filter_map(|arg| {
let mut nested_visitor = FindNestedTypeVisitor {
infcx: &self,
hir_map: &self.tcx.hir,
bound_region: *br,
found_type: None,
};
nested_visitor.visit_ty(&**arg);
if nested_visitor.found_type.is_some() {
nested_visitor.found_type
} else {
None
}
}).next();
}
}
}
}
}
None
}
Notice the fall through for all the else conditions, with only one implicit return at the end.
sub_origin, sub_r, | ||
sup_origin, sup_r) => { | ||
self.report_sub_sup_conflict(var_origin, | ||
sub_origin, sub_r, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix indentation above.
region: Region<'tcx>) | ||
-> Option<(DefId, ty::BoundRegion)> { | ||
match *region { | ||
ty::ReFree(ref free_region) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When match
ing on only one arm it is a good idea to use if let
instead, as it communicates intent to not deal with the other possible matches, as well as yielding fewer lines of code by not having to add _ => ()
.
@nikomatsakis updated |
☔ The latest upstream changes (presumably #43443) made this pull request unmergeable. Please resolve the merge conflicts. |
Awesome! There's a merge conflict against master HEAD, but it shouldn't be too big of a problem. Do you know how to squash commits and rebase against latest master? I find it easier to rebase my work against master instead of merging, and rebasing one commit instead of multiple, which has the benefit of a slightly cleaner log. Regardless, fix the merge conflict and I'll approve 👍 |
3d4d2a4
to
2da6798
Compare
@estebank updated. |
@gaurikholkar two small
|
2da6798
to
4fb1808
Compare
@estebank done |
@bors r+ |
📌 Commit 4fb1808 has been approved by |
improve case with both anonymous lifetime parameters #43269 This is a fix to #43269. Sample output message- ``` error[E0623]: lifetime mismatch --> $DIR/ex3-both-anon-regions.rs:12:12 | 11 | fn foo(x: &mut Vec<&u8>, y: &u8) { | --- --- these references must have the same lifetime 12 | x.push(y); | ^ data from `y` flows into `x` here error: aborting due to 2 previous errors ``` r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
This is a fix to #43269.
Sample output message-
r? @nikomatsakis