Merge #11887

11887: fix: Add missing fields diagnostic fix for patterns r=Veykril a=Veykril

bors r+

Co-authored-by: Lukas Wirth <[email protected]>

Author: bors[bot]

crates/ide_diagnostics/src/handlers/missing_fields.rs

@@ -9,7 +9,7 @@ use stdx::format_to;
 use syntax::{
     algo,
     ast::{self, make},
-    AstNode, SyntaxNodePtr,
+    AstNode, SyntaxNode, SyntaxNodePtr,
 };
 use text_edit::TextEdit;
 
@@ -55,70 +55,95 @@ fn fixes(ctx: &DiagnosticsContext<'_>, d: &hir::MissingFields) -> Option<Vec<Ass
     }
 
     let root = ctx.sema.db.parse_or_expand(d.file)?;
-    let field_list_parent = match &d.field_list_parent {
-        Either::Left(record_expr) => record_expr.to_node(&root),
-        // FIXE: patterns should be fixable as well.
-        Either::Right(_) => return None,
-    };
-    let old_field_list = field_list_parent.record_expr_field_list()?;
 
-    let new_field_list = old_field_list.clone_for_update();
-    let mut locals = FxHashMap::default();
-    ctx.sema.scope(field_list_parent.syntax())?.process_all_names(&mut |name, def| {
-        if let hir::ScopeDef::Local(local) = def {
-            locals.insert(name, local);
-        }
-    });
-    let missing_fields = ctx.sema.record_literal_missing_fields(&field_list_parent);
-
-    let generate_fill_expr = |ty: &Type| match ctx.config.expr_fill_default {
-        crate::ExprFillDefaultMode::Todo => Some(make::ext::expr_todo()),
-        crate::ExprFillDefaultMode::Default => {
-            let default_constr = get_default_constructor(ctx, d, ty);
-            match default_constr {
-                Some(default_constr) => Some(default_constr),
-                _ => Some(make::ext::expr_todo()),
+    let build_text_edit = |parent_syntax, new_syntax: &SyntaxNode, old_syntax| {
+        let edit = {
+            let mut builder = TextEdit::builder();
+            if d.file.is_macro() {
+                // we can't map the diff up into the macro input unfortunately, as the macro loses all
+                // whitespace information so the diff wouldn't be applicable no matter what
+                // This has the downside that the cursor will be moved in macros by doing it without a diff
+                // but that is a trade off we can make.
+                // FIXME: this also currently discards a lot of whitespace in the input... we really need a formatter here
+                let range = ctx.sema.original_range_opt(old_syntax)?;
+                builder.replace(range.range, new_syntax.to_string());
+            } else {
+                algo::diff(old_syntax, new_syntax).into_text_edit(&mut builder);
             }
-        }
+            builder.finish()
+        };
+        Some(vec![fix(
+            "fill_missing_fields",
+            "Fill struct fields",
+            SourceChange::from_text_edit(d.file.original_file(ctx.sema.db), edit),
+            ctx.sema.original_range(parent_syntax).range,
+        )])
     };
 
-    for (f, ty) in missing_fields.iter() {
-        let field_expr = if let Some(local_candidate) = locals.get(&f.name(ctx.sema.db)) {
-            cov_mark::hit!(field_shorthand);
-            let candidate_ty = local_candidate.ty(ctx.sema.db);
-            if ty.could_unify_with(ctx.sema.db, &candidate_ty) {
-                None
-            } else {
-                generate_fill_expr(ty)
+    match &d.field_list_parent {
+        Either::Left(record_expr) => {
+            let field_list_parent = record_expr.to_node(&root);
+            let missing_fields = ctx.sema.record_literal_missing_fields(&field_list_parent);
+
+            let mut locals = FxHashMap::default();
+            ctx.sema.scope(field_list_parent.syntax())?.process_all_names(&mut |name, def| {
+                if let hir::ScopeDef::Local(local) = def {
+                    locals.insert(name, local);
+                }
+            });
+
+            let generate_fill_expr = |ty: &Type| match ctx.config.expr_fill_default {
+                crate::ExprFillDefaultMode::Todo => make::ext::expr_todo(),
+                crate::ExprFillDefaultMode::Default => {
+                    get_default_constructor(ctx, d, ty).unwrap_or_else(|| make::ext::expr_todo())
+                }
+            };
+
+            let old_field_list = field_list_parent.record_expr_field_list()?;
+            let new_field_list = old_field_list.clone_for_update();
+            for (f, ty) in missing_fields.iter() {
+                let field_expr = if let Some(local_candidate) = locals.get(&f.name(ctx.sema.db)) {
+                    cov_mark::hit!(field_shorthand);
+                    let candidate_ty = local_candidate.ty(ctx.sema.db);
+                    if ty.could_unify_with(ctx.sema.db, &candidate_ty) {
+                        None
+                    } else {
+                        Some(generate_fill_expr(ty))
+                    }
+                } else {
+                    Some(generate_fill_expr(ty))
+                };
+                let field = make::record_expr_field(
+                    make::name_ref(&f.name(ctx.sema.db).to_smol_str()),
+                    field_expr,
+                );
+                new_field_list.add_field(field.clone_for_update());
             }
-        } else {
-            generate_fill_expr(ty)
-        };
-        let field =
-            make::record_expr_field(make::name_ref(&f.name(ctx.sema.db).to_smol_str()), field_expr)
-                .clone_for_update();
-        new_field_list.add_field(field);
-    }
-
-    let mut builder = TextEdit::builder();
-    if d.file.is_macro() {
-        // we can't map the diff up into the macro input unfortunately, as the macro loses all
-        // whitespace information so the diff wouldn't be applicable no matter what
-        // This has the downside that the cursor will be moved in macros by doing it without a diff
-        // but that is a trade off we can make.
-        // FIXE: this also currently discards a lot of whitespace in the input... we really need a formatter here
-        let range = ctx.sema.original_range_opt(old_field_list.syntax())?;
-        builder.replace(range.range, new_field_list.to_string());
-    } else {
-        algo::diff(old_field_list.syntax(), new_field_list.syntax()).into_text_edit(&mut builder);
+            build_text_edit(
+                field_list_parent.syntax(),
+                new_field_list.syntax(),
+                old_field_list.syntax(),
+            )
+        }
+        Either::Right(record_pat) => {
+            let field_list_parent = record_pat.to_node(&root);
+            let missing_fields = ctx.sema.record_pattern_missing_fields(&field_list_parent);
+
+            let old_field_list = field_list_parent.record_pat_field_list()?;
+            let new_field_list = old_field_list.clone_for_update();
+            for (f, _) in missing_fields.iter() {
+                let field = make::record_pat_field_shorthand(make::name_ref(
+                    &f.name(ctx.sema.db).to_smol_str(),
+                ));
+                new_field_list.add_field(field.clone_for_update());
+            }
+            build_text_edit(
+                field_list_parent.syntax(),
+                new_field_list.syntax(),
+                old_field_list.syntax(),
+            )
+        }
     }
-    let edit = builder.finish();
-    Some(vec![fix(
-        "fill_missing_fields",
-        "Fill struct fields",
-        SourceChange::from_text_edit(d.file.original_file(ctx.sema.db), edit),
-        ctx.sema.original_range(field_list_parent.syntax()).range,
-    )])
 }
 
 fn make_ty(ty: &hir::Type, db: &dyn HirDatabase, module: hir::Module) -> ast::Type {
@@ -190,7 +215,7 @@ mod tests {
 struct S { foo: i32, bar: () }
 fn baz(s: S) {
     let S { foo: _ } = s;
-      //^ error: missing structure fields:
+      //^ 💡 error: missing structure fields:
       //| - bar
 }
 "#,
@@ -581,6 +606,56 @@ fn f() {
         );
     }
 
+    #[test]
+    fn test_fill_struct_pat_fields() {
+        check_fix(
+            r#"
+struct S { a: &'static str, b: i32 }
+
+fn f() {
+    let S {
+        $0
+    };
+}
+"#,
+            r#"
+struct S { a: &'static str, b: i32 }
+
+fn f() {
+    let S {
+        a,
+        b,
+    };
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn test_fill_struct_pat_fields_partial() {
+        check_fix(
+            r#"
+struct S { a: &'static str, b: i32 }
+
+fn f() {
+    let S {
+        a,$0
+    };
+}
+"#,
+            r#"
+struct S { a: &'static str, b: i32 }
+
+fn f() {
+    let S {
+        a,
+        b,
+    };
+}
+"#,
+        );
+    }
+
     #[test]
     fn import_extern_crate_clash_with_inner_item() {
         // This is more of a resolver test, but doesn't really work with the hir_def testsuite.

crates/ide_diagnostics/src/handlers/missing_match_arms.rs

@@ -396,14 +396,14 @@ fn main() {
         //^ error: missing match arm
     match a {
         Either::A { } => (),
-      //^^^^^^^^^ error: missing structure fields:
+      //^^^^^^^^^ 💡 error: missing structure fields:
       //        | - foo
         Either::B => (),
     }
     match a {
         //^ error: missing match arm
         Either::A { } => (),
-    } //^^^^^^^^^ error: missing structure fields:
+    } //^^^^^^^^^ 💡 error: missing structure fields:
       //        | - foo
 
     match a {

crates/syntax/src/ast/edit_in_place.rs

@@ -563,6 +563,49 @@ impl ast::RecordExprField {
     }
 }
 
+impl ast::RecordPatFieldList {
+    pub fn add_field(&self, field: ast::RecordPatField) {
+        let is_multiline = self.syntax().text().contains_char('\n');
+        let whitespace = if is_multiline {
+            let indent = IndentLevel::from_node(self.syntax()) + 1;
+            make::tokens::whitespace(&format!("\n{}", indent))
+        } else {
+            make::tokens::single_space()
+        };
+
+        if is_multiline {
+            normalize_ws_between_braces(self.syntax());
+        }
+
+        let position = match self.fields().last() {
+            Some(last_field) => {
+                let comma = match last_field
+                    .syntax()
+                    .siblings_with_tokens(Direction::Next)
+                    .filter_map(|it| it.into_token())
+                    .find(|it| it.kind() == T![,])
+                {
+                    Some(it) => it,
+                    None => {
+                        let comma = ast::make::token(T![,]);
+                        ted::insert(Position::after(last_field.syntax()), &comma);
+                        comma
+                    }
+                };
+                Position::after(comma)
+            }
+            None => match self.l_curly_token() {
+                Some(it) => Position::after(it),
+                None => Position::last_child_of(self.syntax()),
+            },
+        };
+
+        ted::insert_all(position, vec![whitespace.into(), field.syntax().clone().into()]);
+        if is_multiline {
+            ted::insert(Position::after(field.syntax()), ast::make::token(T![,]));
+        }
+    }
+}
 impl ast::StmtList {
     pub fn push_front(&self, statement: ast::Stmt) {
         ted::insert(Position::after(self.l_curly_token().unwrap()), statement.syntax());

crates/syntax/src/ast/make.rs

@@ -555,6 +555,10 @@ pub fn record_pat_field(name_ref: ast::NameRef, pat: ast::Pat) -> ast::RecordPat
     ast_from_text(&format!("fn f(S {{ {}: {} }}: ()))", name_ref, pat))
 }
 
+pub fn record_pat_field_shorthand(name_ref: ast::NameRef) -> ast::RecordPatField {
+    ast_from_text(&format!("fn f(S {{ {} }}: ()))", name_ref))
+}
+
 /// Returns a `BindPat` if the path has just one segment, a `PathPat` otherwise.
 pub fn path_pat(path: ast::Path) -> ast::Pat {
     return from_text(&path.to_string());