Correct generic parameter ordering in error note for E0747 #72848
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (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. |
src/librustc_typeck/astconv.rs
Outdated
| "lifetime" => ParamKindOrd::Lifetime, | ||
| "type" => ParamKindOrd::Type, | ||
| "constant" => ParamKindOrd::Const, | ||
| _ => panic!(), |
There was a problem hiding this comment.
Is this the right way to trigger an ICE?
There was a problem hiding this comment.
I believe that bug! is the preferred way to trigger an ICE.
In this case, I would recommend changing opt_span_bug_fmt to take the original GenericParamDefKind, rather than its string description. That will allow you to write an exhaustive match, removing the need for an explicit panic!/bug!
There was a problem hiding this comment.
Thanks for the tip! I'm new to contributing to rustc, so could you describe the change you're recommending in more detail?
There was a problem hiding this comment.
On closer examination, it turns out that the two callers of this function actually have two different "original" types: GenericParamDefKind and GenericArg.
I believe your change is actually perfectly correct.
There was a problem hiding this comment.
I went through the same thought process @Aaron1011. It would be a little cleaner to pass ParamKindOrd directly, but I'm not sure it's worth making more changes.
There was a problem hiding this comment.
Is it worth opening an issue?
There was a problem hiding this comment.
It would be easy to change in this pull request if desired, but I'm not sure it's worth doing: it adds quite a bit of boilerplate code, and it's only used in this one case. If we ever find ourselves doing something similar again in the future, we should revisit it.
Co-authored-by: varkor <[email protected]>
|
📌 Commit fd76d23 has been approved by |
|
Thanks for reviewing it! :) |
Co-authored-by: hafiz <[email protected]>
|
@varkor could you re-approve this? Thanks! |
|
@bors r+ |
|
📌 Commit 56f87ef has been approved by |
Rollup of 5 pull requests Successful merges: - rust-lang#72704 (Remote testing fixes) - rust-lang#72820 (InstCombine: Don't optimize `&mut *x` into `x`) - rust-lang#72848 (Correct generic parameter ordering in error note for E0747) - rust-lang#72902 (Add a test to ensure Fuse stays covariant) - rust-lang#72921 (Add assert to Vec with_capacity docs) Failed merges: r? @ghost
Fixes #72815.
r? @varkor