Repository: prettier/plugin-ruby (javascript)

Issue:
Interpolated expressions inside long strings wrap across multiple lines
Wrapping the interpolated ruby expressions (i.e. `#{something}`) in a long string across multiple lines is technically valid ruby but I don't think I've ever seen it in production code. This behavior is also different from the js/ts (see below).

## Metadata

* Ruby version: 2.7.2
* `@prettier/plugin-ruby` version: 1.3.0
* Options:
  * [x] `rubyHashLabel`
  * [x] `rubyModifier`
  * [x] `rubySingleQuote`
  * [ ] `rubyToProc`
  * [x] `trailingComma`

## Input

```ruby
"Some really long string of text that goes beyond the configured #{variable_1} line length with interpolation #{variable_2}"
```

## Current output

```ruby
"Some really long string of text that goes beyond the configured #{
  variable_1
} line length with interpolation #{variable_2}"
```

## Expected output

```ruby
"Some really long string of text that goes beyond the configured #{variable_1} line length with interpolation #{variable_2}"
```

## In javascript/typescript

This comparable line is kept as-is:
```javascript
`Some really long string of text that goes beyond the configured ${variable_1} line length with interpolation ${variable_2}`;
```

Sorbet type annotations are ignored
I can see that Sorbet type annotations are being explicitly ignored as of [fairly recently](https://github.com/prettier/plugin-ruby/pull/718) so this issue is about reversing that or turning it into an option.

The comment in ruby-prettier where Sorbet signatures are skipped says:

> They look like Ruby code, but they're not actually Ruby code, so we're not going to mess with them at all

But this is not the case; by design, Sorbet type signatures are 100% Ruby. From the [Sorbet docs](https://sorbet.org/docs/sigs):

> Signatures are valid Ruby syntax

 We have ~700 signatures in our Ruby codebase and we never had any issues with them being formatted by prettier in previous versions. In fact, prettier is *fantastic* for Sorbet signatures precisely because it tidies them up. For example, you can write a signature like this:

```rb
sig{params(name: String, address: T::Hash[String, String], tracking: T::Boolean).returns(T::Boolean)}
def ship(name, address, tracking); end
```

And prettier would (in version 0.22) format it into this:

```rb
sig do
  params(name: String, address: T::Hash[String, String], tracking: T::Boolean)
    .returns(T::Boolean)
end
def ship(name, address, tracking); end
```

It's unclear why Sorbet signatures were skipped, but we'd love to have prettier format them again. If excluding them was a stylistic choice (i.e. some people may prefer their signatures remain entirely on one line, even if they breach the line length limit) perhaps it could be an option instead?

## Metadata

* Ruby version: 2.6.6
* `@prettier/plugin-ruby` or `prettier` gem version: 1.3.0
* Options:
  * [all] `trailingComma`



Discussion:

Hi @clarkdave!

I actually did that out of an abundance of precaution. I just got nervous about changing them in case I would accidentally break them. I'd be fine reversing that decision if you're 100% sure.

Feel like making a contribution? I think it should basically just be the opposite of that PR that you linked.
Sure, I'll make a PR!

I'm confident that Sorbet types are 100% Ruby. My only thought is that some people may prefer, stylistically, that signatures always remain on a single line - but that seems like something that can be discussed in the future when there are more Sorbet+Prettier users. I know there's precedent for similar decisions from JS Prettier, e.g. `it(..., () => {` expressions always remaining on one line.
Ehh I'm fine with taking a stand on this and saying no to another option. At least it'll be consistent with the rest of the formatting.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,18 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a
 
 ## [Unreleased]
 
+### Added
+
+- [@ianks], [@kddeisz] - Use `netcat` to communicate to a parser server for better performance when formatting multiple files.
+
+### Changed
+
+- [@jeffcarbs], [@kddeisz] - Long strings with interpolated expressions should only break if the contents in the original source is broken.
+- [@johannesluedke], [@kddeisz] - Fix for rescues with inline comments.
+- [@johncsnyder], [@kddeisz] - Comments should not attach to nodes beyond a double newline.
+- [@blampe], [@kddeisz] - Comments inside of a broken method chain get dropped.
+- [@clarkdave], [@kddeisz] - Don't ignore Sorbet `sig` transformations.
+
 ## [1.3.0] - 2021-01-05
 
 ### Added
@@ -1107,6 +1119,7 @@ would previously result in `array[]`, but now prints properly.
 [@bugthing]: https://github.com/bugthing
 [@cbothner]: https://github.com/cbothner
 [@christoomey]: https://github.com/christoomey
+[@clarkdave]: https://github.com/clarkdave
 [@cldevs]: https://github.com/cldevs
 [@codingitwrong]: https://github.com/CodingItWrong
 [@coiti]: https://github.com/coiti
@@ -1126,7 +1139,9 @@ would previously result in `array[]`, but now prints properly.
 [@jamescostian]: https://github.com/jamescostian
 [@janklimo]: https://github.com/janklimo
 [@jbielick]: https://github.com/jbielick
+[@jeffcarbs]: https://github.com/jeffcarbs
 [@joeyjoejoejr]: https://github.com/joeyjoejoejr
+[@johannesluedke]: https://github.com/johannesluedke
 [@johncsnyder]: https://github.com/johncsnyder
 [@johnschoeman]: https://github.com/johnschoeman
 [@joshuakgoldberg]: https://github.com/JoshuaKGoldberg
diff --git a/src/ruby/nodes/calls.js b/src/ruby/nodes/calls.js
--- a/src/ruby/nodes/calls.js
+++ b/src/ruby/nodes/calls.js
@@ -43,7 +43,7 @@ function printCall(path, opts, print) {
 
   // If our parent node is a chained node then we're not going to group the
   // right side of the expression, as we want to have a nice multi-line layout.
-  if (chained.includes(parentNode.type)) {
+  if (chained.includes(parentNode.type) && !node.comments) {
     parentNode.chain = (node.chain || 0) + 1;
     parentNode.callChain = (node.callChain || 0) + 1;
     parentNode.breakDoc = (node.breakDoc || [receiverDoc]).concat(rightSideDoc);
@@ -103,7 +103,7 @@ function printMethodAddArg(path, opts, print) {
 
   // If our parent node is a chained node then we're not going to group the
   // right side of the expression, as we want to have a nice multi-line layout.
-  if (chained.includes(parentNode.type)) {
+  if (chained.includes(parentNode.type) && !node.comments) {
     parentNode.chain = (node.chain || 0) + 1;
     parentNode.breakDoc = (node.breakDoc || [methodDoc]).concat(argsDoc);
     parentNode.firstReceiverType = node.firstReceiverType;
@@ -136,41 +136,12 @@ function printMethodAddArg(path, opts, print) {
   return concat([methodDoc, argsDoc]);
 }
 
-// Sorbet type annotations look like the following:
-//
-//     {method_add_block
-//       [{method_add_arg
-//          [{fcall
-//             [{@ident "sig"}]},
-//           {args []}]},
-//        {brace_block [nil, {stmts}]}}]}
-//
-function isSorbetTypeAnnotation(node) {
-  const [callNode, blockNode] = node.body;
-
-  return (
-    callNode.type === "method_add_arg" &&
-    callNode.body[0].type === "fcall" &&
-    callNode.body[0].body[0].body === "sig" &&
-    callNode.body[1].type === "args" &&
-    callNode.body[1].body.length === 0 &&
-    blockNode
-  );
-}
-
 function printMethodAddBlock(path, opts, print) {
   const node = path.getValue();
 
   const [callNode, blockNode] = node.body;
   const [callDoc, blockDoc] = path.map(print, "body");
 
-  // Very special handling here for sorbet type annotations. They look like Ruby
-  // code, but they're not actually Ruby code, so we're not going to mess with
-  // them at all.
-  if (isSorbetTypeAnnotation(node)) {
-    return opts.originalText.slice(opts.locStart(node), opts.locEnd(node));
-  }
-
   // Don't bother trying to do any kind of fancy toProc transform if the option
   // is disabled.
   if (opts.rubyToProc) {
diff --git a/src/ruby/nodes/rescue.js b/src/ruby/nodes/rescue.js
--- a/src/ruby/nodes/rescue.js
+++ b/src/ruby/nodes/rescue.js
@@ -26,28 +26,10 @@ function printEnsure(path, opts, print) {
 }
 
 function printRescue(path, opts, print) {
-  const [exception, variable, _stmts, addition] = path.getValue().body;
   const parts = ["rescue"];
 
-  if (exception || variable) {
-    if (exception) {
-      if (Array.isArray(exception)) {
-        // In this case, it's actually only the one exception (it's an array
-        // of length 1).
-        parts.push(" ", path.call(print, "body", 0, 0));
-      } else {
-        // Here we have multiple exceptions from which we're rescuing, so we
-        // need to align them and join them together.
-        const joiner = concat([",", line]);
-        const exceptions = group(join(joiner, path.call(print, "body", 0)));
-
-        parts.push(" ", align("rescue ".length, exceptions));
-      }
-    }
-
-    if (variable) {
-      parts.push(" => ", path.call(print, "body", 1));
-    }
+  if (path.getValue().body[0]) {
+    parts.push(align("rescue ".length, path.call(print, "body", 0)));
   } else {
     // If you don't specify an error to rescue in a `begin/rescue` block, then
     // implicitly you're rescuing from `StandardError`. In this case, we're
@@ -55,16 +37,40 @@ function printRescue(path, opts, print) {
     parts.push(" StandardError");
   }
 
-  const rescueBody = path.call(print, "body", 2);
+  const bodystmt = path.call(print, "body", 1);
 
-  if (rescueBody.parts.length > 0) {
-    parts.push(indent(concat([hardline, rescueBody])));
+  if (bodystmt.parts.length > 0) {
+    parts.push(indent(concat([hardline, bodystmt])));
   }
 
   // This is the next clause on the `begin` statement, either another
   // `rescue`, and `ensure`, or an `else` clause.
-  if (addition) {
-    parts.push(concat([hardline, path.call(print, "body", 3)]));
+  if (path.getValue().body[2]) {
+    parts.push(concat([hardline, path.call(print, "body", 2)]));
+  }
+
+  return group(concat(parts));
+}
+
+// This is a container node that we're adding into the AST that isn't present in
+// Ripper solely so that we have a nice place to attach inline comments.
+function printRescueEx(path, opts, print) {
+  const [exception, variable] = path.getValue().body;
+  const parts = [];
+
+  if (exception) {
+    let exceptionDoc = path.call(print, "body", 0);
+
+    if (Array.isArray(exceptionDoc)) {
+      const joiner = concat([",", line]);
+      exceptionDoc = group(join(joiner, exceptionDoc));
+    }
+
+    parts.push(" ", exceptionDoc);
+  }
+
+  if (variable) {
+    parts.push(" => ", path.call(print, "body", 1));
   }
 
   return group(concat(parts));
@@ -89,6 +95,7 @@ module.exports = {
   ensure: printEnsure,
   redo: literal("redo"),
   rescue: printRescue,
+  rescue_ex: printRescueEx,
   rescue_mod: printRescueMod,
   retry: literal("retry")
 };
diff --git a/src/ruby/nodes/statements.js b/src/ruby/nodes/statements.js
--- a/src/ruby/nodes/statements.js
+++ b/src/ruby/nodes/statements.js
@@ -79,6 +79,9 @@ module.exports = {
     const { body } = path.getValue();
     return concat([trim, "__END__", literalline, body]);
   },
+  "@comment"(path, opts, _print) {
+    return opts.printer.printComment(path);
+  },
   bodystmt: printBodyStmt,
   paren: printParen,
   program: (path, opts, print) =>
diff --git a/src/ruby/nodes/strings.js b/src/ruby/nodes/strings.js
--- a/src/ruby/nodes/strings.js
+++ b/src/ruby/nodes/strings.js
@@ -4,6 +4,7 @@ const {
   hardline,
   indent,
   literalline,
+  removeLines,
   softline,
   join
 } = require("../../prettier");
@@ -103,14 +104,14 @@ function printStringDVar(path, opts, print) {
 }
 
 function printStringEmbExpr(path, opts, print) {
+  const node = path.getValue();
   const parts = path.call(print, "body", 0);
 
-  // If the interpolated expression is inside of a heredoc or an xstring
-  // literal (a string that gets sent to the command line) then we don't want
-  // to automatically indent, as this can lead to some very odd looking
-  // expressions
-  if (["heredoc", "xstring_literal"].includes(path.getParentNode().type)) {
-    return concat(["#{", parts, "}"]);
+  // If the contents of this embedded expression were originally on the same
+  // line in the source, then we're going to leave them in place and assume
+  // that's the way the developer wanted this expression represented.
+  if (node.sl === node.el) {
+    return concat(["#{", removeLines(parts), "}"]);
   }
 
   return group(
diff --git a/src/ruby/parser.rb b/src/ruby/parser.rb
--- a/src/ruby/parser.rb
+++ b/src/ruby/parser.rb
@@ -22,6 +22,10 @@ module Prettier; end
 class Prettier::Parser < Ripper
   attr_reader :source, :lines, :scanner_events, :line_counts
 
+  # This is an attr_accessor so Stmts objects can grab comments out of this
+  # array and attach them to themselves.
+  attr_accessor :comments
+
   def initialize(source, *args)
     super(source, *args)
 
@@ -125,6 +129,7 @@ def on_comment(value)
     @comments << {
       type: :@comment,
       value: value[1..-1].chomp.force_encoding('UTF-8'),
+      inline: value.strip != lines[lineno - 1],
       sl: lineno,
       el: lineno,
       sc: char_pos,
@@ -1768,8 +1773,8 @@ class Rescue < SimpleDelegator
     def bind_end(ec)
       merge!(ec: ec)
 
-      stmts = self[:body][2]
-      consequent = self[:body][3]
+      stmts = self[:body][1]
+      consequent = self[:body][2]
 
       if consequent
         consequent.bind_end(ec)
@@ -1784,16 +1789,30 @@ def bind_end(ec)
   # inside of a bodystmt.
   def on_rescue(exceptions, variable, stmts, consequent)
     beging = find_scanner_event(:@kw, 'rescue')
+    exceptions = exceptions[0] if exceptions.is_a?(Array)
 
-    last_exception = exceptions.is_a?(Array) ? exceptions[-1] : exceptions
-    last_node = variable || last_exception || beging
-
+    last_node = variable || exceptions || beging
     stmts.bind(find_next_statement_start(last_node[:ec]), char_pos)
 
+    # We add an additional inner node here that ripper doesn't provide so that
+    # we have a nice place to attach inline comment. But we only need it if we
+    # have an exception or a variable that we're rescuing.
+    rescue_ex =
+      if exceptions || variable
+        {
+          type: :rescue_ex,
+          body: [exceptions, variable],
+          sl: beging[:sl],
+          sc: beging[:ec] + 1,
+          el: last_node[:el],
+          ec: last_node[:ec]
+        }
+      end
+
     Rescue.new(
       beging.merge!(
         type: :rescue,
-        body: [exceptions, variable, stmts, consequent],
+        body: [rescue_ex, stmts, consequent],
         el: lineno,
         ec: char_pos
       )
@@ -1892,12 +1911,21 @@ def on_sclass(target, bodystmt)
   # propagate that onto void_stmt nodes inside the stmts in order to make sure
   # all comments get printed appropriately.
   class Stmts < SimpleDelegator
+    attr_reader :parser
+
+    def initialize(parser, values)
+      @parser = parser
+      __setobj__(values)
+    end
+
     def bind(sc, ec)
       merge!(sc: sc, ec: ec)
 
       if self[:body][0][:type] == :void_stmt
         self[:body][0].merge!(sc: sc, ec: sc)
       end
+
+      attach_comments(sc, ec)
     end
 
     def bind_end(ec)
@@ -1914,6 +1942,22 @@ def <<(statement)
       self[:body] << statement
       self
     end
+
+    private
+
+    def attach_comments(sc, ec)
+      attachable =
+        parser.comments.select do |comment|
+          comment[:type] == :@comment && !comment[:inline] &&
+            sc <= comment[:sc] && ec >= comment[:ec] &&
+            !comment[:value].include?('prettier-ignore')
+        end
+
+      return if attachable.empty?
+
+      parser.comments -= attachable
+      self[:body] = (self[:body] + attachable).sort_by! { |node| node[:sc] }
+    end
   end
 
   # stmts_new is a parser event that represents the beginning of a list of
@@ -1921,6 +1965,7 @@ def <<(statement)
   # stmts_add events, which we'll append onto an array body.
   def on_stmts_new
     Stmts.new(
+      self,
       type: :stmts,
       body: [],
       sl: lineno,
diff --git a/src/ruby/printer.js b/src/ruby/printer.js
--- a/src/ruby/printer.js
+++ b/src/ruby/printer.js
@@ -54,8 +54,6 @@ function getCommentChildNodes(node) {
   switch (node.type) {
     case "heredoc":
       return [node.beging];
-    case "rescue":
-      return [].concat(node.body[0]).concat(node.body.slice(1));
     case "aryptn":
       return [node.body[0]]
         .concat(node.body[1])


Test patch:
diff --git a/test/js/ruby/layout.test.js b/test/js/ruby/layout.test.js
--- a/test/js/ruby/layout.test.js
+++ b/test/js/ruby/layout.test.js
@@ -1,3 +1,5 @@
+const { ruby } = require("../utils");
+
 describe("layout", () => {
   test("turns multiple blank lines into just one blank line", () =>
     expect("1\n\n\n\n\n2").toChangeFormat("1\n\n2"));
@@ -10,4 +12,26 @@ describe("layout", () => {
 
   test("handles multiple newlines at the end of the file", () =>
     expect("foo\n\n\n").toChangeFormat("foo"));
+
+  test("keeps comments in their place when nothing to attach to", () => {
+    const content = ruby(`
+      # Comment
+      def f
+        print 1
+      end
+      
+      # Comment
+      # Comment
+      
+      def g
+        print 2
+      end
+      
+      # Comment
+      # Comment
+      # Comment
+    `);
+
+    return expect(content).toMatchFormat();
+  });
 });
diff --git a/test/js/ruby/nodes/blocks.test.js b/test/js/ruby/nodes/blocks.test.js
--- a/test/js/ruby/nodes/blocks.test.js
+++ b/test/js/ruby/nodes/blocks.test.js
@@ -129,7 +129,4 @@ describe("blocks", () => {
     test("does not split up args inside pipes", () =>
       expect(`loop do |${long} = 1, a${long} = 2|\nend`).toMatchFormat());
   });
-
-  test("leaves sorbet type annotations in place", () =>
-    expect(`sig { ${long} }`).toMatchFormat());
 });
diff --git a/test/js/ruby/nodes/rescue.test.js b/test/js/ruby/nodes/rescue.test.js
--- a/test/js/ruby/nodes/rescue.test.js
+++ b/test/js/ruby/nodes/rescue.test.js
@@ -110,6 +110,30 @@ describe("rescue", () => {
     return expect(content).toMatchFormat();
   });
 
+  test("comment inline with multiple", () => {
+    const content = ruby(`
+      begin
+        foo
+      rescue Foo, Bar # foo
+        bar
+      end
+    `);
+
+    return expect(content).toMatchFormat();
+  });
+
+  test("comment inline with splat", () => {
+    const content = ruby(`
+      begin
+        foo
+      rescue Foo, *Bar # foo
+        bar
+      end
+    `);
+
+    return expect(content).toMatchFormat();
+  });
+
   test("one error with a comment", () => {
     const content = ruby(`
       begin
diff --git a/test/js/ruby/nodes/strings.test.js b/test/js/ruby/nodes/strings.test.js
--- a/test/js/ruby/nodes/strings.test.js
+++ b/test/js/ruby/nodes/strings.test.js
@@ -105,14 +105,20 @@ describe("strings", () => {
     test("very interpolated", () =>
       expect(`"abc #{"abc #{abc} abc"} abc"`).toMatchFormat());
 
-    test("breaks interpolation on #{ ... } and not some inner node", () =>
-      expect(`"${long} #{foo[:bar]} ${long}"`).toChangeFormat(
-        ruby(`
-          "${long} #{
-            foo[:bar]
-          } ${long}"
-        `)
-      ));
+    test("long strings with interpolation do not break", () =>
+      expect(`"${long} #{foo[:bar]} ${long}"`).toMatchFormat());
+
+    test("long strings with interpolation that were broken do break", () => {
+      const content = ruby(`
+        <<~HERE
+          #{
+          ${long}
+        }
+        HERE
+      `);
+
+      return expect(content).toMatchFormat();
+    });
 
     test("within a heredoc there is no indentation", () => {
       const content = ruby(`