Merge #3960

3960: ellipsis in tuple patterns r=JoshMcguigan a=JoshMcguigan

This PR lowers ellipsis in tuple patterns. It fixes a bug in the way ellipsis were previously lowered (by replacing the ellipsis with a single `Pat::Wild` no matter how many items the `..` was taking the place of).

It also uses this new information to properly handle `..` in tuple struct patterns when perform match statement exhaustiveness checks.

While this PR provides the building blocks for match statement exhaustiveness checks for tuples, there are some additional challenges there, so that is still unimplemented (unlike tuple structs).

Co-authored-by: Josh Mcguigan <[email protected]>

4 files changed, 141 insertions, 58 deletions

diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs
index 6caa87db4..79abe55ce 100644
--- a/crates/ra_hir_def/src/body/lower.rs
+++ b/crates/ra_hir_def/src/body/lower.rs
@@ -33,6 +33,7 @@ use crate::{
 };
 
 use super::{ExprSource, PatSource};
+use ast::AstChildren;
 
 pub(super) fn lower(
     db: &dyn DefDatabase,
@@ -598,8 +599,8 @@ impl ExprCollector<'_> {
             }
             ast::Pat::TupleStructPat(p) => {
                 let path = p.path().and_then(|path| self.expander.parse_path(path));
-                let args = p.args().map(|p| self.collect_pat(p)).collect();
-                Pat::TupleStruct { path, args }
+                let (args, ellipsis) = self.collect_tuple_pat(p.args());
+                Pat::TupleStruct { path, args, ellipsis }
             }
             ast::Pat::RefPat(p) => {
                 let pat = self.collect_pat_opt(p.pat());
@@ -616,10 +617,10 @@ impl ExprCollector<'_> {
             }
             ast::Pat::ParenPat(p) => return self.collect_pat_opt(p.pat()),
             ast::Pat::TuplePat(p) => {
-                let args = p.args().map(|p| self.collect_pat(p)).collect();
-                Pat::Tuple(args)
+                let (args, ellipsis) = self.collect_tuple_pat(p.args());
+                Pat::Tuple { args, ellipsis }
             }
-            ast::Pat::PlaceholderPat(_) | ast::Pat::DotDotPat(_) => Pat::Wild,
+            ast::Pat::PlaceholderPat(_) => Pat::Wild,
             ast::Pat::RecordPat(p) => {
                 let path = p.path().and_then(|path| self.expander.parse_path(path));
                 let record_field_pat_list =
@@ -665,6 +666,9 @@ impl ExprCollector<'_> {
                     Pat::Missing
                 }
             }
+            ast::Pat::DotDotPat(_) => unreachable!(
+                "`DotDotPat` requires special handling and should not be mapped to a Pat."
+            ),
             // FIXME: implement
             ast::Pat::BoxPat(_) | ast::Pat::RangePat(_) | ast::Pat::MacroPat(_) => Pat::Missing,
         };
@@ -679,6 +683,19 @@ impl ExprCollector<'_> {
             self.missing_pat()
         }
     }
+
+    fn collect_tuple_pat(&mut self, args: AstChildren<ast::Pat>) -> (Vec<PatId>, Option<usize>) {
+        // Find the location of the `..`, if there is one. Note that we do not
+        // consider the possiblity of there being multiple `..` here.
+        let ellipsis = args.clone().position(|p| matches!(p, ast::Pat::DotDotPat(_)));
+        // We want to skip the `..` pattern here, since we account for it above.
+        let args = args
+            .filter(|p| !matches!(p, ast::Pat::DotDotPat(_)))
+            .map(|p| self.collect_pat(p))
+            .collect();
+
+        (args, ellipsis)
+    }
 }
 
 impl From<ast::BinOp> for BinaryOp {
diff --git a/crates/ra_hir_def/src/expr.rs b/crates/ra_hir_def/src/expr.rs
index e11bdf3ec..a0cdad529 100644
--- a/crates/ra_hir_def/src/expr.rs
+++ b/crates/ra_hir_def/src/expr.rs
@@ -374,7 +374,7 @@ pub struct RecordFieldPat {
 pub enum Pat {
     Missing,
     Wild,
-    Tuple(Vec<PatId>),
+    Tuple { args: Vec<PatId>, ellipsis: Option<usize> },
     Or(Vec<PatId>),
     Record { path: Option<Path>, args: Vec<RecordFieldPat>, ellipsis: bool },
     Range { start: ExprId, end: ExprId },
@@ -382,7 +382,7 @@ pub enum Pat {
     Path(Path),
     Lit(ExprId),
     Bind { mode: BindingAnnotation, name: Name, subpat: Option<PatId> },
-    TupleStruct { path: Option<Path>, args: Vec<PatId> },
+    TupleStruct { path: Option<Path>, args: Vec<PatId>, ellipsis: Option<usize> },
     Ref { pat: PatId, mutability: Mutability },
 }
 
@@ -393,7 +393,7 @@ impl Pat {
             Pat::Bind { subpat, .. } => {
                 subpat.iter().copied().for_each(f);
             }
-            Pat::Or(args) | Pat::Tuple(args) | Pat::TupleStruct { args, .. } => {
+            Pat::Or(args) | Pat::Tuple { args, .. } | Pat::TupleStruct { args, .. } => {
                 args.iter().copied().for_each(f);
             }
             Pat::Ref { pat, .. } => f(*pat),
diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs
index c482cf619..a64be9848 100644
--- a/crates/ra_hir_ty/src/_match.rs
+++ b/crates/ra_hir_ty/src/_match.rs
@@ -289,7 +289,7 @@ impl PatStack {
         Self::from_slice(&self.0[1..])
     }
 
-    fn replace_head_with(&self, pat_ids: &[PatId]) -> PatStack {
+    fn replace_head_with<T: Into<PatIdOrWild> + Copy>(&self, pat_ids: &[T]) -> PatStack {
         let mut patterns: PatStackInner = smallvec![];
         for pat in pat_ids {
             patterns.push((*pat).into());
@@ -320,12 +320,14 @@ impl PatStack {
         constructor: &Constructor,
     ) -> MatchCheckResult<Option<PatStack>> {
         let result = match (self.head().as_pat(cx), constructor) {
-            (Pat::Tuple(ref pat_ids), Constructor::Tuple { arity }) => {
-                debug_assert_eq!(
-                    pat_ids.len(),
-                    *arity,
-                    "we type check before calling this code, so we should never hit this case",
-                );
+            (Pat::Tuple { args: ref pat_ids, ellipsis }, Constructor::Tuple { arity: _ }) => {
+                if ellipsis.is_some() {
+                    // If there are ellipsis here, we should add the correct number of
+                    // Pat::Wild patterns to `pat_ids`. We should be able to use the
+                    // constructors arity for this, but at the time of writing we aren't
+                    // correctly calculating this arity when ellipsis are present.
+                    return Err(MatchCheckErr::NotImplemented);
+                }
 
                 Some(self.replace_head_with(pat_ids))
             }
@@ -351,19 +353,47 @@ impl PatStack {
                     Some(self.to_tail())
                 }
             }
-            (Pat::TupleStruct { args: ref pat_ids, .. }, Constructor::Enum(enum_constructor)) => {
+            (
+                Pat::TupleStruct { args: ref pat_ids, ellipsis, .. },
+                Constructor::Enum(enum_constructor),
+            ) => {
                 let pat_id = self.head().as_id().expect("we know this isn't a wild");
                 if !enum_variant_matches(cx, pat_id, *enum_constructor) {
                     None
                 } else {
-                    // If the enum variant matches, then we need to confirm
-                    // that the number of patterns aligns with the expected
-                    // number of patterns for that enum variant.
-                    if pat_ids.len() != constructor.arity(cx)? {
-                        return Err(MatchCheckErr::MalformedMatchArm);
+                    let constructor_arity = constructor.arity(cx)?;
+                    if let Some(ellipsis_position) = ellipsis {
+                        // If there are ellipsis in the pattern, the ellipsis must take the place
+                        // of at least one sub-pattern, so `pat_ids` should be smaller than the
+                        // constructor arity.
+                        if pat_ids.len() < constructor_arity {
+                            let mut new_patterns: Vec<PatIdOrWild> = vec![];
+
+                            for pat_id in &pat_ids[0..ellipsis_position] {
+                                new_patterns.push((*pat_id).into());
+                            }
+
+                            for _ in 0..(constructor_arity - pat_ids.len()) {
+                                new_patterns.push(PatIdOrWild::Wild);
+                            }
+
+                            for pat_id in &pat_ids[ellipsis_position..pat_ids.len()] {
+                                new_patterns.push((*pat_id).into());
+                            }
+
+                            Some(self.replace_head_with(&new_patterns))
+                        } else {
+                            return Err(MatchCheckErr::MalformedMatchArm);
+                        }
+                    } else {
+                        // If there is no ellipsis in the tuple pattern, the number
+                        // of patterns must equal the constructor arity.
+                        if pat_ids.len() == constructor_arity {
+                            Some(self.replace_head_with(pat_ids))
+                        } else {
+                            return Err(MatchCheckErr::MalformedMatchArm);
+                        }
                     }
-
-                    Some(self.replace_head_with(pat_ids))
                 }
             }
             (Pat::Or(_), _) => return Err(MatchCheckErr::NotImplemented),
@@ -644,7 +674,11 @@ impl Constructor {
 fn pat_constructor(cx: &MatchCheckCtx, pat: PatIdOrWild) -> MatchCheckResult<Option<Constructor>> {
     let res = match pat.as_pat(cx) {
         Pat::Wild => None,
-        Pat::Tuple(pats) => Some(Constructor::Tuple { arity: pats.len() }),
+        // FIXME somehow create the Tuple constructor with the proper arity. If there are
+        // ellipsis, the arity is not equal to the number of patterns.
+        Pat::Tuple { args: pats, ellipsis } if ellipsis.is_none() => {
+            Some(Constructor::Tuple { arity: pats.len() })
+        }
         Pat::Lit(lit_expr) => match cx.body.exprs[lit_expr] {
             Expr::Literal(Literal::Bool(val)) => Some(Constructor::Bool(val)),
             _ => return Err(MatchCheckErr::NotImplemented),
@@ -1507,6 +1541,67 @@ mod tests {
     }
 
     #[test]
+    fn enum_tuple_partial_ellipsis_2_no_diagnostic() {
+        let content = r"
+            enum Either {
+                A(bool, bool, bool, bool),
+                B,
+            }
+            fn test_fn() {
+                match Either::B {
+                    Either::A(true, .., true) => {},
+                    Either::A(true, .., false) => {},
+                    Either::A(.., true) => {},
+                    Either::A(.., false) => {},
+                    Either::B => {},
+                }
+            }
+        ";
+
+        check_no_diagnostic(content);
+    }
+
+    #[test]
+    fn enum_tuple_partial_ellipsis_missing_arm() {
+        let content = r"
+            enum Either {
+                A(bool, bool, bool, bool),
+                B,
+            }
+            fn test_fn() {
+                match Either::B {
+                    Either::A(true, .., true) => {},
+                    Either::A(true, .., false) => {},
+                    Either::A(false, .., false) => {},
+                    Either::B => {},
+                }
+            }
+        ";
+
+        check_diagnostic(content);
+    }
+
+    #[test]
+    fn enum_tuple_partial_ellipsis_2_missing_arm() {
+        let content = r"
+            enum Either {
+                A(bool, bool, bool, bool),
+                B,
+            }
+            fn test_fn() {
+                match Either::B {
+                    Either::A(true, .., true) => {},
+                    Either::A(true, .., false) => {},
+                    Either::A(.., true) => {},
+                    Either::B => {},
+                }
+            }
+        ";
+
+        check_diagnostic(content);
+    }
+
+    #[test]
     fn enum_tuple_ellipsis_no_diagnostic() {
         let content = r"
             enum Either {
@@ -1645,11 +1740,7 @@ mod false_negatives {
         ";
 
         // This is a false negative.
-        // The `..` pattern is currently lowered to a single `Pat::Wild`
-        // no matter how many fields the `..` pattern is covering. This
-        // causes the match arm in this test not to type check against
-        // the match expression, which causes this diagnostic not to
-        // fire.
+        // We don't currently handle tuple patterns with ellipsis.
         check_no_diagnostic(content);
     }
 
@@ -1664,32 +1755,7 @@ mod false_negatives {
         ";
 
         // This is a false negative.
-        // See comments on `tuple_of_bools_with_ellipsis_at_end_missing_arm`.
-        check_no_diagnostic(content);
-    }
-
-    #[test]
-    fn enum_tuple_partial_ellipsis_missing_arm() {
-        let content = r"
-            enum Either {
-                A(bool, bool, bool, bool),
-                B,
-            }
-            fn test_fn() {
-                match Either::B {
-                    Either::A(true, .., true) => {},
-                    Either::A(true, .., false) => {},
-                    Either::A(false, .., false) => {},
-                    Either::B => {},
-                }
-            }
-        ";
-
-        // This is a false negative.
-        // The `..` pattern is currently lowered to a single `Pat::Wild`
-        // no matter how many fields the `..` pattern is covering. This
-        // causes us to return a `MatchCheckErr::MalformedMatchArm` in
-        // `Pat::specialize_constructor`.
+        // We don't currently handle tuple patterns with ellipsis.
         check_no_diagnostic(content);
     }
 }
diff --git a/crates/ra_hir_ty/src/infer/pat.rs b/crates/ra_hir_ty/src/infer/pat.rs
index 078476f76..8ec4d4ace 100644
--- a/crates/ra_hir_ty/src/infer/pat.rs
+++ b/crates/ra_hir_ty/src/infer/pat.rs
@@ -85,7 +85,7 @@ impl<'a> InferenceContext<'a> {
         let body = Arc::clone(&self.body); // avoid borrow checker problem
 
         let is_non_ref_pat = match &body[pat] {
-            Pat::Tuple(..)
+            Pat::Tuple { .. }
             | Pat::Or(..)
             | Pat::TupleStruct { .. }
             | Pat::Record { .. }
@@ -116,7 +116,7 @@ impl<'a> InferenceContext<'a> {
         let expected = expected;
 
         let ty = match &body[pat] {
-            Pat::Tuple(ref args) => {
+            Pat::Tuple { ref args, .. } => {
                 let expectations = match expected.as_tuple() {
                     Some(parameters) => &*parameters.0,
                     _ => &[],
@@ -155,7 +155,7 @@ impl<'a> InferenceContext<'a> {
                 let subty = self.infer_pat(*pat, expectation, default_bm);
                 Ty::apply_one(TypeCtor::Ref(*mutability), subty)
             }
-            Pat::TupleStruct { path: p, args: subpats } => {
+            Pat::TupleStruct { path: p, args: subpats, .. } => {
                 self.infer_tuple_struct_pat(p.as_ref(), subpats, expected, default_bm, pat)
             }
             Pat::Record { path: p, args: fields, ellipsis: _ } => {