Skip to content

Commit

Permalink
Merge #3960
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@bors @JoshMcguigan
bors[bot] and JoshMcguigan authored Apr 13, 2020
2 parents c388130 + ee822d1 commit d075f49
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 58 deletions.
27 changes: 22 additions & 5 deletions crates/ra_hir_def/src/body/lower.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use crate::{
};

use super::{ExprSource, PatSource};
use ast::AstChildren;

pub(super) fn lower(
db: &dyn DefDatabase,
Expand Down Expand Up @@ -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());
Expand All @@ -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 =
Expand Down Expand Up @@ -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,
};
Expand All @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions crates/ra_hir_def/src/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,15 +374,15 @@ 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 },
Slice { prefix: Vec<PatId>, slice: Option<PatId>, suffix: Vec<PatId> },
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 },
}

Expand All @@ -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),
Expand Down
160 changes: 113 additions & 47 deletions crates/ra_hir_ty/src/_match.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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))
}
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -1506,6 +1540,67 @@ mod tests {
check_no_diagnostic(content);
}

#[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"
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}
}
6 changes: 3 additions & 3 deletions crates/ra_hir_ty/src/infer/pat.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 { .. }
Expand Down Expand Up @@ -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,
_ => &[],
Expand Down Expand Up @@ -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: _ } => {
Expand Down

0 comments on commit d075f49

Please sign in to comment.