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

Fix lifetime error to print 'a instead of &'a #14317

Merged
merged 1 commit into from
May 24, 2014

Conversation

ftxqxd
Copy link
Contributor

@ftxqxd ftxqxd commented May 21, 2014

This changes certain error messages about lifetimes so that they display lifetimes without an &.

Fixes #10291.

BrFresh(_) => prefix.to_strbuf(),
BrAnon(_) => {
format_strbuf!("{}'<anon>{}",
prefix.to_strbuf(),
Copy link
Contributor

Choose a reason for hiding this comment

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

.to_strbuf() here is unnecessary allocation.

@lilyball
Copy link
Contributor

Do we really want to print '<anon> everywhere? That seems likely to be unnecessarily noisy. I feel like we should only be printing anonymous lifetimes when there's a lifetime mismatch, and we should be generating unique names for them then (to stress that they're different).

@ftxqxd
Copy link
Contributor Author

ftxqxd commented May 21, 2014

@kballard: I think it would indeed be better to only print it on errors relating to lifetimes. The only reason I didn’t do so was because I thought it would be slightly more complex to implement, and I wanted to see others’ opinion on this.

Regarding naming, originally in my code I made it print something like &'#1 instead of &<anon> to show that they are different — do you think that I could reinstate that? Anonymous lifetimes are given numbers in other errors, too (not sure which ones, but I’ve seen it before & it's in the source).

@lilyball
Copy link
Contributor

Something like &'#1 seems better, but again, I think it only makes sense to print this when it's important to indicate that the lifetime of two references is different. As opposed to what you're doing right now, which I assume is unconditionally printing the anonymous lifetime for all references.

@alexcrichton
Copy link
Member

I agree that these error messages have predominately taken a step backwards. If this is fixing a problem where the compiler says it found &int but expected &int, is this making it clearer by printing the same string as the lifetime?

@ftxqxd
Copy link
Contributor Author

ftxqxd commented May 21, 2014

@alexcrichton I’m not sure I understand what you mean by “the same string as the lifetime”.

As I’m still finding my way around the compiler code, I’m struggling to locate the actual code relating to the lifetime errors so that I can specialise the display of anonymous lifetimes for them only. Given that the naming of anonymous lifetimes isn’t actually part of #10291 (it only properly concerns a confusing & in an error message), and the indecision relating to how anonymous lifetimes should be displayed, would it be OK if I could simply omit that bug from this PR and leave that for a later date if I don’t find a solution soon? (Perhaps a separate issue could also be created in that case regarding the anonymous lifetime error message confusion.)

@alexcrichton
Copy link
Member

In the compiler message where it says that it found &int but expected &int (I'm still not sure if this is exactly what you're fixing), the new message would be that it found &'<anon> int but expected &'<anon> int which doesn't seem that much different than before.

It'd be fine to leave out '<anon> from this PR for a later date.

@ftxqxd ftxqxd changed the title Fix lifetime error to print 'a instead of '&'a Fix lifetime error to print 'a instead of &'a May 22, 2014
@ftxqxd
Copy link
Contributor Author

ftxqxd commented May 22, 2014

I’ve updated the PR to remove the code about anonymous lifetimes. (I also fixed a typo in the title/commit message.)

@alexcrichton
Copy link
Member

Would it be possible to add a test for this as well? (testing the error message)

This changes certain error messages about lifetimes so that they display
lifetimes without an `&`.

Fixes rust-lang#10291.
@ftxqxd
Copy link
Contributor Author

ftxqxd commented May 23, 2014

I’ve added a test, and it passes locally, but I’m not sure if it’s extensive enough. Should that be good enough?

bors added a commit that referenced this pull request May 23, 2014
This changes certain error messages about lifetimes so that they display lifetimes without an `&`.

Fixes #10291.
@bors bors closed this May 24, 2014
@bors bors merged commit e6b23da into rust-lang:master May 24, 2014
@ftxqxd ftxqxd deleted the lifetime-formatting branch May 24, 2014 01:04
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.

lifetime misformatted (leading &) in rustc note output
4 participants