improve Precedence error message

compiler/can/src/expr.rs

@@ -582,10 +582,14 @@ pub fn canonicalize_expr<'a>(
             )
         }
 
-        ast::Expr::PrecedenceConflict(binop1, binop2, _expr) => {
+        ast::Expr::PrecedenceConflict(whole_region, binop1, binop2, _expr) => {
             use roc_problem::can::RuntimeError::*;
 
-            let problem = PrecedenceProblem::BothNonAssociative(binop1.clone(), binop2.clone());
+            let problem = PrecedenceProblem::BothNonAssociative(
+                *whole_region,
+                binop1.clone(),
+                binop2.clone(),
+            );
 
             env.problem(Problem::PrecedenceProblem(problem.clone()));
 

compiler/can/src/operator.rs

@@ -78,8 +78,8 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located<Expr<'a>>) -> &'a
         | Nested(MalformedIdent(_))
         | MalformedClosure
         | Nested(MalformedClosure)
-        | PrecedenceConflict(_, _, _)
+        | PrecedenceConflict(_, _, _, _)
-        | Nested(PrecedenceConflict(_, _, _))
+        | Nested(PrecedenceConflict(_, _, _, _))
         | GlobalTag(_)
         | Nested(GlobalTag(_))
         | PrivateTag(_)
@@ -418,8 +418,9 @@ fn desugar_bin_op<'a>(arena: &'a Bump, loc_expr: &'a Located<Expr<'_>>) -> &'a L
                                         );
                                         let region = broken_expr.region;
                                         let value = Expr::PrecedenceConflict(
-                                            bad_op,
+                                            loc_expr.region,
                                             stack_op,
+                                            bad_op,
                                             arena.alloc(broken_expr),
                                         );
 

compiler/parse/src/ast.rs

@@ -165,7 +165,7 @@ pub enum Expr<'a> {
     MalformedClosure,
     // Both operators were non-associative, e.g. (True == False == False).
     // We should tell the author to disambiguate by grouping them with parens.
-    PrecedenceConflict(Loc<BinOp>, Loc<BinOp>, &'a Loc<Expr<'a>>),
+    PrecedenceConflict(Region, Loc<BinOp>, Loc<BinOp>, &'a Loc<Expr<'a>>),
 }
 
 #[derive(Debug, Clone, PartialEq)]

compiler/parse/src/expr.rs

@@ -323,7 +323,7 @@ fn expr_to_pattern<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result<Pattern<'a>,
         | Expr::If(_, _, _)
         | Expr::When(_, _)
         | Expr::MalformedClosure
-        | Expr::PrecedenceConflict(_, _, _)
+        | Expr::PrecedenceConflict(_, _, _, _)
         | Expr::Record {
             update: Some(_), ..
         }

compiler/parse/src/operator.rs

@@ -1,5 +1,6 @@
 use self::BinOp::*;
 use std::cmp::Ordering;
+use std::fmt;
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum CalledVia {
@@ -109,3 +110,29 @@ impl Ord for BinOp {
         self.precedence().cmp(&other.precedence())
     }
 }
+
+impl std::fmt::Display for BinOp {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let as_str = match self {
+            Caret => "^",
+            Star => "*",
+            Slash => "/",
+            DoubleSlash => "//",
+            Percent => "%",
+            DoublePercent => "%%",
+            Plus => "+",
+            Minus => "-",
+            Equals => "==",
+            NotEquals => "!=",
+            LessThan => "<",
+            GreaterThan => ">",
+            LessThanOrEq => "<=",
+            GreaterThanOrEq => ">=",
+            And => "&&",
+            Or => "||",
+            Pizza => "|>",
+        };
+
+        write!(f, "({})", as_str)
+    }
+}

compiler/problem/src/can.rs

@@ -25,7 +25,7 @@ pub enum Problem {
 
 #[derive(Clone, Debug, PartialEq)]
 pub enum PrecedenceProblem {
-    BothNonAssociative(Located<BinOp>, Located<BinOp>),
+    BothNonAssociative(Region, Located<BinOp>, Located<BinOp>),
 }
 
 #[derive(Clone, Debug, PartialEq)]

compiler/reporting/src/report.rs

@@ -67,18 +67,14 @@ impl Color {
     }
 }
 
-pub fn can_problem(filename: PathBuf, problem: Problem) -> Report {
+pub fn can_problem(filename: PathBuf, file_content: &str, problem: Problem) -> Report {
     let mut texts = Vec::new();
 
     match problem {
         Problem::UnusedDef(symbol, region) => {
             texts.push(Value(symbol));
             texts.push(plain_text(" is not used anywhere in your code."));
-            texts.push(newline());
-            texts.push(newline());
             texts.push(Region(region));
-            texts.push(newline());
-            texts.push(newline());
             texts.push(plain_text("If you didn't intend on using "));
             texts.push(Value(symbol));
             texts.push(plain_text(
@@ -89,11 +85,7 @@ pub fn can_problem(filename: PathBuf, problem: Problem) -> Report {
             texts.push(plain_text("Nothing from "));
             texts.push(Module(module_id));
             texts.push(plain_text(" is used in this module."));
-            texts.push(newline());
-            texts.push(newline());
             texts.push(Region(region));
-            texts.push(newline());
-            texts.push(newline());
             texts.push(plain_text("Since "));
             texts.push(Module(module_id));
             texts.push(plain_text(" isn't used, you don't need to import it."));
@@ -103,11 +95,7 @@ pub fn can_problem(filename: PathBuf, problem: Problem) -> Report {
             texts.push(plain_text(" doesn't use "));
             texts.push(Value(argument_symbol));
             texts.push(plain_text("."));
-            texts.push(newline());
-            texts.push(newline());
             texts.push(Region(region));
-            texts.push(newline());
-            texts.push(newline());
             texts.push(plain_text("If you don't need "));
             texts.push(Value(argument_symbol));
             texts.push(plain_text(
@@ -120,8 +108,12 @@ pub fn can_problem(filename: PathBuf, problem: Problem) -> Report {
             texts.push(Value(argument_symbol));
             texts.push(plain_text("\". Adding an underscore at the start of a variable name is a way of saying that the variable is not used."));
         }
-        Problem::PrecedenceProblem(BothNonAssociative(_left_bin_op, _right_bin_op)) => {
+        Problem::PrecedenceProblem(BothNonAssociative(region, left_bin_op, right_bin_op)) => {
-            panic!("TODO implement precedence problem report")
+            texts.push(plain_text(&*format!(
+                "You cannot mix {} and {} without parentheses",
+                left_bin_op.value, right_bin_op.value
+            )));
+            texts.push(Region(region));
         }
         Problem::UnsupportedPattern(_pattern_type, _region) => {
             panic!("TODO implement unsupported pattern report")
@@ -299,6 +291,8 @@ impl ReportText {
             }
             Type(content) => buf.push_str(content_to_string(content, subs, home, interns).as_str()),
             Region(region) => {
+                buf.push('\n');
+                buf.push('\n');
                 let max_line_number_length = region.end_line.to_string().len();
 
                 for i in region.start_line..=region.end_line {
@@ -326,6 +320,9 @@ impl ReportText {
                         buf.push('\n');
                     }
                 }
+
+                buf.push('\n');
+                buf.push('\n');
             }
             Docs(_) => {
                 panic!("TODO implment docs");
@@ -398,6 +395,9 @@ impl ReportText {
                 Content::Error => {}
             },
             Region(region) => {
+                // newline before snippet
+                buf.push('\n');
+                buf.push('\n');
                 let max_line_number_length = region.end_line.to_string().len();
 
                 for i in region.start_line..=region.end_line {
@@ -425,6 +425,10 @@ impl ReportText {
                         buf.push('\n');
                     }
                 }
+
+                // newline before next line of text
+                buf.push('\n');
+                buf.push('\n');
             }
             Batch(report_texts) => {
                 for report_text in report_texts {
@@ -435,3 +439,28 @@ impl ReportText {
         }
     }
 }
+
+pub fn region_slice<'a>(region: roc_region::all::Region, buffer: &'a str) -> &'a str {
+    let mut start = 0;
+    let mut end = 0;
+
+    let it = (0..).zip(buffer.lines());
+    for (index, line) in it {
+        if index < region.start_line as usize {
+            let size = line.len() + 1;
+            start += size;
+        } else if index < region.end_line as usize {
+            let size = line.len() + 1;
+            end += size;
+        } else {
+            break;
+        }
+    }
+
+    end += start;
+
+    start += region.start_col as usize;
+    end += region.end_col as usize;
+
+    &buffer[start..end]
+}

compiler/reporting/tests/test_reporting.rs

@@ -12,9 +12,9 @@ mod test_reporting {
     use crate::helpers::test_home;
     use roc_module::symbol::{Interns, ModuleId};
     use roc_reporting::report::{
-        can_problem, em_text, plain_text, url, Report, ReportText, BLUE_CODE, BOLD_CODE, CYAN_CODE,
+        can_problem, em_text, plain_text, region_slice, url, Report, ReportText, BLUE_CODE,
-        GREEN_CODE, MAGENTA_CODE, RED_CODE, RESET_CODE, TEST_PALETTE, UNDERLINE_CODE, WHITE_CODE,
+        BOLD_CODE, CYAN_CODE, GREEN_CODE, MAGENTA_CODE, RED_CODE, RESET_CODE, TEST_PALETTE,
-        YELLOW_CODE,
+        UNDERLINE_CODE, WHITE_CODE, YELLOW_CODE,
     };
     use roc_types::pretty_print::name_all_type_vars;
     use roc_types::subs::Subs;
@@ -97,6 +97,7 @@ mod test_reporting {
             Some(problem) => {
                 let report = can_problem(
                     filename_from_string(r"\code\proj\Main.roc"),
+                    src,
                     problem.clone(),
                 );
                 report
@@ -325,29 +326,57 @@ mod test_reporting {
     // }
 
     #[test]
-    fn report_precedence_problem() {
+    fn report_precedence_problem_single_line() {
         report_problem_as(
             indoc!(
-                r#"
+                r#"x = 1
-                x = 1
                 y =
-                    if selecteId == thisId == adminsId then
+                    if selectedId != thisId == adminsId then
                         4
 
                     else
                         5
 
                 { x, y }
-            "#
+                "#
             ),
             indoc!(
                 r#"
-                Which expression should I evaluate first? "selectedId == thisId" or "thisId == adminsId"?
+                You cannot mix (!=) and (==) without parentheses
 
-                3 ┆  if selecteId == thisId == adminsId then
+                3 ┆      if selectedId != thisId == adminsId then
-                  ┆     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-                Please clarify this for me by adding some parentheses. Either "(selectedId == thisId) == adminsId" or "selectedId == (thisId == adminsId)" would work."#
+                "#
+            ),
+        )
+        // ┆         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    }
+
+    #[test]
+    fn report_precedence_problem_multiline() {
+        report_problem_as(
+            indoc!(
+                r#"
+                if
+                    1
+                        != 2
+                        == 3
+                then
+                    2
+
+                else
+                    3
+                "#
+            ),
+            indoc!(
+                r#"
+                You cannot mix (!=) and (==) without parentheses
+
+                2 ┆      1
+                3 ┆          != 2
+                4 ┆          == 3
+
+                "#
             ),
         )
     }
@@ -549,10 +578,14 @@ mod test_reporting {
             })),
             indoc!(
                 r#"
+
+
                     <cyan>1<reset><magenta> ┆<reset>  <white>isDisabled = \user -> user.isAdmin<reset>
                     <cyan>2<reset><magenta> ┆<reset>
                     <cyan>3<reset><magenta> ┆<reset>  <white>theAdmin<reset>
-                    <cyan>4<reset><magenta> ┆<reset>  <white>    |> isDisabled<reset>"#
+                    <cyan>4<reset><magenta> ┆<reset>  <white>    |> isDisabled<reset>
+
+                "#
             ),
         );
     }
@@ -577,9 +610,13 @@ mod test_reporting {
             })),
             indoc!(
                 r#"
-                    2 ┆  y = 2
+
-                    3 ┆  f = \a -> a + 4
+
-                    4 ┆"#
+                2 ┆  y = 2
+                3 ┆  f = \a -> a + 4
+                4 ┆
+
+                "#
             ),
         );
     }
@@ -612,10 +649,129 @@ mod test_reporting {
             })),
             indoc!(
                 r#"
+
+
                      9 ┆
                     10 ┆  y = 2
-                    11 ┆  f = \a -> a + 4"#
+                    11 ┆  f = \a -> a + 4
+
+                    "#
             ),
         );
     }
+
+    #[test]
+    fn region_slice_test() {
+        use roc_region::all::Region;
+
+        assert_eq!(
+            {
+                let region = Region {
+                    start_line: 0,
+                    end_line: 0,
+                    start_col: 0,
+                    end_col: 0,
+                };
+
+                region_slice(region, "The quick brown fox jumps over the lazy dog")
+            },
+            ""
+        );
+
+        assert_eq!(
+            {
+                let region = Region {
+                    start_line: 0,
+                    end_line: 0,
+                    start_col: 4,
+                    end_col: 9,
+                };
+
+                region_slice(region, "The quick brown fox jumps over the lazy dog")
+            },
+            "quick"
+        );
+
+        assert_eq!(
+            {
+                let region = Region {
+                    start_line: 0,
+                    end_line: 0,
+                    start_col: 4,
+                    end_col: 9,
+                };
+
+                region_slice(region, "The\nquick brown fox jumps over the lazy dog")
+            },
+            "quick"
+        );
+
+        assert_eq!(
+            {
+                let region = Region {
+                    start_line: 1,
+                    end_line: 1,
+                    start_col: 0,
+                    end_col: 5,
+                };
+
+                region_slice(
+                    region,
+                    indoc!(
+                        r#"
+                        The
+                        quick brown fox jumps over the lazy dog
+                        "#
+                    ),
+                )
+            },
+            "quick"
+        );
+
+        assert_eq!(
+            {
+                let region = Region {
+                    start_line: 2,
+                    end_line: 2,
+                    start_col: 0,
+                    end_col: 5,
+                };
+
+                region_slice(
+                    region,
+                    indoc!(
+                        r#"
+                        The
+
+                        quick brown fox jumps over the lazy dog
+                        "#
+                    ),
+                )
+            },
+            "quick"
+        );
+
+        assert_eq!(
+            {
+                let region = Region {
+                    start_line: 2,
+                    end_line: 2,
+                    start_col: 0,
+                    end_col: 5,
+                };
+
+                region_slice(
+                    region,
+                    indoc!(
+                        r#"
+                        The
+
+                        quick brown fox jumps over the lazy dog
+                        "#
+                    ),
+                )
+            },
+            "quick"
+        );
+    }
 }