Skip to content

Adding E0623 for structs #43700

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

Merged
merged 8 commits into from
Aug 25, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
232 changes: 168 additions & 64 deletions src/librustc/infer/error_reporting/anon_anon_conflict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,65 +27,86 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
// { x.push(y); }.
// The example gives
// fn foo(x: &mut Vec<&u8>, y: &u8) {
// --- --- these references must have the same lifetime
// --- --- these references are declared with different lifetimes...
// x.push(y);
// ^ data from `y` flows into `x` here
// It will later be extended to trait objects and structs.
// ^ ...but data from `y` flows into `x` here
// It has been extended for the case of structs too.
// Consider the example
// struct Ref<'a> { x: &'a u32 }
// fn foo(mut x: Vec<Ref>, y: Ref) {
// --- --- these structs are declared with different lifetimes...
// x.push(y);
// ^ ...but data from `y` flows into `x` here
// }
// It will later be extended to trait objects.
pub fn try_report_anon_anon_conflict(&self, error: &RegionResolutionError<'tcx>) -> bool {

let (span, sub, sup) = match *error {
ConcreteFailure(ref origin, sub, sup) => (origin.span(), sub, sup),
_ => return false, // inapplicable
};

// Determine whether the sub and sup consist of both anonymous (elided) regions.
let (ty1, ty2) = if self.is_suitable_anonymous_region(sup).is_some() &&
self.is_suitable_anonymous_region(sub).is_some() {
if let (Some(anon_reg1), Some(anon_reg2)) =
(self.is_suitable_anonymous_region(sup), self.is_suitable_anonymous_region(sub)) {
let ((_, br1), (_, br2)) = (anon_reg1, anon_reg2);
if self.find_anon_type(sup, &br1).is_some() &&
self.find_anon_type(sub, &br2).is_some() {
(self.find_anon_type(sup, &br1).unwrap(),
self.find_anon_type(sub, &br2).unwrap())
let (ty_sup, ty_sub, scope_def_id_sup, scope_def_id_sub, bregion_sup, bregion_sub) =
if let (Some(anon_reg_sup), Some(anon_reg_sub)) =
(self.is_suitable_anonymous_region(sup, true),
self.is_suitable_anonymous_region(sub, true)) {
let (def_id_sup, br_sup, def_id_sub, br_sub) = (anon_reg_sup.def_id,
anon_reg_sup.boundregion,
anon_reg_sub.def_id,
anon_reg_sub.boundregion);
if let (Some(anonarg_sup), Some(anonarg_sub)) =
(self.find_anon_type(sup, &br_sup), self.find_anon_type(sub, &br_sub)) {
(anonarg_sup, anonarg_sub, def_id_sup, def_id_sub, br_sup, br_sub)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: you can return anon_reg_sup & anon_reg_sub.

Copy link
Author

@gaurikholkar-zz gaurikholkar-zz Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arielb1 Are you saying use anon_reg_sup instead of Some(anon_reg_sup). Can you elaborate on this please?

} else {
return false;
}
} else {
return false;
}
} else {
return false; // inapplicable
};
};

if let (Some(sup_arg), Some(sub_arg)) =
let (label1, label2) = if let (Some(sup_arg), Some(sub_arg)) =
(self.find_arg_with_anonymous_region(sup, sup),
self.find_arg_with_anonymous_region(sub, sub)) {
let ((anon_arg1, _, _, _), (anon_arg2, _, _, _)) = (sup_arg, sub_arg);

let span_label_var1 = if let Some(simple_name) = anon_arg1.pat.simple_name() {
format!(" from `{}` ", simple_name)
} else {
format!(" ")
};
let (anon_arg_sup, is_first_sup, anon_arg_sub, is_first_sub) =
(sup_arg.arg, sup_arg.is_first, sub_arg.arg, sub_arg.is_first);
if self.is_self_anon(is_first_sup, scope_def_id_sup) ||
self.is_self_anon(is_first_sub, scope_def_id_sub) {
return false;
}

let span_label_var2 = if let Some(simple_name) = anon_arg2.pat.simple_name() {
format!(" into `{}` ", simple_name)
if self.is_return_type_anon(scope_def_id_sup, bregion_sup) ||
self.is_return_type_anon(scope_def_id_sub, bregion_sub) {
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are there so many newlines here?

if anon_arg_sup == anon_arg_sub {
(format!(" with one lifetime"), format!(" into the other"))
} else {
format!(" ")
};
let span_label_var1 = if let Some(simple_name) = anon_arg_sup.pat.simple_name() {
format!(" from `{}`", simple_name)
} else {
format!("")
};

struct_span_err!(self.tcx.sess, span, E0623, "lifetime mismatch")
.span_label(ty1.span,
format!("these references are not declared with the same lifetime..."))
.span_label(ty2.span, format!(""))
.span_label(span,
format!("...but data{}flows{}here", span_label_var1, span_label_var2))
.emit();
let span_label_var2 = if let Some(simple_name) = anon_arg_sub.pat.simple_name() {
format!(" into `{}`", simple_name)
} else {
format!("")
};

(span_label_var1, span_label_var2)
}
} else {
return false;
}
};

struct_span_err!(self.tcx.sess, span, E0623, "lifetime mismatch")
.span_label(ty_sup.span,
format!("these two types are declared with different lifetimes..."))
.span_label(ty_sub.span, format!(""))
.span_label(span, format!("...but data{} flows{} here", label1, label2))
.emit();
return true;
}

Expand All @@ -94,7 +115,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
/// contains the anonymous type.
///
/// # Arguments
///
/// region - the anonymous region corresponding to the anon_anon conflict
/// br - the bound region corresponding to the above region which is of type `BrAnon(_)`
///
Expand All @@ -105,39 +125,56 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
/// ```
/// The function returns the nested type corresponding to the anonymous region
/// for e.g. `&u8` and Vec<`&u8`.
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;
pub 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, true) {
let def_id = anon_reg.def_id;
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();
}
}
let inputs: &[_] =
match self.tcx.hir.get(node_id) {
hir_map::NodeItem(&hir::Item {
node: hir::ItemFn(ref fndecl, ..), ..
}) => &fndecl.inputs,
hir_map::NodeTraitItem(&hir::TraitItem {
node: hir::TraitItemKind::Method(ref fndecl, ..),
..
}) => &fndecl.decl.inputs,
hir_map::NodeImplItem(&hir::ImplItem {
node: hir::ImplItemKind::Method(ref fndecl, ..),
..
}) => &fndecl.decl.inputs,

_ => &[],
};

return inputs
.iter()
.filter_map(|arg| {
self.find_component_for_bound_region(&**arg, br)
})
.next();
}
}
}
None
}

// This method creates a FindNestedTypeVisitor which returns the type corresponding
// to the anonymous region.
fn find_component_for_bound_region(&self,
arg: &'gcx hir::Ty,
br: &ty::BoundRegion)
-> Option<(&'gcx hir::Ty)> {
let mut nested_visitor = FindNestedTypeVisitor {
infcx: &self,
hir_map: &self.tcx.hir,
bound_region: *br,
found_type: None,
};
nested_visitor.visit_ty(arg);
nested_visitor.found_type
}
}

// The FindNestedTypeVisitor captures the corresponding `hir::Ty` of the
Expand Down Expand Up @@ -176,8 +213,8 @@ impl<'a, 'gcx, 'tcx> Visitor<'gcx> for FindNestedTypeVisitor<'a, 'gcx, 'tcx> {
hir::TyRptr(ref lifetime, _) => {
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 {
Some(&rl::Region::LateBoundAnon(debruijn_index, anon_index)) => {
if debruijn_index.depth == 1 && anon_index == br_index {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can get the deBruijn indexes wrong in cases like fn foo<'a>(data: &for<> Fn(&'a u32)) {}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be hard to fix, and it will only cause silly error messages (rather than e.g. crashing the compiler, which other debruijn index mistakes did), so I think it's ok if you leave it broken for now.

Copy link
Author

@gaurikholkar-zz gaurikholkar-zz Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be opening up a different PR for the case you mentioned. My guess is, it falls under TyBareFn

self.found_type = Some(arg);
return; // we can stop visiting now
}
Expand All @@ -191,10 +228,77 @@ impl<'a, 'gcx, 'tcx> Visitor<'gcx> for FindNestedTypeVisitor<'a, 'gcx, 'tcx> {
}
}
}
// Checks if it is of type `hir::TyPath` which corresponds to a struct.
hir::TyPath(_) => {
let subvisitor = &mut TyPathVisitor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did run rustfmt. This is what I got.

infcx: self.infcx,
found_it: false,
bound_region: self.bound_region,
hir_map: self.hir_map,
};
intravisit::walk_ty(subvisitor, arg); // call walk_ty; as visit_ty is empty,
// this will visit only outermost type
if subvisitor.found_it {
self.found_type = Some(arg);
}
}
_ => {}
}
// walk the embedded contents: e.g., if we are visiting `Vec<&Foo>`,
// go on to visit `&Foo`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing comments?

intravisit::walk_ty(self, arg);
}
}

// The visitor captures the corresponding `hir::Ty` of the anonymous region
// in the case of structs ie. `hir::TyPath`.
// This visitor would be invoked for each lifetime corresponding to a struct,
// and would walk the types like Vec<Ref> in the above example and Ref looking for the HIR
// where that lifetime appears. This allows us to highlight the
// specific part of the type in the error message.
struct TyPathVisitor<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> {
infcx: &'a InferCtxt<'a, 'gcx, 'tcx>,
hir_map: &'a hir::map::Map<'gcx>,
found_it: bool,
bound_region: ty::BoundRegion,
}

impl<'a, 'gcx, 'tcx> Visitor<'gcx> for TyPathVisitor<'a, 'gcx, 'tcx> {
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'gcx> {
NestedVisitorMap::OnlyBodies(&self.hir_map)
}

fn visit_lifetime(&mut self, lifetime: &hir::Lifetime) {
let br_index = match self.bound_region {
ty::BrAnon(index) => index,
_ => return,
};

match self.infcx.tcx.named_region_map.defs.get(&lifetime.id) {
// the lifetime of the TyPath!
Some(&rl::Region::LateBoundAnon(debruijn_index, anon_index)) => {
if debruijn_index.depth == 1 && anon_index == br_index {
self.found_it = true;
}
}
Some(&rl::Region::Static) |
Some(&rl::Region::EarlyBound(_, _)) |
Some(&rl::Region::LateBound(_, _)) |
Some(&rl::Region::Free(_, _)) |
None => {
debug!("no arg found");
}
}
}

fn visit_ty(&mut self, arg: &'gcx hir::Ty) {
// ignore nested types
//
// If you have a type like `Foo<'a, &Ty>` we
// are only interested in the immediate lifetimes ('a).
//
// Making `visit_ty` empty will ignore the `&Ty` embedded
// inside, it will get reached by the outer visitor.
debug!("`Ty` corresponding to a struct is {:?}", arg);
}
}
Loading