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 fix to error msg which relied on order #75444

Closed
wants to merge 1 commit into from

Conversation

JulianKnodt
Copy link
Contributor

Previously types needed to be ordered before consts. Since relaxing that restriction, this
created a bug wherever relying on that. This produces a strange error message, so it's not awful
but should still be fixed.

I rushed a bit to make this PR, but I'll look more closely if there's better ways to write tmrw morning.

r? @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2020
@JulianKnodt JulianKnodt force-pushed the quick_fix branch 6 times, most recently from cd8c288 to 6524c71 Compare August 18, 2020 05:18
@JulianKnodt
Copy link
Contributor Author

Currently this removes some errors emitted when there are fewer type arguments than parameters.

@JulianKnodt
Copy link
Contributor Author

JulianKnodt commented Aug 20, 2020

The code itself is quite nasty, but is this generally what we want to do? I can remove extra span messages which are semi redundant fairly easily.

If this is what we want to do, I'll try to clean up the code more

@bors

This comment has been minimized.

@JulianKnodt JulianKnodt force-pushed the quick_fix branch 2 times, most recently from 8914dee to 7672e43 Compare August 22, 2020 08:40
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| | |
| | expected type, found lifetime
| expected at most 1 type argument
Copy link
Contributor

@lcnr lcnr Aug 22, 2020

Choose a reason for hiding this comment

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

I would expect us to put types before constants here, so rn you recommend Example<'a, const, type> but Example<'a, type, const> would be better imo.

The "expected at most 1 type argument" also seems slightly incorrect here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah wait, I'm a bit confused, since the struct has the order <'a, type, const>, shouldn't it just stick to that order

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@JulianKnodt JulianKnodt force-pushed the quick_fix branch 2 times, most recently from 31c8a3f to 14ab09a Compare September 12, 2020 04:34
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-const-generics Area: const generics (parameters and arguments) const-generics-bad-diagnostics An error is correctly emitted, but is confusing, for `min_const_generics`. labels Sep 15, 2020
@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 15, 2020
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2020
@bors
Copy link
Contributor

bors commented Oct 20, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2020
@Dylan-DPC-zz
Copy link

@JulianKnodt can you rebless the tests (which should fix the conflicts) while wait for @lcnr to review? thanks

@JulianKnodt
Copy link
Contributor Author

Ah I think part of it was if it was unsure if we wanted to follow this path, but I'll update it.

Previously types needed to be ordered before consts. Since relaxing that restriction, this
created a bug wherever relying on that. This produces a strange error message, so it's not awful
but should still be fixed.

Add ParamKindOrd instead of &str

Switches from using strings to ParamKindOrd, as dealing with enums provides better guarantees to
not miss things.

Add test

Add new loop for checking types/consts

Update to check const & type in 1 loop
@bors
Copy link
Contributor

bors commented Nov 17, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@JulianKnodt
Copy link
Contributor Author

I believe this was implemented for the most part in #79032, so I'll close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) A-diagnostics Area: Messages for errors, warnings, and lints const-generics-bad-diagnostics An error is correctly emitted, but is confusing, for `min_const_generics`. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants