Accept `Enum::<Param>::Variant {}` over `Enum::Variant::<Param> {}` #69363

estebank · 2020-02-21T23:53:40Z

src/test/ui/nll/user-annotations/pattern_substs_on_brace_enum_variant.rs

petrochenkov · 2020-02-23T16:35:18Z

So, I remember the rule here - paths in "value" contexts get all their omitted generic arguments filled with inference variables (_), but paths in "type" contexts get all their omitted generic arguments filled with defaults or errors if some necessary defaults don't exist.

All the examples in #69356 and in the tests are in "value" context.

I did touch the implementation of this at some point, but it was rewritten so many times since then that I have no idea how this works now.

src/librustc_ast_lowering/path.rs

src/librustc_typeck/astconv.rs

petrochenkov · 2020-02-23T17:33:06Z

IIRC, @varkor the author of the current implementation?
cc him then.

…es of path checking

estebank · 2020-02-25T01:08:59Z

I've got a commit to make the lint allow by default if that is preferable.

Edit: Given #69363 (comment) I guess we need to make it allow by default.

eddyb · 2020-02-25T18:51:35Z

src/librustc_ast_lowering/path.rs

+                        Res::Def(DefKind::Variant, def_id)
+                            if i + 2 == proj_start && segment.args.is_some() =>
+                        {
+                            // Handle `Enum::<Param>::Variant {}`.
+                            // We need to choose one here so that the method `res_to_ty` in
+                            // `astconv` works correctly, but it has to be *only* one, so track
+                            // whether we've already set the `DefId` in the previous segment.
+                            type_param_in_enum = true;
+                            Some(parent_def_id(self, def_id))
+                        }
+                        Res::Def(DefKind::Variant, def_id)
+                            if i + 1 == proj_start
+                                && (segment.args.is_some() || !type_param_in_enum) =>
+                        {
+                            // Handle `Enum::Variant::<Param> {}`.
                            Some(parent_def_id(self, def_id))
                        }


Two things here:

you can avoid mutable state by looking at p.segments

btw I doubt a bit that .args.is_some() is the correct way to check this, it might be true even for ::<> which I'm not sure if it's compatible with the similar check in typeck

these only make sense if the param mode is explicit, which it can never be for Expr::Struct right now (<Enum>::Variant {...} is not valid syntax, and Enum::Variant is not valid in type paths yet, only one of these two could actually be relevant here)

the comments referring to Enum::Variant {...} are therefore inaccurate IMO

this needs some extra checks, probably for the param mode being explicit

it's really mostly forward compat for enum variant types so you could just as well add an assert that DefKind::Variant parts are never ParamMode::Explicit

eddyb · 2020-02-25T18:53:19Z

src/librustc_session/lint/builtin.rs

@@ -531,6 +538,7 @@ declare_lint_pass! {
        PATTERNS_IN_FNS_WITHOUT_BODY,
        MISSING_FRAGMENT_SPECIFIER,
        LATE_BOUND_LIFETIME_ARGUMENTS,
+        TYPE_PARAM_ON_VARIANT_CTOR,


I'm not sure I would include CTOR in the name. Also, this isn't limited to type parameters.

I'd call it VARIANT_GENERICS or VARIANT_GENERIC_ARGS.

eddyb · 2020-02-25T19:25:09Z

src/librustc_typeck/astconv.rs

+    pub fn prohibit_multiple_params<'a, T: IntoIterator<Item = &'a hir::PathSegment<'a>>>(
+        &self,
+        segments: T,
+    ) -> bool {
+        let segments_with_params = segments
+            .into_iter()
+            .filter_map(|segment| segment.args.map(|arg| arg.span))
+            .collect::<Vec<_>>();
+        if segments_with_params.len() <= 1 {
+            return false;
+        }
+        self.tcx()
+            .sess
+            .struct_span_err(
+                segments_with_params,
+                "multiple segments with type parameters are not allowed",
+            )
+            .note("only a single path segment can specify type parameters")
+            .emit();
+        true
+    }


I think this is unnecessarily complicated, it doesn't distinguish this "enum vs variant" situation from others (e.g. trying to pass generic args to a mod) and it's wrong with generic associated items like methods or GATs.

This could be a help message on the error from prohibit_generics, such as pointing to the enum segment and saying "this segment already specified generic args for the enum" or something.

I think this is unnecessarily complicated, it doesn't distinguish this "enum vs variant" situation from others (e.g. trying to pass generic args to a mod) and it's wrong with generic associated items like methods or GATs.

That check is being done when called. If it were called without that check it would indeed be incorrect.

This could be a help message on the error from prohibit_generics, such as pointing to the enum segment and saying "this segment already specified generic args for the enum" or something.

Wanted to separate this from E0109 to potentially give it its own error code to explain that this will never be supported as opposed to the more generic current error.

to explain that this will never be supported

What do you mean? Generics on both the enum and a variant is a plausible feature (GADT), and generics on modules have been talked about in the past.

The error message looks pretty misleading to me, it seems to claim some absolute truth.

eddyb · 2020-02-25T19:26:17Z

src/librustc_typeck/astconv.rs

+            if sp.hi() == sp.lo() || sp == DUMMY_SP || sp.parent().is_some() {
+                // The params were not written by the user, but rather derived. These are expected.
+                // `Enum::Variant` where `Variant` has inferred lifetimes.
+                return;
+            }


I don't like this check, and you can get rid of the lifetimes for now (they shouldn't be added to value paths anyway).

Agree. It's hacky as hell.

you can get rid of the lifetimes for now

How?

they shouldn't be added to value paths anyway

I've spent too much time trying to figure out of a way of making this change without affecting currently accepted code.

Basically you want all Expr::Struct with variant paths to behave in HRI lowering like they did when you had removed the DefKind::Variant arm.
You don't want the elision lifetimes being injected at all.

They're for fn foo(x: &T) -> Enum::Variant, or perhaps <Enum>::Variant {...} (neither of which compile right now), not Enum::Variant {...}.

But if none of the PathSegments have the elision lifetimes injected (in src/librustc_ast_lowering/path.rs) then you get knock down errors about the type for a HirId not existing .

then you get knock down errors about the type for a HirId not existing

I don't recall you bringing this up, I thought it just worked while you had that arm removed? I don't see why it would, other value paths have no lifetimes injected into them at all.

bors · 2020-02-27T22:08:26Z

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

petrochenkov · 2020-02-29T16:55:07Z

r? @varkor

Dylan-DPC-zz · 2020-03-21T20:13:03Z

@estebank waiting on your answers to the latest questions on the above review. Thanks

Dylan-DPC-zz · 2020-04-09T18:00:43Z

Closing this due to inactivity

rust-highfive assigned petrochenkov Feb 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2020

estebank commented Feb 21, 2020

View reviewed changes

src/test/ui/nll/user-annotations/pattern_substs_on_brace_enum_variant.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

eddyb approved these changes Feb 23, 2020

View reviewed changes

petrochenkov reviewed Feb 23, 2020

View reviewed changes

src/librustc_ast_lowering/path.rs Show resolved Hide resolved

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2020

Treat ExprKind::Path and ExprKind::Struct the same for the purpos…

aafdbb6

…es of path checking

estebank force-pushed the variant-path-lt branch from ea43e14 to 20d2258 Compare February 24, 2020 23:19

estebank changed the title ~~[Do not merge yet] Treat ExprKind::Path and ExprKind::Struct the same for the purposes of path checking~~ Accept Enum::<Param>::Variant {} over Enum::Variant::<Param> {} Feb 24, 2020

Lint against E::V::<P>, suggest E::<P>::V

0339831

This comment has been minimized.

Sign in to view

Specific error for +1 path segments with params

9d58e12

estebank force-pushed the variant-path-lt branch from 20d2258 to 9d58e12 Compare February 25, 2020 00:32

Allow lint type_param_on_variant_ctor by default

63379aa

This comment has been minimized.

Sign in to view

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2020

estebank mentioned this pull request Feb 25, 2020

Possibly incorrect syntax Enum::<Type, Params>::Variant allowed since 1.33 #69356

Open

eddyb reviewed Feb 25, 2020

View reviewed changes

rust-highfive unassigned petrochenkov Feb 29, 2020

rust-highfive assigned varkor Feb 29, 2020

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2020

joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 30, 2020

joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2020

Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2020

Dylan-DPC-zz closed this Apr 15, 2020

ijackson mentioned this pull request Feb 18, 2023

bizarre error message for one of the universal constructor syntaxes #108224

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept `Enum::<Param>::Variant {}` over `Enum::Variant::<Param> {}` #69363

Accept `Enum::<Param>::Variant {}` over `Enum::Variant::<Param> {}` #69363

estebank commented Feb 21, 2020

This comment has been minimized.

petrochenkov commented Feb 23, 2020

petrochenkov commented Feb 23, 2020 •

edited

Loading

This comment has been minimized.

estebank commented Feb 25, 2020 •

edited

Loading

This comment has been minimized.

eddyb Feb 25, 2020 •

edited

Loading

eddyb Feb 25, 2020

eddyb Feb 25, 2020

estebank Feb 25, 2020

eddyb Feb 25, 2020

eddyb Feb 25, 2020

estebank Feb 25, 2020

eddyb Feb 25, 2020

estebank Feb 25, 2020

eddyb Feb 25, 2020

bors commented Feb 27, 2020

petrochenkov commented Feb 29, 2020

Dylan-DPC-zz commented Mar 21, 2020

Dylan-DPC-zz commented Apr 9, 2020

Accept Enum::<Param>::Variant {} over Enum::Variant::<Param> {} #69363

Accept Enum::<Param>::Variant {} over Enum::Variant::<Param> {} #69363

Conversation

estebank commented Feb 21, 2020

This comment has been minimized.

petrochenkov commented Feb 23, 2020

petrochenkov commented Feb 23, 2020 • edited Loading

This comment has been minimized.

estebank commented Feb 25, 2020 • edited Loading

This comment has been minimized.

eddyb Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 27, 2020

petrochenkov commented Feb 29, 2020

Dylan-DPC-zz commented Mar 21, 2020

Dylan-DPC-zz commented Apr 9, 2020

Accept `Enum::<Param>::Variant {}` over `Enum::Variant::<Param> {}` #69363

Accept `Enum::<Param>::Variant {}` over `Enum::Variant::<Param> {}` #69363

petrochenkov commented Feb 23, 2020 •

edited

Loading

estebank commented Feb 25, 2020 •

edited

Loading

eddyb Feb 25, 2020 •

edited

Loading