From 532671b8eee37199ae9e41d377a82a910c6e25b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Esteban=20K=C3=BCber?= <esteban@kuber.com.ar>
Date: Tue, 31 May 2022 12:53:52 -0700
Subject: [PATCH] More accurately handle suggestions

* Confirm the path segment being modified is an `enum`
* Check whether type has type param before suggesting changing `Self`
* Wording changes
* Add clarifying comments
* Suggest removing args from `Self` if the type doesn't have type params
---
 compiler/rustc_typeck/src/astconv/mod.rs      | 110 +++++++++++++-----
 src/test/ui/structs/struct-path-self.stderr   |  26 +++--
 .../enum-variant-generic-args.stderr          |  24 ++--
 3 files changed, 106 insertions(+), 54 deletions(-)

diff --git a/compiler/rustc_typeck/src/astconv/mod.rs b/compiler/rustc_typeck/src/astconv/mod.rs
index 56a4025f297..4555e31016f 100644
--- a/compiler/rustc_typeck/src/astconv/mod.rs
+++ b/compiler/rustc_typeck/src/astconv/mod.rs
@@ -1803,52 +1803,84 @@ pub fn associated_path_to_ty(
                         tcx.check_stability(variant_def.def_id, Some(hir_ref_id), span, None);
                         self.prohibit_generics(slice::from_ref(assoc_segment).iter(), |err| {
                             err.note("enum variants can't have type parameters");
-                            let type_name = tcx.opt_item_name(adt_def.did());
-                            let the_enum = type_name.map(|n| format!("enum `{n}`")).unwrap_or_else(|| "the enum".to_string());
-                            let msg = format!("you might have meant to specity type parameters on {the_enum}");
+                            let type_name = tcx.item_name(adt_def.did());
+                            let msg = format!(
+                                "you might have meant to specity type parameters on enum \
+                                 `{type_name}`"
+                            );
                             let Some(args) = assoc_segment.args else { return; };
+                            // Get the span of the generics args *including* the leading `::`.
                             let args_span = assoc_segment.ident.span.shrink_to_hi().to(args.span_ext);
+                            if tcx.generics_of(adt_def.did()).count() == 0 {
+                                // FIXME(estebank): we could also verify that the arguments being
+                                // work for the `enum`, instead of just looking if it takes *any*.
+                                err.span_suggestion_verbose(
+                                    args_span,
+                                    &format!("{type_name} doesn't have type parameters"),
+                                    String::new(),
+                                    Applicability::MachineApplicable,
+                                );
+                                return;
+                            }
                             let Ok(snippet) = tcx.sess.source_map().span_to_snippet(args_span) else {
                                 err.note(&msg);
                                 return;
                             };
-                            let (qself_sugg_span, is_self) = if let hir::TyKind::Path(hir::QPath::Resolved(_, ref path)) = qself.kind {
+                            let (qself_sugg_span, is_self) = if let hir::TyKind::Path(
+                                hir::QPath::Resolved(_, ref path)
+                            ) = qself.kind {
                                 // If the path segment already has type params, we want to overwrite
                                 // them.
                                 match &path.segments[..] {
-                                    [.., segment, _] => (
-                                        segment.ident.span.shrink_to_hi().to(segment.args.map_or(
-                                            segment.ident.span.shrink_to_hi(),
+                                    // `segment` is the previous to last element on the path,
+                                    // which would normally be the `enum` itself, while the last
+                                    // `_` `PathSegment` corresponds to the variant.
+                                    [.., hir::PathSegment {
+                                        ident,
+                                        args,
+                                        res: Some(Res::Def(DefKind::Enum, _)),
+                                        ..
+                                    }, _] => (
+                                        // We need to include the `::` in `Type::Variant::<Args>`
+                                        // to point the span to `::<Args>`, not just `<Args>`.
+                                        ident.span.shrink_to_hi().to(args.map_or(
+                                            ident.span.shrink_to_hi(),
                                             |a| a.span_ext)),
                                         false,
                                     ),
                                     [segment] => (
+                                        // We need to include the `::` in `Type::Variant::<Args>`
+                                        // to point the span to `::<Args>`, not just `<Args>`.
                                         segment.ident.span.shrink_to_hi().to(segment.args.map_or(
                                             segment.ident.span.shrink_to_hi(),
                                             |a| a.span_ext)),
                                         kw::SelfUpper == segment.ident.name,
                                     ),
-                                    _ => unreachable!(),
+                                    _ => {
+                                        err.note(&msg);
+                                        return;
+                                    }
                                 }
                             } else {
                                 err.note(&msg);
                                 return;
                             };
-                            let Some(type_name) = type_name else {
-                                err.note(&msg);
-                                return;
-                            };
                             let suggestion = vec![
                                 if is_self {
                                     // Account for people writing `Self::Variant::<Args>`, where
-                                    // `Self` is the enum.
+                                    // `Self` is the enum, and suggest replacing `Self` with the
+                                    // appropriate type: `Type::<Args>::Variant`.
                                     (qself.span, format!("{type_name}{snippet}"))
                                 } else {
                                     (qself_sugg_span, snippet)
                                 },
                                 (args_span, String::new()),
                             ];
-                            err.multipart_suggestion_verbose(&msg, suggestion, Applicability::MaybeIncorrect);
+                            err.multipart_suggestion_verbose(
+                                &msg,
+                                suggestion,
+                                Applicability::MaybeIncorrect,
+                            );
                         });
                         return Ok((qself_ty, DefKind::Variant, variant_def.def_id));
                     } else {
@@ -2369,8 +2401,6 @@ pub fn res_to_ty(
                 // Try to evaluate any array length constants.
                 let ty = tcx.at(span).type_of(def_id);
                 let span_of_impl = tcx.span_of_impl(def_id);
-                // TODO: confirm that `def_id`'s type accepts type params at all before suggesting
-                // using that instead.
                 self.prohibit_generics(path.segments.iter(), |err| {
                     let def_id = match *ty.kind() {
                         ty::Adt(self_def, _) => self_def.did(),
@@ -2379,32 +2409,52 @@ pub fn res_to_ty(
 
                     let type_name = tcx.item_name(def_id);
                     let span_of_ty = tcx.def_ident_span(def_id);
+                    let generics = tcx.generics_of(def_id).count();
 
-                    let msg = format!("the `Self` type is `{ty}`");
+                    let msg = format!("`Self` is of type `{ty}`");
                     if let (Ok(i_sp), Some(t_sp)) = (span_of_impl, span_of_ty) {
                         let i_sp = tcx.sess.source_map().guess_head_span(i_sp);
                         let mut span: MultiSpan = vec![t_sp].into();
                         span.push_span_label(
                             i_sp,
-                            &format!("`Self` is `{type_name}` in this `impl`"),
+                            &format!("`Self` is or type `{type_name}` in this `impl`"),
+                        );
+                        let mut postfix = "";
+                        if generics == 0 {
+                            postfix = ", which doesn't have type parameters";
+                        }
+                        span.push_span_label(
+                            t_sp,
+                            &format!("`Self` corresponds to this type{postfix}"),
                         );
-                        span.push_span_label(t_sp, "`Self` corresponds to this type");
                         err.span_note(span, &msg);
                     } else {
                         err.note(&msg);
                     }
                     for segment in path.segments {
-                        if let Some(_args) = segment.args && segment.ident.name == kw::SelfUpper {
-                            err.span_suggestion_verbose(
-                                segment.ident.span,
-                                format!(
-                                    "the `Self` type doesn't accept type parameters, use the \
-                                     concrete type's name `{type_name}` instead if you want to \
-                                     specify its type parameters"
-                                ),
-                                type_name.to_string(),
-                                Applicability::MaybeIncorrect,
-                            );
+                        if let Some(args) = segment.args && segment.ident.name == kw::SelfUpper {
+                            if generics == 0 {
+                                // FIXME(estebank): we could also verify that the arguments being
+                                // work for the `enum`, instead of just looking if it takes *any*.
+                                err.span_suggestion_verbose(
+                                    segment.ident.span.shrink_to_hi().to(args.span_ext),
+                                    "the `Self` type doesn't accept type parameters",
+                                    String::new(),
+                                    Applicability::MachineApplicable,
+                                );
+                                return;
+                            } else {
+                                err.span_suggestion_verbose(
+                                    segment.ident.span,
+                                    format!(
+                                        "the `Self` type doesn't accept type parameters, use the \
+                                        concrete type's name `{type_name}` instead if you want to \
+                                        specify its type parameters"
+                                    ),
+                                    type_name.to_string(),
+                                    Applicability::MaybeIncorrect,
+                                );
+                            }
                         }
                     }
                 });
diff --git a/src/test/ui/structs/struct-path-self.stderr b/src/test/ui/structs/struct-path-self.stderr
index f8b85559db8..bf9fe72d718 100644
--- a/src/test/ui/structs/struct-path-self.stderr
+++ b/src/test/ui/structs/struct-path-self.stderr
@@ -34,18 +34,19 @@ error[E0109]: type arguments are not allowed for this type
 LL |         let z = Self::<u8> {};
    |                        ^^ type argument not allowed
    |
-note: the `Self` type is `S`
+note: `Self` is of type `S`
   --> $DIR/struct-path-self.rs:1:8
    |
 LL | struct S;
-   |        ^ `Self` corresponds to this type
+   |        ^ `Self` corresponds to this type, which doesn't have type parameters
 ...
 LL | impl Tr for S {
-   | ------------- `Self` is `S` in this `impl`
-help: the `Self` type doesn't accept type parameters, use the concrete type's name `S` instead if you want to specify its type parameters
+   | ------------- `Self` is or type `S` in this `impl`
+help: the `Self` type doesn't accept type parameters
    |
-LL |         let z = S::<u8> {};
-   |                 ~
+LL -         let z = Self::<u8> {};
+LL +         let z = Self {};
+   | 
 
 error[E0109]: type arguments are not allowed for this type
   --> $DIR/struct-path-self.rs:30:24
@@ -53,18 +54,19 @@ error[E0109]: type arguments are not allowed for this type
 LL |         let z = Self::<u8> {};
    |                        ^^ type argument not allowed
    |
-note: the `Self` type is `S`
+note: `Self` is of type `S`
   --> $DIR/struct-path-self.rs:1:8
    |
 LL | struct S;
-   |        ^ `Self` corresponds to this type
+   |        ^ `Self` corresponds to this type, which doesn't have type parameters
 ...
 LL | impl S {
-   | ------ `Self` is `S` in this `impl`
-help: the `Self` type doesn't accept type parameters, use the concrete type's name `S` instead if you want to specify its type parameters
+   | ------ `Self` is or type `S` in this `impl`
+help: the `Self` type doesn't accept type parameters
    |
-LL |         let z = S::<u8> {};
-   |                 ~
+LL -         let z = Self::<u8> {};
+LL +         let z = Self {};
+   | 
 
 error: aborting due to 6 previous errors
 
diff --git a/src/test/ui/type-alias-enum-variants/enum-variant-generic-args.stderr b/src/test/ui/type-alias-enum-variants/enum-variant-generic-args.stderr
index a0df5c416b2..98c164ff418 100644
--- a/src/test/ui/type-alias-enum-variants/enum-variant-generic-args.stderr
+++ b/src/test/ui/type-alias-enum-variants/enum-variant-generic-args.stderr
@@ -29,14 +29,14 @@ error[E0109]: type arguments are not allowed for this type
 LL |         Self::<()>::TSVariant(());
    |                ^^ type argument not allowed
    |
-note: the `Self` type is `Enum<T>`
+note: `Self` is of type `Enum<T>`
   --> $DIR/enum-variant-generic-args.rs:7:6
    |
 LL | enum Enum<T> { TSVariant(T), SVariant { v: T }, UVariant }
    |      ^^^^ `Self` corresponds to this type
 ...
 LL | impl<T> Enum<T> {
-   | --------------- `Self` is `Enum` in this `impl`
+   | --------------- `Self` is or type `Enum` in this `impl`
 help: the `Self` type doesn't accept type parameters, use the concrete type's name `Enum` instead if you want to specify its type parameters
    |
 LL |         Enum::<()>::TSVariant(());
@@ -67,14 +67,14 @@ error[E0109]: type arguments are not allowed for this type
 LL |         Self::<()>::TSVariant::<()>(());
    |                ^^ type argument not allowed
    |
-note: the `Self` type is `Enum<T>`
+note: `Self` is of type `Enum<T>`
   --> $DIR/enum-variant-generic-args.rs:7:6
    |
 LL | enum Enum<T> { TSVariant(T), SVariant { v: T }, UVariant }
    |      ^^^^ `Self` corresponds to this type
 ...
 LL | impl<T> Enum<T> {
-   | --------------- `Self` is `Enum` in this `impl`
+   | --------------- `Self` is or type `Enum` in this `impl`
 help: the `Self` type doesn't accept type parameters, use the concrete type's name `Enum` instead if you want to specify its type parameters
    |
 LL |         Enum::<()>::TSVariant::<()>(());
@@ -129,14 +129,14 @@ error[E0109]: type arguments are not allowed for this type
 LL |         Self::<()>::SVariant { v: () };
    |                ^^ type argument not allowed
    |
-note: the `Self` type is `Enum<T>`
+note: `Self` is of type `Enum<T>`
   --> $DIR/enum-variant-generic-args.rs:7:6
    |
 LL | enum Enum<T> { TSVariant(T), SVariant { v: T }, UVariant }
    |      ^^^^ `Self` corresponds to this type
 ...
 LL | impl<T> Enum<T> {
-   | --------------- `Self` is `Enum` in this `impl`
+   | --------------- `Self` is or type `Enum` in this `impl`
 help: the `Self` type doesn't accept type parameters, use the concrete type's name `Enum` instead if you want to specify its type parameters
    |
 LL |         Enum::<()>::SVariant { v: () };
@@ -160,14 +160,14 @@ error[E0109]: type arguments are not allowed for this type
 LL |         Self::<()>::SVariant::<()> { v: () };
    |                ^^ type argument not allowed
    |
-note: the `Self` type is `Enum<T>`
+note: `Self` is of type `Enum<T>`
   --> $DIR/enum-variant-generic-args.rs:7:6
    |
 LL | enum Enum<T> { TSVariant(T), SVariant { v: T }, UVariant }
    |      ^^^^ `Self` corresponds to this type
 ...
 LL | impl<T> Enum<T> {
-   | --------------- `Self` is `Enum` in this `impl`
+   | --------------- `Self` is or type `Enum` in this `impl`
 help: the `Self` type doesn't accept type parameters, use the concrete type's name `Enum` instead if you want to specify its type parameters
    |
 LL |         Enum::<()>::SVariant::<()> { v: () };
@@ -210,14 +210,14 @@ error[E0109]: type arguments are not allowed for this type
 LL |         Self::<()>::UVariant;
    |                ^^ type argument not allowed
    |
-note: the `Self` type is `Enum<T>`
+note: `Self` is of type `Enum<T>`
   --> $DIR/enum-variant-generic-args.rs:7:6
    |
 LL | enum Enum<T> { TSVariant(T), SVariant { v: T }, UVariant }
    |      ^^^^ `Self` corresponds to this type
 ...
 LL | impl<T> Enum<T> {
-   | --------------- `Self` is `Enum` in this `impl`
+   | --------------- `Self` is or type `Enum` in this `impl`
 help: the `Self` type doesn't accept type parameters, use the concrete type's name `Enum` instead if you want to specify its type parameters
    |
 LL |         Enum::<()>::UVariant;
@@ -229,14 +229,14 @@ error[E0109]: type arguments are not allowed for this type
 LL |         Self::<()>::UVariant::<()>;
    |                ^^ type argument not allowed
    |
-note: the `Self` type is `Enum<T>`
+note: `Self` is of type `Enum<T>`
   --> $DIR/enum-variant-generic-args.rs:7:6
    |
 LL | enum Enum<T> { TSVariant(T), SVariant { v: T }, UVariant }
    |      ^^^^ `Self` corresponds to this type
 ...
 LL | impl<T> Enum<T> {
-   | --------------- `Self` is `Enum` in this `impl`
+   | --------------- `Self` is or type `Enum` in this `impl`
 help: the `Self` type doesn't accept type parameters, use the concrete type's name `Enum` instead if you want to specify its type parameters
    |
 LL |         Enum::<()>::UVariant::<()>;