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

Don't provide hint to add lifetime on impl items #37481

Merged
merged 1 commit into from
Nov 12, 2016

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Oct 30, 2016

use std::str::FromStr;

pub struct Foo<'a> {
    field: &'a str,
}

impl<'a> FromStr for Foo<'a> {
    type Err = ();
    fn from_str(path: &str) -> Result<Self, ()> {
        Ok(Foo { field: path })
    }
}

would give the following hint:

help: consider using an explicit lifetime parameter as shown: fn from_str(path: &'a str) -> Result<Self, ()>
  --> <anon>:9:5
   |
9  |     fn from_str(path: &str) -> Result<Self, ()> {
   |     ^

which is never correct, since then there will be a lifetime mismatch between the impl and the trait.

Remove this hint for all impl items.

Re: #37363.

@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 @eddyb (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.

@estebank estebank force-pushed the lifetime-help-removal-for-impl branch from 8b3646f to b00ad7b Compare October 30, 2016 06:01
@estebank estebank changed the title Do't provide hint to add lifetime on impl items Don't provide hint to add lifetime on impl items Oct 30, 2016
}
}
ast_map::NodeImplItem(item) => {
match item.node {
hir::ImplItemKind::Method(ref sig, _) => {
Some((&sig.decl,
(true, Some((&sig.decl,
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just add an early return here and that might be enough. Also this should only be done if the impl is for a trait, not inherent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Changed to early return
  • Check that impl if for a trait
  • Add unittest

@eddyb
Copy link
Member

eddyb commented Nov 10, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2016

📌 Commit 0c4104a has been approved by eddyb

@bors
Copy link
Contributor

bors commented Nov 10, 2016

⌛ Testing commit 0c4104a with merge 6b14097...

@bors
Copy link
Contributor

bors commented Nov 10, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@estebank estebank force-pushed the lifetime-help-removal-for-impl branch from 0c4104a to 3e83628 Compare November 10, 2016 07:38
@alexcrichton
Copy link
Member

@estebank looks like the travis failures are legit?

@estebank
Copy link
Contributor Author

@alexcrichton Yes. I'll fix tonight. I need a new r+ after pushing a fix, right?

@alexcrichton
Copy link
Member

@estebank indeed!

Don't provide hint to add lifetime on impl items that implement a trait.

```rust
use std::str::FromStr;

pub struct Foo<'a> {
    field: &'a str,
}

impl<'a> FromStr for Foo<'a> {
    type Err = ();
    fn from_str(path: &str) -> Result<Self, ()> {
        Ok(Foo { field: path })
    }
}
```

would give the following hint:

```nocode
help: consider using an explicit lifetime parameter as shown: fn from_str(path: &'a str) -> Result<Self, ()>
  --> <anon>:9:5
   |
9  |     fn from_str(path: &str) -> Result<Self, ()> {
   |     ^
```

which is never correct, since then there will be a lifetime mismatch
between the impl and the trait.

Remove this hint for impl items that implement a trait.
@estebank estebank force-pushed the lifetime-help-removal-for-impl branch from 3e83628 to 87b6d38 Compare November 11, 2016 00:22
@estebank
Copy link
Contributor Author

@alexcrichton @eddyb fixed the test error.

@alexcrichton
Copy link
Member

@bors: r=eddyb

@bors
Copy link
Contributor

bors commented Nov 11, 2016

📌 Commit 87b6d38 has been approved by eddyb

eddyb added a commit to eddyb/rust that referenced this pull request Nov 11, 2016
…impl, r=eddyb

Don't provide hint to add lifetime on impl items

``` rust
use std::str::FromStr;

pub struct Foo<'a> {
    field: &'a str,
}

impl<'a> FromStr for Foo<'a> {
    type Err = ();
    fn from_str(path: &str) -> Result<Self, ()> {
        Ok(Foo { field: path })
    }
}
```

would give the following hint:

``` nocode
help: consider using an explicit lifetime parameter as shown: fn from_str(path: &'a str) -> Result<Self, ()>
  --> <anon>:9:5
   |
9  |     fn from_str(path: &str) -> Result<Self, ()> {
   |     ^
```

which is never correct, since then there will be a lifetime mismatch between the `impl` and the trait.

Remove this hint for all `impl` items.

Re: rust-lang#37363.
@bors
Copy link
Contributor

bors commented Nov 12, 2016

⌛ Testing commit 87b6d38 with merge 65ba338...

eddyb added a commit to eddyb/rust that referenced this pull request Nov 12, 2016
…impl, r=eddyb

Don't provide hint to add lifetime on impl items

``` rust
use std::str::FromStr;

pub struct Foo<'a> {
    field: &'a str,
}

impl<'a> FromStr for Foo<'a> {
    type Err = ();
    fn from_str(path: &str) -> Result<Self, ()> {
        Ok(Foo { field: path })
    }
}
```

would give the following hint:

``` nocode
help: consider using an explicit lifetime parameter as shown: fn from_str(path: &'a str) -> Result<Self, ()>
  --> <anon>:9:5
   |
9  |     fn from_str(path: &str) -> Result<Self, ()> {
   |     ^
```

which is never correct, since then there will be a lifetime mismatch between the `impl` and the trait.

Remove this hint for all `impl` items.

Re: rust-lang#37363.
@bors
Copy link
Contributor

bors commented Nov 12, 2016

⛄ The build was interrupted to prioritize another pull request.

bors added a commit that referenced this pull request Nov 12, 2016
@bors bors merged commit 87b6d38 into rust-lang:master Nov 12, 2016
@estebank estebank deleted the lifetime-help-removal-for-impl branch November 9, 2023 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants