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

Conversation

gaurikholkar-zz
Copy link

@gaurikholkar-zz gaurikholkar-zz commented Jun 15, 2017

This is a fix for #42517
Note that this only handles the above case for function declarations and traits.
impl items and closures will be handled in a later PR.
Example

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

now displays the following error message. ui tests have been added for the same.

error[E0611]: explicit lifetime required in the type of `x`
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

#42516
r? @nikomatsakis

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nikomatsakis
Copy link
Contributor

I wonder if we should change the role of primary (^) vs secondary (-). It just seems a bit surprising to have ^ have the label "consider"; but I'm not sure if we want to put stronger wording, given that it may indeed be the function body that is at fault, not the signature.

@nikomatsakis
Copy link
Contributor

Also, something else just occurred to me -- we are already avoiding giving this message in closures, but we may want to be careful in trait impls too. Specifically, if the impl is of a trait, then the signature is sometimes specified by the trait:

trait Foo {
    fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32;
}

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

In this event, the user is not (necessarily) at liberty to change the signature. It's a bit tricky to judge, because the user may be able to change the trait. It's also a bit tricky because sometimes the trait signature is stricter than the one in the impl (we allow that, for better or worse):

trait Foo {
    fn foo<'a>(x: &'a i32, y: &'a i32) -> &'a i32;
}

impl Foo for () {
    fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {
        // adding a `'a` to `x` would be legal here...
        y
    }
}

So there are kind of three scenarios in the case of a function in a trait impl (none of this applies to inherent impls):

  • user may be able to change the trait (e.g., because it is within the same crate and private)
  • user cannot change the trait (e.g., because it's in another crate or has been published and others are relying on it)
  • user has written a more permissive impl than the trait specifies, and hence could make changes

We can't really tell the difference between the first two. The final one we can detect, with some more effort. For the time being, we might want to either turn off the error for impl items (and file an issue to detect the remaining cases and handle them better), or else change the wording.

@gaurikholkar-zz
Copy link
Author

@nikomatsakis Added a new commit that is not considering impl of traits for now.

@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2017

if let Some(simple_name) = var.pat.simple_name() {
struct_span_err!(self.tcx.sess,
var.pat.span,
Copy link
Contributor

Choose a reason for hiding this comment

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

to swap the role of ^ etc, just change this to span

@gaurikholkar-zz
Copy link
Author

This is how the error message now looks

error[E0611]: explicit lifetime required in the type of `y`
  --> $DIR/ex1-return-one-existing-name-if-else.rs:12:27
   |
11 | fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32 {
   |                        - consider changing the type of `y` to `&'a i32`
12 |     if x > y { x } else { y }
   |                           ^ lifetime `'a` required

@bors
Copy link
Contributor

bors commented Jun 16, 2017

☔ The latest upstream changes (presumably #41840) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@gaurikholkar can you rebase instead of merging? I think this branch is probably ready to land, although I Was thinking that I might like to try and make test cases for more scenarios. Maybe a good idea to do that before hand, so that then we can land follow-up PRs targeting each one (sort of like we did for closures).

@gaurikholkar-zz
Copy link
Author

gaurikholkar-zz commented Jun 16, 2017

@nikomatsakis I agree, we should make more test cases for the same. Currently it's only closures and trait impl. Rebased.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Left some comments.

ConcreteFailure(origin, sub, sup) => {
self.report_concrete_failure(origin, sub, sup).emit();
}
// If ConcreteFailure does not have an anonymous region
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: indentation

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, though, I think I'd remove this comment. Maybe replace it with:

// 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.

// 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, the `y` and the `Ty` of `y` is returned after being substituted
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say: Here, we would return the hir::Arg for y, and we return the type &'a i32, which is the type of y but with the anonymous region replaced with 'a.

_ => return false, // inapplicable
};

let (named, (var, new_ty)) =
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename var to arg, for consistency. Probably a good idea to put a comment before this block of code, too:

// 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.

} 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.

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)

}

// This method returns whether the given Region is Anonymous
pub fn is_anonymous_region(&self, region: Region<'tcx>) -> bool {
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 we should change this function to return Option<DefId>, where the def-id is free_region.scope. We can call it something like anonymous_region_binding_scope. Then we can test if something is an anonymous region but also extract its scope at the same time.

match *region {
ty::ReFree(ref free_region) => {
match free_region.bound_region {
ty::BrAnon(..) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

i.e., here we would return Some(free_region.scope)

ty::BrAnon(..) => {
let id = free_region.scope;
let node_id = self.tcx.hir.as_local_node_id(id).unwrap();
match self.tcx.hir.find(node_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this logic out into the "named vs anonymous" function, it seems more appropriate there to me. I'll put a comment there.

// Consider the example `fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32`
// Here, the `y` and the `Ty` of `y` is returned after being substituted
// by that of the named region.
pub fn find_arg_with_anonymous_region(&self,
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not need to be public, you can just make this fn

// 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 report_named_anon_conflict(&self, error: &RegionResolutionError<'tcx>) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this try_report_named_anon_conflict

(sup, self.find_arg_with_anonymous_region(sub, sup).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.

@gaurikholkar-zz
Copy link
Author

gaurikholkar-zz commented Jun 18, 2017

@nikomatsakis added a check for Trait Impls and anonymous region in return type

@gaurikholkar-zz
Copy link
Author

@nikomatsakis added the anonymous region check for self and the ui test too

if body.arguments.iter().nth(0) == Some(&arg) {
is_first = true;
}
return Some((arg,
Copy link
Contributor

@nikomatsakis nikomatsakis Jun 22, 2017

Choose a reason for hiding this comment

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

Nit: we don't need the return here, right? That is, I would either have return here, or else below, but not both.

let body = self.tcx.hir.body(body_id);
body.arguments
.iter()
.filter_map(|arg| if let Some(tables) = self.in_progress_tables {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe we should move the if let outside of the body.arguments.iter(). Something like:

if let Some(tables) = self.in_progress_tables {
    body.arguments
    .iter()
    .filter_map(|arg| {
        ...
    })
    .next()
} else {
    None
}

@gaurikholkar-zz
Copy link
Author

@nikomatsakis made the changes

if x > y { x } else { y }
}

fn main () { }
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this.

if x > y { x } else { y }
}

fn main() {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this.

@GuillaumeGomez
Copy link
Member

Sorry to come with bad news, but I added a lot of error codes meanwhile. The current highest is E0620, so you should take E0621. Also, please add a corresponding test into the src/test/compile-fail folder (look at the other E0*.rs).

To be honest, I think it'd be better to just wait that my last two PRs get merged. Great job otherwise! :)

@nikomatsakis
Copy link
Contributor

@GuillaumeGomez

Sorry to come with bad news, but I added a lot of error codes meanwhile.

Heh, thanks for the warning! Man I hate the way error codes are guaranteed merge conflcits. :(

@nikomatsakis
Copy link
Contributor

Here is a proposed rewrite for the extended error description: https://gist.github.com/nikomatsakis/e9ec2091abe1b9a7a3371c6894132463

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 30, 2017

📌 Commit 37a88f4 has been approved by nikomatsakis

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 30, 2017
Adding diagnostic code 0611 for lifetime errors with one named, one anonymous lifetime parameter

This is a fix for rust-lang#42517
Note that this only handles the above case for **function declarations** and **traits**.
`impl items` and `closures` will be handled in a later PR.
Example
```
fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {
    if x > y { x } else { y }
}
```
now displays the following error message. ui tests have been added for the same.
```
error[E0611]: explicit lifetime required in the type of `x`
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
```
rust-lang#42516
r? @nikomatsakis
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 30, 2017
Adding diagnostic code 0611 for lifetime errors with one named, one anonymous lifetime parameter

This is a fix for rust-lang#42517
Note that this only handles the above case for **function declarations** and **traits**.
`impl items` and `closures` will be handled in a later PR.
Example
```
fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {
    if x > y { x } else { y }
}
```
now displays the following error message. ui tests have been added for the same.
```
error[E0611]: explicit lifetime required in the type of `x`
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
```
rust-lang#42516
r? @nikomatsakis
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 30, 2017
Adding diagnostic code 0611 for lifetime errors with one named, one anonymous lifetime parameter

This is a fix for rust-lang#42517
Note that this only handles the above case for **function declarations** and **traits**.
`impl items` and `closures` will be handled in a later PR.
Example
```
fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {
    if x > y { x } else { y }
}
```
now displays the following error message. ui tests have been added for the same.
```
error[E0611]: explicit lifetime required in the type of `x`
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
```
rust-lang#42516
r? @nikomatsakis
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 30, 2017
Adding diagnostic code 0611 for lifetime errors with one named, one anonymous lifetime parameter

This is a fix for rust-lang#42517
Note that this only handles the above case for **function declarations** and **traits**.
`impl items` and `closures` will be handled in a later PR.
Example
```
fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {
    if x > y { x } else { y }
}
```
now displays the following error message. ui tests have been added for the same.
```
error[E0611]: explicit lifetime required in the type of `x`
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
```
rust-lang#42516
r? @nikomatsakis
bors added a commit that referenced this pull request Jun 30, 2017
Rollup of 6 pull requests

- Successful merges: #42669, #42911, #42925, #42957, #42985, #42987
- Failed merges: #42936
@bors bors merged commit 37a88f4 into rust-lang:master Jul 1, 2017
bors added a commit that referenced this pull request Jul 11, 2017
@gaurikholkar-zz
Copy link
Author

#43851
Modifying the error message

@gaurikholkar-zz gaurikholkar-zz changed the title Adding diagnostic code 0611 for lifetime errors with one named, one anonymous lifetime parameter Adding diagnostic code 0623 for lifetime errors with one named, one anonymous lifetime parameter Aug 16, 2017
@gaurikholkar-zz gaurikholkar-zz changed the title Adding diagnostic code 0623 for lifetime errors with one named, one anonymous lifetime parameter Adding diagnostic code 0621 for lifetime errors with one named, one anonymous lifetime parameter Aug 29, 2017
estebank added a commit to estebank/rust that referenced this pull request Nov 5, 2017
When there's a lifetime mismatch between an argument with an anonymous
lifetime being returned in a method with a return type that has a named
lifetime, show specialized lifetime error pointing at argument with a
hint to give it an explicit lifetime matching the return type.

```
error[E0621]: explicit lifetime required in the type of `other`
  --> file2.rs:21:21
   |
17 |     fn bar(&self, other: Foo) -> Foo<'a> {
   |                   ----- consider changing the type of `other` to `Foo<'a>`
...
21 |                     other
   |                     ^^^^^ lifetime `'a` required
```

Follow up to rust-lang#44124 and rust-lang#42669.
kennytm added a commit to kennytm/rust that referenced this pull request Nov 7, 2017
Handle anon lifetime arg being returned with named lifetime return type

When there's a lifetime mismatch between an argument with an anonymous
lifetime being returned in a method with a return type that has a named
lifetime, show specialized lifetime error pointing at argument with a
hint to give it an explicit lifetime matching the return type.

```
error[E0621]: explicit lifetime required in the type of `other`
  --> file2.rs:21:21
   |
17 |     fn bar(&self, other: Foo) -> Foo<'a> {
   |                   ----- consider changing the type of `other` to `Foo<'a>`
...
21 |                     other
   |                     ^^^^^ lifetime `'a` required
```

Follow up to rust-lang#44124 and rust-lang#42669. Fix rust-lang#44684.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants