-
Notifications
You must be signed in to change notification settings - Fork 13.3k
rustc: catch non-trait methods before typeck. #14974
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
Conversation
Not sure to remove the potentially dead code in typeck, or update it to a compiler bug (should never be reached), or just leave it? See here. |
It looks like travis found a test case that also needs to be altered. It'd ok to leave the "dead code" for now, but you can change it to |
@alexcrichton yep, fixed up the travis problems, had to move another error to resolve too, else it's confusing to the user of what they did wrong in some scenarios. Should be grand now other than a lame span for the typedef case. |
@@ -9,6 +9,7 @@ | |||
// except according to those terms. | |||
|
|||
trait I {} | |||
type K = I; //~ ERROR: reference to trait | |||
type K = I; //FIXME: Get note to appear here ~ ERROR: reference to trait |
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.
What's going on here?
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.
Previously it would error after resolve on this line due to using a typedef for a trait. I've changed to a note //~^ NOTE: type aliases cannot be used for traits
but the span is on the impl.
If I drop the additional note and leave just a 'x is not a trait' then a user might still be confused, so it helps.
The 'x is not a trait' error was moved to resolve, as otherwise the user would encounter the 'method is not a member of trait' error, which, if they've mixed up the syntax, will be difficult for them to figure out what's wrong. (If they have impl type for trait
)
I believe the span could be improved, but I don't feel it's a blocker as the note should give enough information to correct (and is an improvement over the old error message).
But on those lines, the 'reference to trait' error should still popup if there's a typedef for a trait which isn't used in an impl, I can add a new test for that case, as the initial reason for testing that is that it used to ICE (according to the issue).
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.
Ok, could you ensure that there's a test case for type Foo = Trait
yielding an error, and remove the FIXME
annotations in the tests?
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.
@alexcrichton Done.
This looks pretty good, but the lack of |
Properly format documentation for `SignatureHelpRequest`s Properly formats function documentation instead of returning it raw when responding to `SignatureHelpRequest`s. I added a test in `crates/rust-analyzer/tests/slow-tests/main.rs` -- not sure if this is the best location given the relevant code is in `crates/rust-analyzer` or if it's possible to test in a less heavyweight manner. Closes rust-lang#14958
Closes #3973.