improve tag union errors

compiler/reporting/src/type_error.rs

@@ -4,7 +4,7 @@ use crate::report::{
 };
 use roc_can::expected::{Expected, PExpected};
 use roc_collections::all::SendMap;
-use roc_module::ident::Lowercase;
+use roc_module::ident::{Lowercase, TagName};
 use roc_module::symbol::Symbol;
 use roc_solve::solve;
 use roc_types::pretty_print::Parens;
@@ -316,10 +316,10 @@ fn to_expr_report(
                 plain_text("But it should be:"),
                 plain_text("Record update syntax does not allow you to change the type of fields. You can achieve that with record literal syntax."),
             )}
-            Reason::FnArg { name , arg_index } => { 
+            Reason::FnArg { name , arg_index } => {
                 let ith = int_to_ordinal(arg_index as usize + 1);
 
-                let this_function = match name { 
+                let this_function = match name {
                     None => plain_text("this function"),
                     Some(symbol) => ReportText::Value(symbol),
                 };
@@ -337,7 +337,7 @@ fn to_expr_report(
                     Concat(vec![ plain_text("But "), this_function, plain_text(&format!(" needs the {} argument to be:", ith))]),
                     plain_text(""),
                 )
-                
+
             }
             other => {
                 //    NamedFnArg(String /* function name */, u8 /* arg index */),
@@ -393,7 +393,6 @@ fn lone_type(
 }
 
 fn add_category(this_is: ReportText, category: &Category) -> ReportText {
-    use roc_module::ident::TagName;
     use Category::*;
     use ReportText::*;
 
@@ -426,12 +425,12 @@ fn add_category(this_is: ReportText, category: &Category) -> ReportText {
         TagApply(TagName::Global(name)) => Concat(vec![
             plain_text("This "),
             global_tag_text(name.as_str()),
-            plain_text(" global tag application produces:"),
+            plain_text(" global tag application has the type:"),
         ]),
         TagApply(TagName::Private(name)) => Concat(vec![
             plain_text("This "),
             private_tag_text(*name),
-            plain_text(" private tag application produces:"),
+            plain_text(" private tag application has the type:"),
         ]),
 
         Record => Concat(vec![this_is, plain_text(" a record of type:")]),
@@ -503,8 +502,12 @@ fn to_circular_report(
 pub enum Problem {
     IntFloat,
     ArityMismatch(usize, usize),
-    FieldTypo(Vec<Lowercase>),
+    FieldTypo(Lowercase, Vec<Lowercase>),
     FieldsMissing(Vec<Lowercase>),
+
+    // TODO maybe these should include the arguments too?
+    TagTypo(TagName, Vec<TagName>),
+    TagsMissing(Vec<TagName>),
     BadRigidVar(Lowercase, ErrorType),
 }
 
@@ -744,6 +747,8 @@ fn to_diff(parens: Parens, type1: &ErrorType, type2: &ErrorType) -> Diff<ReportT
         }
 
         (Record(fields1, ext1), Record(fields2, ext2)) => diff_record(fields1, ext1, fields2, ext2),
+
+        (TagUnion(tags1, ext1), TagUnion(tags2, ext2)) => diff_tag_union(tags1, ext1, tags2, ext2),
         _ => {
             // TODO actually diff
 
@@ -828,9 +833,10 @@ fn diff_record(
 
     let status = match (ext_has_fixed_fields(&ext1), ext_has_fixed_fields(&ext2)) {
         (true, true) => match left.peek() {
-            Some((_, _f, _)) => {
+            Some((f, _, _)) => Status::Different(vec![Problem::FieldTypo(
-                Status::Different(vec![Problem::FieldTypo(fields2.keys().cloned().collect())])
+                f.clone(),
-            }
+                fields2.keys().cloned().collect(),
+            )]),
             None => {
                 if right.peek().is_none() {
                     Status::Similar
@@ -845,15 +851,17 @@ fn diff_record(
             }
         },
         (false, true) => match left.peek() {
-            Some((_, _f, _)) => {
+            Some((f, _, _)) => Status::Different(vec![Problem::FieldTypo(
-                Status::Different(vec![Problem::FieldTypo(fields2.keys().cloned().collect())])
+                f.clone(),
-            }
+                fields2.keys().cloned().collect(),
+            )]),
             None => Status::Similar,
         },
         (true, false) => match right.peek() {
-            Some((_, _f, _)) => {
+            Some((f, _, _)) => Status::Different(vec![Problem::FieldTypo(
-                Status::Different(vec![Problem::FieldTypo(fields1.keys().cloned().collect())])
+                f.clone(),
-            }
+                fields1.keys().cloned().collect(),
+            )]),
             None => Status::Similar,
         },
         (false, false) => Status::Similar,
@@ -891,6 +899,125 @@ fn diff_record(
     }
 }
 
+fn diff_tag_union(
+    fields1: &SendMap<TagName, Vec<ErrorType>>,
+    ext1: &TypeExt,
+    fields2: &SendMap<TagName, Vec<ErrorType>>,
+    ext2: &TypeExt,
+) -> Diff<ReportText> {
+    let to_overlap_docs = |(field, (t1, t2)): &(TagName, (Vec<ErrorType>, Vec<ErrorType>))| {
+        let diff = traverse(Parens::Unnecessary, t1, t2);
+
+        Diff {
+            left: (field.clone(), tag_name_text(field.clone()), diff.left),
+            right: (field.clone(), tag_name_text(field.clone()), diff.right),
+            status: diff.status,
+        }
+    };
+    let to_unknown_docs = |(field, args): &(TagName, Vec<ErrorType>)| {
+        (
+            field.clone(),
+            tag_name_text(field.clone()),
+            // TODO add spaces between args
+            args.iter()
+                .map(|arg| to_doc(Parens::Unnecessary, arg))
+                .collect(),
+        )
+    };
+    let shared_keys = fields1
+        .clone()
+        .intersection_with(fields2.clone(), |v1, v2| (v1, v2));
+
+    let left_keys = fields1.clone().relative_complement(fields2.clone());
+    let right_keys = fields2.clone().relative_complement(fields1.clone());
+
+    let both = shared_keys.iter().map(to_overlap_docs);
+    let mut left = left_keys.iter().map(to_unknown_docs).peekable();
+    let mut right = right_keys.iter().map(to_unknown_docs).peekable();
+
+    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(
+                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
+                }
+            }
+        },
+        (false, true) => match left.peek() {
+            Some((f, _, _)) => Status::Different(vec![Problem::TagTypo(
+                f.clone(),
+                fields2.keys().cloned().collect(),
+            )]),
+            None => Status::Similar,
+        },
+        (true, false) => match right.peek() {
+            Some((f, _, _)) => Status::Different(vec![Problem::TagTypo(
+                f.clone(),
+                fields1.keys().cloned().collect(),
+            )]),
+            None => Status::Similar,
+        },
+        (false, false) => Status::Similar,
+    };
+
+    let ext_diff = ext_to_diff(ext1, ext2);
+
+    let mut fields_diff: Diff<Vec<(TagName, ReportText, Vec<ReportText>)>> = Diff {
+        left: vec![],
+        right: vec![],
+        status: Status::Similar,
+    };
+
+    for diff in both {
+        fields_diff.left.push(diff.left);
+        fields_diff.right.push(diff.right);
+        fields_diff.status.merge(diff.status);
+    }
+
+    if !all_fields_shared {
+        fields_diff.left.extend(left);
+        fields_diff.right.extend(right);
+        fields_diff.status.merge(Status::Different(vec![]));
+    }
+
+    fields_diff.left.sort_by(|a, b| a.0.cmp(&b.0));
+    fields_diff.right.sort_by(|a, b| a.0.cmp(&b.0));
+
+    let lefts = fields_diff
+        .left
+        .into_iter()
+        .map(|(_, a, b)| (a, b))
+        .collect();
+    let rights = fields_diff
+        .right
+        .into_iter()
+        .map(|(_, a, b)| (a, b))
+        .collect();
+
+    let doc1 = report_text::tag_union(lefts, ext_diff.left);
+    let doc2 = report_text::tag_union(rights, ext_diff.right);
+
+    fields_diff.status.merge(status);
+
+    Diff {
+        left: doc1,
+        right: doc2,
+        status: fields_diff.status,
+    }
+}
+
 fn ext_to_diff(ext1: &TypeExt, ext2: &TypeExt) -> Diff<Option<ReportText>> {
     let status = ext_to_status(ext1, ext2);
     let ext_doc_1 = ext_to_doc(ext1);
@@ -995,7 +1122,8 @@ mod report_text {
                 separate(vec![concat(vec![field_name, plain_text(" :")]), field_type])
             };
 
-            let starts = std::iter::once(plain_text("{ ")).chain(std::iter::repeat(plain_text(", ")));
+            let starts =
+                std::iter::once(plain_text("{ ")).chain(std::iter::repeat(plain_text(", ")));
 
             let mut lines: Vec<_> = entries
                 .into_iter()
@@ -1023,9 +1151,16 @@ mod report_text {
         if entries.is_empty() {
             concat(vec![plain_text("[]"), ext_text])
         } else {
-            let entry_to_text = |(tag_name, arguments)| concat(vec![tag_name, separate(arguments)]);
+            let entry_to_text = |(tag_name, arguments): (ReportText, Vec<_>)| {
+                if arguments.is_empty() {
+                    tag_name
+                } else {
+                    separate(vec![tag_name, separate(arguments)])
+                }
+            };
 
-            let starts = std::iter::once(plain_text("[ ")).chain(std::iter::repeat(plain_text(", ")));
+            let starts =
+                std::iter::once(plain_text("[ ")).chain(std::iter::repeat(plain_text(", ")));
 
             let mut lines: Vec<_> = entries
                 .into_iter()
@@ -1054,9 +1189,16 @@ mod report_text {
         if entries.is_empty() {
             concat(vec![plain_text("[]"), ext_text])
         } else {
-            let entry_to_text = |(tag_name, arguments)| concat(vec![tag_name, separate(arguments)]);
+            let entry_to_text = |(tag_name, arguments): (ReportText, Vec<_>)| {
+                if arguments.is_empty() {
+                    tag_name
+                } else {
+                    separate(vec![tag_name, separate(arguments)])
+                }
+            };
 
-            let starts = std::iter::once(plain_text("[ ")).chain(std::iter::repeat(plain_text(", ")));
+            let starts =
+                std::iter::once(plain_text("[ ")).chain(std::iter::repeat(plain_text(",")));
 
             let mut lines: Vec<_> = entries
                 .into_iter()

compiler/reporting/tests/test_reporting.rs

@@ -1214,15 +1214,47 @@ mod test_reporting {
                 4 ┆  f Blue
                   ┆    ^^^^
 
-                This `Blue` global tag application produces:
+                This `Blue` global tag application has the type:
 
-                    [ Blue, Green, Red ]
+                    [ Blue ]a
 
                 But `f` needs the 1st argument to be:
 
                     [ Green, Red ]
 
 
+                "#
+            ),
+        )
+    }
+
+    #[test]
+    fn tag_with_arguments_mismatch() {
+        report_problem_as(
+            indoc!(
+                r#"
+                f : [ Red Int, Green Bool ] -> Bool
+                f = \_ -> True
+
+                f (Blue 3.14)
+                "#
+            ),
+            indoc!(
+                r#"
+                The 1st argument to `f` is not what I expect:
+
+                4 ┆  f (Blue 3.14)
+                  ┆     ^^^^^^^^^
+
+                This `Blue` global tag application has the type:
+
+                    [ Blue Float ]a
+
+                But `f` needs the 1st argument to be:
+
+                    [ Green Bool, Red Int ]
+
+
                 "#
             ),
         )

compiler/types/src/subs.rs

@@ -410,6 +410,10 @@ impl Subs {
     pub fn rollback_to(&mut self, snapshot: Snapshot<InPlace<Variable>>) {
         self.utable.rollback_to(snapshot)
     }
+
+    pub fn commit_snapshot(&mut self, snapshot: Snapshot<InPlace<Variable>>) {
+        self.utable.commit(snapshot)
+    }
 }
 
 #[inline(always)]

compiler/unify/src/unify.rs

@@ -457,16 +457,36 @@ fn unify_tag_union(
         let sub1 = fresh(subs, pool, ctx, Structure(flat_type1));
         let sub2 = fresh(subs, pool, ctx, Structure(flat_type2));
 
+        // NOTE: for clearer error messages, we rollback unification of the ext vars when either fails
+        //
+        // This is inspired by
+        //
+        //
+        //      f : [ Red, Green ] -> Bool
+        //      f = \_ -> True
+        //
+        //      f Blue
+        //
+        //  In this case, we want the mismatch to be between `[ Blue ]a` and `[ Red, Green ]`, but
+        //  without rolling back, the mismatch is between `[ Blue, Red, Green ]a` and `[ Red, Green ]`.
+        //  TODO is this also required for the other cases?
+
+        let snapshot = subs.snapshot();
+
         let ext1_problems = unify_pool(subs, pool, rec1.ext, sub2);
         if !ext1_problems.is_empty() {
+            subs.rollback_to(snapshot);
             return ext1_problems;
         }
 
         let ext2_problems = unify_pool(subs, pool, sub1, rec2.ext);
         if !ext2_problems.is_empty() {
+            subs.rollback_to(snapshot);
             return ext2_problems;
         }
 
+        subs.commit_snapshot(snapshot);
+
         let mut tag_problems =
             unify_shared_tags(subs, pool, ctx, shared_tags, other_tags, ext, recursion_var);