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.

Sign up for GitHub

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

Jump to bottom

Edit error messages for rustc_resolve::AmbiguityKind variants #90075

Merged
merged 1 commit into from
Oct 26, 2021
Merged

Edit error messages for rustc_resolve::AmbiguityKind variants #90075

merged 1 commit into from
Oct 26, 2021

Conversation

pierwill
Copy link
Member

@pierwill pierwill commented Oct 20, 2021 •
edited
Loading

Edit the language of the ambiguity descriptions for E0659. These strings now appear as notes.

Closes #79717.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2021
@camelid camelid added the A-diagnostics Area: Messages for errors, warnings, and lints label Oct 20, 2021
@camelid
Copy link
Member

camelid commented Oct 20, 2021

@pierwill you need to bless tests :)

@camelid
Copy link
Member

camelid commented Oct 20, 2021

This new pattern should be easier for readers to follow. The parentheses answer the question "why is this ambiguous?" Answer: "[because of] conflicts with..."

We could consider going further: "foo is ambiguous due to conflicts with...". But maybe the current version you have balances concision and clarity better.

camelid
camelid reviewed Oct 20, 2021
View reviewed changes
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
@pierwill
Copy link
Member Author

I'll take another look at these, @camelid.

@pierwill pierwill requested a review from camelid October 20, 2021 01:55
@camelid
Copy link
Member

camelid commented Oct 20, 2021 •
edited
Loading

Stepping back a bit, I think it might be better if these notes were displayed as actual notes, rather than in parentheses in the main message. I'm not sure what others think, but do you want to try changing that and see how it looks?

Basically, I'm envisioning something like this:

error: `foo` is ambiguous
   --> $INFO
    |
$LL | $SRC_CODE
    |
    = note: ambiguous due to conflicts between ...

I think using a note would both be more consistent with other errors and make the error easier to read.

@petrochenkov
Copy link
Contributor

r? @petrochenkov

@pierwill pierwill requested a review from camelid October 20, 2021 16:29
@pierwill
Copy link
Member Author

Tests pass locally with err.note. 🎉

@camelid camelid self-assigned this Oct 20, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill pierwill requested a review from camelid October 20, 2021 18:42
@pierwill pierwill requested a review from camelid October 21, 2021 01:15
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pierwill pierwill force-pushed the fix-79717 branch 2 times, most recently from 27e9138 to 8b643a3 Compare October 24, 2021 18:15
camelid
camelid reviewed Oct 24, 2021
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

I left a few comments on some changes that I felt could be made more informative. In general, some of the existing descriptions included "during import/macro resolution" or similar, which seems unhelpful and confusing. @petrochenkov i see you've assigned yourself on this PR; what do you think of removing the "during import/macro resolution" part?

compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
@camelid camelid added the A-resolve Area: Name resolution label Oct 24, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
petrochenkov
petrochenkov reviewed Oct 25, 2021
View reviewed changes
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Sorry for more nitpicking, I just want to have comments from the previous round fully addressed (consistent wording and no /).

@pierwill
Copy link
Member Author

Thank you, @petrochenkov!

@pierwill pierwill requested a review from petrochenkov October 25, 2021 22:16
@rust-log-analyzer

This comment has been minimized.

Sign in to view
camelid
camelid approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

r=petrochenkov,camelid with the below nits addressed and petrochenkov's approval. Thanks for making this change!

compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
@camelid
Copy link
Member

camelid commented Oct 26, 2021

(Also, it looks like tests need to be blessed.)

@pierwill @camelid @petrochenkov
Edit error messages for rustc_resolve::AmbiguityKind variants
7de1ff1 
Emit description of the ambiguity as a note.

Co-authored-by: Noah Lev <[email protected]>
Co-authored-by: Vadim Petrochenkov <[email protected]>
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2021

📌 Commit 7de1ff1 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2021
@bors
Copy link
Contributor

bors commented Oct 26, 2021

⌛ Testing commit 7de1ff1 with merge c7a30c8...

@bors
Copy link
Contributor

bors commented Oct 26, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing c7a30c8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 26, 2021
@bors bors merged commit c7a30c8 into rust-lang:master Oct 26, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 26, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c7a30c8): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@pierwill pierwill deleted the fix-79717 branch October 26, 2021 14:54
@ehuss ehuss mentioned this pull request Jun 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid approved these changes

@petrochenkov petrochenkov Awaiting requested review from petrochenkov

Assignees

@petrochenkov petrochenkov

@camelid camelid

Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-resolve Area: Name resolution merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.

'ambiguous name' error message has weird wording
9 participants
@pierwill @rust-highfive @camelid @petrochenkov @rust-log-analyzer @estebank @bors @rust-timer @rustbot