-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Explain that in paths generics belong on the enum, not the variant #134981
base: master
Are you sure you want to change the base?
Conversation
HIR ty lowering was modified cc @fmease |
@@ -1118,6 +1118,14 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ { | |||
for (what, span) in types_and_spans { | |||
err.span_label(span, format!("not allowed on {what}")); | |||
} | |||
if segments.clone().any(|segment| { |
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.
if segments.clone().any(|segment| { | |
if segments.as_ref().any(|segment| { |
?
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.
segments is a
impl Iterator + Clone`, so cloning it is cheap and doesn't really allocate much 😅
If it was a Vec
, then I wouldn't need to clone it, it'd be .iter()
and nothing else.
@@ -279,6 +279,8 @@ LL | Enum::<()>::TSVariant::<()>(()); | |||
| --------- ^^ type argument not allowed | |||
| | | |||
| not allowed on tuple variant `TSVariant` | |||
| | |||
= note: type arguments are not allowed for enum variants; specify them on the enum itself |
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 doesn't add anything? Doesn't this just regurgitate the message and labels?
Also, generic arguments are permitted on variants — provided none are provided to the owning enum. Option::None::<i32>
is legal, so is Option::<i32>::None
. However Option::<i32>::None::<i32>
is not. I'd say this would be a helpful thing to point out (in a concise manner ofc). E.g., having a label pointing at the parent generic args (which we have here) stating sth. along the lines of "due to this" (er, you get my point).
(On mobile rn, couldn't look more carefully)
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.
Wanted to make the smallest diff that would address this. I agree that this should likely have a structured suggestion, like others in the test file, but that would have needed more code to address (I've noticed that the amount of wrangling needed for structured suggestions pushes "obviously good wanted changes" to "we might not want to land this").
I also tried getting the path segment for the enum, but the Res
wasn't Def(Enum, _)
as I expected so left it as a note for now. But sure, we can not land this, it's not big loss.
``` error[E0109]: type arguments are not allowed on tuple variant `TSVariant` --> $DIR/enum-variant-generic-args.rs:54:29 | LL | TSVariant::<()>(()); | --------- ^^ type argument not allowed | | | not allowed on tuple variant `TSVariant` | = note: type arguments are not allowed for enum variants; specify them on the enum itself ``` Fix rust-lang#93993.
12aa567
to
7de7c36
Compare
Fix #93993.