From dd3f7664fdd25917901a141a2bf81b377aa09128 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 3 Apr 2022 15:11:04 +0200 Subject: [PATCH] fix: Add missing fields diagnostic fix for patterns --- .../src/handlers/missing_fields.rs | 195 ++++++++++++------ .../src/handlers/missing_match_arms.rs | 4 +- crates/syntax/src/ast/edit_in_place.rs | 43 ++++ crates/syntax/src/ast/make.rs | 4 + 4 files changed, 184 insertions(+), 62 deletions(-) diff --git a/crates/ide_diagnostics/src/handlers/missing_fields.rs b/crates/ide_diagnostics/src/handlers/missing_fields.rs index 414878b2308..6b70db9bfda 100644 --- a/crates/ide_diagnostics/src/handlers/missing_fields.rs +++ b/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 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()), - } - } - }; - - 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 + 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 { - generate_fill_expr(ty) + algo::diff(old_syntax, new_syntax).into_text_edit(&mut builder); } - } else { - generate_fill_expr(ty) + builder.finish() }; - 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); - } + 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, + )]) + }; - 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); + 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()); + } + 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. diff --git a/crates/ide_diagnostics/src/handlers/missing_match_arms.rs b/crates/ide_diagnostics/src/handlers/missing_match_arms.rs index fe6a8683c11..3fb84c9f3c2 100644 --- a/crates/ide_diagnostics/src/handlers/missing_match_arms.rs +++ b/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 { diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs index bf4f9d6d18b..7945252636a 100644 --- a/crates/syntax/src/ast/edit_in_place.rs +++ b/crates/syntax/src/ast/edit_in_place.rs @@ -546,6 +546,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()); diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index d11d658a273..d7a7d2d32a5 100644 --- a/crates/syntax/src/ast/make.rs +++ b/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());