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

Improve error message for one named, one anonymous lifetime parameters - underline Type #43851

Closed
wants to merge 2 commits into from

Conversation

gaurikholkar-zz
Copy link

@gaurikholkar-zz gaurikholkar-zz commented Aug 13, 2017

This is a fix to #43433

Please ignore the previous commits. Only review
changing error message format for E0621

The other review is being done in PR #43700
Sample error message

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

r? @nikomatsakis
cc @estebank @arielb1

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2017
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.

I'm a bit nervous about this change unless we also change the message somewhat -- see my comment below. What do you think?

@@ -2,7 +2,7 @@ error[E0621]: explicit lifetime required in parameter type
--> $DIR/ex1-return-one-existing-name-if-else-3.rs:12:27
|
11 | fn foo<'a>((x, y): (&'a i32, &i32)) -> &'a i32 {
| ------ consider changing type to `(&'a i32, &'a i32)`
| ---- consider changing type to `(&'a i32, &'a i32)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, so here we are underlining only a piece of the type, but the replacement that we are suggesting is the entire type. This seems a bit confusing to me.

@gaurikholkar-zz
Copy link
Author

Currently closing the issue. As per my discussions with @nikomatsakis, we have decided to go ahead with E0621 for now. Opening up an issue for the same and updating it with the observations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants