-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix #53525 - Unify E0243, E0244, E0087, E0088, E0089, and E0090 into E0107 #53584
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
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (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. |
r? @varkor |
referencing #53525, consolidating error codes E0087, E0088, E0089, E0090, E0243, and E0244 into E0107. Let me know if there are any changes I should make |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
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.
This is looking good so far! It looks like there are still some tests failing, which you'll need to update. Also, it's not linking the committer for each commit with your GitHub profile, which you might want to fix with a rebase.
@@ -1041,6 +1041,7 @@ enum NightsWatch {} | |||
"##, | |||
|
|||
E0087: r##" | |||
#### Note: this error code is no longer emitted by the compiler. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/librustc_typeck/diagnostics.rs
Outdated
@@ -1258,18 +1262,53 @@ extern "rust-intrinsic" { | |||
"##, | |||
|
|||
E0107: r##" | |||
This error means that an incorrect number of lifetime parameters were provided | |||
for a type (like a struct or enum) or trait: | |||
This error means that an incorrect number of type or lifetime parameters |
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.
Since we're going to be getting const parameters fairly soon, it's easier just to call them "generic arguments" rather than "type or lifetime parameters".
src/librustc_typeck/diagnostics.rs
Outdated
|
||
struct Bar { x: Foo } // error: wrong number of type arguments: | ||
// expected 1, found 0 | ||
struct Baz<S, T> { x: Foo<S, T> } // error: wrong number of type arugemtns: |
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.
*arguments
src/librustc_typeck/diagnostics.rs
Outdated
} | ||
``` | ||
|
||
```compile_fail,E0088 |
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.
Should be E0107
.
src/librustc_typeck/diagnostics.rs
Outdated
} | ||
``` | ||
|
||
```compile_fail,E0107 |
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.
These examples can be in the same code block. You don't need an example of less than/greater than the expected number for every single position — maybe one example for each position (type, function call, method call).
If you put the pull request description in the first comment (and title), it helps people to tell what the pull request is doing, at a glance, too. I think you'll need to put "Fix #53525" in the first comment for GitHub to automatically close the issue when this is merged. |
Noted. Will probably not get a chance to update this before this evening |
If you could also rebase so that the relevant changes were together, that would make for a cleaner history — maybe a commit for:
Thanks! |
8540117
to
e53cc0e
Compare
@varkor I rebased and changed order of code. Honestly not positive if I got it right since was my first time reordering git commits. Also still haven't fixed all failing errors so will do that tomorrow and will need to rebase again. Will update when its ready to review |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@mcr431: that looks good, thanks! (I think git has recorded users for the author/committer for your commits, which is why it's showing up as two different users on here — it's not a huge problem, but it might mean you're not attributed when the pull request gets merged — if you didn't find the rebase too much of a pain, you could try overwriting the author for each commit so that the right name/email was being used 😄.) |
Yeah I had set up a dummy account to test stuff and forgot to switch it back. When rebasing with fixed test cases I'll make sure it has proper author |
e53cc0e
to
ddf0a8f
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ah, the snippets in the diagnostics are also run by the test-suite, so you'll need to update the |
ddf0a8f
to
7f0311b
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
7f0311b
to
df00402
Compare
finally! |
@mcr431: that's looking great! If you could just fix the typo here, it'll be good to go! (You can do the other two comments as well, but they'll have to be modified when const generics are merged anyway, so it's fine to leave them like that for now). |
df00402
to
79afc6e
Compare
@varkor whoops! some commits got removed while rebasing. just made the changes again. Let me know if theres anything else |
Thanks @mcr431, that all looks great to me: nice work! @bors r+ |
📌 Commit 79afc6e has been approved by |
⌛ Testing commit 79afc6e with merge 7c94ead85a820e1d887b2a95b39bc48273250f36... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☀️ Test successful - status-appveyor, status-travis |
Fix #53525
This pr merges all errors related to too many or too few generic arguments in types and functions. E0243, E0244, E0087, E0088, E0089, E0090 errors will no longer be emitted and E0107 will be used instead.