Merge pull request #3429 from rtfeldman/fix-fmt-comments

Fix some formatter comment bugs

crates/compiler/fmt/src/expr.rs

@@ -550,7 +550,7 @@ fn fmt_bin_ops<'a, 'buf>(
         loc_left_side.format_with_options(buf, apply_needs_parens, Newlines::No, curr_indent);
 
         if is_multiline {
-            buf.newline();
+            buf.ensure_ends_in_newline();
             curr_indent = indent + INDENT;
             buf.indent(curr_indent);
         } else {
@@ -577,12 +577,13 @@ fn format_spaces<'a, 'buf>(
     newlines: Newlines,
     indent: u16,
 ) {
-    let format_newlines = newlines == Newlines::Yes;
-
-    if format_newlines {
-        fmt_spaces(buf, spaces.iter(), indent);
-    } else {
-        fmt_comments_only(buf, spaces.iter(), NewlineAt::Bottom, indent);
+    match newlines {
+        Newlines::Yes => {
+            fmt_spaces(buf, spaces.iter(), indent);
+        }
+        Newlines::No => {
+            fmt_comments_only(buf, spaces.iter(), NewlineAt::Bottom, indent);
+        }
     }
 }
 

crates/compiler/fmt/src/lib.rs

@@ -53,7 +53,14 @@ impl<'a> Buf<'a> {
 
     pub fn push(&mut self, ch: char) {
         debug_assert!(!self.beginning_of_line);
-        debug_assert!(ch != '\n' && ch != ' ');
+        debug_assert!(
+            ch != '\n',
+            "Don't call buf.push('\\n') - rather, call buf.newline()"
+        );
+        debug_assert!(
+            ch != ' ',
+            "Don't call buf.push(' ') - rather, call buf.spaces(1)"
+        );
 
         self.flush_spaces();
 
@@ -92,7 +99,10 @@ impl<'a> Buf<'a> {
     /// Ensures the current buffer ends in a newline, if it didn't already.
     /// Doesn't add a newline if the buffer already ends in one.
     pub fn ensure_ends_in_newline(&mut self) {
-        if !self.text.ends_with('\n') {
+        if self.spaces_to_flush > 0 {
+            self.flush_spaces();
+            self.newline();
+        } else if !self.text.ends_with('\n') {
             self.newline()
         }
     }
@@ -110,6 +120,18 @@ impl<'a> Buf<'a> {
     pub fn fmt_end_of_file(&mut self) {
         fmt_text_eof(&mut self.text)
     }
+
+    pub fn ends_with_space(&self) -> bool {
+        self.spaces_to_flush > 0 || self.text.ends_with(' ')
+    }
+
+    pub fn ends_with_newline(&self) -> bool {
+        self.spaces_to_flush == 0 && self.text.ends_with('\n')
+    }
+
+    fn is_empty(&self) -> bool {
+        self.spaces_to_flush == 0 && self.text.is_empty()
+    }
 }
 
 /// Ensures the text ends in a newline with no whitespace preceding it.

crates/compiler/fmt/src/spaces.rs

@@ -125,6 +125,12 @@ pub fn fmt_comments_only<'a, 'buf, I>(
 }
 
 fn fmt_comment<'buf>(buf: &mut Buf<'buf>, comment: &str) {
+    // The '#' in a comment should always be preceded by a newline or a space,
+    // unless it's the very beginning of the buffer.
+    if !buf.is_empty() && !buf.ends_with_space() && !buf.ends_with_newline() {
+        buf.spaces(1);
+    }
+
     buf.push('#');
     if !comment.starts_with(' ') {
         buf.spaces(1);
@@ -158,6 +164,12 @@ where
 }
 
 fn fmt_docs<'buf>(buf: &mut Buf<'buf>, docs: &str) {
+    // The "##" in a doc comment should always be preceded by a newline or a space,
+    // unless it's the very beginning of the buffer.
+    if !buf.is_empty() && !buf.ends_with_space() && !buf.ends_with_newline() {
+        buf.spaces(1);
+    }
+
     buf.push_str("##");
     if !docs.is_empty() {
         buf.spaces(1);

crates/compiler/fmt/tests/test_fmt.rs

@@ -199,6 +199,31 @@ mod test_fmt {
         ));
     }
 
+    #[test]
+    fn comment_with_trailing_space() {
+        expr_formats_to(
+            &format!(
+                indoc!(
+                    r#"
+            # first comment{space}
+            x = 0 # second comment{space}
+
+            x
+            "#
+                ),
+                space = " ",
+            ),
+            indoc!(
+                r#"
+            # first comment
+            x = 0 # second comment
+
+            x
+            "#
+            ),
+        );
+    }
+
     #[test]
     fn def_with_inline_comment() {
         expr_formats_same(indoc!(
@@ -3958,6 +3983,60 @@ mod test_fmt {
         );
     }
 
+    #[test]
+    fn multiline_binop_with_comments() {
+        expr_formats_same(indoc!(
+            r#"
+            x = 1
+                + 1 # comment 1
+                - 1 # comment 2
+                * 1 # comment 3
+
+            x
+            "#
+        ));
+
+        expr_formats_same(indoc!(
+            r#"
+            x = 1
+                + 1 # comment 1
+                * 1 # comment 2
+
+            x
+            "#
+        ));
+
+        expr_formats_same(indoc!(
+            r#"
+            x = 1
+                + 1 # comment
+
+            x
+            "#
+        ));
+
+        expr_formats_same(indoc!(
+            r#"
+            x = 1
+                * 1
+                + 1 # comment
+
+            x
+            "#
+        ));
+
+        expr_formats_same(indoc!(
+            r#"
+            x = 1
+                - 1
+                * 1
+                + 1
+
+            x
+            "#
+        ));
+    }
+
     #[test]
     fn precedence_conflict_greater_than() {
         expr_formats_same(indoc!(
@@ -4266,7 +4345,7 @@ mod test_fmt {
             indoc!(
                 r#"
             interface Foo exposes [] imports []
-            a = 42# Yay greetings
+            a = 42 # Yay greetings
             "#
             ),
         );
@@ -4339,6 +4418,37 @@ mod test_fmt {
         );
     }
 
+    #[test]
+    fn module_defs_with_comments() {
+        module_formats_to(
+            &format!(
+                indoc!(
+                    r#"
+                    interface Foo
+                        exposes []
+                        imports []
+
+                    # comment 1{space}
+                    def = "" # comment 2{space}
+                    # comment 3{space}
+                "#
+                ),
+                space = " "
+            ),
+            indoc!(
+                r#"
+                    interface Foo
+                        exposes []
+                        imports []
+
+                    # comment 1
+                    def = "" # comment 2
+                    # comment 3
+                "#
+            ),
+        );
+    }
+
     #[test]
     fn format_tui_package_config() {
         // At one point this failed to reformat.

examples/benchmarks/CFold.roc

@@ -9,7 +9,7 @@ main =
     Task.after
         Task.getInt
         \n ->
-            e = mkExpr n 1# original koka n = 20 (set `ulimit -s unlimited` to avoid stack overflow for n = 20)
+            e = mkExpr n 1 # original koka n = 20 (set `ulimit -s unlimited` to avoid stack overflow for n = 20)
             unoptimized = eval e
             optimized = eval (constFolding (reassoc e))
 

examples/benchmarks/Deriv.roc

@@ -17,8 +17,7 @@ main =
             f : Expr
             f = pow x x
 
-            nest deriv n f# original koka n = 10
-
+            nest deriv n f # original koka n = 10
                 |> Task.map (\_ -> {})
 
 nest : (I64, Expr -> IO Expr), I64, Expr -> IO Expr

examples/benchmarks/NQueens.roc

@@ -8,8 +8,7 @@ main =
     Task.after
         Task.getInt
         \n ->
-            queens n# original koka 13
-
+            queens n # original koka 13
                 |> Num.toStr
                 |> Task.putLine
 

examples/benchmarks/RBTreeDel.roc

@@ -16,7 +16,7 @@ main =
     Task.after
         Task.getInt
         \n ->
-            m = makeMap n# koka original n = 4_200_000
+            m = makeMap n # koka original n = 4_200_000
             val = fold (\_, v, r -> if v then r + 1 else r) m 0
 
             val

examples/breakout/breakout.roc

@@ -3,11 +3,11 @@ app "breakout"
     imports [pf.Game.{ Bounds, Elem, Event }]
     provides [program] { Model } to pf
 
-paddleWidth = 0.2# width of the paddle, as a % of screen width
-paddleHeight = 50# height of the paddle, in pixels
-paddleSpeed = 65# how many pixels the paddle moves per keypress
-blockHeight = 80# height of a block, in pixels
-blockBorder = 0.025# border of a block, as a % of its width
+paddleWidth = 0.2 # width of the paddle, as a % of screen width
+paddleHeight = 50 # height of the paddle, in pixels
+paddleSpeed = 65 # how many pixels the paddle moves per keypress
+blockHeight = 80 # height of a block, in pixels
+blockBorder = 0.025 # border of a block, as a % of its width
 ballSize = 55
 numRows = 4
 numCols = 8

examples/breakout/platform/Elem.roc

@@ -60,7 +60,7 @@ lazy = \state, render ->
                     { state, elem: render state }
 
 none : Elem *
-none = None# I've often wanted this in elm/html. Usually end up resorting to (Html.text "") - this seems nicer.
+none = None # I've often wanted this in elm/html. Usually end up resorting to (Html.text "") - this seems nicer.
 ## Change an element's state type.
 ##
 ## TODO: indent the following once https://github.com/rtfeldman/roc/issues/2585 is fixed.

examples/false-interpreter/False.roc

@@ -58,23 +58,19 @@ interpretFile = \filename ->
 isDigit : U8 -> Bool
 isDigit = \char ->
     char
-        >= 0x30# `0`
-
+        >= 0x30 # `0`
         && char
-        <= 0x39# `0`
+        <= 0x39 # `0`
 isWhitespace : U8 -> Bool
 isWhitespace = \char ->
     char
-        == 0xA# new line
-
+        == 0xA # new line
         || char
-        == 0xB# carriage return
-
+        == 0xB # carriage return
         || char
-        == 0x20# space
-
+        == 0x20 # space
         || char
-        == 0x9# tab
+        == 0x9 # tab
 interpretCtx : Context -> Task Context InterpreterErrors
 interpretCtx = \ctx ->
     Task.loop ctx interpretCtxLoop

examples/false-interpreter/Variable.roc

@@ -8,10 +8,8 @@ Variable := U8
 
 totalCount : Nat
 totalCount =
-    0x7A# "z"
-
-        - 0x61# "a"
-
+    0x7A # "z"
+        - 0x61 # "a"
         + 1
 
 toStr : Variable -> Str
@@ -26,8 +24,7 @@ fromUtf8 : U8 -> Result Variable [InvalidVariableUtf8]
 fromUtf8 = \char ->
     if
         char
-            >= 0x61# "a"
-
+            >= 0x61 # "a"
             && char
             <= 0x7A
         # "z"
@@ -38,5 +35,5 @@ fromUtf8 = \char ->
 
 toIndex : Variable -> Nat
 toIndex = \@Variable char ->
-    Num.intCast (char - 0x61)# "a"
+    Num.intCast (char - 0x61) # "a"
 # List.first (Str.toUtf8 "a")

examples/false-interpreter/platform/Stdin.roc

@@ -5,4 +5,4 @@ interface Stdin
 # line : Task.Task Str *
 # line = Effect.after Effect.getLine Task.succeed # TODO FIXME Effect.getLine should suffice
 char : Task.Task U8 *
-char = Effect.after Effect.getChar Task.succeed# TODO FIXME Effect.getLine should suffice
+char = Effect.after Effect.getChar Task.succeed # TODO FIXME Effect.getLine should suffice

examples/gui/Hello.roc

@@ -1,6 +1,6 @@
 app "hello-gui"
     packages { pf: "platform/main.roc" }
-    imports []# [pf.Action.{ Action }, pf.Elem.{ button, text, row, col }]
+    imports [] # [pf.Action.{ Action }, pf.Elem.{ button, text, row, col }]
     provides [render] to pf
 
 render =

examples/gui/platform/Elem.roc

@@ -60,7 +60,7 @@ lazy = \state, render ->
                     { state, elem: render state }
 
 none : Elem *
-none = None# I've often wanted this in elm/html. Usually end up resorting to (Html.text "") - this seems nicer.
+none = None # I've often wanted this in elm/html. Usually end up resorting to (Html.text "") - this seems nicer.
 ## Change an element's state type.
 ##
 ## TODO: indent the following once https://github.com/rtfeldman/roc/issues/2585 is fixed.

examples/interactive/cli-platform/Stdin.roc

@@ -3,4 +3,4 @@ interface Stdin
     imports [pf.Effect, Task]
 
 line : Task.Task Str *
-line = Effect.after Effect.getLine Task.succeed# TODO FIXME Effect.getLine should suffice
+line = Effect.after Effect.getLine Task.succeed # TODO FIXME Effect.getLine should suffice