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

Add error file for E0152 #31957

Merged
merged 3 commits into from
Mar 8, 2016
Merged

Conversation

GuillaumeGomez
Copy link
Member

It completes #31818.

However it is not complete yet:

  • test will need to be updated
  • the file name displayed is a bit too unclear.

I'm not sure yet what's the "correct" file name to display. If anyone has an idea on this, it'd be very appreciated.

r? @brson

@brson
Copy link
Contributor

brson commented Feb 29, 2016

Hm, the span of the duplicate is not being printed.

After reviewing @Zoxc's comments on the issue I think what needs to happen is that span needs to be fixed so the error message prints the source location of the duplicate item as normal, then a second note should indicate further info about the previous definition.

The self.session.local_crate_source_file in use here isn't going to be helpful once the span is printing correctly. What we really want is the span of the original definition of the lang item, here represented by original_def_id.

@GuillaumeGomez
Copy link
Member Author

@brson: I provided more information. Does it seem enough for you? If not, what could I add?

@GuillaumeGomez
Copy link
Member Author

@brson: This is ready.

@brson
Copy link
Contributor

brson commented Mar 4, 2016

Thanks @GuillaumeGomez.

Sadly, I think there's more to do here.

I didn't realize before that local definitions are checked first and then external definitions. That really changes my perspective on what this code does.

First here, self.ast_map.span_if_local(original_def_id).unwrap_or(span);. I'm not sure that span_if_local will ever return None. It looks to be like it will always return Some since this check only fails if the duplicate is in the same crate. So the unwrap_or call should I think be something like expect("we should have found a local duplicate earlier"). Is my observation correct?

Second, rustc uses uncapitalized diagnostics by convention, so "Duplicate lang item found" should be "duplicate lang item found", "First defined here" should be "first defined here", "First defined in crate", should be "first defined in crate".

Next, in this note where the original def was in an external create, please do not call span_note, but just note, since the object of the note is not located at the span.

@GuillaumeGomez
Copy link
Member Author

@brson: Updated.

@brson
Copy link
Contributor

brson commented Mar 8, 2016

@bors r+

This looks really great now. Thanks for persevering.

@bors
Copy link
Contributor

bors commented Mar 8, 2016

📌 Commit 7ab9345 has been approved by brson

@bors
Copy link
Contributor

bors commented Mar 8, 2016

⌛ Testing commit 7ab9345 with merge 81e0323...

bors added a commit that referenced this pull request Mar 8, 2016
Add error file for E0152

It completes #31818.

However it is not complete yet:
 * test will need to be updated
 * the file name displayed is a bit too unclear.

I'm not sure yet what's the "correct" file name to display. If anyone has an idea on this, it'd be very appreciated.

r? @brson
@bors bors merged commit 7ab9345 into rust-lang:master Mar 8, 2016
@GuillaumeGomez GuillaumeGomez deleted the error_display branch March 8, 2016 13:58
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.

3 participants