Skip to content
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

Adding diagnostic code 0621 for lifetime errors with one named, one anonymous lifetime parameter #42669

Merged
merged 17 commits into from
Jul 1, 2017
Merged
Show file tree
Hide file tree
Changes from 13 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
38 changes: 38 additions & 0 deletions src/librustc/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1946,6 +1946,44 @@ Maybe you just misspelled the lint name or the lint doesn't exist anymore.
Either way, try to update/remove it in order to fix the error.
"##,

E0621: r##"
This error code indicates a mismatch between the function signature (i.e.,
the parameter types and the return type) and the function body. Most of
the time, this indicates that the function signature needs to be changed to
match the body, but it may be that the body needs to be changed to match
the signature.

Specifically, one or more of the parameters contain borrowed data that
needs to have a named lifetime in order for the body to type-check. Most of
the time, this is because the borrowed data is being returned from the
function, as in this example:

```compile_fail,E0621
fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32 { // explicit lifetime required
// in the type of `y`
if x > y { x } else { y }
}
```

Here, the function is returning data borrowed from either x or y, but the
'a annotation indicates that it is returning data only from x. We can make
the signature match the body by changing the type of y to &'a i32, like so:

```
fn foo<'a>(x: &'a i32, y: &'a i32) -> &'a i32 {
if x > y { x } else { y }
}
```

Alternatively, you could change the body not to return data from y:

```
fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32 {
x
}
```
"##,

}


Expand Down
44 changes: 30 additions & 14 deletions src/librustc/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,11 @@ use ty::error::TypeError;
use syntax::ast::DUMMY_NODE_ID;
use syntax_pos::{Pos, Span};
use errors::{DiagnosticBuilder, DiagnosticStyledString};

mod note;

mod need_type_info;
mod named_anon_conflict;


impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub fn note_and_explain_region(self,
Expand Down Expand Up @@ -255,34 +257,48 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}

impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
pub fn report_region_errors(&self,
errors: &Vec<RegionResolutionError<'tcx>>) {

pub fn report_region_errors(&self, errors: &Vec<RegionResolutionError<'tcx>>) {
debug!("report_region_errors(): {} errors to start", errors.len());

// try to pre-process the errors, which will group some of them
// together into a `ProcessedErrors` group:
let errors = self.process_errors(errors);

debug!("report_region_errors: {} errors after preprocessing", errors.len());
debug!("report_region_errors: {} errors after preprocessing",
errors.len());

for error in errors {

debug!("report_region_errors: error = {:?}", error);
match error.clone() {
ConcreteFailure(origin, sub, sup) => {
self.report_concrete_failure(origin, sub, sup).emit();
}

GenericBoundFailure(kind, param_ty, sub) => {
self.report_generic_bound_failure(kind, param_ty, sub);
}
if !self.try_report_named_anon_conflict(&error){

match error.clone() {
// These errors could indicate all manner of different
// problems with many different solutions. Rather
// than generate a "one size fits all" error, what we
// attempt to do is go through a number of specific
// scenarios and try to find the best way to present
// the error. If all of these fails, we fall back to a rather
// general bit of code that displays the error information
ConcreteFailure(origin, sub, sup) => {

self.report_concrete_failure(origin, sub, sup).emit();
}

SubSupConflict(var_origin,
GenericBoundFailure(kind, param_ty, sub) => {
self.report_generic_bound_failure(kind, param_ty, sub);
}

SubSupConflict(var_origin,
sub_origin, sub_r,
sup_origin, sup_r) => {
self.report_sub_sup_conflict(var_origin,
self.report_sub_sup_conflict(var_origin,
sub_origin, sub_r,
sup_origin, sup_r);
}
}
}
}
}
}
Expand Down
199 changes: 199 additions & 0 deletions src/librustc/infer/error_reporting/named_anon_conflict.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Error Reporting for Anonymous Region Lifetime Errors.
use hir;
use infer::InferCtxt;
use ty::{self, Region};
use infer::region_inference::RegionResolutionError::*;
use infer::region_inference::RegionResolutionError;
use hir::map as hir_map;
use hir::def_id::DefId;

impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
// This method walks the Type of the function body arguments using
// `fold_regions()` function and returns the
// &hir::Arg of the function argument corresponding to the anonymous
// region and the Ty corresponding to the named region.
// Currently only the case where the function declaration consists of
// one named region and one anonymous region is handled.
// Consider the example `fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32`
// Here, we would return the hir::Arg for y, we return the type &'a
// i32, which is the type of y but with the anonymous region replaced
// with 'a, the corresponding bound region and is_first which is true if
// the hir::Arg is the first argument in the function declaration.
fn find_arg_with_anonymous_region
(&self,
anon_region: Region<'tcx>,
named_region: Region<'tcx>)
-> Option<(&hir::Arg, ty::Ty<'tcx>, ty::BoundRegion, bool)> {

match *anon_region {
ty::ReFree(ref free_region) => {

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();
let body = self.tcx.hir.body(body_id);
if let Some(tables) = self.in_progress_tables {
body.arguments
.iter()
.enumerate()
.filter_map(|(index, arg)| {
let ty = tables.borrow().node_id_to_type(arg.id);
let mut found_anon_region = false;
let new_arg_ty = self.tcx
.fold_regions(&ty, &mut false, |r, _| if *r == *anon_region {
found_anon_region = true;
named_region
} else {
r
});
if found_anon_region {
let is_first = index == 0;
Some((arg, new_arg_ty, free_region.bound_region, is_first))
} else {
None
}
})
.next()
} else {
None
}
}
_ => None,

}
}

// This method generates the error message for the case when
// the function arguments consist of a named region and an anonymous
// region and corresponds to `ConcreteFailure(..)`
pub fn try_report_named_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 one named region ('a)
// and one anonymous (elided) region. If so, find the parameter arg
// where the anonymous region appears (there must always be one; we
// only introduced anonymous regions in parameters) as well as a
// version new_ty of its type where the anonymous region is replaced
// with the named one.
let (named, (arg, new_ty, br, is_first), scope_def_id) =
if sub.is_named_region() && self.is_suitable_anonymous_region(sup).is_some() {
(sub,
self.find_arg_with_anonymous_region(sup, sub).unwrap(),
self.is_suitable_anonymous_region(sup).unwrap())
} else if sup.is_named_region() && self.is_suitable_anonymous_region(sub).is_some() {
(sup,
self.find_arg_with_anonymous_region(sub, sup).unwrap(),
self.is_suitable_anonymous_region(sub).unwrap())
} else {
return false; // inapplicable
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhere in here I think we should get the scope of the anonymous region and check that it is defined on something suitable. Something like:

let scope_node_id = self.tcx.hir.as_local_node_id(scope_def_id).unwrap();
match self.tcx.hir.find(scope_node_id) {
  Some(hir_map::NodeItem(..)) |
  Some(hir_map::NodeTraitItem(..)) => { /* proceed ahead */ }
  Some(hir_map::NodeImplItem(..)) => {
    if tcx.impl_trait_ref(scope_def_id).is_some() {
      // For now, we do not try to target impls of traits. This is
      // because this message is going to suggest that the user
      // change the fn signature, but they may not be free to do so,
      // since the signature must match the trait.
      //
      // FIXME(#42706) -- in some cases, we could do better here.
      return false;
    }
  }
  _ => return false, // inapplicable; we target only top-level functions, not closures
}

This code currently lives in is_anonymous_region.

Alternatively, we could do the check in that function, but then let's move it into this file (it's now rather specific to this error message) and give it a different name, like is_suitable_anonymous_region(). That's maybe the best path.


// Here, we check for the case where the anonymous region
// is in the return type.
// FIXME(#42703) - Need to handle certain cases here.
let ret_ty = self.tcx.type_of(scope_def_id);
match ret_ty.sty {
ty::TyFnDef(_, _) => {
let sig = ret_ty.fn_sig(self.tcx);
let late_bound_regions = self.tcx
.collect_referenced_late_bound_regions(&sig.output());
if late_bound_regions.iter().any(|r| *r == br) {
return false;
} else {
}
}
_ => {}
}

// Here we check for the case where anonymous region
// corresponds to self and if yes, we display E0312.
// FIXME(#42700) - Need to format self properly to
// enable E0621 for it.
if is_first &&
self.tcx
.opt_associated_item(scope_def_id)
.map(|i| i.method_has_self_argument)
.unwrap_or(false) {
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.

So, per our conversation earlier, I think we want to watch out for the case where the anonymous region appeared in the function return type as well and not try to handle that (that would be #42703 now). To do this check is a bit of a pain though. I think we need to do something like this:

  • extract the free_region.scope from the anonymous region (just as we do in find_arg_with_anonymous_region()); let's call that scope_def_id,
  • lookup the type ty associated with scope_def_id, using tcx.type_of(scope_def_id)
    • since we are only matching functions, this will be a TyFnDef (we can just return for other types)
    • that is the type of a named function like fn foo()
    • if we do match ty.sty { ty::TyFnDef(_, _, sig) => ... } we can extract the function signature sig (it is a PolyFnSig)
    • we can extract the return type ret_ty by doing this incantation (I can explain more on gitter): tcx.liberate_late_bound_regions(scope_def_id, sig.output())
  • then we can search ret_ty for the anonymous region as we did before (something like ret_ty.walk().flat_map(|t| t.regions()).any(|r| r == anon_region)

let (error_var, span_label_var) = if let Some(simple_name) = arg.pat.simple_name() {
(format!("the type of `{}`", simple_name), format!("the type of `{}`", simple_name))
} else {
(format!("parameter type"), format!("type"))
};


struct_span_err!(self.tcx.sess,
span,
E0621,
"explicit lifetime required in {}",
error_var)
.span_label(arg.pat.span,
format!("consider changing {} to `{}`", span_label_var, new_ty))
.span_label(span, format!("lifetime `{}` required", named))
.emit();

return true;

}

// This method returns whether the given Region is Anonymous
// and returns the DefId corresponding to the region.
pub fn is_suitable_anonymous_region(&self, region: Region<'tcx>) -> Option<DefId> {

match *region {
ty::ReFree(ref free_region) => {
match free_region.bound_region {
ty::BrAnon(..) => {
let anonymous_region_binding_scope = free_region.scope;
let node_id = self.tcx
.hir
.as_local_node_id(anonymous_region_binding_scope)
.unwrap();
match self.tcx.hir.find(node_id) {
Some(hir_map::NodeItem(..)) |
Some(hir_map::NodeTraitItem(..)) => {
// proceed ahead //
}
Some(hir_map::NodeImplItem(..)) => {
let container_id = self.tcx
.associated_item(anonymous_region_binding_scope)
.container
.id();
if self.tcx.impl_trait_ref(container_id).is_some() {
// For now, we do not try to target impls of traits. This is
// because this message is going to suggest that the user
// change the fn signature, but they may not be free to do so,
// since the signature must match the trait.
//
// FIXME(#42706) -- in some cases, we could do better here.
return None;
}
}
_ => return None, // inapplicable
// we target only top-level functions
}
return Some(anonymous_region_binding_scope);
}
_ => None,
}
}
_ => None,
}
}
}
2 changes: 1 addition & 1 deletion src/librustc/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ use errors::DiagnosticBuilder;
use syntax_pos::{self, Span, DUMMY_SP};
use util::nodemap::FxHashMap;
use arena::DroplessArena;

use self::combine::CombineFields;
use self::higher_ranked::HrMatchResult;
use self::region_inference::{RegionVarBindings, RegionSnapshot};
Expand Down Expand Up @@ -1077,6 +1076,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
region_map,
free_regions);
let errors = self.region_vars.resolve_regions(&region_rels);

if !self.is_tainted_by_errors() {
// As a heuristic, just skip reporting region errors
// altogether if other errors have been reported while
Expand Down
14 changes: 14 additions & 0 deletions src/librustc/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -990,6 +990,20 @@ impl RegionKind {

flags
}

// This method returns whether the given Region is Named
pub fn is_named_region(&self) -> bool {

match *self {
ty::ReFree(ref free_region) => {
match free_region.bound_region {
ty::BrNamed(..) => true,
_ => false,
}
}
_ => false,
}
}
}

/// Type utilities
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {
if x > y { x } else { y }
}

fn main() { }
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error[E0621]: explicit lifetime required in the type of `x`
--> $DIR/ex1-return-one-existing-name-if-else-2.rs:12:16
|
11 | fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {
| - consider changing the type of `x` to `&'a i32`
12 | if x > y { x } else { y }
| ^ lifetime `'a` required

error: aborting due to previous error(s)

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn foo<'a>((x, y): (&'a i32, &i32)) -> &'a i32 {
if x > y { x } else { y }
}

fn main () { }
Loading