From 6087c014608108e2b971608e214a74759743e95e Mon Sep 17 00:00:00 2001 From: Steffen Lyngbaek Date: Mon, 16 Mar 2020 23:30:25 -0700 Subject: [PATCH 1/3] 'Fill match arms' should work with existing match arms Addresses #3039 This essentially adds missing match arms. The algorithm for this can get complicated rather quickly so bail in certain conditions and rely on a PlaceholderPat. The algorighm works as such: - Iterate through the Enum Def Variants - Attempt to see if the variant already exists as a match arm - If yes, skip the enum variant. If no, include it. - If it becomes complicated, rather than exhaustively deal with every branch, mark it as a "partial match" and simply include the placeholder. Conditions for "complication": - The match arm contains a match guard - Any kind of nested destrucuring Order the resulting merged match branches as such: 1. Provided match arms 2. Missing enum variant branch arms 3. End with Placeholder if required - Add extra tests --- .../src/handlers/fill_match_arms.rs | 306 ++++++++++++++++-- 1 file changed, 281 insertions(+), 25 deletions(-) diff --git a/crates/ra_assists/src/handlers/fill_match_arms.rs b/crates/ra_assists/src/handlers/fill_match_arms.rs index a1e4c2eb755..6e6c2d5ccf3 100644 --- a/crates/ra_assists/src/handlers/fill_match_arms.rs +++ b/crates/ra_assists/src/handlers/fill_match_arms.rs @@ -1,12 +1,17 @@ //! FIXME: write short doc here -use std::iter; +use std::{collections::LinkedList, iter}; use hir::{Adt, HasSource, Semantics}; use ra_ide_db::RootDatabase; -use ra_syntax::ast::{self, edit::IndentLevel, make, AstNode, NameOwner}; use crate::{Assist, AssistCtx, AssistId}; +use ra_syntax::{ + ast::{self, edit::IndentLevel, make, AstNode, NameOwner}, + SyntaxKind, SyntaxNode, +}; + +use ast::{MatchArm, MatchGuard, Pat}; // Assist: fill_match_arms // @@ -36,16 +41,6 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option { let match_expr = ctx.find_node_at_offset::()?; let match_arm_list = match_expr.match_arm_list()?; - // We already have some match arms, so we don't provide any assists. - // Unless if there is only one trivial match arm possibly created - // by match postfix complete. Trivial match arm is the catch all arm. - let mut existing_arms = match_arm_list.arms(); - if let Some(arm) = existing_arms.next() { - if !is_trivial(&arm) || existing_arms.next().is_some() { - return None; - } - }; - let expr = match_expr.expr()?; let enum_def = resolve_enum_def(&ctx.sema, &expr)?; let module = ctx.sema.scope(expr.syntax()).module()?; @@ -56,17 +51,53 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option { } let db = ctx.db; - ctx.add_assist(AssistId("fill_match_arms"), "Fill match arms", |edit| { - let indent_level = IndentLevel::from_node(match_arm_list.syntax()); + let mut arms: Vec = match_arm_list.arms().collect(); + if arms.len() == 1 { + if let Some(Pat::PlaceholderPat(..)) = arms[0].pat() { + arms.clear(); + } + } - let new_arm_list = { - let arms = variants - .into_iter() - .filter_map(|variant| build_pat(db, module, variant)) - .map(|pat| make::match_arm(iter::once(pat), make::expr_unit())); - indent_level.increase_indent(make::match_arm_list(arms)) - }; + let mut has_partial_match = false; + let variants: Vec = variants + .into_iter() + .filter_map(|variant| build_pat(db, module, variant)) + .filter(|variant_pat| { + !arms.iter().filter_map(|arm| arm.pat().map(|_| arm)).any(|arm| { + let pat = arm.pat().unwrap(); + + // Special casee OrPat as separate top-level pats + let pats: Vec = match Pat::from(pat.clone()) { + Pat::OrPat(pats) => pats.pats().collect::>(), + _ => vec![pat], + }; + + pats.iter().any(|pat| { + match does_arm_pat_match_variant(pat, arm.guard(), variant_pat) { + ArmMatch::Yes => true, + ArmMatch::No => false, + ArmMatch::Partial => { + has_partial_match = true; + true + } + } + }) + }) + }) + .map(|pat| make::match_arm(iter::once(pat), make::expr_unit())) + .collect(); + + arms.extend(variants); + if has_partial_match { + arms.push(make::match_arm( + iter::once(make::placeholder_pat().into()), + make::expr_unit(), + )); + } + + let indent_level = IndentLevel::from_node(match_arm_list.syntax()); + let new_arm_list = indent_level.increase_indent(make::match_arm_list(arms)); edit.target(match_expr.syntax().text_range()); edit.set_cursor(expr.syntax().text_range().start()); @@ -74,11 +105,59 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option { }) } -fn is_trivial(arm: &ast::MatchArm) -> bool { - match arm.pat() { - Some(ast::Pat::PlaceholderPat(..)) => true, - _ => false, +enum ArmMatch { + Yes, + No, + Partial, +} + +fn does_arm_pat_match_variant(arm: &Pat, arm_guard: Option, var: &Pat) -> ArmMatch { + let arm = flatten_pats(arm.clone()); + let var = flatten_pats(var.clone()); + let mut arm = arm.iter(); + let mut var = var.iter(); + + // If the first part of the Pat don't match, there's no match + match (arm.next(), var.next()) { + (Some(arm), Some(var)) if arm.text() == var.text() => {} + _ => return ArmMatch::No, } + + // If we have a guard we automatically know we have a partial match + if arm_guard.is_some() { + return ArmMatch::Partial; + } + + if arm.clone().count() != var.clone().count() { + return ArmMatch::Partial; + } + + let direct_match = arm.zip(var).all(|(arm, var)| { + if arm.text() == var.text() { + return true; + } + match (arm.kind(), var.kind()) { + (SyntaxKind::PLACEHOLDER_PAT, SyntaxKind::PLACEHOLDER_PAT) => true, + (SyntaxKind::DOT_DOT_PAT, SyntaxKind::PLACEHOLDER_PAT) => true, + (SyntaxKind::BIND_PAT, SyntaxKind::PLACEHOLDER_PAT) => true, + _ => false, + } + }); + + match direct_match { + true => ArmMatch::Yes, + false => ArmMatch::Partial, + } +} + +fn flatten_pats(pat: Pat) -> Vec { + let mut pats: LinkedList = pat.syntax().children().collect(); + let mut out: Vec = vec![]; + while let Some(p) = pats.pop_front() { + pats.extend(p.children()); + out.push(p); + } + out } fn resolve_enum_def(sema: &Semantics, expr: &ast::Expr) -> Option { @@ -114,6 +193,183 @@ mod tests { use super::fill_match_arms; + #[test] + fn partial_fill_multi() { + check_assist( + fill_match_arms, + r#" + enum A { + As, + Bs(i32, Option) + } + fn main() { + match A::As<|> { + A::Bs(_, Some(_)) => (), + } + } + "#, + r#" + enum A { + As, + Bs(i32, Option) + } + fn main() { + match <|>A::As { + A::Bs(_, Some(_)) => (), + A::As => (), + _ => (), + } + } + "#, + ); + } + + #[test] + fn partial_fill_record() { + check_assist( + fill_match_arms, + r#" + enum A { + As, + Bs{x:i32, y:Option}, + } + fn main() { + match A::As<|> { + A::Bs{x,y:Some(_)} => (), + } + } + "#, + r#" + enum A { + As, + Bs{x:i32, y:Option}, + } + fn main() { + match <|>A::As { + A::Bs{x,y:Some(_)} => (), + A::As => (), + _ => (), + } + } + "#, + ); + } + + #[test] + fn partial_fill_or_pat() { + check_assist( + fill_match_arms, + r#" + enum A { + As, + Bs, + Cs(Option), + } + fn main() { + match A::As<|> { + A::Cs(_) | A::Bs => (), + } + } + "#, + r#" + enum A { + As, + Bs, + Cs(Option), + } + fn main() { + match <|>A::As { + A::Cs(_) | A::Bs => (), + A::As => (), + } + } + "#, + ); + } + + #[test] + fn partial_fill_or_pat2() { + check_assist( + fill_match_arms, + r#" + enum A { + As, + Bs, + Cs(Option), + } + fn main() { + match A::As<|> { + A::Cs(Some(_)) | A::Bs => (), + } + } + "#, + r#" + enum A { + As, + Bs, + Cs(Option), + } + fn main() { + match <|>A::As { + A::Cs(Some(_)) | A::Bs => (), + A::As => (), + _ => (), + } + } + "#, + ); + } + + #[test] + fn partial_fill() { + check_assist( + fill_match_arms, + r#" + enum A { + As, + Bs, + Cs, + Ds(String), + Es(B), + } + enum B { + Xs, + Ys, + } + fn main() { + match A::As<|> { + A::Bs if 0 < 1 => (), + A::Ds(_value) => (), + A::Es(B::Xs) => (), + } + } + "#, + r#" + enum A { + As, + Bs, + Cs, + Ds(String), + Es(B), + } + enum B { + Xs, + Ys, + } + fn main() { + match <|>A::As { + A::Bs if 0 < 1 => (), + A::Ds(_value) => (), + A::Es(B::Xs) => (), + A::As => (), + A::Cs => (), + _ => (), + } + } + "#, + ); + } + #[test] fn fill_match_arms_empty_body() { check_assist( From 5f8f8a38a251197c8b07e349d9782c3ec5fde383 Mon Sep 17 00:00:00 2001 From: Steffen Lyngbaek Date: Tue, 17 Mar 2020 12:26:55 -0700 Subject: [PATCH 2/3] Don't show assist if all arms are present --- .../src/handlers/fill_match_arms.rs | 66 ++++++++++--------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/crates/ra_assists/src/handlers/fill_match_arms.rs b/crates/ra_assists/src/handlers/fill_match_arms.rs index 6e6c2d5ccf3..f8859ff6d8f 100644 --- a/crates/ra_assists/src/handlers/fill_match_arms.rs +++ b/crates/ra_assists/src/handlers/fill_match_arms.rs @@ -50,45 +50,49 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option { return None; } - let db = ctx.db; - ctx.add_assist(AssistId("fill_match_arms"), "Fill match arms", |edit| { - let mut arms: Vec = match_arm_list.arms().collect(); - if arms.len() == 1 { - if let Some(Pat::PlaceholderPat(..)) = arms[0].pat() { - arms.clear(); - } + let mut arms: Vec = match_arm_list.arms().collect(); + if arms.len() == 1 { + if let Some(Pat::PlaceholderPat(..)) = arms[0].pat() { + arms.clear(); } + } - let mut has_partial_match = false; - let variants: Vec = variants - .into_iter() - .filter_map(|variant| build_pat(db, module, variant)) - .filter(|variant_pat| { - !arms.iter().filter_map(|arm| arm.pat().map(|_| arm)).any(|arm| { - let pat = arm.pat().unwrap(); + let mut has_partial_match = false; + let db = ctx.db; + let missing_arms: Vec = variants + .into_iter() + .filter_map(|variant| build_pat(db, module, variant)) + .filter(|variant_pat| { + !arms.iter().filter_map(|arm| arm.pat().map(|_| arm)).any(|arm| { + let pat = arm.pat().unwrap(); - // Special casee OrPat as separate top-level pats - let pats: Vec = match Pat::from(pat.clone()) { - Pat::OrPat(pats) => pats.pats().collect::>(), - _ => vec![pat], - }; + // Special casee OrPat as separate top-level pats + let pats: Vec = match Pat::from(pat.clone()) { + Pat::OrPat(pats) => pats.pats().collect::>(), + _ => vec![pat], + }; - pats.iter().any(|pat| { - match does_arm_pat_match_variant(pat, arm.guard(), variant_pat) { - ArmMatch::Yes => true, - ArmMatch::No => false, - ArmMatch::Partial => { - has_partial_match = true; - true - } + pats.iter().any(|pat| { + match does_arm_pat_match_variant(pat, arm.guard(), variant_pat) { + ArmMatch::Yes => true, + ArmMatch::No => false, + ArmMatch::Partial => { + has_partial_match = true; + true } - }) + } }) }) - .map(|pat| make::match_arm(iter::once(pat), make::expr_unit())) - .collect(); + }) + .map(|pat| make::match_arm(iter::once(pat), make::expr_unit())) + .collect(); - arms.extend(variants); + if missing_arms.is_empty() && !has_partial_match { + return None; + } + + ctx.add_assist(AssistId("fill_match_arms"), "Fill match arms", |edit| { + arms.extend(missing_arms); if has_partial_match { arms.push(make::match_arm( iter::once(make::placeholder_pat().into()), From b5ba9c3e3ae25b6add36c670de75967a7b38bb53 Mon Sep 17 00:00:00 2001 From: Steffen Lyngbaek Date: Tue, 17 Mar 2020 16:02:39 -0700 Subject: [PATCH 3/3] Address nits and suggestions. Simplify the logic a lot by removing the check for a placeholder pat. This means the auto-fill no longer returns a compile-able value. --- .../src/handlers/fill_match_arms.rs | 172 ++++-------------- 1 file changed, 32 insertions(+), 140 deletions(-) diff --git a/crates/ra_assists/src/handlers/fill_match_arms.rs b/crates/ra_assists/src/handlers/fill_match_arms.rs index f8859ff6d8f..fbd6a3ec36c 100644 --- a/crates/ra_assists/src/handlers/fill_match_arms.rs +++ b/crates/ra_assists/src/handlers/fill_match_arms.rs @@ -1,17 +1,14 @@ //! FIXME: write short doc here -use std::{collections::LinkedList, iter}; +use std::iter; use hir::{Adt, HasSource, Semantics}; use ra_ide_db::RootDatabase; use crate::{Assist, AssistCtx, AssistId}; -use ra_syntax::{ - ast::{self, edit::IndentLevel, make, AstNode, NameOwner}, - SyntaxKind, SyntaxNode, -}; +use ra_syntax::ast::{self, edit::IndentLevel, make, AstNode, NameOwner}; -use ast::{MatchArm, MatchGuard, Pat}; +use ast::{MatchArm, Pat}; // Assist: fill_match_arms // @@ -57,48 +54,20 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option { } } - let mut has_partial_match = false; let db = ctx.db; let missing_arms: Vec = variants .into_iter() .filter_map(|variant| build_pat(db, module, variant)) - .filter(|variant_pat| { - !arms.iter().filter_map(|arm| arm.pat().map(|_| arm)).any(|arm| { - let pat = arm.pat().unwrap(); - - // Special casee OrPat as separate top-level pats - let pats: Vec = match Pat::from(pat.clone()) { - Pat::OrPat(pats) => pats.pats().collect::>(), - _ => vec![pat], - }; - - pats.iter().any(|pat| { - match does_arm_pat_match_variant(pat, arm.guard(), variant_pat) { - ArmMatch::Yes => true, - ArmMatch::No => false, - ArmMatch::Partial => { - has_partial_match = true; - true - } - } - }) - }) - }) + .filter(|variant_pat| is_variant_missing(&mut arms, variant_pat)) .map(|pat| make::match_arm(iter::once(pat), make::expr_unit())) .collect(); - if missing_arms.is_empty() && !has_partial_match { + if missing_arms.is_empty() { return None; } ctx.add_assist(AssistId("fill_match_arms"), "Fill match arms", |edit| { arms.extend(missing_arms); - if has_partial_match { - arms.push(make::match_arm( - iter::once(make::placeholder_pat().into()), - make::expr_unit(), - )); - } let indent_level = IndentLevel::from_node(match_arm_list.syntax()); let new_arm_list = indent_level.increase_indent(make::match_arm_list(arms)); @@ -109,59 +78,23 @@ pub(crate) fn fill_match_arms(ctx: AssistCtx) -> Option { }) } -enum ArmMatch { - Yes, - No, - Partial, +fn is_variant_missing(existing_arms: &mut Vec, var: &Pat) -> bool { + existing_arms.iter().filter_map(|arm| arm.pat()).all(|pat| { + // Special casee OrPat as separate top-level pats + let top_level_pats: Vec = match pat { + Pat::OrPat(pats) => pats.pats().collect::>(), + _ => vec![pat], + }; + + !top_level_pats.iter().any(|pat| does_pat_match_variant(pat, var)) + }) } -fn does_arm_pat_match_variant(arm: &Pat, arm_guard: Option, var: &Pat) -> ArmMatch { - let arm = flatten_pats(arm.clone()); - let var = flatten_pats(var.clone()); - let mut arm = arm.iter(); - let mut var = var.iter(); +fn does_pat_match_variant(pat: &Pat, var: &Pat) -> bool { + let pat_head = pat.syntax().first_child().map(|node| node.text()); + let var_head = var.syntax().first_child().map(|node| node.text()); - // If the first part of the Pat don't match, there's no match - match (arm.next(), var.next()) { - (Some(arm), Some(var)) if arm.text() == var.text() => {} - _ => return ArmMatch::No, - } - - // If we have a guard we automatically know we have a partial match - if arm_guard.is_some() { - return ArmMatch::Partial; - } - - if arm.clone().count() != var.clone().count() { - return ArmMatch::Partial; - } - - let direct_match = arm.zip(var).all(|(arm, var)| { - if arm.text() == var.text() { - return true; - } - match (arm.kind(), var.kind()) { - (SyntaxKind::PLACEHOLDER_PAT, SyntaxKind::PLACEHOLDER_PAT) => true, - (SyntaxKind::DOT_DOT_PAT, SyntaxKind::PLACEHOLDER_PAT) => true, - (SyntaxKind::BIND_PAT, SyntaxKind::PLACEHOLDER_PAT) => true, - _ => false, - } - }); - - match direct_match { - true => ArmMatch::Yes, - false => ArmMatch::Partial, - } -} - -fn flatten_pats(pat: Pat) -> Vec { - let mut pats: LinkedList = pat.syntax().children().collect(); - let mut out: Vec = vec![]; - while let Some(p) = pats.pop_front() { - pats.extend(p.children()); - out.push(p); - } - out + pat_head == var_head } fn resolve_enum_def(sema: &Semantics, expr: &ast::Expr) -> Option { @@ -193,35 +126,25 @@ fn build_pat(db: &RootDatabase, module: hir::Module, var: hir::EnumVariant) -> O #[cfg(test)] mod tests { - use crate::helpers::{check_assist, check_assist_target}; + use crate::helpers::{check_assist, check_assist_not_applicable, check_assist_target}; use super::fill_match_arms; #[test] - fn partial_fill_multi() { - check_assist( + fn all_match_arms_provided() { + check_assist_not_applicable( fill_match_arms, r#" enum A { As, - Bs(i32, Option) + Bs{x:i32, y:Option}, + Cs(i32, Option), } fn main() { match A::As<|> { - A::Bs(_, Some(_)) => (), - } - } - "#, - r#" - enum A { - As, - Bs(i32, Option) - } - fn main() { - match <|>A::As { - A::Bs(_, Some(_)) => (), - A::As => (), - _ => (), + A::As, + A::Bs{x,y:Some(_)} => (), + A::Cs(_, Some(_)) => (), } } "#, @@ -229,17 +152,19 @@ mod tests { } #[test] - fn partial_fill_record() { + fn partial_fill_record_tuple() { check_assist( fill_match_arms, r#" enum A { As, Bs{x:i32, y:Option}, + Cs(i32, Option), } fn main() { match A::As<|> { A::Bs{x,y:Some(_)} => (), + A::Cs(_, Some(_)) => (), } } "#, @@ -247,12 +172,13 @@ mod tests { enum A { As, Bs{x:i32, y:Option}, + Cs(i32, Option), } fn main() { match <|>A::As { A::Bs{x,y:Some(_)} => (), + A::Cs(_, Some(_)) => (), A::As => (), - _ => (), } } "#, @@ -291,39 +217,6 @@ mod tests { ); } - #[test] - fn partial_fill_or_pat2() { - check_assist( - fill_match_arms, - r#" - enum A { - As, - Bs, - Cs(Option), - } - fn main() { - match A::As<|> { - A::Cs(Some(_)) | A::Bs => (), - } - } - "#, - r#" - enum A { - As, - Bs, - Cs(Option), - } - fn main() { - match <|>A::As { - A::Cs(Some(_)) | A::Bs => (), - A::As => (), - _ => (), - } - } - "#, - ); - } - #[test] fn partial_fill() { check_assist( @@ -367,7 +260,6 @@ mod tests { A::Es(B::Xs) => (), A::As => (), A::Cs => (), - _ => (), } } "#,