-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Include type of missing trait methods in error #37370
Include type of missing trait methods in error #37370
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
22479b0
to
77d6892
Compare
@emilio would you mind converting the affected tests to ui tests? or at least some representative set? It's a bit hard for me to visualize the formatting currently =) |
@nikomatsakis will do. Should I duplicate the tests, adding UI tests that match the current ones, or just replace the current tests with the new ones? |
@estebank any tests you move to ui tests can be removed from |
395bed6
to
02209f9
Compare
@nikomatsakis I've moved the tests from compile-fail to ui. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @estebank -- think you could add a few more tests (see comments)?
@@ -0,0 +1,56 @@ | |||
error[E0323]: item `bar` is an associated const, which doesn't match its trait `<FooConstForMethod as Foo>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, I think, but this text "<FooConstForMethod as Foo>
" seems pretty funny to me; I expect to see just Foo
... is there an open issue on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After going through the open tickets, I don't think there's one for this, so I opened one.
30 | impl Deref for Thing { | ||
| ^ missing `Target` in implementation | ||
| | ||
= note: `Target` from trait: `type Target;` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a test case where the item from the other trait is a fn
with a non-trivial signature?
Probably worth testing the constant case too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test/run-make/missing-items
covering all three cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait...why did you make it a run-make
test? (Those are generally frowned upon, since they don't fit the usual scheme.) But in particular, this all seems like a good candidate for a ui test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duly noted, I didn't know they were frowned upon.
In order for this codepath to be executed the trait shouldn't have a span available, which as far as I know only happens when compiling against an extern crate. I hadn't noticed that test/ui/cross-crate-macro-backtrace does exactly that. I'll change this to a ui test.
Provide either a span pointing to the original definition of missing trait items, or a message with the inferred definitions.
02209f9
to
40c2c0f
Compare
@bors r+ |
📌 Commit 40c2c0f has been approved by |
Nice work @estebank =) |
…-back, r=nikomatsakis Include type of missing trait methods in error Provide either a span pointing to the original definition of missing trait items, or a message with the inferred definitions. Fixes rust-lang#24626. Follow up to PR rust-lang#36371. If PR rust-lang#37369 lands, missing trait items that present a multiline span will be able to show the entirety of the item definition on the error itself, instead of just the first line.
Hm looks like the run-make test here is failing on nightly, preventing nightlies. Still investigating why. |
cc @brson |
I tried to reproduce what the Makefile does,
gives the folloving error:
So I thought it's a linking error. And indeed, EDIT: Or the stuff in tools.mk should take care of this somehow?
The note is missing. |
Provide either a span pointing to the original definition of missing
trait items, or a message with the inferred definitions.
Fixes #24626. Follow up to PR #36371.
If PR #37369 lands, missing trait items that present a multiline span will be able to show the entirety of the item definition on the error itself, instead of just the first line.