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 more context to E0599 errors #69255

Merged
merged 17 commits into from
Feb 29, 2020
Merged

Add more context to E0599 errors #69255

merged 17 commits into from
Feb 29, 2020

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Feb 18, 2020

Point at the intermediary unfulfilled trait bounds.

Fix #52523, fix #61661, cc #36513, fix #68131, fix #64417, fix #61768, cc #57457, cc #9082, fix #57994, cc #64934, cc #65149.

@rust-highfive
Copy link
Collaborator

r? @varkor

(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 Feb 18, 2020
src/librustc_typeck/check/method/suggest.rs Outdated Show resolved Hide resolved
Comment on lines +2 to +4
// ignore-i586-unknown-linux-gnu
// ignore-i586-unknown-linux-musl
// ignore-i686-unknown-linux-musl
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unfortunate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least we're tracking the underlying issue now :o)

Comment on lines 1 to 9
error[E0599]: no associated item named `ID` found for type `i32` in the current scope
--> $DIR/associated-const-no-item.rs:5:23
|
LL | trait Foo {
| --------- `Foo` defines an item `ID`
...
LL | const X: i32 = <i32>::ID;
| ^^ associated item not found in `i32`
|
= help: items from traits can only be used if the trait is implemented and in scope
= note: the following trait defines an item `ID`, perhaps you need to implement it:
candidate #1: `Foo`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: we should determine whether the implementing trait is in scope. If it isn't, suggest importing it. If it is, check whether the found adt implements it directly but the bounds weren't fulfilled. If not, suggest implementing it.

@estebank
Copy link
Contributor Author

This PR now accounts for arbitrary self types when reporting E0599, but it doesn't help when the trait definition is not crate local.

The suggestion to constrain type parameters is not as good as I would wish (it should suggest constraining the impl directly), but for newcomers it at least leads them in the right direction (they get hit with a second wave of suggestions that when applied make the code compile).

src/test/ui/derives/derive-assoc-type-not-impl.stderr Outdated Show resolved Hide resolved
src/test/ui/issues/issue-31173.stderr Outdated Show resolved Hide resolved
src/test/ui/issues/issue-5153.stderr Show resolved Hide resolved
| --------- `Bar` defines an item `foo`, perhaps you need to implement it
...
LL | self.foo();
| ^^^ method not found in `&Foo<T>`
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go before both the struct and trait above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmandry What do you think of this?

Screen Shot 2020-02-24 at 6 53 40 PM

Copy link
Member

Choose a reason for hiding this comment

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

This looks really, really good!

My only critique is that the two notes (one on the span, one outside) are redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the new commit, this test looks like this now:

Screen Shot 2020-02-24 at 10 38 59 PM

It is a very niche case, but with this we handle it well enough, I think.

Copy link
Member

Choose a reason for hiding this comment

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

Great! And I forgot to say again that I think the order of the spans ought to be reversed :)

@bors

This comment has been minimized.

= note: the method `foo` exists but the following trait bounds were not satisfied:
`T: std::default::Default`
which is required by `Fin<T>: Bar`
help: consider restricting the type parameter to satisfy the obligation
Copy link
Member

@csmoe csmoe Feb 25, 2020

Choose a reason for hiding this comment

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

consider rewording obligations to requirements(or a better alternative)? as obligation isn't a common term outside the internal rustc.
ps. a google search result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about "consider restricting the type parameter to satisfy the trait bound"?

@@ -579,6 +579,20 @@ impl<'tcx, N> Vtable<'tcx, N> {
}
}

pub fn borrow_nested_obligations(&self) -> &[N] {
match &self {
VtableImpl(i) => &i.nested[..],
Copy link
Member

Choose a reason for hiding this comment

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

Huh, these are unpleasant variant names.

@varkor
Copy link
Member

varkor commented Feb 27, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Feb 27, 2020

📌 Commit c02e56a has been approved by varkor

@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 Feb 27, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2020
bors added a commit that referenced this pull request Feb 27, 2020
Rollup of 8 pull requests

Successful merges:

 - #60826 (Implement new gdb/lldb pretty-printers)
 - #69255 (Add more context to E0599 errors)
 - #69379 (Fail on multiple declarations of `main`.)
 - #69430 (librustc_typeck: remove loop that never actually loops)
 - #69449 (Do not ping PR reviewers in toolstate breakage)
 - #69491 (rustc_span: Add `Symbol::to_ident_string` for use in diagnostic messages)
 - #69495 (don't take redundant references to operands)
 - #69496 (use find(x) instead of filter(x).next())

Failed merges:

r? @ghost
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 28, 2020
@JohnTitor
Copy link
Member

Failed in #69543 (comment)
@bors r-

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

@bors r=varkor rollup=never

@bors
Copy link
Contributor

bors commented Feb 28, 2020

📌 Commit 2fb35ad has been approved by varkor

@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 Feb 28, 2020
@Dylan-DPC-zz
Copy link

@bors p=1

@bors
Copy link
Contributor

bors commented Feb 29, 2020

⌛ Testing commit 2fb35ad with merge 55aee8d...

@bors
Copy link
Contributor

bors commented Feb 29, 2020

☀️ Test successful - checks-azure
Approved by: varkor
Pushing 55aee8d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 29, 2020
@bors bors merged commit 55aee8d into rust-lang:master Feb 29, 2020
@jonhoo
Copy link
Contributor

jonhoo commented Mar 3, 2020

This appears to have caused a regression:
#69629 (comment)

estebank added a commit to estebank/rust that referenced this pull request Mar 4, 2020
bors added a commit that referenced this pull request Mar 5, 2020
Correctly reject `TraitCandidate` in all cases

Follow up to #69255, addresses #69629.

When `self.select_trait_candidate(trait_ref)` returned `Err(_)`, `result` wasn't being set to `NoMatch`, causing invalid methods to be selected.
@estebank estebank deleted the e0599-details branch November 9, 2023 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
10 participants