From 9a341e3d755a0ef8c565b957375f18ca5e089e65 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Thu, 21 Apr 2022 10:10:50 -0400 Subject: [PATCH] Improve non-exhaustive typechecking errors We will replace this with proper exhaustiveness checking when we have that in the typechecking phase, but for now this should do. --- compiler/constrain/src/expr.rs | 4 +- reporting/src/error/type.rs | 119 +++++++++++++++++++++--------- reporting/tests/test_reporting.rs | 36 +++------ 3 files changed, 99 insertions(+), 60 deletions(-) diff --git a/compiler/constrain/src/expr.rs b/compiler/constrain/src/expr.rs index 78f82d13e1..df3806a872 100644 --- a/compiler/constrain/src/expr.rs +++ b/compiler/constrain/src/expr.rs @@ -708,7 +708,9 @@ pub fn constrain_expr( // After solving the condition variable with what's expected from the branch patterns, // check it against the condition expression. - // This is basically exhaustiveness checking, but doesn't check for redundancy. + // TODO: when we have exhaustiveness checking during the typechecking phase, perform + // exhaustiveness checking when this expectation fails. That will produce better error + // messages. let cond_constraint = constrain_expr( constraints, env, diff --git a/reporting/src/error/type.rs b/reporting/src/error/type.rs index 71286b58ac..ca4d9e8c0c 100644 --- a/reporting/src/error/type.rs +++ b/reporting/src/error/type.rs @@ -1151,34 +1151,50 @@ fn to_expr_report<'b>( ) } - Reason::WhenBranches => report_mismatch( - alloc, - lines, - filename, - &category, - found, - expected_type, - // TODO: these should be flipped but `report_mismatch` takes the wrong arguments to - // `region_with_subregion` - expr_region, - Some(region), - alloc.concat([ - alloc.reflow("The branches of this "), - alloc.keyword("when"), - alloc.reflow(" expression don't match the condition:"), - ]), - alloc.concat([ + Reason::WhenBranches => { + let snippet = alloc.region_with_subregion( + lines.convert_region(region), + lines.convert_region(expr_region), + ); + + let this_is = alloc.concat([ alloc.reflow("The "), alloc.keyword("when"), alloc.reflow(" condition is"), - ]), - alloc.reflow("But the branch patterns have type:"), - Some(alloc.concat([ + ]); + + let wanted = alloc.reflow("But the branch patterns have type:"); + let details = Some(alloc.concat([ alloc.reflow("The branches must be cases of the "), alloc.keyword("when"), alloc.reflow(" condition's type!"), - ])), - ), + ])); + + let lines = [ + alloc.concat([ + alloc.reflow("The branches of this "), + alloc.keyword("when"), + alloc.reflow(" expression don't match the condition:"), + ]), + snippet, + type_comparison( + alloc, + found, + expected_type, + ExpectationContext::WhenCondition, + add_category(alloc, this_is, &category), + wanted, + details, + ), + ]; + + Report { + title: "TYPE MISMATCH".to_string(), + filename, + doc: alloc.stack(lines), + severity: Severity::RuntimeError, + } + } Reason::LowLevelOpArg { op, arg_index } => { panic!( @@ -1253,7 +1269,10 @@ fn count_arguments(tipe: &ErrorType) -> usize { enum ExpectationContext<'a> { /// An expected type was discovered from a type annotation. Corresponds to /// [`Expected::FromAnnotation`](Expected::FromAnnotation). - Annotation { on: RocDocBuilder<'a> }, + Annotation { + on: RocDocBuilder<'a>, + }, + WhenCondition, /// When we don't know the context, or it's not relevant. Arbitrary, } @@ -1262,6 +1281,7 @@ impl<'a> std::fmt::Debug for ExpectationContext<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { ExpectationContext::Annotation { .. } => f.write_str("Annotation"), + ExpectationContext::WhenCondition => f.write_str("WhenCondition"), ExpectationContext::Arbitrary => f.write_str("Arbitrary"), } } @@ -2679,22 +2699,24 @@ fn diff_tag_union<'b>( let all_fields_shared = left.peek().is_none() && right.peek().is_none(); let status = match (ext_has_fixed_fields(&ext1), ext_has_fixed_fields(&ext2)) { - (true, true) => match left.peek() { - Some((f, _, _, _)) => Status::Different(vec![Problem::TagTypo( + (true, true) => match (left.peek(), right.peek()) { + (Some((f, _, _, _)), Some(_)) => Status::Different(vec![Problem::TagTypo( f.clone(), fields2.keys().cloned().collect(), )]), - None => { - if right.peek().is_none() { - Status::Similar - } else { - let result = - Status::Different(vec![Problem::TagsMissing(right.map(|v| v.0).collect())]); - // we just used the values in `right`. in - right = right_keys.iter().map(to_unknown_docs).peekable(); - result - } + (Some(_), None) => { + let status = + Status::Different(vec![Problem::TagsMissing(left.map(|v| v.0).collect())]); + left = left_keys.iter().map(to_unknown_docs).peekable(); + status } + (None, Some(_)) => { + let status = + Status::Different(vec![Problem::TagsMissing(right.map(|v| v.0).collect())]); + right = right_keys.iter().map(to_unknown_docs).peekable(); + status + } + (None, None) => Status::Similar, }, (false, true) => match left.peek() { Some((f, _, _, _)) => Status::Different(vec![Problem::TagTypo( @@ -3294,6 +3316,33 @@ fn type_problem_to_pretty<'b>( alloc.reflow("."), ])), + (TagsMissing(missing), ExpectationContext::WhenCondition) => match missing.split_last() { + None => alloc.nil(), + Some(split) => { + let missing_tags = match split { + (f1, []) => alloc.tag_name(f1.clone()).append(alloc.reflow(" tag.")), + (last, init) => alloc + .intersperse(init.iter().map(|v| alloc.tag_name(v.clone())), ", ") + .append(alloc.reflow(" and ")) + .append(alloc.tag_name(last.clone())) + .append(alloc.reflow(" tags.")), + }; + + let tip1 = alloc + .tip() + .append(alloc.reflow("Looks like the branches are missing coverage of the ")) + .append(missing_tags); + + let tip2 = alloc + .tip() + .append(alloc.reflow("Maybe you need to add a catch-all branch, like ")) + .append(alloc.keyword("_")) + .append(alloc.reflow("?")); + + alloc.stack([tip1, tip2]) + } + }, + (TagsMissing(missing), _) => match missing.split_last() { None => alloc.nil(), Some((f1, [])) => { diff --git a/reporting/tests/test_reporting.rs b/reporting/tests/test_reporting.rs index 6918dfd6d9..a1b0e4b69c 100644 --- a/reporting/tests/test_reporting.rs +++ b/reporting/tests/test_reporting.rs @@ -2740,11 +2740,10 @@ mod test_reporting { [ Left a ] - Tip: Seems like a tag typo. Maybe `Right` should be `Left`? + Tip: Looks like a closed tag union does not have the `Right` tag. - Tip: Can more type annotations be added? Type annotations always help - me give more specific messages, and I think they could help a lot in - this case + Tip: Closed tag unions can't grow, because that might change the size + in memory. Can you use an open tag union? "# ), ) @@ -2790,7 +2789,6 @@ mod test_reporting { Red -> 3 "# ), - // TODO(2903): improve tag typo quality indoc!( r#" ── TYPE MISMATCH ───────────────────────────────────────── /code/proj/Main.roc ─ @@ -2810,11 +2808,9 @@ mod test_reporting { The branches must be cases of the `when` condition's type! - Tip: Seems like a tag typo. Maybe `Green` should be `Red`? + Tip: Looks like the branches are missing coverage of the `Green` tag. - Tip: Can more type annotations be added? Type annotations always help - me give more specific messages, and I think they could help a lot in - this case + Tip: Maybe you need to add a catch-all branch, like `_`? "# ), ) @@ -2833,7 +2829,6 @@ mod test_reporting { Green -> 1 "# ), - // TODO(2903): improve tag typo quality indoc!( r#" ── TYPE MISMATCH ───────────────────────────────────────── /code/proj/Main.roc ─ @@ -2854,11 +2849,9 @@ mod test_reporting { The branches must be cases of the `when` condition's type! - Tip: Seems like a tag typo. Maybe `Blue` should be `Red`? + Tip: Looks like the branches are missing coverage of the `Blue` tag. - Tip: Can more type annotations be added? Type annotations always help - me give more specific messages, and I think they could help a lot in - this case + Tip: Maybe you need to add a catch-all branch, like `_`? "# ), ) @@ -2877,7 +2870,6 @@ mod test_reporting { NotAsked -> 3 "# ), - // TODO(2903): improve tag typo quality indoc!( r#" ── TYPE MISMATCH ───────────────────────────────────────── /code/proj/Main.roc ─ @@ -2897,11 +2889,10 @@ mod test_reporting { The branches must be cases of the `when` condition's type! - Tip: Seems like a tag typo. Maybe `Success` should be `NotAsked`? + Tip: Looks like the branches are missing coverage of the + `Success`, `Failure` and `Loading` tags. - Tip: Can more type annotations be added? Type annotations always help - me give more specific messages, and I think they could help a lot in - this case + Tip: Maybe you need to add a catch-all branch, like `_`? "# ), ) @@ -8714,7 +8705,6 @@ I need all branches in an `if` to have the same type! $F B -> "" "# ), - // TODO(2903): improve tag typo quality indoc!( r#" ── TYPE MISMATCH ───────────────────────────────────────── /code/proj/Main.roc ─ @@ -8735,11 +8725,9 @@ I need all branches in an `if` to have the same type! The branches must be cases of the `when` condition's type! - Tip: Seems like a tag typo. Maybe `C` should be `A`? + Tip: Looks like the branches are missing coverage of the `C` tag. - Tip: Can more type annotations be added? Type annotations always help - me give more specific messages, and I think they could help a lot in - this case + Tip: Maybe you need to add a catch-all branch, like `_`? "# ), )