Skip to content

Fix some simple associated const issues. - #25065

Merged
bors merged 2 commits into
rust-lang:masterfrom
quantheory:fix_associated_const_ambiguity_message
May 14, 2015
Merged

Fix some simple associated const issues.#25065
bors merged 2 commits into
rust-lang:masterfrom
quantheory:fix_associated_const_ambiguity_message

Conversation

@quantheory

@quantheoryquantheory commented May 3, 2015

Copy link
Copy Markdown
Contributor

This fixes#24922 and #25017, and reduces the number of error messages that talk about "methods" when associated constants rather than methods are involved.

I will admit that I haven't thought very carefully about the error messages. My goal has been to make more of the messages technically correct in all situations, and to avoid ICEs. But in some cases we could probably talk specifically about "methods" rather than "items".

@rust-highfive

rust-highfive commented May 3, 2015

Copy link
Copy Markdown
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

alexcrichton commented May 3, 2015

Copy link
Copy Markdown
Member

r? @nikomatsakis

@quantheoryquantheory mentioned this pull request May 4, 2015
Merged
@nikomatsakis

nikomatsakis commented May 5, 2015

Copy link
Copy Markdown
Contributor

This all seems good to me modulo that one error message (which also happens to be pretty frequent). I think we should ensure it's phrased in as comprehensible a way as possible. I'm now thinking:

No method {} found for type {} in the current scope

or, when not doing a method lookup,

No item {} found for type {} in the current scope

@quantheory

quantheory commented May 5, 2015

Copy link
Copy Markdown
ContributorAuthor

@nikomatsakis Better?

@nikomatsakis

nikomatsakis commented May 11, 2015

Copy link
Copy Markdown
Contributor

@quantheory much. Sorry for being slow, was on partial vacation last week.

@bors r+

@bors

bors commented May 11, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit a5e6075 has been approved by nikomatsakis

@bors

bors commented May 11, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit a5e6075 with merge 1eb21ca...

@bors

bors commented May 11, 2015

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton

alexcrichton commented May 11, 2015

Copy link
Copy Markdown
Member

Perhaps a rebase for this PR is in order? Failures like that generally don't turn out to be false negatives unfortunately.

@oli-obk

oli-obk commented May 11, 2015

Copy link
Copy Markdown
Contributor

i tried it locally, even with the rebase that line has to go.

@quantheory
quantheoryforce-pushed the fix_associated_const_ambiguity_message branch 2 times, most recently from 94c3e8a to 5beaeabCompareMay 11, 2015 19:23
@quantheory

quantheory commented May 11, 2015

Copy link
Copy Markdown
ContributorAuthor

I haven't had the chance to do a full make check, but I tried just rebasing and fixing the one line, if you want to give it another go.

@quantheory

quantheory commented May 12, 2015

Copy link
Copy Markdown
ContributorAuthor

The version here passed make check as well.

@alexcrichton

alexcrichton commented May 12, 2015

Copy link
Copy Markdown
Member

@bors: r=nikomatsakis 5beaeab

@bors

bors commented May 12, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 5beaeab with merge b5277ba...

@bors

bors commented May 12, 2015

Copy link
Copy Markdown
Collaborator

⛄ The build was interrupted to prioritize another pull request.

@bors

bors commented May 12, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 5beaeab with merge 543ef45...

@bors

bors commented May 12, 2015

Copy link
Copy Markdown
Collaborator

⛄ The build was interrupted to prioritize another pull request.

@bors

bors commented May 13, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 5beaeab with merge 4a5e4cd...

@bors

bors commented May 13, 2015

Copy link
Copy Markdown
Collaborator

⛄ The build was interrupted to prioritize another pull request.

@bors

bors commented May 13, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 5beaeab with merge 65ea478...

@bors

bors commented May 13, 2015

Copy link
Copy Markdown
Collaborator

⛄ The build was interrupted to prioritize another pull request.

@bors

bors commented May 13, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 5beaeab with merge bcc76d4...

@bors

bors commented May 13, 2015

Copy link
Copy Markdown
Collaborator

⛄ The build was interrupted to prioritize another pull request.

@bors

bors commented May 13, 2015

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #24619) made this pull request unmergeable. Please resolve the merge conflicts.

@quantheory
quantheoryforce-pushed the fix_associated_const_ambiguity_message branch from 5beaeab to 29b31d0CompareMay 14, 2015 00:08
@quantheory
Fix ICE that occurs when an associated const is ambiguous.
b4bbf3a
Also change several error messages to refer to "items" rather than
"methods", since associated items that require resolution during type
checking are not always methods.
@quantheory
quantheoryforce-pushed the fix_associated_const_ambiguity_message branch from 29b31d0 to b4bbf3aCompareMay 14, 2015 00:10
@quantheory

quantheory commented May 14, 2015

Copy link
Copy Markdown
ContributorAuthor

Rebased, passes make check.

@alexcrichton

alexcrichton commented May 14, 2015

Copy link
Copy Markdown
Member

@bors: r=nikomatsakis b4bbf3a

@bors

bors commented May 14, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit b4bbf3a with merge 1b5b639...

bors added a commit that referenced this pull request May 14, 2015
@bors
Auto merge of#25065- quantheory:fix_associated_const_ambiguity_mess…
1b5b639
…age, r=nikomatsakis
This fixes#24922 and #25017, and reduces the number of error messages that talk about "methods" when associated constants rather than methods are involved.
I will admit that I haven't thought very carefully about the error messages. My goal has been to make more of the messages technically correct in all situations, and to avoid ICEs. But in some cases we could probably talk specifically about "methods" rather than "items".
@bors

bors commented May 14, 2015

Copy link
Copy Markdown
Collaborator

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

@bors
bors merged commit b4bbf3a into rust-lang:masterMay 14, 2015
@quantheoryquantheory mentioned this pull request May 14, 2015
Closed
bors added a commit that referenced this pull request May 26, 2015
@bors
Auto merge of#25091- quantheory:trait_associated_const_fixes, r=nik…
0ea80fa
…omatsakis
Closes#25046 (by rejecting the code that causes the ICE) and #24946. I haven't been able to deal with the array size or recursion issues yet for associated consts, though my hope was that the change I made for range match patterns might help with array sizes, too.
This PR is pretty much orthogonal to #25065.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nikomatsakisnikomatsakis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ICE on ambiguous associated const reference

6 participants

@quantheory@rust-highfive@alexcrichton@nikomatsakis@bors@oli-obk