Fix several fmt+parse bugs found by fuzzing

crates/compiler/fmt/src/def.rs

@@ -61,7 +61,7 @@ impl<'a> Formattable for TypeDef<'a> {
         &self,
         buf: &mut Buf<'buf>,
         _parens: Parens,
-        _newlines: Newlines,
+        newlines: Newlines,
         indent: u16,
     ) {
         use roc_parse::ast::TypeDef::*;
@@ -97,22 +97,10 @@ impl<'a> Formattable for TypeDef<'a> {
                 ann.format(buf, indent)
             }
             Opaque {
-                header: TypeHeader { name, vars },
+                header,
                 typ: ann,
                 derived: has_abilities,
             } => {
-                buf.indent(indent);
-                buf.push_str(name.value);
-
-                for var in *vars {
-                    buf.spaces(1);
-                    fmt_pattern(buf, &var.value, indent, Parens::NotNeeded);
-                    buf.indent(indent);
-                }
-
-                buf.push_str(" :=");
-                buf.spaces(1);
-
                 let ann_is_where_clause =
                     matches!(ann.extract_spaces().item, TypeAnnotation::Where(..));
 
@@ -126,7 +114,7 @@ impl<'a> Formattable for TypeDef<'a> {
 
                 let make_multiline = ann.is_multiline() || has_abilities_multiline;
 
-                ann.format(buf, indent);
+                fmt_general_def(header, buf, indent, ":=", &ann.value, newlines);
 
                 if let Some(has_abilities) = has_abilities {
                     buf.spaces(1);
@@ -178,6 +166,29 @@ impl<'a> Formattable for TypeDef<'a> {
     }
 }
 
+impl<'a> Formattable for TypeHeader<'a> {
+    fn is_multiline(&self) -> bool {
+        self.vars.iter().any(|v| v.is_multiline())
+    }
+
+    fn format_with_options<'buf>(
+        &self,
+        buf: &mut Buf<'buf>,
+        _parens: Parens,
+        _newlines: Newlines,
+        indent: u16,
+    ) {
+        buf.indent(indent);
+        buf.push_str(self.name.value);
+
+        for var in self.vars.iter() {
+            buf.spaces(1);
+            fmt_pattern(buf, &var.value, indent, Parens::NotNeeded);
+            buf.indent(indent);
+        }
+    }
+}
+
 impl<'a> Formattable for ValueDef<'a> {
     fn is_multiline(&self) -> bool {
         use roc_parse::ast::ValueDef::*;
@@ -204,7 +215,14 @@ impl<'a> Formattable for ValueDef<'a> {
         use roc_parse::ast::ValueDef::*;
         match self {
             Annotation(loc_pattern, loc_annotation) => {
-                fmt_annotation(loc_pattern, buf, indent, loc_annotation, newlines);
+                fmt_general_def(
+                    loc_pattern,
+                    buf,
+                    indent,
+                    ":",
+                    &loc_annotation.value,
+                    newlines,
+                );
             }
             Body(loc_pattern, loc_expr) => {
                 fmt_body(buf, &loc_pattern.value, &loc_expr.value, indent);
@@ -221,7 +239,7 @@ impl<'a> Formattable for ValueDef<'a> {
                 body_pattern,
                 body_expr,
             } => {
-                fmt_annotation(ann_pattern, buf, indent, ann_type, newlines);
+                fmt_general_def(ann_pattern, buf, indent, ":", &ann_type.value, newlines);
 
                 if let Some(comment_str) = comment {
                     buf.push_str(" #");
@@ -236,54 +254,63 @@ impl<'a> Formattable for ValueDef<'a> {
     }
 }
 
-fn fmt_annotation(
+fn fmt_general_def<L: Formattable>(
-    loc_pattern: &Loc<Pattern>,
+    lhs: L,
     buf: &mut Buf,
     indent: u16,
-    loc_annotation: &Loc<TypeAnnotation>,
+    sep: &str,
+    rhs: &TypeAnnotation,
     newlines: Newlines,
 ) {
-    loc_pattern.format(buf, indent);
+    lhs.format(buf, indent);
     buf.indent(indent);
 
-    if loc_annotation.is_multiline() {
+    if rhs.is_multiline() {
-        buf.push_str(" :");
+        buf.spaces(1);
+        buf.push_str(sep);
         buf.spaces(1);
 
-        let should_outdent = match loc_annotation.value {
+        let should_outdent = should_outdent(rhs);
-            TypeAnnotation::SpaceBefore(sub_def, spaces) => match sub_def {
-                TypeAnnotation::Record { .. } | TypeAnnotation::TagUnion { .. } => {
-                    let is_only_newlines = spaces.iter().all(|s| s.is_newline());
-                    is_only_newlines && sub_def.is_multiline()
-                }
-                _ => false,
-            },
-            TypeAnnotation::Record { .. } | TypeAnnotation::TagUnion { .. } => true,
-            _ => false,
-        };
 
         if should_outdent {
-            match loc_annotation.value {
+            match rhs {
                 TypeAnnotation::SpaceBefore(sub_def, _) => {
                     sub_def.format_with_options(buf, Parens::NotNeeded, Newlines::No, indent);
                 }
                 _ => {
-                    loc_annotation.format_with_options(
+                    rhs.format_with_options(buf, Parens::NotNeeded, Newlines::No, indent);
-                        buf,
-                        Parens::NotNeeded,
-                        Newlines::No,
-                        indent,
-                    );
                 }
             }
         } else {
-            loc_annotation.format_with_options(buf, Parens::NotNeeded, newlines, indent + INDENT);
+            rhs.format_with_options(buf, Parens::NotNeeded, newlines, indent + INDENT);
         }
     } else {
         buf.spaces(1);
-        buf.push(':');
+        buf.push_str(sep);
         buf.spaces(1);
-        loc_annotation.format_with_options(buf, Parens::NotNeeded, Newlines::No, indent);
+        rhs.format_with_options(buf, Parens::NotNeeded, Newlines::No, indent);
+    }
+}
+
+fn should_outdent(mut rhs: &TypeAnnotation) -> bool {
+    loop {
+        match rhs {
+            TypeAnnotation::SpaceBefore(sub_def, spaces) => {
+                let is_only_newlines = spaces.iter().all(|s| s.is_newline());
+                if !is_only_newlines || !sub_def.is_multiline() {
+                    return false;
+                }
+                rhs = sub_def;
+            }
+            TypeAnnotation::Where(ann, _clauses) => {
+                if !ann.is_multiline() {
+                    return false;
+                }
+                rhs = &ann.value;
+            }
+            TypeAnnotation::Record { .. } | TypeAnnotation::TagUnion { .. } => return true,
+            _ => return false,
+        }
     }
 }
 

crates/compiler/fmt/src/expr.rs

@@ -1,4 +1,4 @@
-use crate::annotation::{except_last, Formattable, Newlines, Parens};
+use crate::annotation::{except_last, is_collection_multiline, Formattable, Newlines, Parens};
 use crate::collection::{fmt_collection, Braces};
 use crate::def::fmt_defs;
 use crate::pattern::fmt_pattern;
@@ -49,7 +49,7 @@ impl<'a> Formattable for Expr<'a> {
             // These expressions always have newlines
             Defs(_, _) | When(_, _) => true,
 
-            List(items) => items.iter().any(|loc_expr| loc_expr.is_multiline()),
+            List(items) => is_collection_multiline(items),
 
             Str(literal) => is_str_multiline(literal),
             Apply(loc_expr, args, _) => {
@@ -96,9 +96,9 @@ impl<'a> Formattable for Expr<'a> {
                         .any(|loc_pattern| loc_pattern.is_multiline())
             }
 
-            Record(fields) => fields.iter().any(|loc_field| loc_field.is_multiline()),
+            Record(fields) => is_collection_multiline(fields),
-            Tuple(fields) => fields.iter().any(|loc_field| loc_field.is_multiline()),
+            Tuple(fields) => is_collection_multiline(fields),
-            RecordUpdate { fields, .. } => fields.iter().any(|loc_field| loc_field.is_multiline()),
+            RecordUpdate { fields, .. } => is_collection_multiline(fields),
         }
     }
 
@@ -1319,7 +1319,7 @@ fn fmt_record<'a, 'buf>(
     let loc_fields = fields.items;
     let final_comments = fields.final_comments();
     buf.indent(indent);
-    if loc_fields.is_empty() && final_comments.iter().all(|c| c.is_newline()) {
+    if loc_fields.is_empty() && final_comments.iter().all(|c| c.is_newline()) && update.is_none() {
         buf.push_str("{}");
     } else {
         buf.push('{');

crates/compiler/parse/src/expr.rs

@@ -1,6 +1,6 @@
 use crate::ast::{
     AssignedField, Collection, CommentOrNewline, Defs, Expr, ExtractSpaces, Has, HasAbilities,
-    Pattern, Spaceable, TypeAnnotation, TypeDef, TypeHeader, ValueDef,
+    Pattern, Spaceable, Spaces, TypeAnnotation, TypeDef, TypeHeader, ValueDef,
 };
 use crate::blankspace::{
     space0_after_e, space0_around_e_no_after_indent_check, space0_around_ee, space0_before_e,
@@ -1085,6 +1085,29 @@ enum AliasOrOpaque {
     Opaque,
 }
 
+fn extract_tag_and_spaces<'a>(arena: &'a Bump, expr: Expr<'a>) -> Option<Spaces<'a, &'a str>> {
+    let mut expr = expr.extract_spaces();
+
+    loop {
+        match &expr.item {
+            Expr::ParensAround(inner_expr) => {
+                let inner_expr = inner_expr.extract_spaces();
+                expr.item = inner_expr.item;
+                expr.before = merge_spaces(arena, expr.before, inner_expr.before);
+                expr.after = merge_spaces(arena, inner_expr.after, expr.after);
+            }
+            Expr::Tag(tag) => {
+                return Some(Spaces {
+                    before: expr.before,
+                    item: tag,
+                    after: expr.after,
+                });
+            }
+            _ => return None,
+        }
+    }
+}
+
 #[allow(clippy::too_many_arguments)]
 fn finish_parsing_alias_or_opaque<'a>(
     min_indent: u32,
@@ -1105,120 +1128,113 @@ fn finish_parsing_alias_or_opaque<'a>(
 
     let mut defs = Defs::default();
 
-    let state = match &expr.value.extract_spaces().item {
+    let state = if let Some(tag) = extract_tag_and_spaces(arena, expr.value) {
-        Expr::ParensAround(Expr::SpaceBefore(Expr::Tag(name), _))
+        let name = tag.item;
-        | Expr::ParensAround(Expr::SpaceAfter(Expr::Tag(name), _))
+        let mut type_arguments = Vec::with_capacity_in(arguments.len(), arena);
-        | Expr::ParensAround(Expr::Tag(name))
-        | Expr::Tag(name) => {
-            let mut type_arguments = Vec::with_capacity_in(arguments.len(), arena);
 
-            for argument in arguments {
+        for argument in arguments {
-                match expr_to_pattern_help(arena, &argument.value) {
+            match expr_to_pattern_help(arena, &argument.value) {
-                    Ok(good) => {
+                Ok(good) => {
-                        type_arguments.push(Loc::at(argument.region, good));
+                    type_arguments.push(Loc::at(argument.region, good));
-                    }
-                    Err(()) => {
-                        return Err((
-                            MadeProgress,
-                            EExpr::Pattern(
-                                arena.alloc(EPattern::NotAPattern(state.pos())),
-                                state.pos(),
-                            ),
-                        ));
-                    }
                 }
-            }
+                Err(()) => {
-
+                    return Err((
-            match kind {
+                        MadeProgress,
-                AliasOrOpaque::Alias => {
+                        EExpr::Pattern(
-                    let (_, signature, state) =
+                            arena.alloc(EPattern::NotAPattern(state.pos())),
-                        alias_signature_with_space_before().parse(arena, state, min_indent)?;
+                            state.pos(),
-
+                        ),
-                    let def_region = Region::span_across(&expr.region, &signature.region);
+                    ));
-
-                    let header = TypeHeader {
-                        name: Loc::at(expr.region, name),
-                        vars: type_arguments.into_bump_slice(),
-                    };
-
-                    let def = TypeDef::Alias {
-                        header,
-                        ann: signature,
-                    };
-
-                    defs.push_type_def(def, def_region, &[], &[]);
-
-                    state
-                }
-
-                AliasOrOpaque::Opaque => {
-                    let (_, (signature, derived), state) =
-                        opaque_signature_with_space_before().parse(arena, state, indented_more)?;
-
-                    let def_region = Region::span_across(&expr.region, &signature.region);
-
-                    let header = TypeHeader {
-                        name: Loc::at(expr.region, name),
-                        vars: type_arguments.into_bump_slice(),
-                    };
-
-                    let def = TypeDef::Opaque {
-                        header,
-                        typ: signature,
-                        derived,
-                    };
-
-                    defs.push_type_def(def, def_region, &[], &[]);
-
-                    state
                 }
             }
         }
 
-        _ => {
+        match kind {
-            let call = to_call(arena, arguments, expr);
+            AliasOrOpaque::Alias => {
+                let (_, signature, state) =
+                    alias_signature_with_space_before().parse(arena, state, min_indent)?;
 
-            match expr_to_pattern_help(arena, &call.value) {
+                let def_region = Region::span_across(&expr.region, &signature.region);
-                Ok(good) => {
-                    let parser = specialize(
-                        EExpr::Type,
-                        space0_before_e(
-                            set_min_indent(indented_more, type_annotation::located(false)),
-                            EType::TIndentStart,
-                        ),
-                    );
 
-                    match parser.parse(arena, state.clone(), min_indent) {
+                let header = TypeHeader {
-                        Err((_, fail)) => return Err((MadeProgress, fail)),
+                    name: Loc::at(expr.region, name),
-                        Ok((_, mut ann_type, state)) => {
+                    vars: type_arguments.into_bump_slice(),
-                            // put the spaces from after the operator in front of the call
+                };
-                            if !spaces_after_operator.is_empty() {
-                                ann_type = arena
-                                    .alloc(ann_type.value)
-                                    .with_spaces_before(spaces_after_operator, ann_type.region);
-                            }
 
-                            let def_region = Region::span_across(&call.region, &ann_type.region);
+                let def = TypeDef::Alias {
+                    header,
+                    ann: signature,
+                };
 
-                            let value_def =
+                defs.push_type_def(def, def_region, &[], &[]);
-                                ValueDef::Annotation(Loc::at(expr_region, good), ann_type);
 
-                            defs.push_value_def(value_def, def_region, &[], &[]);
+                state
+            }
 
-                            state
+            AliasOrOpaque::Opaque => {
+                let (_, (signature, derived), state) =
+                    opaque_signature_with_space_before().parse(arena, state, indented_more)?;
+
+                let def_region = Region::span_across(&expr.region, &signature.region);
+
+                let header = TypeHeader {
+                    name: Loc::at(expr.region, name),
+                    vars: type_arguments.into_bump_slice(),
+                };
+
+                let def = TypeDef::Opaque {
+                    header,
+                    typ: signature,
+                    derived,
+                };
+
+                defs.push_type_def(def, def_region, &[], &[]);
+
+                state
+            }
+        }
+    } else {
+        let call = to_call(arena, arguments, expr);
+
+        match expr_to_pattern_help(arena, &call.value) {
+            Ok(good) => {
+                let parser = specialize(
+                    EExpr::Type,
+                    space0_before_e(
+                        set_min_indent(indented_more, type_annotation::located(false)),
+                        EType::TIndentStart,
+                    ),
+                );
+
+                match parser.parse(arena, state.clone(), min_indent) {
+                    Err((_, fail)) => return Err((MadeProgress, fail)),
+                    Ok((_, mut ann_type, state)) => {
+                        // put the spaces from after the operator in front of the call
+                        if !spaces_after_operator.is_empty() {
+                            ann_type = arena
+                                .alloc(ann_type.value)
+                                .with_spaces_before(spaces_after_operator, ann_type.region);
                         }
+
+                        let def_region = Region::span_across(&call.region, &ann_type.region);
+
+                        let value_def = ValueDef::Annotation(Loc::at(expr_region, good), ann_type);
+
+                        defs.push_value_def(value_def, def_region, &[], &[]);
+
+                        state
                     }
                 }
-                Err(_) => {
+            }
-                    // this `:`/`:=` likely occurred inline; treat it as an invalid operator
+            Err(_) => {
-                    let op = match kind {
+                // this `:`/`:=` likely occurred inline; treat it as an invalid operator
-                        AliasOrOpaque::Alias => ":",
+                let op = match kind {
-                        AliasOrOpaque::Opaque => ":=",
+                    AliasOrOpaque::Alias => ":",
-                    };
+                    AliasOrOpaque::Opaque => ":=",
-                    let fail = EExpr::BadOperator(op, loc_op.region.start());
+                };
+                let fail = EExpr::BadOperator(op, loc_op.region.start());
 
-                    return Err((MadeProgress, fail));
+                return Err((MadeProgress, fail));
-                }
             }
         }
     };

crates/compiler/test_syntax/src/test_helpers.rs

@@ -243,10 +243,11 @@ impl<'a> Input<'a> {
             let reformatted = reparsed_ast.format();
 
             if output != reformatted {
-                eprintln!("Formatting bug; formatting is not stable.\nOriginal code:\n{}\n\nFormatted code:\n{}\n\nAST:\n{:#?}\n\n",
+                eprintln!("Formatting bug; formatting is not stable.\nOriginal code:\n{}\n\nFormatted code:\n{}\n\nAST:\n{:#?}\n\nReparsed AST:\n{:#?}\n\n",
                     self.as_str(),
                     output.as_ref().as_str(),
-                    actual);
+                    actual,
+                    reparsed_ast);
                 eprintln!("Reformatting the formatted code changed it again, as follows:\n\n");
 
                 assert_multiline_str_eq!(output.as_ref().as_str(), reformatted.as_ref().as_str());

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

@@ -0,0 +1 @@
+{ e &  }

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

@@ -0,0 +1,7 @@
+RecordUpdate {
+    update: @1-2 Var {
+        module_name: "",
+        ident: "e",
+    },
+    fields: [],
+}

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

@@ -0,0 +1 @@
+{e&}

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

@@ -0,0 +1,4 @@
+[
+    K,
+]
+- i

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

@@ -0,0 +1,23 @@
+BinOps(
+    [
+        (
+            @0-5 List(
+                Collection {
+                    items: [
+                        @1-2 Tag(
+                            "K",
+                        ),
+                    ],
+                    final_comments: [
+                        Newline,
+                    ],
+                },
+            ),
+            @5-6 Minus,
+        ),
+    ],
+    @6-7 Var {
+        module_name: "",
+        ident: "i",
+    },
+)

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

@@ -0,0 +1,2 @@
+[K,
+]-i

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

@@ -0,0 +1,4 @@
+a : e
+Na :=
+    e
+e0

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

@@ -0,0 +1,54 @@
+Defs(
+    Defs {
+        tags: [
+            Index(2147483648),
+            Index(0),
+        ],
+        regions: [
+            @0-3,
+            @4-11,
+        ],
+        space_before: [
+            Slice(start = 0, length = 0),
+            Slice(start = 0, length = 1),
+        ],
+        space_after: [
+            Slice(start = 0, length = 0),
+            Slice(start = 1, length = 0),
+        ],
+        spaces: [
+            Newline,
+        ],
+        type_defs: [
+            Opaque {
+                header: TypeHeader {
+                    name: @4-6 "Na",
+                    vars: [],
+                },
+                typ: @10-11 SpaceBefore(
+                    BoundVariable(
+                        "e",
+                    ),
+                    [
+                        Newline,
+                    ],
+                ),
+                derived: None,
+            },
+        ],
+        value_defs: [
+            Annotation(
+                @0-1 Identifier(
+                    "a",
+                ),
+                @2-3 BoundVariable(
+                    "e",
+                ),
+            ),
+        ],
+    },
+    @12-14 Var {
+        module_name: "",
+        ident: "e0",
+    },
+)

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

@@ -0,0 +1,3 @@
+a:e
+Na:=
+ e e0

crates/compiler/test_syntax/tests/test_snapshots.rs

@@ -304,6 +304,7 @@ mod test_snapshots {
         pass/empty_package_header.header,
         pass/empty_platform_header.header,
         pass/empty_record.expr,
+        pass/empty_record_update.expr,
         pass/empty_string.expr,
         pass/equals.expr,
         pass/equals_with_spaces.expr,
@@ -327,6 +328,7 @@ mod test_snapshots {
         pass/list_closing_indent_not_enough.expr,
         pass/list_closing_same_indent_no_trailing_comma.expr,
         pass/list_closing_same_indent_with_trailing_comma.expr,
+        pass/list_minus_newlines.expr,
         pass/list_pattern_weird_indent.expr,
         pass/list_patterns.expr,
         pass/lowest_float.expr,
@@ -389,6 +391,7 @@ mod test_snapshots {
         pass/opaque_reference_pattern.expr,
         pass/opaque_reference_pattern_with_arguments.expr,
         pass/opaque_simple.moduledefs,
+        pass/opaque_type_def_with_newline.expr,
         pass/opaque_with_type_arguments.moduledefs,
         pass/ops_with_newlines.expr,
         pass/outdented_app_with_record.expr,