From 4c29214bba65d23e18875bd060325c489be5a8e4 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Tue, 7 Apr 2020 17:09:02 +0200 Subject: Move computation of missing fields into hir --- crates/ra_hir_ty/src/expr.rs | 158 +++++++++++++++++++++++++------------------ 1 file changed, 91 insertions(+), 67 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 1e7395b16..b4592fbf5 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -2,12 +2,8 @@ use std::sync::Arc; -use hir_def::{ - path::{path, Path}, - resolver::HasResolver, - AdtId, FunctionId, -}; -use hir_expand::{diagnostics::DiagnosticSink, name::Name}; +use hir_def::{path::path, resolver::HasResolver, AdtId, FunctionId}; +use hir_expand::diagnostics::DiagnosticSink; use ra_syntax::ast; use ra_syntax::AstPtr; use rustc_hash::FxHashSet; @@ -29,7 +25,7 @@ pub use hir_def::{ ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Literal, LogicOp, MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField, Statement, UnaryOp, }, - VariantId, + LocalStructFieldId, VariantId, }; pub struct ExprValidator<'a, 'b: 'a> { @@ -50,14 +46,37 @@ impl<'a, 'b> ExprValidator<'a, 'b> { pub fn validate_body(&mut self, db: &dyn HirDatabase) { let body = db.body(self.func.into()); - for e in body.exprs.iter() { - if let (id, Expr::RecordLit { path, fields, spread }) = e { - self.validate_record_literal(id, path, fields, *spread, db); - } else if let (id, Expr::Match { expr, arms }) = e { + for (id, expr) in body.exprs.iter() { + if let Some((variant_def, missed_fields, true)) = + record_literal_missing_fields(db, &self.infer, id, expr) + { + // XXX: only look at source_map if we do have missing fields + let (_, source_map) = db.body_with_source_map(self.func.into()); + + if let Ok(source_ptr) = source_map.expr_syntax(id) { + if let Some(expr) = source_ptr.value.left() { + let root = source_ptr.file_syntax(db.upcast()); + if let ast::Expr::RecordLit(record_lit) = expr.to_node(&root) { + if let Some(field_list) = record_lit.record_field_list() { + let variant_data = variant_data(db.upcast(), variant_def); + let missed_fields = missed_fields + .into_iter() + .map(|idx| variant_data.fields()[idx].name.clone()) + .collect(); + self.sink.push(MissingFields { + file: source_ptr.file_id, + field_list: AstPtr::new(&field_list), + missed_fields, + }) + } + } + } + } + } + if let Expr::Match { expr, arms } = expr { self.validate_match(id, *expr, arms, db, self.infer.clone()); } } - let body_expr = &body[body.body_expr]; if let Expr::Block { tail: Some(t), .. } = body_expr { self.validate_results_in_tail_expr(body.body_expr, *t, db); @@ -146,61 +165,6 @@ impl<'a, 'b> ExprValidator<'a, 'b> { } } - fn validate_record_literal( - &mut self, - id: ExprId, - _path: &Option, - fields: &[RecordLitField], - spread: Option, - db: &dyn HirDatabase, - ) { - if spread.is_some() { - return; - }; - let variant_def: VariantId = match self.infer.variant_resolution_for_expr(id) { - Some(VariantId::UnionId(_)) | None => return, - Some(it) => it, - }; - if let VariantId::UnionId(_) = variant_def { - return; - } - - let variant_data = variant_data(db.upcast(), variant_def); - - let lit_fields: FxHashSet<_> = fields.iter().map(|f| &f.name).collect(); - let missed_fields: Vec = variant_data - .fields() - .iter() - .filter_map(|(_f, d)| { - let name = d.name.clone(); - if lit_fields.contains(&name) { - None - } else { - Some(name) - } - }) - .collect(); - if missed_fields.is_empty() { - return; - } - let (_, source_map) = db.body_with_source_map(self.func.into()); - - if let Ok(source_ptr) = source_map.expr_syntax(id) { - if let Some(expr) = source_ptr.value.left() { - let root = source_ptr.file_syntax(db.upcast()); - if let ast::Expr::RecordLit(record_lit) = expr.to_node(&root) { - if let Some(field_list) = record_lit.record_field_list() { - self.sink.push(MissingFields { - file: source_ptr.file_id, - field_list: AstPtr::new(&field_list), - missed_fields, - }) - } - } - } - } - } - fn validate_results_in_tail_expr(&mut self, body_id: ExprId, id: ExprId, db: &dyn HirDatabase) { // the mismatch will be on the whole block currently let mismatch = match self.infer.type_mismatch_for_expr(body_id) { @@ -233,3 +197,63 @@ impl<'a, 'b> ExprValidator<'a, 'b> { } } } + +pub fn record_literal_missing_fields( + db: &dyn HirDatabase, + infer: &InferenceResult, + id: ExprId, + expr: &Expr, +) -> Option<(VariantId, Vec, /*exhaustive*/ bool)> { + let (fields, exhausitve) = match expr { + Expr::RecordLit { path: _, fields, spread } => (fields, spread.is_none()), + _ => return None, + }; + + let variant_def = infer.variant_resolution_for_expr(id)?; + if let VariantId::UnionId(_) = variant_def { + return None; + } + + let variant_data = variant_data(db.upcast(), variant_def); + + let specified_fields: FxHashSet<_> = fields.iter().map(|f| &f.name).collect(); + let missed_fields: Vec = variant_data + .fields() + .iter() + .filter_map(|(f, d)| if specified_fields.contains(&d.name) { None } else { Some(f) }) + .collect(); + if missed_fields.is_empty() { + return None; + } + Some((variant_def, missed_fields, exhausitve)) +} + +pub fn record_pattern_missing_fields( + db: &dyn HirDatabase, + infer: &InferenceResult, + id: PatId, + pat: &Pat, +) -> Option<(VariantId, Vec)> { + let fields = match pat { + Pat::Record { path: _, args } => args, + _ => return None, + }; + + let variant_def = infer.variant_resolution_for_pat(id)?; + if let VariantId::UnionId(_) = variant_def { + return None; + } + + let variant_data = variant_data(db.upcast(), variant_def); + + let specified_fields: FxHashSet<_> = fields.iter().map(|f| &f.name).collect(); + let missed_fields: Vec = variant_data + .fields() + .iter() + .filter_map(|(f, d)| if specified_fields.contains(&d.name) { None } else { Some(f) }) + .collect(); + if missed_fields.is_empty() { + return None; + } + Some((variant_def, missed_fields)) +} -- cgit v1.2.3 From 941615748d9000e66ff4400ae519dc60410a11f7 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 7 Apr 2020 15:32:14 -0700 Subject: fix panic in match checking when tuple enum missing pattern --- crates/ra_hir_ty/src/_match.rs | 60 +++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 15 deletions(-) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index f29a25505..037db5cf8 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -235,7 +235,10 @@ impl From for PatIdOrWild { } #[derive(Debug, Clone, Copy, PartialEq)] -pub struct MatchCheckNotImplemented; +pub enum MatchCheckErr { + NotImplemented, + MalformedMatchArm, +} /// The return type of `is_useful` is either an indication of usefulness /// of the match arm, or an error in the case the match statement @@ -244,7 +247,7 @@ pub struct MatchCheckNotImplemented; /// /// The `std::result::Result` type is used here rather than a custom enum /// to allow the use of `?`. -pub type MatchCheckResult = Result; +pub type MatchCheckResult = Result; #[derive(Debug)] /// A row in a Matrix. @@ -335,12 +338,12 @@ impl PatStack { Expr::Literal(Literal::Bool(_)) => None, // perhaps this is actually unreachable given we have // already checked that these match arms have the appropriate type? - _ => return Err(MatchCheckNotImplemented), + _ => return Err(MatchCheckErr::NotImplemented), } } (Pat::Wild, constructor) => Some(self.expand_wildcard(cx, constructor)?), (Pat::Path(_), Constructor::Enum(constructor)) => { - // enums with no associated data become `Pat::Path` + // unit enum variants become `Pat::Path` let pat_id = self.head().as_id().expect("we know this isn't a wild"); if !enum_variant_matches(cx, pat_id, *constructor) { None @@ -348,16 +351,23 @@ impl PatStack { Some(self.to_tail()) } } - (Pat::TupleStruct { args: ref pat_ids, .. }, Constructor::Enum(constructor)) => { + (Pat::TupleStruct { args: ref pat_ids, .. }, 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, *constructor) { + 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); + } + Some(self.replace_head_with(pat_ids)) } } - (Pat::Or(_), _) => return Err(MatchCheckNotImplemented), - (_, _) => return Err(MatchCheckNotImplemented), + (Pat::Or(_), _) => return Err(MatchCheckErr::NotImplemented), + (_, _) => return Err(MatchCheckErr::NotImplemented), }; Ok(result) @@ -514,7 +524,7 @@ pub(crate) fn is_useful( return if any_useful { Ok(Usefulness::Useful) } else if found_unimplemented { - Err(MatchCheckNotImplemented) + Err(MatchCheckErr::NotImplemented) } else { Ok(Usefulness::NotUseful) }; @@ -567,7 +577,7 @@ pub(crate) fn is_useful( } if found_unimplemented { - Err(MatchCheckNotImplemented) + Err(MatchCheckErr::NotImplemented) } else { Ok(Usefulness::NotUseful) } @@ -604,7 +614,7 @@ impl Constructor { match cx.db.enum_data(e.parent).variants[e.local_id].variant_data.as_ref() { VariantData::Tuple(struct_field_data) => struct_field_data.len(), VariantData::Unit => 0, - _ => return Err(MatchCheckNotImplemented), + _ => return Err(MatchCheckErr::NotImplemented), } } }; @@ -637,20 +647,20 @@ fn pat_constructor(cx: &MatchCheckCtx, pat: PatIdOrWild) -> MatchCheckResult 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(MatchCheckNotImplemented), + _ => return Err(MatchCheckErr::NotImplemented), }, Pat::TupleStruct { .. } | Pat::Path(_) => { let pat_id = pat.as_id().expect("we already know this pattern is not a wild"); let variant_id = - cx.infer.variant_resolution_for_pat(pat_id).ok_or(MatchCheckNotImplemented)?; + cx.infer.variant_resolution_for_pat(pat_id).ok_or(MatchCheckErr::NotImplemented)?; match variant_id { VariantId::EnumVariantId(enum_variant_id) => { Some(Constructor::Enum(enum_variant_id)) } - _ => return Err(MatchCheckNotImplemented), + _ => return Err(MatchCheckErr::NotImplemented), } } - _ => return Err(MatchCheckNotImplemented), + _ => return Err(MatchCheckErr::NotImplemented), }; Ok(res) @@ -1324,6 +1334,26 @@ mod tests { check_diagnostic(content); } + #[test] + fn malformed_match_arm_tuple_enum_missing_pattern() { + let content = r" + enum Either { + A, + B(u32), + } + fn test_fn() { + match Either::A { + Either::A => (), + Either::B() => (), + } + } + "; + + // We are testing to be sure we don't panic here when the match + // arm `Either::B` is missing its pattern. + check_no_diagnostic(content); + } + #[test] fn enum_not_in_scope() { let content = r" -- cgit v1.2.3 From 36c110ee096d46b02aa2a5adfaf138cd8c3872a7 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Tue, 7 Apr 2020 15:39:50 -0700 Subject: match checking add additional test for match checking tuple with missing pattern --- crates/ra_hir_ty/src/_match.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'crates/ra_hir_ty') diff --git a/crates/ra_hir_ty/src/_match.rs b/crates/ra_hir_ty/src/_match.rs index 037db5cf8..9e9a9d047 100644 --- a/crates/ra_hir_ty/src/_match.rs +++ b/crates/ra_hir_ty/src/_match.rs @@ -1334,6 +1334,20 @@ mod tests { check_diagnostic(content); } + #[test] + fn malformed_match_arm_tuple_missing_pattern() { + let content = r" + fn test_fn() { + match (0) { + () => (), + } + } + "; + + // Match arms with the incorrect type are filtered out. + check_diagnostic(content); + } + #[test] fn malformed_match_arm_tuple_enum_missing_pattern() { let content = r" -- cgit v1.2.3