Be more lenient with indentation in record parsing

... and extract a shared helper with collection_trailing_sep_e, thereby resolving some inconsistencies with normal collection parsing.

... and delete some dead code while I'm at it

crates/compiler/fmt/src/expr.rs

@@ -1372,6 +1372,7 @@ fn format_field_multiline<'a, 'buf, T>(
 
             if !spaces.is_empty() {
                 fmt_spaces(buf, spaces.iter(), indent);
+                buf.indent(indent);
             }
 
             buf.push_str(separator_prefix);
@@ -1387,6 +1388,7 @@ fn format_field_multiline<'a, 'buf, T>(
 
             if !spaces.is_empty() {
                 fmt_spaces(buf, spaces.iter(), indent);
+                buf.indent(indent);
             }
 
             buf.push_str(separator_prefix);

crates/compiler/parse/src/blankspace.rs

@@ -27,6 +27,15 @@ where
     )
 }
 
+pub fn spaces_around<'a, P, S, E>(parser: P) -> impl Parser<'a, Loc<S>, E>
+where
+    S: 'a + Spaceable<'a>,
+    P: 'a + Parser<'a, Loc<S>, E>,
+    E: 'a + SpaceProblem,
+{
+    parser::map_with_arena(and(spaces(), and(parser, spaces())), spaces_around_help)
+}
+
 pub fn space0_around_e_no_after_indent_check<'a, P, S, E>(
     parser: P,
     indent_before_problem: fn(Position) -> E,

crates/compiler/parse/src/expr.rs

@@ -4,15 +4,15 @@ use crate::ast::{
 };
 use crate::blankspace::{
     space0_after_e, space0_around_e_no_after_indent_check, space0_around_ee, space0_before_e,
-    space0_before_optional_after, space0_e,
+    space0_e, spaces, spaces_around, spaces_before,
 };
 use crate::ident::{integer_ident, lowercase_ident, parse_ident, Accessor, Ident};
 use crate::keyword;
 use crate::parser::{
     self, backtrackable, increment_min_indent, line_min_indent, optional, reset_min_indent,
-    sep_by1, sep_by1_e, set_min_indent, specialize, specialize_ref, then, trailing_sep_by0, word1,
+    sep_by1, sep_by1_e, set_min_indent, specialize, specialize_ref, then, word1, word1_indent,
-    word1_indent, word2, EClosure, EExpect, EExpr, EIf, EInParens, EList, ENumber, EPattern,
+    word2, EClosure, EExpect, EExpr, EIf, EInParens, EList, ENumber, EPattern, ERecord, EString,
-    ERecord, EString, ETuple, EType, EWhen, Either, ParseResult, Parser,
+    EType, EWhen, Either, ParseResult, Parser,
 };
 use crate::pattern::{closure_param, loc_has_parser};
 use crate::state::State;
@@ -2468,40 +2468,25 @@ fn list_literal_help<'a>() -> impl Parser<'a, Expr<'a>, EList<'a>> {
     .trace("list_literal")
 }
 
-pub fn tuple_value_field<'a>() -> impl Parser<'a, Loc<Expr<'a>>, ETuple<'a>> {
-    space0_before_e(
-        specialize_ref(ETuple::Expr, loc_expr(false)),
-        ETuple::IndentEnd,
-    )
-}
-
 pub fn record_value_field<'a>() -> impl Parser<'a, AssignedField<'a, Expr<'a>>, ERecord<'a>> {
     use AssignedField::*;
 
-    move |arena, state: State<'a>, min_indent| {
+    map_with_arena!(
-        // You must have a field name, e.g. "email"
+        and!(
-        let (progress, loc_label, state) =
+            specialize(|_, pos| ERecord::Field(pos), loc!(lowercase_ident())),
-            specialize(|_, pos| ERecord::Field(pos), loc!(lowercase_ident()))
+            and!(
-                .parse(arena, state, min_indent)?;
+                spaces(),
-        debug_assert_eq!(progress, MadeProgress);
+                optional(and!(
-
-        let (_, spaces, state) = space0_e(ERecord::IndentColon).parse(arena, state, min_indent)?;
-
-        // Having a value is optional; both `{ email }` and `{ email: blah }` work.
-        // (This is true in both literals and types.)
-        let (_, opt_loc_val, state) = optional(and!(
                     either!(
                         word1(b':', ERecord::Colon),
                         word1(b'?', ERecord::QuestionMark)
                     ),
-            space0_before_e(
+                    spaces_before(specialize_ref(ERecord::Expr, loc_expr(false)))
-                specialize_ref(ERecord::Expr, loc_expr(false)),
-                ERecord::IndentEnd,
-            )
                 ))
-        .parse(arena, state, min_indent)?;
+            )
-
+        ),
-        let answer = match opt_loc_val {
+        |arena: &'a bumpalo::Bump, (loc_label, (spaces, opt_loc_val))| {
+            match opt_loc_val {
                 Some((Either::First(_), loc_val)) => {
                     RequiredValue(loc_label, spaces, arena.alloc(loc_val))
                 }
@@ -2519,10 +2504,9 @@ pub fn record_value_field<'a>() -> impl Parser<'a, AssignedField<'a, Expr<'a>>,
                         LabelOnly(loc_label)
                     }
                 }
-        };
-
-        Ok((MadeProgress, answer, state))
             }
+        }
+    )
 }
 
 fn record_updateable_identifier<'a>() -> impl Parser<'a, Expr<'a>, ERecord<'a>> {
@@ -2532,84 +2516,52 @@ fn record_updateable_identifier<'a>() -> impl Parser<'a, Expr<'a>, ERecord<'a>>
     )
 }
 
-fn record_help<'a>() -> impl Parser<
+struct RecordHelp<'a> {
-    'a,
+    update: Option<Loc<Expr<'a>>>,
-    (
+    fields: Collection<'a, Loc<AssignedField<'a, Expr<'a>>>>,
-        Option<Loc<Expr<'a>>>,
+}
-        Loc<(
+
-            Vec<'a, Loc<AssignedField<'a, Expr<'a>>>>,
+fn record_help<'a>() -> impl Parser<'a, RecordHelp<'a>, ERecord<'a>> {
-            &'a [CommentOrNewline<'a>],
+    between!(
-        )>,
-    ),
-    ERecord<'a>,
-> {
-    skip_first!(
         word1(b'{', ERecord::Open),
-        and!(
+        reset_min_indent(record!(RecordHelp {
             // You can optionally have an identifier followed by an '&' to
             // make this a record update, e.g. { Foo.user & username: "blah" }.
-            optional(backtrackable(skip_second!(
+            update: optional(backtrackable(skip_second!(
-                space0_around_ee(
+                spaces_around(
                     // We wrap the ident in an Expr here,
                     // so that we have a Spaceable value to work with,
                     // and then in canonicalization verify that it's an Expr::Var
                     // (and not e.g. an `Expr::Access`) and extract its string.
                     loc!(record_updateable_identifier()),
-                    ERecord::IndentEnd,
-                    ERecord::IndentAmpersand,
                 ),
                 word1(b'&', ERecord::Ampersand)
             ))),
-            loc!(skip_first!(
+            fields: collection_inner!(
-                // We specifically allow space characters inside here, so that
-                // `{  }` can be successfully parsed as an empty record, and then
-                // changed by the formatter back into `{}`.
-                zero_or_more!(word1(b' ', ERecord::End)),
-                skip_second!(
-                    and!(
-                        trailing_sep_by0(
-                            word1(b',', ERecord::End),
-                            space0_before_optional_after(
                 loc!(record_value_field()),
-                                ERecord::IndentEnd,
+                word1(b',', ERecord::End),
-                                ERecord::IndentEnd
+                AssignedField::SpaceBefore
-                            ),
-                        ),
-                        // Allow outdented closing braces
-                        reset_min_indent(space0_e(ERecord::IndentEnd))
             ),
+        })),
         word1(b'}', ERecord::End)
     )
-            ))
-        )
-    )
 }
 
 fn record_literal_help<'a>() -> impl Parser<'a, Expr<'a>, EExpr<'a>> {
     then(
         and!(
-            loc!(specialize(EExpr::Record, record_help())),
+            specialize(EExpr::Record, record_help()),
             // there can be field access, e.g. `{ x : 4 }.x`
             record_field_access_chain()
         ),
-        move |arena, state, _, (loc_record, accessors)| {
+        move |arena, state, _, (record, accessors)| {
-            let (opt_update, loc_assigned_fields_with_comments) = loc_record.value;
-
             // This is a record literal, not a destructure.
-            let value = match opt_update {
+            let value = match record.update {
                 Some(update) => Expr::RecordUpdate {
                     update: &*arena.alloc(update),
-                    fields: Collection::with_items_and_comments(
+                    fields: record.fields,
-                        arena,
-                        loc_assigned_fields_with_comments.value.0.into_bump_slice(),
-                        arena.alloc(loc_assigned_fields_with_comments.value.1),
-                    ),
                 },
-                None => Expr::Record(Collection::with_items_and_comments(
+                None => Expr::Record(record.fields),
-                    arena,
-                    loc_assigned_fields_with_comments.value.0.into_bump_slice(),
-                    loc_assigned_fields_with_comments.value.1,
-                )),
             };
 
             let value = apply_expr_access_chain(arena, value, accessors);

crates/compiler/parse/src/parser.rs

@@ -109,8 +109,7 @@ impl_space_problem! {
     EAbility<'a>,
     PInParens<'a>,
     PRecord<'a>,
-    PList<'a>,
+    PList<'a>
-    ETuple<'a>
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -356,7 +355,6 @@ pub enum EExpr<'a> {
 
     InParens(EInParens<'a>, Position),
     Record(ERecord<'a>, Position),
-    Tuple(ETuple<'a>, Position),
     Str(EString<'a>, Position),
     SingleQuote(EString<'a>, Position),
     Number(ENumber, Position),
@@ -396,48 +394,12 @@ pub enum ERecord<'a> {
     Field(Position),
     Colon(Position),
     QuestionMark(Position),
-    Bar(Position),
     Ampersand(Position),
 
     // TODO remove
     Expr(&'a EExpr<'a>, Position),
 
     Space(BadInputError, Position),
-
-    IndentOpen(Position),
-    IndentColon(Position),
-    IndentBar(Position),
-    IndentAmpersand(Position),
-    IndentEnd(Position),
-}
-
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub enum ETuple<'a> {
-    // Empty tuples are not allowed
-    Empty(Position),
-
-    // Single element tuples are not allowed
-    Single(Position),
-
-    End(Position),
-    Open(Position),
-
-    Updateable(Position),
-    Field(Position),
-    Colon(Position),
-    QuestionMark(Position),
-    Bar(Position),
-    Ampersand(Position),
-
-    Expr(&'a EExpr<'a>, Position),
-
-    Space(BadInputError, Position),
-
-    IndentOpen(Position),
-    IndentColon(Position),
-    IndentBar(Position),
-    IndentAmpersand(Position),
-    IndentEnd(Position),
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -700,13 +662,8 @@ pub enum ETypeAbilityImpl<'a> {
 
     Space(BadInputError, Position),
 
-    IndentOpen(Position),
-    IndentColon(Position),
-    IndentOptional(Position),
-    IndentEnd(Position),
     Updateable(Position),
     QuestionMark(Position),
-    Bar(Position),
     Ampersand(Position),
     Expr(&'a EExpr<'a>, Position),
     IndentBar(Position),
@@ -721,16 +678,10 @@ impl<'a> From<ERecord<'a>> for ETypeAbilityImpl<'a> {
             ERecord::Field(p) => ETypeAbilityImpl::Field(p),
             ERecord::Colon(p) => ETypeAbilityImpl::Colon(p),
             ERecord::Space(s, p) => ETypeAbilityImpl::Space(s, p),
-            ERecord::IndentOpen(p) => ETypeAbilityImpl::IndentOpen(p),
-            ERecord::IndentColon(p) => ETypeAbilityImpl::IndentColon(p),
-            ERecord::IndentEnd(p) => ETypeAbilityImpl::IndentEnd(p),
             ERecord::Updateable(p) => ETypeAbilityImpl::Updateable(p),
             ERecord::QuestionMark(p) => ETypeAbilityImpl::QuestionMark(p),
-            ERecord::Bar(p) => ETypeAbilityImpl::Bar(p),
             ERecord::Ampersand(p) => ETypeAbilityImpl::Ampersand(p),
             ERecord::Expr(e, p) => ETypeAbilityImpl::Expr(e, p),
-            ERecord::IndentBar(p) => ETypeAbilityImpl::IndentBar(p),
-            ERecord::IndentAmpersand(p) => ETypeAbilityImpl::IndentAmpersand(p),
         }
     }
 }
@@ -1260,46 +1211,11 @@ macro_rules! skip_second {
     };
 }
 
-/// Parse zero or more elements between two braces (e.g. square braces).
-/// Elements can be optionally surrounded by spaces, and are separated by a
-/// delimiter (e.g comma-separated). Braces and delimiters get discarded.
 #[macro_export]
-macro_rules! collection {
+macro_rules! collection_inner {
-    ($opening_brace:expr, $elem:expr, $delimiter:expr, $closing_brace:expr, $min_indent:expr) => {
+    ($elem:expr, $delimiter:expr, $space_before:expr) => {
-        skip_first!(
-            $opening_brace,
-            skip_first!(
-                // We specifically allow space characters inside here, so that
-                // `[  ]` can be successfully parsed as an empty list, and then
-                // changed by the formatter back into `[]`.
-                //
-                // We don't allow newlines or comments in the middle of empty
-                // roc_collections because those are normally stored in an Expr,
-                // and there's no Expr in which to store them in an empty collection!
-                //
-                // We could change the AST to add extra storage specifically to
-                // support empty literals containing newlines or comments, but this
-                // does not seem worth even the tiniest regression in compiler performance.
-                zero_or_more!($crate::parser::ascii_char(b' ')),
-                skip_second!(
-                    $crate::parser::sep_by0(
-                        $delimiter,
-                        $crate::blankspace::space0_around($elem, $min_indent)
-                    ),
-                    $closing_brace
-                )
-            )
-        )
-    };
-}
-
-#[macro_export]
-macro_rules! collection_trailing_sep_e {
-    ($opening_brace:expr, $elem:expr, $delimiter:expr, $closing_brace:expr, $space_before:expr) => {
         map_with_arena!(
-            skip_first!(
+            and!(
-                $opening_brace,
-                $crate::parser::reset_min_indent(and!(
                 and!(
                     $crate::blankspace::spaces(),
                     $crate::parser::trailing_sep_by0(
@@ -1307,8 +1223,7 @@ macro_rules! collection_trailing_sep_e {
                         $crate::blankspace::spaces_before_optional_after($elem,)
                     )
                 ),
-                    skip_second!($crate::blankspace::spaces(), $closing_brace)
+                $crate::blankspace::spaces()
-                ))
             ),
             |arena: &'a bumpalo::Bump,
              ((spaces, mut parsed_elems), mut final_comments): (
@@ -1322,7 +1237,7 @@ macro_rules! collection_trailing_sep_e {
                     if let Some(first) = parsed_elems.first_mut() {
                         first.value = $space_before(arena.alloc(first.value), spaces)
                     } else {
-                        assert!(final_comments.is_empty());
+                        debug_assert!(final_comments.is_empty());
                         final_comments = spaces;
                     }
                 }
@@ -1337,6 +1252,21 @@ macro_rules! collection_trailing_sep_e {
     };
 }
 
+#[macro_export]
+macro_rules! collection_trailing_sep_e {
+    ($opening_brace:expr, $elem:expr, $delimiter:expr, $closing_brace:expr, $space_before:expr) => {
+        between!(
+            $opening_brace,
+            $crate::parser::reset_min_indent($crate::collection_inner!(
+                $elem,
+                $delimiter,
+                $space_before
+            )),
+            $closing_brace
+        )
+    };
+}
+
 #[macro_export]
 macro_rules! succeed {
     ($value:expr) => {

crates/compiler/test_syntax/tests/snapshots/pass/outdented_colon_in_record.expr.formatted.roc

@@ -0,0 +1,5 @@
+x = foo {
+    bar
+    : blah,
+}
+x

crates/compiler/test_syntax/tests/snapshots/pass/outdented_colon_in_record.expr.result-ast

@@ -0,0 +1,72 @@
+Defs(
+    Defs {
+        tags: [
+            Index(2147483648),
+        ],
+        regions: [
+            @0-22,
+        ],
+        space_before: [
+            Slice(start = 0, length = 0),
+        ],
+        space_after: [
+            Slice(start = 0, length = 0),
+        ],
+        spaces: [],
+        type_defs: [],
+        value_defs: [
+            Body(
+                @0-1 Identifier(
+                    "x",
+                ),
+                @4-22 Apply(
+                    @4-7 Var {
+                        module_name: "",
+                        ident: "foo",
+                    },
+                    [
+                        @8-22 Record(
+                            [
+                                @10-20 SpaceBefore(
+                                    SpaceAfter(
+                                        RequiredValue(
+                                            @10-13 "bar",
+                                            [
+                                                Newline,
+                                            ],
+                                            @16-20 SpaceBefore(
+                                                Var {
+                                                    module_name: "",
+                                                    ident: "blah",
+                                                },
+                                                [
+                                                    Newline,
+                                                ],
+                                            ),
+                                        ),
+                                        [
+                                            Newline,
+                                        ],
+                                    ),
+                                    [
+                                        Newline,
+                                    ],
+                                ),
+                            ],
+                        ),
+                    ],
+                    Space,
+                ),
+            ),
+        ],
+    },
+    @23-24 SpaceBefore(
+        Var {
+            module_name: "",
+            ident: "x",
+        },
+        [
+            Newline,
+        ],
+    ),
+)

crates/compiler/test_syntax/tests/snapshots/pass/outdented_colon_in_record.expr.roc

@@ -0,0 +1,6 @@
+x = foo {
+bar
+:
+blah
+}
+x

crates/compiler/test_syntax/tests/snapshots/pass/outdented_record.expr.result-ast

@@ -26,9 +26,9 @@ Defs(
                     },
                     [
                         @8-23 Record(
-                            Collection {
+                            [
-                                items: [
                                 @12-21 SpaceBefore(
+                                    SpaceAfter(
                                         RequiredValue(
                                             @12-15 "bar",
                                             [],
@@ -41,11 +41,11 @@ Defs(
                                             Newline,
                                         ],
                                     ),
-                                ],
+                                    [
-                                final_comments: [
                                         Newline,
                                     ],
-                            },
+                                ),
+                            ],
                         ),
                     ],
                     Space,

crates/compiler/test_syntax/tests/test_snapshots.rs

@@ -193,15 +193,15 @@ mod test_snapshots {
         fail/list_without_end.expr,
         fail/multi_no_end.expr,
         fail/pattern_binds_keyword.expr,
-        fail/pattern_in_parens_end_comma.expr,
         fail/pattern_in_parens_end.expr,
+        fail/pattern_in_parens_end_comma.expr,
         fail/pattern_in_parens_indent_open.expr,
         fail/pattern_in_parens_open.expr,
         fail/record_type_end.expr,
         fail/record_type_keyword_field_name.expr,
         fail/record_type_missing_comma.expr,
-        fail/record_type_open_indent.expr,
         fail/record_type_open.expr,
+        fail/record_type_open_indent.expr,
         fail/record_type_tab.expr,
         fail/single_no_end.expr,
         fail/tab_crash.header,
@@ -270,13 +270,13 @@ mod test_snapshots {
         pass/empty_platform_header.header,
         pass/empty_record.expr,
         pass/empty_string.expr,
-        pass/equals_with_spaces.expr,
         pass/equals.expr,
-        pass/expect_fx.moduledefs,
+        pass/equals_with_spaces.expr,
         pass/expect.expr,
+        pass/expect_fx.moduledefs,
         pass/float_with_underscores.expr,
-        pass/full_app_header_trailing_commas.header,
         pass/full_app_header.header,
+        pass/full_app_header_trailing_commas.header,
         pass/function_effect_types.header,
         pass/function_with_tuple_ext_type.expr,
         pass/function_with_tuple_type.expr,
@@ -305,8 +305,8 @@ mod test_snapshots {
         pass/multi_char_string.expr,
         pass/multiline_string.expr,
         pass/multiline_tuple_with_comments.expr,
-        pass/multiline_type_signature_with_comment.expr,
         pass/multiline_type_signature.expr,
+        pass/multiline_type_signature_with_comment.expr,
         pass/multiple_fields.expr,
         pass/multiple_operators.expr,
         pass/neg_inf_float.expr,
@@ -338,14 +338,15 @@ mod test_snapshots {
         pass/one_spaced_def.expr,
         pass/opaque_destructure_first_item_in_body.expr,
         pass/opaque_has_abilities.expr,
-        pass/opaque_reference_expr_with_arguments.expr,
         pass/opaque_reference_expr.expr,
-        pass/opaque_reference_pattern_with_arguments.expr,
+        pass/opaque_reference_expr_with_arguments.expr,
         pass/opaque_reference_pattern.expr,
+        pass/opaque_reference_pattern_with_arguments.expr,
         pass/opaque_simple.moduledefs,
         pass/opaque_with_type_arguments.moduledefs,
         pass/ops_with_newlines.expr,
         pass/outdented_app_with_record.expr,
+        pass/outdented_colon_in_record.expr,
         pass/outdented_list.expr,
         pass/outdented_record.expr,
         pass/packed_singleton_list.expr,
@@ -389,8 +390,8 @@ mod test_snapshots {
         pass/three_arg_closure.expr,
         pass/tuple_access_after_record.expr,
         pass/tuple_accessor_function.expr,
-        pass/tuple_type_ext.expr,
         pass/tuple_type.expr,
+        pass/tuple_type_ext.expr,
         pass/two_arg_closure.expr,
         pass/two_backpassing.expr,
         pass/two_branch_when.expr,
@@ -398,12 +399,12 @@ mod test_snapshots {
         pass/type_decl_with_underscore.expr,
         pass/type_signature_def.expr,
         pass/type_signature_function_def.expr,
+        pass/unary_negation.expr,
         pass/unary_negation_access.expr, // Regression test for https://github.com/roc-lang/roc/issues/509
         pass/unary_negation_arg.expr,
         pass/unary_negation_with_parens.expr,
-        pass/unary_negation.expr,
-        pass/unary_not_with_parens.expr,
         pass/unary_not.expr,
+        pass/unary_not_with_parens.expr,
         pass/underscore_backpassing.expr,
         pass/underscore_in_assignment_pattern.expr,
         pass/var_else.expr,
@@ -414,10 +415,10 @@ mod test_snapshots {
         pass/var_when.expr,
         pass/when_if_guard.expr,
         pass/when_in_assignment.expr,
-        pass/when_in_function_python_style_indent.expr,
         pass/when_in_function.expr,
-        pass/when_in_parens_indented.expr,
+        pass/when_in_function_python_style_indent.expr,
         pass/when_in_parens.expr,
+        pass/when_in_parens_indented.expr,
         pass/when_with_alternative_patterns.expr,
         pass/when_with_function_application.expr,
         pass/when_with_negative_numbers.expr,
@@ -427,8 +428,8 @@ mod test_snapshots {
         pass/when_with_tuples.expr,
         pass/where_clause_function.expr,
         pass/where_clause_multiple_bound_abilities.expr,
-        pass/where_clause_multiple_has_across_newlines.expr,
         pass/where_clause_multiple_has.expr,
+        pass/where_clause_multiple_has_across_newlines.expr,
         pass/where_clause_non_function.expr,
         pass/where_clause_on_newline.expr,
         pass/zero_float.expr,