Improve compiler error message for wrong generic parameter order #72271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-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 |
|
You'll need to add a regression test in |
|
Okay. But is my approach right though? |
lcnr
reviewed
May 17, 2020
|
@rakshith-ravi: this looks like the right idea, yes! I'll leave some more detailed comments soon. I would just use one |
|
#72441 will definitely interfere with this unfortunately |
|
Sounds fun. I'm looking forward to it xD Can you please help me understand what exactly that PR does? |
|
@rakshith-ravi Yeah definitely! Are you active on the |
|
Yup. You'll find me as |
|
@doctorn I've merged your branch into mine. So all merge conflicts are now resolved. You can merge this into master once #72441 gets merged. @varkor I've added the changes you suggested as well. Sorting it out with a sort function call now, and removed those 3 Still need to update the tests. Will get on that soon |
|
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 |
varkor
reviewed
May 24, 2020
rakshith-ravi
changed the title
|
@rakshith-ravi: once #72441 is merged, could you rebase over it? That would make it easier to review changes (it's frustrating that GitHub doesn't support dependent pull requests). |
|
Ignore me |
doctorn
reviewed
May 30, 2020
| @@ -133,6 +134,7 @@ LL | struct Dl<'a, 'b, 'c, T, U, V, M: ThreeWithLifetime<T, 'a, A=(), B=(), U, ' | |||
| | ^^ | |||
| | | |||
| = note: type arguments must be provided before lifetime arguments | |||
There was a problem hiding this comment.
Wait hold on, shouldn't type arguments be provided after lifetime arguments?
There was a problem hiding this comment.
Huh. Yes, they should be. But that didn't come from my PR though, so I'm scared to touch it. 😆
I mean, I know exactly where this error is thrown, but I'm not sure I should be doing it lol
|
☔ The latest upstream changes (presumably #72778) made this pull request unmergeable. Please resolve the merge conflicts. |
varkor
suggested changes
May 31, 2020
There was a problem hiding this comment.
This is looking good, thanks @rakshith-ravi! Just a few comments!
src/test/ui/const-generics/const-arg-type-arg-misordered.stderr
Outdated
Show resolved
Hide resolved
|
☔ The latest upstream changes (presumably #72935) made this pull request unmergeable. Please resolve the merge conflicts. |
|
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 |
varkor
reviewed
Jun 3, 2020
|
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 |
|
Sorry about the spam, my laptop isn't good enough for me to build this repo, so I'm trying my best to figure out the code without building it. |
|
You can use |
varkor
suggested changes
Jun 12, 2020
src/librustc_typeck/astconv.rs
Outdated
| Self::generic_arg_mismatch_err(tcx.sess, arg, kind.descr()); | ||
| // We're going to iterate over the parameters to sort them out, and | ||
| // show that order to the user as a possible order for the parameters | ||
| let mut ordered_params = defs.params.clone(); |
There was a problem hiding this comment.
Instead of two very similar match conditions, could we do something like the following?
let (ordered_params, mut params_present) = defs.params.clone().map(|param| (param, match param.kind { … })).sort_by_key(|(_, ord)| ord);
params_present.dedup();There was a problem hiding this comment.
This was marked as resolved, but we're still matching on param.kind twice?
There was a problem hiding this comment.
Yeah the code mentioned there doesn't seem to compile. It seems to complain something about not being able to convert from map to tuple
There was a problem hiding this comment.
I couldn't quite think off the top of my head what the types would be, so it may require a little adaption, but I think it would be good to only match on param.kind once if that results in cleaner code.
There was a problem hiding this comment.
Alright, cool. I'll come up with something
There was a problem hiding this comment.
Hey @varkor. I'm getting this error:
the trait `std::hash::Hash` is not implemented for `rustc_middle::ty::GenericParamDef`
when trying to insert a GenericParamDef into a hashset.
Any idea how I can overcome this?
There was a problem hiding this comment.
My idea was not to use a hash set any more: we're using a vector instead, but because it's sorted, once we dedup, the elements will be unique.
|
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 |
varkor
reviewed
Jun 16, 2020
|
See if you can get something like this to work. In any case, I think it should be good after that, so could you squash your commits down to one or two commits to keep the history clean, and then I think it's ready to merge :) |
varkor
reviewed
Jun 17, 2020
src/librustc_typeck/astconv.rs
Outdated
| }) | ||
| .collect::<Vec<(ParamKindOrd, GenericParamDef)>>(); | ||
| param_types_present.sort_by_key(|(ord, _)| *ord); | ||
| let mut param_types_present = param_types_present |
There was a problem hiding this comment.
Can you simply use unzip for this part?
|
One more comment; sorry for being so picky! |
|
No worries. Sorry, I didn't know |
|
@rakshith-ravi: great, thanks for your hard work! Could you squash your changes down to 1 or 2 commits? After that, it's good to be merged :) |
|
Squashed 😄 |
|
Thanks @rakshith-ravi! @bors r+ rollup |
|
📌 Commit 0624a5a has been approved by |
bors
added
S-waiting-on-bors
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 21, 2020
Improve compiler error message for wrong generic parameter order - Added optional "help" parameter that shows a help message on the compiler error if required. - Added a simple ordered parameter as a sample help. @varkor will make more changes as required. Let me know if I'm heading in the right direction. Fixes rust-lang#68437 r? @varkor
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 21, 2020
Improve compiler error message for wrong generic parameter order - Added optional "help" parameter that shows a help message on the compiler error if required. - Added a simple ordered parameter as a sample help. @varkor will make more changes as required. Let me know if I'm heading in the right direction. Fixes rust-lang#68437 r? @varkor
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Jun 22, 2020
Improve compiler error message for wrong generic parameter order - Added optional "help" parameter that shows a help message on the compiler error if required. - Added a simple ordered parameter as a sample help. @varkor will make more changes as required. Let me know if I'm heading in the right direction. Fixes rust-lang#68437 r? @varkor
This was referenced Jun 22, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 23, 2020
…arth Rollup of 9 pull requests Successful merges: - rust-lang#72271 (Improve compiler error message for wrong generic parameter order) - rust-lang#72493 ( move leak-check to during coherence, candidate eval) - rust-lang#73398 (A way forward for pointer equality in const eval) - rust-lang#73472 (Clean up E0689 explanation) - rust-lang#73496 (Account for multiple impl/dyn Trait in return type when suggesting `'_`) - rust-lang#73515 (Add second message for LiveDrop errors) - rust-lang#73567 (Clarify --extern documentation.) - rust-lang#73572 (Fix typos in doc comments) - rust-lang#73590 (bootstrap: no `config.toml` exists regression) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@varkor will make more changes as required. Let me know if I'm heading in the right direction.
Fixes #68437
r? @varkor