Fix LineTooLong propagation, add failing op tests

src/parse/ast.rs

@@ -1,6 +1,6 @@
 use bumpalo::collections::vec::Vec;
 use operator::Operator;
-use region::Loc;
+use region::{Loc, Region};
 use std::fmt::{self, Display, Formatter};
 
 pub type VariantName = str;
@@ -182,6 +182,30 @@ pub enum Attempting {
     Identifier,
 }
 
+impl<'a> Expr<'a> {
+    pub fn loc_ref(&'a self, region: Region) -> Loc<&'a Self> {
+        Loc {
+            region,
+            value: self,
+        }
+    }
+
+    pub fn loc(self, region: Region) -> Loc<Self> {
+        Loc {
+            region,
+            value: self,
+        }
+    }
+
+    pub fn with_spaces_before(spaces: &'a [Space<'a>], loc_expr: &'a Loc<Expr<'a>>) -> Self {
+        Expr::SpaceBefore(spaces, loc_expr)
+    }
+
+    pub fn with_spaces_after(loc_expr: &'a Loc<Expr<'a>>, spaces: &'a [Space<'a>]) -> Self {
+        Expr::SpaceAfter(loc_expr, spaces)
+    }
+}
+
 impl<'a> Display for Expr<'a> {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         use self::Expr::*;

src/parse/blankspace.rs

@@ -89,7 +89,7 @@ pub fn space1<'a>(min_indent: u16) -> impl Parser<'a, &'a [Space<'a>]> {
 }
 
 #[inline(always)]
-fn spaces<'a>(require_at_least_one: bool, min_indent: u16) -> impl Parser<'a, &'a [Space<'a>]> {
+fn spaces<'a>(require_at_least_one: bool, _min_indent: u16) -> impl Parser<'a, &'a [Space<'a>]> {
     move |arena: &'a Bump, state: State<'a>| {
         let mut chars = state.input.chars().peekable();
         let mut space_list = Vec::new_in(arena);

src/parse/mod.rs

@@ -31,20 +31,32 @@ fn parse_expr<'a>(min_indent: u16, arena: &'a Bump, state: State<'a>) -> ParseRe
     let expr_parser = map_with_arena(
         and(
             loc(one_of6(
+                string_literal(),
                 record_literal(),
                 number_literal(),
-                string_literal(),
                 when(min_indent),
                 conditional(min_indent),
                 ident_etc(min_indent),
             )),
             optional(and(
-                loc(operator()),
+                and(space0(min_indent), and(loc(operator()), space0(min_indent))),
                 loc(move |arena, state| parse_expr(min_indent, arena, state)),
             )),
         ),
         |arena, (loc_expr1, opt_operator)| match opt_operator {
-            Some((loc_op, loc_expr2)) => {
+            Some(((spaces_before_op, (loc_op, spaces_after_op)), loc_expr2)) => {
+                let region1 = loc_expr1.region.clone();
+                let region2 = loc_expr2.region.clone();
+                let loc_expr1 = if spaces_before_op.is_empty() {
+                    loc_expr1
+                } else {
+                    Expr::with_spaces_after(arena.alloc(loc_expr1), spaces_before_op).loc(region1)
+                };
+                let loc_expr2 = if spaces_after_op.is_empty() {
+                    loc_expr2
+                } else {
+                    Expr::with_spaces_after(arena.alloc(loc_expr2), spaces_after_op).loc(region2)
+                };
                 let tuple = arena.alloc((loc_expr1, loc_op, loc_expr2));
 
                 Expr::Operator(tuple)
@@ -110,20 +122,28 @@ pub fn conditional<'a>(min_indent: u16) -> impl Parser<'a, Expr<'a>> {
 /// 1. A standalone variable with trailing whitespace (e.g. because an operator is next)
 /// 2. The beginning of a function call (e.g. `foo bar baz`)
 /// 3. The beginning of a defniition (e.g. `foo =`)
-/// 4. A reserved keyword (e.g. `if ` or `case `), meaning we should do something else.
+/// 4. The beginning of a type annotation (e.g. `foo :`)
+/// 5. A reserved keyword (e.g. `if ` or `case `), meaning we should do something else.
 pub fn ident_etc<'a>(min_indent: u16) -> impl Parser<'a, Expr<'a>> {
-    let followed_by_equals = and(space0(min_indent), ch('='));
-
     map_with_arena(
         and(
             loc(ident()),
-            either(followed_by_equals, loc_function_args(min_indent)),
+            either(
+                // Check if this is either a def or type annotation
+                and(space0(min_indent), either(ch('='), ch(':'))),
+                // Check if this is function application
+                loc_function_args(min_indent),
+            ),
         ),
         |arena, (loc_ident, equals_or_loc_args)| {
             match equals_or_loc_args {
-                Either::First((_space_list, ())) => {
+                Either::First((_space_list, Either::First(()))) => {
                     // We have now parsed the beginning of a def (e.g. `foo =`)
-                    panic!("TODO parse def, making sure to use the space_list we got - don't drop comments!");
+                    panic!("TODO parse def, making sure not to drop comments!");
+                }
+                Either::First((_space_list, Either::Second(()))) => {
+                    // We have now parsed the beginning of a type annotation (e.g. `foo :`)
+                    panic!("TODO parse type annotation, making sure not to drop comments!");
                 }
                 Either::Second(loc_args) => {
                     // This appears to be a var, keyword, or function application.

src/parse/parser.rs

@@ -2,7 +2,7 @@ use bumpalo::collections::vec::Vec;
 use bumpalo::Bump;
 use parse::ast::Attempting;
 use region::{Located, Region};
-use std::char;
+use std::{char, u16};
 
 // Strategy:
 //
@@ -81,7 +81,7 @@ impl<'a> State<'a> {
     /// they weren't eligible to indent anyway.
     pub fn advance_without_indenting(&self, quantity: usize) -> Result<Self, (Fail, Self)> {
         match (self.column as usize).checked_add(quantity) {
-            Some(column_usize) if column_usize <= std::u16::MAX as usize => {
+            Some(column_usize) if column_usize <= u16::MAX as usize => {
                 Ok(State {
                     input: &self.input[quantity..],
                     line: self.line,
@@ -92,20 +92,14 @@ impl<'a> State<'a> {
                     attempting: self.attempting,
                 })
             }
-            _ => Err((
+            _ => Err(line_too_long(self.attempting, self.clone())),
-                Fail {
-                    reason: FailReason::LineTooLong(self.line),
-                    attempting: self.attempting,
-                },
-                self.clone(),
-            )),
         }
     }
     /// Advance the parser while also indenting as appropriate.
     /// This assumes we are only advancing with spaces, since they can indent.
     pub fn advance_spaces(&self, spaces: usize) -> Result<Self, (Fail, Self)> {
         match (self.column as usize).checked_add(spaces) {
-            Some(column_usize) if column_usize <= std::u16::MAX as usize => {
+            Some(column_usize) if column_usize <= u16::MAX as usize => {
                 // Spaces don't affect is_indenting; if we were previously indneting,
                 // we still are, and if we already finished indenting, we're still done.
                 let is_indenting = self.is_indenting;
@@ -117,8 +111,8 @@ impl<'a> State<'a> {
                     // already have errored out from the column calculation.
                     //
                     // Leaving debug assertions in case this invariant someday disappers.
-                    debug_assert!(std::u16::MAX - self.indent_col >= spaces as u16);
+                    debug_assert!(u16::MAX - self.indent_col >= spaces as u16);
-                    debug_assert!(spaces <= std::u16::MAX as usize);
+                    debug_assert!(spaces <= u16::MAX as usize);
 
                     self.indent_col + spaces as u16
                 } else {
@@ -134,13 +128,7 @@ impl<'a> State<'a> {
                     attempting: self.attempting,
                 })
             }
-            _ => Err((
+            _ => Err(line_too_long(self.attempting, self.clone())),
-                Fail {
-                    reason: FailReason::LineTooLong(self.line),
-                    attempting: self.attempting,
-                },
-                self.clone(),
-            )),
         }
     }
 }
@@ -340,7 +328,12 @@ where
     F: FnOnce(Region) -> Fail,
 {
     match (state.column as usize).checked_add(chars_consumed) {
-        Some(end_col) if end_col <= std::u16::MAX as usize => {
+        // Crucially, this is < u16::MAX and not <= u16::MAX. This means if
+        // column ever gets set to u16::MAX, we will automatically bail out
+        // with LineTooLong - which is exactly what we want! Once a line has
+        // been discovered to be too long, we don't want to parse anything else
+        // until that's fixed.
+        Some(end_col) if end_col < u16::MAX as usize => {
             let region = Region {
                 start_col: state.column,
                 end_col: end_col as u16,
@@ -350,14 +343,31 @@ where
 
             (problem_from_region(region), state)
         }
-        _ => {
+        _ => line_too_long(state.attempting, state),
+    }
+}
+
+fn line_too_long<'a>(attempting: Attempting, state: State<'a>) -> (Fail, State<'a>) {
     let reason = FailReason::LineTooLong(state.line);
-            let attempting = state.attempting;
     let fail = Fail { reason, attempting };
+    // Set column to MAX and advance the parser to end of input.
+    // This way, all future parsers will fail on EOF, and then
+    // unexpected_eof will take them back here - thus propagating
+    // the initial LineTooLong error all the way to the end, even if
+    // (for example) the LineTooLong initially occurs in the middle of
+    // a one_of chain, which would otherwise prevent it from propagating.
+    let column = u16::MAX;
+    let input = state.input.get(0..state.input.len()).unwrap();
+    let state = State {
+        input,
+        line: state.line,
+        indent_col: state.indent_col,
+        is_indenting: state.is_indenting,
+        column,
+        attempting,
+    };
 
     (fail, state)
-        }
-    }
 }
 
 /// A single char.

tests/test_parse.rs

@@ -254,6 +254,62 @@ mod test_parse {
         assert_eq!(Ok(expected), actual);
     }
 
+    #[test]
+    fn ops_with_spaces() {
+        let arena = Bump::new();
+        let tuple = arena.alloc((
+            Located::new(0, 0, 0, 1, Int("1")),
+            Located::new(0, 3, 0, 4, Plus),
+            Located::new(0, 7, 0, 8, Int("2")),
+        ));
+        let expected = Operator(tuple);
+        let actual = parse_with(&arena, "1  +   2");
+
+        assert_eq!(Ok(expected), actual);
+    }
+
+    #[test]
+    fn newline_before_op() {
+        let arena = Bump::new();
+        let tuple = arena.alloc((
+            Located::new(0, 0, 0, 1, Int("3")),
+            Located::new(0, 3, 0, 4, Plus),
+            Located::new(0, 7, 0, 8, Int("4")),
+        ));
+        let expected = Operator(tuple);
+        let actual = parse_with(&arena, "3  \n+ 4");
+
+        assert_eq!(Ok(expected), actual);
+    }
+
+    #[test]
+    fn newline_after_op() {
+        let arena = Bump::new();
+        let tuple = arena.alloc((
+            Located::new(0, 0, 0, 1, Int("3")),
+            Located::new(0, 3, 0, 4, Star),
+            Located::new(0, 7, 0, 8, Int("4")),
+        ));
+        let expected = Operator(tuple);
+        let actual = parse_with(&arena, "3  *\n 4");
+
+        assert_eq!(Ok(expected), actual);
+    }
+
+    #[test]
+    fn ops_with_newlines() {
+        let arena = Bump::new();
+        let tuple = arena.alloc((
+            Located::new(0, 0, 0, 1, Int("3")),
+            Located::new(0, 3, 0, 4, Plus),
+            Located::new(0, 7, 0, 8, Int("4")),
+        ));
+        let expected = Operator(tuple);
+        let actual = parse_with(&arena, "3  \n+ \n\n4");
+
+        assert_eq!(Ok(expected), actual);
+    }
+
     #[test]
     fn minus_twelve_minus_five() {
         let arena = Bump::new();