Merge #11686

11686: feat: Enum variant field completion, enum variant / struct consistency r=Veykril a=m0rg-dev

This addresses several related inconsistencies:
 - tuple structs use tab stops instead of placeholders
 - tuple structs display in the completion menu as `Struct {…}` instead of `Struct(…)`
 - enum variants don't receive field completions at all
 - enum variants display differently from structs in the completion menu

Also, structs now display their type in the completion detail rather than the raw snippet text to be inserted.

As far as what's user-visible, that looks like this:

| | Menu | Completion | Detail |
|-|-|-|-|
| Record struct (old) | `Struct {…}` | `Struct { x: ${1:()}, y: ${2:()} }$0` | `Struct { x: ${1:()}, y: ${2:()} }$0` |
| Record struct (new) | `Struct {…}` | `Struct { x: ${1:()}, y: ${2:()} }$0` | `Struct { x: i32, y: i32 }` |
| Tuple struct (old) | `Struct {…}`  | `Struct($1, $2)$0` | `Struct($1, $2)` |
| Tuple struct (new) | `Struct(…)` | `Struct(${1:()}, ${2:()})$0` | `Struct(i32, i32)` |
| Unit variant (old) | `Variant` | `Variant` | `()` |
| Unit variant (new) | `Variant` | `Variant$0` | `Variant` |
| Record variant (old) | `Variant` | `Variant` | `{x: i32, y: i32}` |
| Record variant (new) | `Variant {…}` | `Variant { x: ${1:()}, y: ${2:()} }$0` | `Variant { x: i32, y: i32 }` |
| Tuple variant (old) | `Variant(…)` | `Variant($0)` | `(i32, i32)` |
| Tuple variant (new) | `Variant(…)` | `Variant(${1:()}, ${2:()})$0` | `Variant(i32, i32)` |

Additionally, tuple variants no longer set `triggers_call_info` because it conflicts with placeholder generation, and tuple variants that require a qualified path should now use the qualified path.

Internally, this also lets us break the general "format an item with fields on it" code out into a shared module, so that means it'll be a lot easier to implement features like #11568.


Co-authored-by: Morgan Thomas <corp@m0rg.dev>

crates/ide_completion/src/completions/qualified_path.rs

@@ -580,8 +580,8 @@ impl Foo {
 }
 "#,
             expect![[r#"
-                ev Bar    ()
-                ev Baz    ()
+                ev Bar    Bar
+                ev Baz    Baz
                 me foo(…) fn(self)
             "#]],
         );
@@ -626,7 +626,7 @@ fn main() {
 }
 "#,
             expect![[r#"
-                ev Bar ()
+                ev Bar Bar
             "#]],
         );
     }

crates/ide_completion/src/render.rs

@@ -8,6 +8,7 @@ pub(crate) mod const_;
 pub(crate) mod pattern;
 pub(crate) mod type_alias;
 pub(crate) mod struct_literal;
+pub(crate) mod compound;
 
 mod builder_ext;
 
@@ -428,14 +429,14 @@ fn main() { Foo::Fo$0 }
             expect![[r#"
                 [
                     CompletionItem {
-                        label: "Foo",
+                        label: "Foo {…}",
                         source_range: 54..56,
                         delete: 54..56,
-                        insert: "Foo",
+                        insert: "Foo { x: ${1:()}, y: ${2:()} }$0",
                         kind: SymbolKind(
                             Variant,
                         ),
-                        detail: "{x: i32, y: i32}",
+                        detail: "Foo { x: i32, y: i32 }",
                     },
                 ]
             "#]],
@@ -443,7 +444,7 @@ fn main() { Foo::Fo$0 }
     }
 
     #[test]
-    fn enum_detail_doesnt_include_tuple_fields() {
+    fn enum_detail_includes_tuple_fields() {
         check(
             r#"
 enum Foo { Foo (i32, i32) }
@@ -457,13 +458,11 @@ fn main() { Foo::Fo$0 }
                         label: "Foo(…)",
                         source_range: 46..48,
                         delete: 46..48,
-                        insert: "Foo($0)",
+                        insert: "Foo(${1:()}, ${2:()})$0",
                         kind: SymbolKind(
                             Variant,
                         ),
-                        lookup: "Foo",
-                        detail: "(i32, i32)",
-                        trigger_call_info: true,
+                        detail: "Foo(i32, i32)",
                     },
                 ]
             "#]],
@@ -510,7 +509,7 @@ fn main() { fo$0 }
     }
 
     #[test]
-    fn enum_detail_just_parentheses_for_unit() {
+    fn enum_detail_just_name_for_unit() {
         check(
             r#"
 enum Foo { Foo }
@@ -524,11 +523,11 @@ fn main() { Foo::Fo$0 }
                         label: "Foo",
                         source_range: 35..37,
                         delete: 35..37,
-                        insert: "Foo",
+                        insert: "Foo$0",
                         kind: SymbolKind(
                             Variant,
                         ),
-                        detail: "()",
+                        detail: "Foo",
                     },
                 ]
             "#]],
@@ -572,15 +571,15 @@ fn main() { let _: m::Spam = S$0 }
                         ),
                     },
                     CompletionItem {
-                        label: "Spam::Bar(…)",
+                        label: "m::Spam::Bar(…)",
                         source_range: 75..76,
                         delete: 75..76,
-                        insert: "Spam::Bar($0)",
+                        insert: "m::Spam::Bar(${1:()})$0",
                         kind: SymbolKind(
                             Variant,
                         ),
                         lookup: "Spam::Bar",
-                        detail: "(i32)",
+                        detail: "m::Spam::Bar(i32)",
                         relevance: CompletionRelevance {
                             exact_name_match: false,
                             type_match: Some(
@@ -591,18 +590,17 @@ fn main() { let _: m::Spam = S$0 }
                             is_private_editable: false,
                             exact_postfix_snippet_match: false,
                         },
-                        trigger_call_info: true,
                     },
                     CompletionItem {
                         label: "m::Spam::Foo",
                         source_range: 75..76,
                         delete: 75..76,
-                        insert: "m::Spam::Foo",
+                        insert: "m::Spam::Foo$0",
                         kind: SymbolKind(
                             Variant,
                         ),
                         lookup: "Spam::Foo",
-                        detail: "()",
+                        detail: "m::Spam::Foo",
                         relevance: CompletionRelevance {
                             exact_name_match: false,
                             type_match: Some(
@@ -787,11 +785,11 @@ use self::E::*;
                         label: "V",
                         source_range: 10..12,
                         delete: 10..12,
-                        insert: "V",
+                        insert: "V$0",
                         kind: SymbolKind(
                             Variant,
                         ),
-                        detail: "()",
+                        detail: "V",
                         documentation: Documentation(
                             "variant docs",
                         ),

crates/ide_completion/src/render/builder_ext.rs

@@ -8,6 +8,7 @@ use crate::{context::PathKind, item::Builder, patterns::ImmediateLocation, Compl
 #[derive(Debug)]
 pub(super) enum Params {
     Named(Option<hir::SelfParam>, Vec<hir::Param>),
+    #[allow(dead_code)]
     Anonymous(usize),
 }
 

crates/ide_completion/src/render/compound.rs

@@ -0,0 +1,95 @@
+//! Code common to structs, unions, and enum variants.
+
+use crate::render::RenderContext;
+use hir::{db::HirDatabase, HasAttrs, HasVisibility, HirDisplay, StructKind};
+use ide_db::SnippetCap;
+use itertools::Itertools;
+use syntax::SmolStr;
+
+/// A rendered struct, union, or enum variant, split into fields for actual
+/// auto-completion (`literal`, using `field: ()`) and display in the
+/// completions menu (`detail`, using `field: type`).
+pub(crate) struct RenderedCompound {
+    pub(crate) literal: String,
+    pub(crate) detail: String,
+}
+
+/// Render a record type (or sub-type) to a `RenderedCompound`. Use `None` for
+/// the `name` argument for an anonymous type.
+pub(crate) fn render_record(
+    db: &dyn HirDatabase,
+    snippet_cap: Option<SnippetCap>,
+    fields: &[hir::Field],
+    name: Option<&str>,
+) -> RenderedCompound {
+    let completions = fields.iter().enumerate().format_with(", ", |(idx, field), f| {
+        if snippet_cap.is_some() {
+            f(&format_args!("{}: ${{{}:()}}", field.name(db), idx + 1))
+        } else {
+            f(&format_args!("{}: ()", field.name(db)))
+        }
+    });
+
+    let types = fields.iter().format_with(", ", |field, f| {
+        f(&format_args!("{}: {}", field.name(db), field.ty(db).display(db)))
+    });
+
+    RenderedCompound {
+        literal: format!("{} {{ {} }}", name.unwrap_or(""), completions),
+        detail: format!("{} {{ {} }}", name.unwrap_or(""), types),
+    }
+}
+
+/// Render a tuple type (or sub-type) to a `RenderedCompound`. Use `None` for
+/// the `name` argument for an anonymous type.
+pub(crate) fn render_tuple(
+    db: &dyn HirDatabase,
+    snippet_cap: Option<SnippetCap>,
+    fields: &[hir::Field],
+    name: Option<&str>,
+) -> RenderedCompound {
+    let completions = fields.iter().enumerate().format_with(", ", |(idx, _), f| {
+        if snippet_cap.is_some() {
+            f(&format_args!("${{{}:()}}", idx + 1))
+        } else {
+            f(&format_args!("()"))
+        }
+    });
+
+    let types = fields.iter().format_with(", ", |field, f| f(&field.ty(db).display(db)));
+
+    RenderedCompound {
+        literal: format!("{}({})", name.unwrap_or(""), completions),
+        detail: format!("{}({})", name.unwrap_or(""), types),
+    }
+}
+
+/// Find all the visible fields in a given list. Returns the list of visible
+/// fields, plus a boolean for whether the list is comprehensive (contains no
+/// private fields and its item is not marked `#[non_exhaustive]`).
+pub(crate) fn visible_fields(
+    ctx: &RenderContext<'_>,
+    fields: &[hir::Field],
+    item: impl HasAttrs,
+) -> Option<(Vec<hir::Field>, bool)> {
+    let module = ctx.completion.module?;
+    let n_fields = fields.len();
+    let fields = fields
+        .iter()
+        .filter(|field| field.is_visible_from(ctx.db(), module))
+        .copied()
+        .collect::<Vec<_>>();
+
+    let fields_omitted =
+        n_fields - fields.len() > 0 || item.attrs(ctx.db()).by_key("non_exhaustive").exists();
+    Some((fields, fields_omitted))
+}
+
+/// Format a struct, etc. literal option for display in the completions menu.
+pub(crate) fn format_literal_label(name: &str, kind: StructKind) -> SmolStr {
+    match kind {
+        StructKind::Tuple => SmolStr::from_iter([name, "(…)"]),
+        StructKind::Record => SmolStr::from_iter([name, " {…}"]),
+        StructKind::Unit => name.into(),
+    }
+}

crates/ide_completion/src/render/enum_variant.rs

@@ -1,13 +1,15 @@
 //! Renderer for `enum` variants.
 
-use hir::{db::HirDatabase, HasAttrs, HirDisplay, StructKind};
+use hir::{HasAttrs, StructKind};
 use ide_db::SymbolKind;
-use itertools::Itertools;
 use syntax::SmolStr;
 
 use crate::{
     item::{CompletionItem, ImportEdit},
-    render::{builder_ext::Params, compute_ref_match, compute_type_match, RenderContext},
+    render::{
+        compound::{format_literal_label, render_record, render_tuple, RenderedCompound},
+        compute_ref_match, compute_type_match, RenderContext,
+    },
     CompletionRelevance,
 };
 
@@ -46,20 +48,42 @@ fn render(
     let qualified_name = qualified_name.to_string();
     let short_qualified_name: SmolStr = short_qualified_name.to_string().into();
 
-    let mut item = CompletionItem::new(SymbolKind::Variant, ctx.source_range(), qualified_name);
+    let mut rendered = match variant_kind {
+        StructKind::Tuple => {
+            render_tuple(db, ctx.snippet_cap(), &variant.fields(db), Some(&qualified_name))
+        }
+        StructKind::Record => {
+            render_record(db, ctx.snippet_cap(), &variant.fields(db), Some(&qualified_name))
+        }
+        StructKind::Unit => {
+            RenderedCompound { literal: qualified_name.clone(), detail: qualified_name.clone() }
+        }
+    };
+
+    if ctx.snippet_cap().is_some() {
+        rendered.literal.push_str("$0");
+    }
+
+    let mut item = CompletionItem::new(
+        SymbolKind::Variant,
+        ctx.source_range(),
+        format_literal_label(&qualified_name, variant_kind),
+    );
+
     item.set_documentation(variant.docs(db))
         .set_deprecated(ctx.is_deprecated(variant))
-        .detail(detail(db, variant, variant_kind));
+        .detail(rendered.detail);
+
+    match ctx.snippet_cap() {
+        Some(snippet_cap) => item.insert_snippet(snippet_cap, rendered.literal),
+        None => item.insert_text(rendered.literal),
+    };
 
     if let Some(import_to_add) = import_to_add {
         item.add_import(import_to_add);
     }
 
-    if variant_kind == hir::StructKind::Tuple {
-        cov_mark::hit!(inserts_parens_for_tuple_enums);
-        let params = Params::Anonymous(variant.fields(db).len());
-        item.add_call_parens(completion, short_qualified_name, params);
-    } else if qualified {
+    if qualified {
         item.lookup_by(short_qualified_name);
     }
 
@@ -75,50 +99,3 @@ fn render(
 
     item.build()
 }
-
-fn detail(db: &dyn HirDatabase, variant: hir::Variant, variant_kind: StructKind) -> String {
-    let detail_types = variant.fields(db).into_iter().map(|field| (field.name(db), field.ty(db)));
-
-    match variant_kind {
-        hir::StructKind::Tuple | hir::StructKind::Unit => {
-            format!("({})", detail_types.format_with(", ", |(_, t), f| f(&t.display(db))))
-        }
-        hir::StructKind::Record => {
-            format!(
-                "{{{}}}",
-                detail_types.format_with(", ", |(n, t), f| {
-                    f(&n)?;
-                    f(&": ")?;
-                    f(&t.display(db))
-                }),
-            )
-        }
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use crate::tests::check_edit;
-
-    #[test]
-    fn inserts_parens_for_tuple_enums() {
-        cov_mark::check!(inserts_parens_for_tuple_enums);
-        check_edit(
-            "Some",
-            r#"
-enum Option<T> { Some(T), None }
-use Option::*;
-fn main() -> Option<i32> {
-    Som$0
-}
-"#,
-            r#"
-enum Option<T> { Some(T), None }
-use Option::*;
-fn main() -> Option<i32> {
-    Some($0)
-}
-"#,
-        );
-    }
-}

crates/ide_completion/src/render/struct_literal.rs

@@ -1,11 +1,15 @@
 //! Renderer for `struct` literal.
 
-use hir::{db::HirDatabase, HasAttrs, HasVisibility, Name, StructKind};
-use ide_db::SnippetCap;
-use itertools::Itertools;
+use hir::{HasAttrs, Name, StructKind};
 use syntax::SmolStr;
 
-use crate::{render::RenderContext, CompletionItem, CompletionItemKind};
+use crate::{
+    render::compound::{
+        format_literal_label, render_record, render_tuple, visible_fields, RenderedCompound,
+    },
+    render::RenderContext,
+    CompletionItem, CompletionItemKind,
+};
 
 pub(crate) fn render_struct_literal(
     ctx: RenderContext<'_>,
@@ -25,29 +29,31 @@ pub(crate) fn render_struct_literal(
 
     let name = local_name.unwrap_or_else(|| strukt.name(ctx.db())).to_smol_str();
 
-    let literal = render_literal(&ctx, path, &name, strukt.kind(ctx.db()), &visible_fields)?;
+    let rendered = render_literal(&ctx, path, &name, strukt.kind(ctx.db()), &visible_fields)?;
 
-    Some(build_completion(ctx, name, literal, strukt))
+    Some(build_completion(&ctx, name, rendered, strukt.kind(ctx.db()), strukt))
 }
 
 fn build_completion(
-    ctx: RenderContext<'_>,
+    ctx: &RenderContext<'_>,
     name: SmolStr,
-    literal: String,
+    rendered: RenderedCompound,
+    kind: StructKind,
     def: impl HasAttrs + Copy,
 ) -> CompletionItem {
     let mut item = CompletionItem::new(
         CompletionItemKind::Snippet,
         ctx.source_range(),
-        SmolStr::from_iter([&name, " {…}"]),
+        format_literal_label(&name, kind),
     );
+
     item.set_documentation(ctx.docs(def))
         .set_deprecated(ctx.is_deprecated(def))
-        .detail(&literal)
+        .detail(&rendered.detail)
         .set_relevance(ctx.completion_relevance());
     match ctx.snippet_cap() {
-        Some(snippet_cap) => item.insert_snippet(snippet_cap, literal),
-        None => item.insert_text(literal),
+        Some(snippet_cap) => item.insert_snippet(snippet_cap, rendered.literal),
+        None => item.insert_text(rendered.literal),
     };
     item.build()
 }
@@ -58,7 +64,7 @@ fn render_literal(
     name: &str,
     kind: StructKind,
     fields: &[hir::Field],
-) -> Option<String> {
+) -> Option<RenderedCompound> {
     let path_string;
 
     let qualified_name = if let Some(path) = path {
@@ -68,69 +74,18 @@ fn render_literal(
         name
     };
 
-    let mut literal = match kind {
+    let mut rendered = match kind {
         StructKind::Tuple if ctx.snippet_cap().is_some() => {
-            render_tuple_as_literal(fields, qualified_name)
+            render_tuple(ctx.db(), ctx.snippet_cap(), fields, Some(qualified_name))
         }
         StructKind::Record => {
-            render_record_as_literal(ctx.db(), ctx.snippet_cap(), fields, qualified_name)
+            render_record(ctx.db(), ctx.snippet_cap(), fields, Some(qualified_name))
         }
         _ => return None,
     };
 
     if ctx.snippet_cap().is_some() {
-        literal.push_str("$0");
+        rendered.literal.push_str("$0");
     }
-    Some(literal)
-}
-
-fn render_record_as_literal(
-    db: &dyn HirDatabase,
-    snippet_cap: Option<SnippetCap>,
-    fields: &[hir::Field],
-    name: &str,
-) -> String {
-    let fields = fields.iter();
-    if snippet_cap.is_some() {
-        format!(
-            "{name} {{ {} }}",
-            fields
-                .enumerate()
-                .map(|(idx, field)| format!("{}: ${{{}:()}}", field.name(db), idx + 1))
-                .format(", "),
-            name = name
-        )
-    } else {
-        format!(
-            "{name} {{ {} }}",
-            fields.map(|field| format!("{}: ()", field.name(db))).format(", "),
-            name = name
-        )
-    }
-}
-
-fn render_tuple_as_literal(fields: &[hir::Field], name: &str) -> String {
-    format!(
-        "{name}({})",
-        fields.iter().enumerate().map(|(idx, _)| format!("${}", idx + 1)).format(", "),
-        name = name
-    )
-}
-
-fn visible_fields(
-    ctx: &RenderContext<'_>,
-    fields: &[hir::Field],
-    item: impl HasAttrs,
-) -> Option<(Vec<hir::Field>, bool)> {
-    let module = ctx.completion.module?;
-    let n_fields = fields.len();
-    let fields = fields
-        .iter()
-        .filter(|field| field.is_visible_from(ctx.db(), module))
-        .copied()
-        .collect::<Vec<_>>();
-
-    let fields_omitted =
-        n_fields - fields.len() > 0 || item.attrs(ctx.db()).by_key("non_exhaustive").exists();
-    Some((fields, fields_omitted))
+    Some(rendered)
 }

crates/ide_completion/src/tests/expression.rs

@@ -61,7 +61,7 @@ fn baz() {
             fn function()    fn()
             sc STATIC
             un Union
-            ev TupleV(…)     (u32)
+            ev TupleV(…)     TupleV(u32)
             ct CONST
         "#]],
     )
@@ -171,7 +171,7 @@ impl Unit {
             fn function()   fn()
             sc STATIC
             un Union
-            ev TupleV(…)    (u32)
+            ev TupleV(…)    TupleV(u32)
             ct CONST
         "#]],
     );
@@ -200,7 +200,7 @@ impl Unit {
             fn function() fn()
             sc STATIC
             un Union
-            ev TupleV(…)  (u32)
+            ev TupleV(…)  TupleV(u32)
             ct CONST
         "#]],
     );
@@ -543,9 +543,9 @@ fn func() {
 }
 "#,
         expect![[r#"
-            ev TupleV(…)   (u32)
-            ev RecordV     {field: u32}
-            ev UnitV       ()
+            ev TupleV(…)   TupleV(u32)
+            ev RecordV {…} RecordV { field: u32 }
+            ev UnitV       UnitV
             ct ASSOC_CONST const ASSOC_CONST: ()
             fn assoc_fn()  fn()
             ta AssocType   type AssocType = ()

crates/ide_completion/src/tests/pattern.rs

@@ -218,7 +218,7 @@ fn foo() {
         expect![[r#"
             kw ref
             kw mut
-            ev E::X  ()
+            ev E::X  E::X
             en E
             ma m!(…) macro_rules! m
         "#]],
@@ -291,9 +291,9 @@ fn func() {
 }
 "#,
         expect![[r#"
-            ev TupleV(…) (u32)
-            ev RecordV   {field: u32}
-            ev UnitV     ()
+            ev TupleV(…)   TupleV(u32)
+            ev RecordV {…} RecordV { field: u32 }
+            ev UnitV       UnitV
         "#]],
     );
 }

crates/ide_completion/src/tests/record.rs

@@ -166,7 +166,7 @@ fn main() {
             kw true
             kw false
             kw return
-            sn Foo {…}              Foo { foo1: ${1:()}, foo2: ${2:()} }$0
+            sn Foo {…}              Foo { foo1: u32, foo2: u32 }
             fd ..Default::default()
             fd foo1                 u32
             fd foo2                 u32

crates/ide_completion/src/tests/use_tree.rs

@@ -167,7 +167,7 @@ impl Foo {
 }
 "#,
         expect![[r#"
-            ev Variant ()
+            ev Variant Variant
         "#]],
     );
 }