Skip to content

structured suggestion for E0223 ambiguous associated type #54980

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
Merged
merged 1 commit into from
Oct 13, 2018
Merged

structured suggestion for E0223 ambiguous associated type #54980

merged 1 commit into from
Oct 13, 2018

Conversation

zackmdavis
Copy link
Member

(routine (and when are we going to be done finding these, anyway?) but something that stuck out to me while glancing at #54970)

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2018
@zackmdavis
Copy link
Member Author

Wait, @estebank is currently the assignee for four other open PRs; we can do reviewer load-balancing better than this

r? @davidtwco

@rust-highfive rust-highfive assigned davidtwco and unassigned estebank Oct 11, 2018
oli-obk
oli-obk reviewed Oct 11, 2018
View reviewed changes
src/librustc_typeck/astconv.rs Outdated
@@ -1093,10 +1093,12 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx>+'o {
name: &str) {
struct_span_err!(self.tcx().sess, span, E0223, "ambiguous associated type")
.span_label(span, "ambiguous associated type")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this label which simply repeats the error message.

Copy link
Member

@davidtwco davidtwco Oct 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, with the change to a suggestion, this no longer improves the error.

estebank
estebank reviewed Oct 11, 2018
View reviewed changes
src/test/ui/error-codes/E0223.stderr Outdated
| ^^^^^^^^^^
| |
| ambiguous associated type
| help: use fully-qualified syntax: `<Type as MyTrait>::X`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Type a default? If it is, then it shouldn't be machine applicable in those cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch ⚾ 💔

src/test/ui/did_you_mean/bad-assoc-ty.stderr Outdated
| ^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| ambiguous associated type
| help: use fully-qualified syntax: `<(dyn std::marker::Send + 'static) as Trait>::AssocTy`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you verify that this suggestion is actually accepted code?

@zackmdavis
structured suggestion for E0223 ambiguous associated type
a5de379 
It looks like we tend to use angle-brackets around the placeholder in
the few other places we use `Applicability::HasPlaceholders`, but that
would be confusing here, so ...
@zackmdavis zackmdavis force-pushed the and_the_case_of_the_universal_call branch from 7deaef2 to a5de379 Compare October 12, 2018 04:15
@zackmdavis
Copy link
Member Author

It turns out that I was super-careless yesterday and failed to notice that the suggestions here actually contain placeholders (1 2). It's lucky that we have Applicability::HasPlaceholders for this. It looks like the few other places where we use this surround the placeholders with angle brackets (e.g., 1 2), but that would be really confusing in this context, so I'm just going to leave them as Type and Trait. (Force-pushed.)

@estebank
Copy link
Contributor

Lgtm

@oli-obk
Copy link
Contributor

oli-obk commented Oct 12, 2018

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 12, 2018

📌 Commit a5de379 has been approved by oli-obk

@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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2018
@bors
Copy link
Collaborator

bors commented Oct 13, 2018

⌛ Testing commit a5de379 with merge fb3b47a...

bors added a commit that referenced this pull request Oct 13, 2018
@bors
Auto merge of #54980 - zackmdavis:and_the_case_of_the_universal_call,…
fb3b47a 
… r=oli-obk

structured suggestion for E0223 ambiguous associated type

(routine (and when are we going to be done finding these, anyway?) but something that stuck out to me while glancing at #54970)

r? @estebank
@bors
Copy link
Collaborator

bors commented Oct 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing fb3b47a to master...

@bors bors merged commit a5de379 into rust-lang:master Oct 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@davidtwco davidtwco davidtwco left review comments

@estebank estebank estebank left review comments

Assignees

@davidtwco davidtwco

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zackmdavis @estebank @oli-obk @bors @davidtwco @rust-highfive