Skip to content

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

Merged
bors merged 1 commit intorust-lang:masterfrom
ftxqxd:lifetime-formatting
May 24, 2014
Merged

Fix lifetime error to print 'a instead of &'a#14317
bors merged 1 commit intorust-lang:masterfrom
ftxqxd:lifetime-formatting

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.

lilyball
lilyball reviewed May 21, 2014
View reviewed changes
src/librustc/util/ppaux.rs Outdated
Copy link
Contributor

@lilyball lilyball May 21, 2014

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

lilyball commented May 21, 2014

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

lilyball commented May 21, 2014

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

alexcrichton commented May 21, 2014

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

alexcrichton commented May 21, 2014

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

alexcrichton commented May 22, 2014

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

@ftxqxd
Fix lifetime error to print 'a instead of &'a
e6b23da 
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
@bors
auto merge of #14317 : P1start/rust/lifetime-formatting, r=alexcrichton
4462687 
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
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 27, 2025
@Centri3
macro_use_import: Don't check is attribute comes from expansion (rust…
15180d4 
…-lang#14317)

It is not possible to write a declarative macro, that produces an
attribute w/o
an item attached to it. This means that the `check_item` will already
insert the
span in the map, if it came from an expansion. So additionally checking
if the
macro came from an expansion doesn't add anything here. So the
`check_attribute` function, and with that the problematic `attr.span()`
call can
be completely removed.

Fixes rust-lang/rust-clippy#14303

r? @y21

cc @jdonszelmann

changelog: Fix ICE in [`macro_use_import`] lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

lifetime misformatted (leading &) in rustc note output

4 participants

@ftxqxd @lilyball @alexcrichton @bors