Repository: gleam-lang/gleam (rust)

Issue:
`echo` throws error when used at start of pipeline
When you have an `echo` as the first term of a pipeline, the compiler returns a rather cryptic error

Gleam: 1.9.0
OS: Ubuntu 22.04.5 LTS
Runtime: Erlang

Logs (filtered for the error part):
```
{compiling,"/home/pedrohcf/dev/chess_bot/build/dev/erlang/chess_bot/_gleam_artefacts/tests.erl"}
src/tests.gleam:4:5: function echo/4 undefined
%    4| }
%     |  ^^^^

src/tests.gleam:5:14: variable '_pipe' is unbound
src/tests.gleam:4:10: Warning: variable '_pipe' is bound multiple times in this pattern.
If you mean to ignore this value, use '_' or
a different underscore-prefixed name
%    4| }
%     |  ^^^^^^^^^

{failed,"/home/pedrohcf/dev/chess_bot/build/dev/erlang/chess_bot/_gleam_artefacts/tests.erl"}
{compiled,chess_bot,
          "/home/pedrohcf/dev/chess_bot/build/dev/erlang/chess_bot/ebin/chess_bot.beam"}
ERROR Failed error=ShellCommand { program: "escript", reason: Unknown }
error: Shell command failure

There was a problem when running the shell command `escript`.
```


Pull request description:
Fix echo code generation on the Erlang target
This PR fixes #4322 
- I fixed a bug in code generation that meant using echo to print an entire pipeline or record update would end up generating invalid erlang code, sorry for missing this! 🙇
- Since I was there I made a small change to the formatter to make sure it's easier to see echo is printing an entire pipeline if used at the beginning of it

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 075f93343..8b976d3f0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,4 +10,11 @@
 
 ### Formatter
 
+- Improved the formatting of pipelines printed with `echo`.
+  ([Giacomo Cavalieri](https://github.com/giacomocavalieri))
+
 ### Bug fixes
+
+- Fixed a bug where `echo` used before a pipeline would generate invalid code
+  for the Erlang target.
+  ([Giacomo Cavalieri](https://github.com/giacomocavalieri))
diff --git a/compiler-core/src/erlang.rs b/compiler-core/src/erlang.rs
index 7e5133dcb..50638e8ff 100644
--- a/compiler-core/src/erlang.rs
+++ b/compiler-core/src/erlang.rs
@@ -1988,7 +1988,7 @@ fn expr<'a>(expression: &'a TypedExpr, env: &mut Env<'a>) -> Document<'a> {
             let expression = expression
                 .as_ref()
                 .expect("echo with no expression outside of pipe");
-            echo(expr(expression, env), location, env)
+            echo(maybe_block_expr(expression, env), location, env)
         }
 
         TypedExpr::Int { value, .. } => int(value),
diff --git a/compiler-core/src/format.rs b/compiler-core/src/format.rs
index 400eb7959..b3d551965 100644
--- a/compiler-core/src/format.rs
+++ b/compiler-core/src/format.rs
@@ -973,14 +973,9 @@ impl<'comments> Formatter<'comments> {
             } => docvec!["todo as ", self.expr(l)],
 
             UntypedExpr::Echo {
-                expression: None,
+                expression,
                 location: _,
-            } => "echo".to_doc(),
-
-            UntypedExpr::Echo {
-                expression: Some(e),
-                location: _,
-            } => docvec!["echo ", self.expr(e)],
+            } => self.echo(expression),
 
             UntypedExpr::PipeLine { expressions, .. } => self.pipeline(expressions, false),
 
@@ -1478,6 +1473,24 @@ impl<'comments> Formatter<'comments> {
         }
     }
 
+    fn spans_multiple_lines(&self, start: u32, end: u32) -> bool {
+        self.new_lines
+            .binary_search_by(|newline| {
+                if *newline <= start {
+                    Ordering::Less
+                } else if *newline >= end {
+                    Ordering::Greater
+                } else {
+                    // If the newline is in between the pipe start and end
+                    // then we've found it!
+                    Ordering::Equal
+                }
+            })
+            // If we couldn't find any newline between the start and end of
+            // the pipeline then we will try and keep it on a single line.
+            .is_ok()
+    }
+
     fn pipeline<'a>(
         &mut self,
         expressions: &'a Vec1<UntypedExpr>,
@@ -1491,22 +1504,7 @@ impl<'comments> Formatter<'comments> {
 
         let pipeline_start = expressions.first().location().start;
         let pipeline_end = expressions.last().location().end;
-        let try_to_keep_on_one_line = self
-            .new_lines
-            .binary_search_by(|newline| {
-                if *newline <= pipeline_start {
-                    Ordering::Less
-                } else if *newline >= pipeline_end {
-                    Ordering::Greater
-                } else {
-                    // If the newline is in between the pipe start and end
-                    // then we've found it!
-                    Ordering::Equal
-                }
-            })
-            // If we couldn't find any newline between the start and end of
-            // the pipeline then we will try and keep it on a single line.
-            .is_err();
+        let try_to_keep_on_one_line = !self.spans_multiple_lines(pipeline_start, pipeline_end);
 
         for expr in expressions.iter().skip(1) {
             let comments = self.pop_comments(expr.location().start);
@@ -2720,6 +2718,36 @@ impl<'comments> Formatter<'comments> {
         let doc = concat(doc);
         Some(doc.force_break())
     }
+
+    fn echo<'a>(&mut self, expression: &'a Option<Box<UntypedExpr>>) -> Document<'a> {
+        let Some(expression) = expression else {
+            return "echo".to_doc();
+        };
+
+        match expression.as_ref() {
+            // When a pipeline gets broken on multiple lines we don't want it to
+            // be on the same line as echo, or it would look confusing; instead
+            // it's nested onto a new line:
+            //
+            // ```gleam
+            // echo first
+            //   |> wobble
+            //   |> wibble
+            // ```
+            //
+            // So it's easier to see echo is printing the whole thing. Otherwise,
+            // it would look like echo is printing just the first item:
+            //
+            // ```gleam
+            // echo first
+            // |> wobble
+            // |> wibble
+            // ```
+            //
+            UntypedExpr::PipeLine { .. } => docvec!["echo ", self.expr(expression).nest(INDENT)],
+            _ => docvec!["echo ", self.expr(expression)],
+        }
+    }
 }
 
 fn init_and_last<T>(vec: &[T]) -> Option<(&[T], &T)> {


Test patch:
diff --git a/compiler-core/src/erlang/tests/echo.rs b/compiler-core/src/erlang/tests/echo.rs
index 3100e8467..0c6c14b40 100644
--- a/compiler-core/src/erlang/tests/echo.rs
+++ b/compiler-core/src/erlang/tests/echo.rs
@@ -106,3 +106,33 @@ pub fn wibble(n) { n }
 "#
     )
 }
+
+#[test]
+pub fn pipeline_printed_by_echo_is_wrapped_in_begin_end_block() {
+    assert_erl!(
+        r#"
+pub fn main() {
+  echo
+    123
+    |> wibble
+    |> wibble
+}
+
+pub fn wibble(n) { n }
+"#
+    )
+}
+
+#[test]
+pub fn record_update_printed_by_echo_is_wrapped_in_begin_end_block() {
+    assert_erl!(
+        r#"
+pub type Wobble { Wobble(id: Int, name: String) }
+
+pub fn main() {
+  let wobble = Wobble(1, "wobble")
+  echo Wobble(..wobble, id: 1)
+}
+"#
+    )
+}
diff --git a/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__echo__pipeline_printed_by_echo_is_wrapped_in_begin_end_block.snap b/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__echo__pipeline_printed_by_echo_is_wrapped_in_begin_end_block.snap
new file mode 100644
index 000000000..fef8d64ff
--- /dev/null
+++ b/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__echo__pipeline_printed_by_echo_is_wrapped_in_begin_end_block.snap
@@ -0,0 +1,41 @@
+---
+source: compiler-core/src/erlang/tests/echo.rs
+expression: "\npub fn main() {\n  echo\n    123\n    |> wibble\n    |> wibble\n}\n\npub fn wibble(n) { n }\n"
+---
+----- SOURCE CODE
+
+pub fn main() {
+  echo
+    123
+    |> wibble
+    |> wibble
+}
+
+pub fn wibble(n) { n }
+
+
+----- COMPILED ERLANG
+-module(my@mod).
+-compile([no_auto_import, nowarn_unused_vars, nowarn_unused_function, nowarn_nomatch]).
+
+-export([wibble/1, main/0]).
+
+-file("project/test/my/mod.gleam", 9).
+-spec wibble(J) -> J.
+wibble(N) ->
+    N.
+
+-file("project/test/my/mod.gleam", 2).
+-spec main() -> integer().
+main() ->
+    echo(
+        begin
+            _pipe = 123,
+            _pipe@1 = wibble(_pipe),
+            wibble(_pipe@1)
+        end,
+        "project/test/my/mod.gleam",
+        3
+    ).
+
+% ...omitted code from `templates/echo.erl`...
diff --git a/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__echo__record_update_printed_by_echo_is_wrapped_in_begin_end_block.snap b/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__echo__record_update_printed_by_echo_is_wrapped_in_begin_end_block.snap
new file mode 100644
index 000000000..ef66d496f
--- /dev/null
+++ b/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__echo__record_update_printed_by_echo_is_wrapped_in_begin_end_block.snap
@@ -0,0 +1,37 @@
+---
+source: compiler-core/src/erlang/tests/echo.rs
+expression: "\npub type Wobble { Wobble(id: Int, name: String) }\n\npub fn main() {\n  let wobble = Wobble(1, \"wobble\")\n  echo Wobble(..wobble, id: 1)\n}\n"
+---
+----- SOURCE CODE
+
+pub type Wobble { Wobble(id: Int, name: String) }
+
+pub fn main() {
+  let wobble = Wobble(1, "wobble")
+  echo Wobble(..wobble, id: 1)
+}
+
+
+----- COMPILED ERLANG
+-module(my@mod).
+-compile([no_auto_import, nowarn_unused_vars, nowarn_unused_function, nowarn_nomatch]).
+
+-export([main/0]).
+-export_type([wobble/0]).
+
+-type wobble() :: {wobble, integer(), binary()}.
+
+-file("project/test/my/mod.gleam", 4).
+-spec main() -> wobble().
+main() ->
+    Wobble = {wobble, 1, <<"wobble"/utf8>>},
+    echo(
+        begin
+            _record = Wobble,
+            {wobble, 1, erlang:element(3, _record)}
+        end,
+        "project/test/my/mod.gleam",
+        6
+    ).
+
+% ...omitted code from `templates/echo.erl`...
diff --git a/compiler-core/src/format/tests.rs b/compiler-core/src/format/tests.rs
index d08350d93..57d65cd53 100644
--- a/compiler-core/src/format/tests.rs
+++ b/compiler-core/src/format/tests.rs
@@ -6512,3 +6512,31 @@ fn echo_with_following_value_in_a_pipeline() {
 "#
     );
 }
+
+#[test]
+fn echo_printing_multiline_pipeline() {
+    assert_format_rewrite!(
+        r#"fn main() {
+  echo first
+  |> wibble
+  |> wobble
+}
+"#,
+        r#"fn main() {
+  echo first
+    |> wibble
+    |> wobble
+}
+"#
+    );
+}
+
+#[test]
+fn echo_printing_one_line_pipeline() {
+    assert_format!(
+        r#"fn main() {
+  echo first |> wibble |> wobble
+}
+"#
+    );
+}
diff --git a/compiler-core/src/parse/snapshots/gleam_core__parse__tests__echo_at_start_of_pipeline_wraps_the_whole_thing.snap b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__echo_at_start_of_pipeline_wraps_the_whole_thing.snap
new file mode 100644
index 000000000..6008b9abe
--- /dev/null
+++ b/compiler-core/src/parse/snapshots/gleam_core__parse__tests__echo_at_start_of_pipeline_wraps_the_whole_thing.snap
@@ -0,0 +1,42 @@
+---
+source: compiler-core/src/parse/tests.rs
+expression: echo 1 |> wibble |> wobble
+---
+[
+    Expression(
+        Echo {
+            location: SrcSpan {
+                start: 0,
+                end: 26,
+            },
+            expression: Some(
+                PipeLine {
+                    expressions: [
+                        Int {
+                            location: SrcSpan {
+                                start: 5,
+                                end: 6,
+                            },
+                            value: "1",
+                            int_value: 1,
+                        },
+                        Var {
+                            location: SrcSpan {
+                                start: 10,
+                                end: 16,
+                            },
+                            name: "wibble",
+                        },
+                        Var {
+                            location: SrcSpan {
+                                start: 20,
+                                end: 26,
+                            },
+                            name: "wobble",
+                        },
+                    ],
+                },
+            ),
+        },
+    ),
+]
diff --git a/compiler-core/src/parse/tests.rs b/compiler-core/src/parse/tests.rs
index cc0f1039f..c31654c9d 100644
--- a/compiler-core/src/parse/tests.rs
+++ b/compiler-core/src/parse/tests.rs
@@ -519,6 +519,11 @@ fn repeated_echos() {
     assert_parse!("echo echo echo 1");
 }
 
+#[test]
+fn echo_at_start_of_pipeline_wraps_the_whole_thing() {
+    assert_parse!("echo 1 |> wibble |> wobble");
+}
+
 #[test]
 fn no_let_binding_snapshot_1() {
     assert_error!("wibble = 4");