From 6b6d3375e6c9448c38e92265eeb296d755d263b2 Mon Sep 17 00:00:00 2001
From: Csaba Henk <csaba@lowlife.hu>
Date: Sat, 1 Jul 2023 23:34:32 +0200
Subject: [PATCH] formatting fixes for YARP

Following issues are addressed:

---
"constant misnomer (has: AssocSplat, should have: AssocSplatNode) brings on formatting failure": "{a => b}"
hash label key duplicated: "{a: b}"
identity hash formatting failure: "{a:}"
mixed hash content loss: "{a => b, c: d}"
funcall with kwargs gets bogus '=>': "fun(a: b)"
"empty #call formatting failure": fun[]
"#-@ appended to arg": "-1"
empty funcall formatting failure: fun()
empty super call formatting failure: super()
ternary conditional formatting failure: "a ? b : c"
incorrect postfix unless for compound statements: unless a; b; c; end
AlternationPatternNode: foo => bar | baz
CallOperatorAndWriteNode: foo.bar &&= value
CallOperatorOrWriteNode: foo.bar ||= value
ClassVariableOperatorAndWriteNode: "@@target &&= value"
ClassVariableOperatorOrWriteNode: "@@target ||= value"
ClassVariableOperatorWriteNode: "@@target += value"
ConstantOperatorAndWriteNode: Target &&= value
ConstantOperatorOrWriteNode: Target ||= value
ConstantOperatorWriteNode: Target += value
ConstantPathOperatorWriteNode: Parent::Child += value
ConstantPathOperatorAndWriteNode: Parent::Child &&= value
ConstantPathOperatorOrWriteNode: Parent::Child ||= value
GlobalVariableOperatorAndWriteNode: $target &&= value
GlobalVariableOperatorOrWriteNode: $target ||= value
GlobalVariableOperatorWriteNode: $target += value
InstanceVariableOperatorAndWriteNode: "@target &&= value"
InstanceVariableOperatorOrWriteNode: "@target ||= value"
InstanceVariableOperatorWriteNode: "@target += value"
KeywordHashNode: "[**{}]"
LocalVariableOperatorAndWriteNode: target &&= value
LocalVariableOperatorOrWriteNode: target ||= value
OrNode: left or right
RequiredDestructuredParameterNode: def foo((bar, baz)); end
SourceFileNode: __FILE__
SourceLineNode: __LINE__
SourceEncodingNode: __ENCODING__

Notes:

- Entries associated with a YARP Node class indicate a formatting
  failure due to #format method not being implemented for given
  class; so in these cases the fix boils down to implementing format.
- For these entries the sample code that's given in the entry is the
  sample code from the YARP definition (config.yml), with one exception:
  the sample code for KeywordHashNode is 'foo(a: b)', but for that
  expression the formatting code preempts winding down to the
  KeywordHashNode instance, so the lack of KeywordHashNode#format is
  not triggered. The expression given for KeywordHashNode does indeed
  trigger KeywordHashNode#format. (Ironically, 'foo(a: b)' still had
  a formatting issue, but for a different reason.)
- The aforementioned class tagged entries and the ones where
  "formatting failure" is indicated raise an exception with
  SyntaxTree::Formatter.format; the other cases do return but with a
  bogus result.
- The hash related issues (ones where the word 'hash' occurs in the label,
  entries 2. to 4.) are shadowed by the first one (constant misnomer).
  Without fixing the name these entries will also end up with an exception.
  When the constant name is fixed, the indicated issue will be present.

Tested with YARP 0eb5f7c3.
---
 lib/syntax_tree/formatting.rb | 529 +++++++++++++++++++++++-----------
 1 file changed, 364 insertions(+), 165 deletions(-)

diff --git a/lib/syntax_tree/formatting.rb b/lib/syntax_tree/formatting.rb
index e416a04d..ea34ed26 100644
--- a/lib/syntax_tree/formatting.rb
+++ b/lib/syntax_tree/formatting.rb
@@ -174,8 +174,6 @@ class Labels
       def format_key(q, key)
         case key
         when SymbolNode
-          q.slice(key.value_loc)
-          q.text(":")
         when InterpolatedSymbolNode
           
         else
@@ -237,7 +235,8 @@ class Identity
       def format_key(q, key)
         case key
         when SymbolNode
-
+          q.slice(key.value_loc)
+          q.text(":")
         when InterpolatedSymbolNode
 
         else
@@ -247,47 +246,80 @@ def format_key(q, key)
       end
     end
 
-    def self.for(q, node)
-      node.elements.each do |element|
-        if element.is_a?(AssocSplatNode)
-          # Splat nodes do not impact the formatting choice.
-        elsif element.value.nil?
-          # If the value is nil, then it has been omitted. In this case we have
-          # to match the existing formatting because standardizing would
-          # potentially break the code. For example:
-          #
-          #     { first:, "second" => "value" }
-          #
-          return Identity.new
+    def self.for(q, element)
+      if element.is_a?(AssocSplatNode)
+        # Splat nodes do not impact the formatting choice.
+      elsif element.value.nil?
+        # If the value is nil, then it has been omitted. In this case we have
+        # to match the existing formatting because standardizing would
+        # potentially break the code. For example:
+        #
+        #     { first:, "second" => "value" }
+        #
+        return Identity.new
+      else
+        # Otherwise, we need to check the type of the key. If it's a label or
+        # dynamic symbol, we can use labels. If it's a symbol literal then it
+        # needs to match a certain pattern to be used as a label. If it's
+        # anything else, then we need to use hash rockets.
+        case (key = element.key)
+        when SymbolNode
+          if key.opening_loc.nil? && !key.closing_loc.nil?
+            # Here it's a label.
+          else
+            # Otherwise we need to check the actual value of the symbol to see
+            # if it would work as a label.
+            value_loc = key.value_loc
+            value = q.source.byteslice(value_loc.start_offset...value_loc.end_offset)
+            return Rockets.new if !value.match?(/^[_A-Za-z]/) || value.end_with?("=")
+          end
+        when InterpolatedSymbolNode
+          # Labels can be used if this is an interpolated symbol that begins
+          # with a :. If not, then it's a %s symbol so we'll use hash rockets.
+          return Rockets.new if q.source.byteslice(key.opening_loc, 1) != ":"
         else
-          # Otherwise, we need to check the type of the key. If it's a label or
-          # dynamic symbol, we can use labels. If it's a symbol literal then it
-          # needs to match a certain pattern to be used as a label. If it's
-          # anything else, then we need to use hash rockets.
-          case (key = element.key)
-          when SymbolNode
-            if key.opening_loc.nil? && !key.closing_loc.nil?
-              # Here it's a label.
+          # Otherwise, we need to use hash rockets.
+          return Rockets.new
+        end
+      end
+
+      Labels.new
+    end
+  end
+
+  module HashFormatter
+
+    def format(q)
+      delims = delimiters()
+      q.group do
+        q.text(delims[0]) if delims
+        q.indent do
+          q.breakable_space
+
+          q.seplist(elements) do |element|
+            if element.is_a?(AssocSplatNode)
+              q.format(element)
             else
-              # Otherwise we need to check the actual value of the symbol to see
-              # if it would work as a label.
-              value_loc = key.value_loc
-              value = q.source.byteslice(value_loc.start_offset...value_loc.end_offset)
-              return Rockets.new if !value.match?(/^[_A-Za-z]/) || value.end_with?("=")
+              HashKeyFormatter.for(q, element).format_key(q, element.key)
+              q.indent do
+                q.breakable_space
+                q.format(element.value) if element.value
+              end
             end
-          when InterpolatedSymbolNode
-            # Labels can be used if this is an interpolated symbol that begins
-            # with a :. If not, then it's a %s symbol so we'll use hash rockets.
-            return Rockets.new if q.source.byteslice(key.opening_loc, 1) != ":"
-          else
-            # Otherwise, we need to use hash rockets.
-            return Rockets.new
           end
         end
+        q.breakable_space
+        q.text(delims[1]) if delims
       end
+    end
 
-      Labels.new
+    private
+
+    # return delimiter pair or nil
+    def delimiters
+      raise NotImplementedError
     end
+
   end
 
   module LiteralNode
@@ -349,6 +381,89 @@ def format(q)
     end
   end
 
+  module BinaryOperationBaseFormatter
+    def format(q)
+      q.group do
+        lhs(q)
+        q.text(" ")
+        q.slice(operator_loc)
+
+        q.indent do
+          q.breakable_space
+          rhs(q)
+        end
+      end
+    end
+
+    private
+
+    def lhs(q)
+      raise NotImplementedError
+    end
+
+    def rhs(q)
+      raise NotImplementedError
+    end
+
+  end
+
+  module AndOrFormatter
+    include BinaryOperationBaseFormatter
+
+    private
+
+    def lhs(q)
+      q.format(left)
+    end
+
+    def rhs(q)
+      q.format(right)
+    end
+  end
+
+  module BinaryOperationFormatter
+    include BinaryOperationBaseFormatter
+
+    private
+
+    def rhs(q)
+      q.format(value)
+    end
+  end
+
+  module CallOperationFormatter
+    include BinaryOperationFormatter
+
+    private
+
+    def lhs(q)
+      q.format(target)
+    end
+  end
+
+  module VariableOperationFormatter
+    include BinaryOperationFormatter
+
+    private
+
+    def lhs(q)
+      q.slice(name_loc)
+    end
+  end
+
+  module MetaConstantFormatter
+     def format(q)
+       q.text("__#{metaname()}__")
+     end
+
+     private
+
+     def metaname
+       raise NotImplementedError
+     end
+  end
+
+
   class Node
     def comments
       []
@@ -380,21 +495,20 @@ def format_name(q, name)
     end
   end
 
-  class AndNode
+  class AlternationPatternNode
     def format(q)
       q.group do
         q.format(left)
-        q.text(" ")
-        q.slice(operator_loc)
-
-        q.indent do
-          q.breakable_space
-          q.format(right)
-        end
+        q.text(" | ")
+        q.format(right)
       end
     end
   end
 
+  class AndNode
+    include AndOrFormatter
+  end
+
   class ArgumentsNode
     def format(q)
       parts = []
@@ -461,7 +575,7 @@ def format(q)
         q.format(key)
 
         if value
-          q.text(" =>")
+          q.text(" =>") unless key.is_a? SymbolNode
           q.indent do
             q.breakable_space
             q.format(value)
@@ -612,12 +726,17 @@ def format(q)
 
           q.indent do
             q.breakable_empty
-            q.format(arguments)
+            q.format(arguments) if arguments
           end
 
           q.breakable_empty
           q.text("]")
         end
+      when "-@"
+        q.group do
+          q.text("-")
+          q.format(receiver)
+        end
       else
         if opening_loc
           q.format(receiver) if receiver
@@ -628,7 +747,7 @@ def format(q)
             q.slice(opening_loc)
             q.indent do
               q.breakable_empty
-              q.format(arguments)
+              q.format(arguments) if arguments
             end
             q.breakable_empty
             q.slice(closing_loc)
@@ -662,19 +781,16 @@ def format(q)
     end
   end
 
-  class CallOperatorWriteNode
-    def format(q)
-      q.group do
-        q.format(target)
-        q.text(" ")
-        q.slice(operator_loc)
+  class CallOperatorAndWriteNode
+    include CallOperationFormatter
+  end
 
-        q.indent do
-          q.breakable_space
-          q.format(value)
-        end
-      end
-    end
+  class CallOperatorOrWriteNode
+    include CallOperationFormatter
+  end
+
+  class CallOperatorWriteNode
+    include CallOperationFormatter
   end
 
   class CapturePatternNode
@@ -739,6 +855,18 @@ def format(q)
     end
   end
 
+  class ClassVariableOperatorAndWriteNode
+    include VariableOperationFormatter
+  end
+
+  class ClassVariableOperatorOrWriteNode
+    include VariableOperationFormatter
+  end
+
+  class ClassVariableOperatorWriteNode
+    include VariableOperationFormatter
+  end
+
   class ClassVariableReadNode
     include LiteralNode
   end
@@ -759,6 +887,18 @@ def format(q)
     end
   end
 
+  class ConstantOperatorAndWriteNode
+    include VariableOperationFormatter
+  end
+
+  class ConstantOperatorOrWriteNode
+    include VariableOperationFormatter
+  end
+
+  class ConstantOperatorWriteNode
+    include VariableOperationFormatter
+  end
+
   class ConstantPathNode
     def format(q)
       q.format(parent) if parent
@@ -767,6 +907,18 @@ def format(q)
     end
   end
 
+  class ConstantPathOperatorWriteNode
+    include CallOperationFormatter
+  end
+
+  class ConstantPathOperatorAndWriteNode
+    include CallOperationFormatter
+  end
+
+  class ConstantPathOperatorOrWriteNode
+    include CallOperationFormatter
+  end
+
   class ConstantPathWriteNode
     def format(q)
       q.group do
@@ -855,12 +1007,22 @@ def format(q)
   class ElseNode
     def format(q)
       q.group do
-        q.text("else")
+        keyword = q.source.byteslice(else_keyword_loc.start_offset...else_keyword_loc.end_offset)
 
         if statements
-          q.indent do
-            q.breakable_force
-            q.format(statements)
+          if keyword == "else"
+            q.text(keyword)
+            q.indent do
+              q.breakable_force
+              q.format(statements)
+            end
+          else # keyword == ":"
+            q.group do
+              q.breakable_space
+              q.text(keyword)
+              q.breakable_space
+              q.format(statements)
+            end
           end
         end
       end
@@ -982,73 +1144,27 @@ def format(q)
     end
   end
 
-  class HashNode
-    def format(q)
-      q.group do
-        q.text("{")
-        q.indent do
-          q.breakable_space
+  class GlobalVariableOperatorAndWriteNode
+    include VariableOperationFormatter
+  end
 
-          formatter = HashKeyFormatter.for(q, self)
-          q.seplist(elements) do |element|
-            if element.is_a?(AssocSplat)
-              q.format(element)
-            else
-              formatter.format_key(q, element.key)
-              q.indent do
-                q.breakable_space
-                q.format(element.value)
-              end
-            end
-          end
-        end
-        q.breakable_space
-        q.text("}")
-      end
-    end
+  class GlobalVariableOperatorOrWriteNode
+    include VariableOperationFormatter
+  end
 
-    private
+  class GlobalVariableOperatorWriteNode
+    include VariableOperationFormatter
+  end
 
-    def key_formatter
-      elements.each do |element|
-        if element.is_a?(AssocSplat)
-          # Splat nodes do not impact the formatting choice.
-        elsif element.value.nil?
-          # If the value is nil, then it has been omitted. In this case we have
-          # to match the existing formatting because standardizing would
-          # potentially break the code. For example:
-          #
-          #     { first:, "second" => "value" }
-          #
-          return Identity.new
-        else
-          # Otherwise, we need to check the type of the key. If it's a label or
-          # dynamic symbol, we can use labels. If it's a symbol literal then it
-          # needs to match a certain pattern to be used as a label. If it's
-          # anything else, then we need to use hash rockets.
-          case assoc.key
-          when InterpolatedSymbolNode
-            # Here labels can be used.
-          when SymbolLiteral
-            # When attempting to convert a hash rocket into a hash label,
-            # you need to take care because only certain patterns are
-            # allowed. Ruby source says that they have to match keyword
-            # arguments to methods, but don't specify what that is. After
-            # some experimentation, it looks like it's:
-            value = assoc.key.value.value
-
-            if !value.match?(/^[_A-Za-z]/) || value.end_with?("=")
-              return Rockets.new
-            end
-          else
-            # If the value is anything else, we have to use hash rockets.
-            return Rockets.new
-          end
-        end
-      end
+  class HashNode
+    include HashFormatter
 
-      Labels.new
+    private
+
+    def delimiters
+      ["{", "}"]
     end
+
   end
 
   class HashPatternNode
@@ -1084,37 +1200,46 @@ def format(q)
 
   class IfNode
     def format(q)
-      keyword = q.source.byteslice(if_keyword_loc.start_offset...if_keyword_loc.end_offset)
+      if if_keyword_loc
+        keyword = q.source.byteslice(if_keyword_loc.start_offset...if_keyword_loc.end_offset)
 
-      if keyword == "if" && q.parent.is_a?(InNode) && q.parent.pattern == self
-        q.group do
-          q.format(statements)
-          q.text(" if ")
-          q.format(predicate)
-        end
-      else
-        q.group do
-          q.text(keyword)
-          q.text(" ")
-          q.nest(3) { q.format(predicate) }
+        if keyword == "if" && q.parent.is_a?(InNode) && q.parent.pattern == self
+          q.group do
+            q.format(statements)
+            q.text(" if ")
+            q.format(predicate)
+          end
+        else
+          q.group do
+            q.text(keyword)
+            q.text(" ")
+            q.nest(3) { q.format(predicate) }
 
-          if statements
-            q.indent do
-              q.breakable_force
-              q.format(statements)
+            if statements
+              q.indent do
+                q.breakable_force
+                q.format(statements)
+              end
             end
-          end
 
-          if consequent
-            q.breakable_force
-            q.format(consequent)
-          end
+            if consequent
+              q.breakable_force
+              q.format(consequent)
+            end
 
-          if keyword == "if"
-            q.breakable_force
-            q.text("end")
+            if keyword == "if"
+              q.breakable_force
+              q.text("end")
+            end
           end
         end
+      else # a ? b : c
+        q.group do
+          q.format(predicate)
+          q.text(" ? ")
+          q.format(statements)
+          q.format(consequent)
+        end
       end
     end
   end
@@ -1143,6 +1268,18 @@ class InstanceVariableReadNode
     include LiteralNode
   end
 
+  class InstanceVariableOperatorAndWriteNode
+    include VariableOperationFormatter
+  end
+
+  class InstanceVariableOperatorOrWriteNode
+    include VariableOperationFormatter
+  end
+
+  class InstanceVariableOperatorWriteNode
+    include VariableOperationFormatter
+  end
+
   class InstanceVariableWriteNode
     def format(q)
       q.group do
@@ -1241,6 +1378,15 @@ def format(q)
     end
   end
 
+  class KeywordHashNode
+    include HashFormatter
+
+    private
+
+    def delimiters
+    end
+  end
+
   class KeywordParameterNode
     def format(q)
       q.group do
@@ -1298,19 +1444,16 @@ def format(q)
     end
   end
 
-  class LocalVariableOperatorWriteNode
-    def format(q)
-      q.group do
-        q.slice(name_loc)
-        q.text(" ")
-        q.slice(operator_loc)
+  class LocalVariableOperatorAndWriteNode
+    include VariableOperationFormatter
+  end
 
-        q.indent do
-          q.breakable_space
-          q.format(value)
-        end
-      end
-    end
+  class LocalVariableOperatorOrWriteNode
+    include VariableOperationFormatter
+  end
+
+  class LocalVariableOperatorWriteNode
+    include VariableOperationFormatter
   end
 
   class LocalVariableReadNode
@@ -1407,6 +1550,10 @@ def format(q)
     end
   end
 
+  class OrNode
+    include AndOrFormatter
+  end
+
   class ParametersNode
     def format(q)
       parts = []
@@ -1538,6 +1685,17 @@ class RequiredParameterNode
     include LiteralNode
   end
 
+  class RequiredDestructuredParameterNode
+    def format(q)
+      q.group do
+        q.slice(opening_loc)
+        q.seplist(parameters) { |parameter| q.format(parameter) }
+        q.slice(closing_loc)
+      end
+    end
+  end
+
+
   class RescueNode
     def format(q)
       q.group do
@@ -1665,6 +1823,36 @@ def format(q)
     end
   end
 
+  class SourceFileNode
+    include MetaConstantFormatter
+
+    private
+
+    def metaname
+      "FILE"
+    end
+  end
+
+  class SourceLineNode
+    include MetaConstantFormatter
+
+    private
+
+    def metaname
+      "LINE"
+    end
+  end
+
+  class SourceEncodingNode
+    include MetaConstantFormatter
+
+    private
+
+    def metaname
+      "ENCODING"
+    end
+  end
+
   class SplatNode
     def format(q)
       q.text("*")
@@ -1700,7 +1888,7 @@ def format(q)
           q.text("super(")
           q.indent do
             q.breakable_empty
-            q.format(arguments)
+            q.format(arguments) if arguments
           end
 
           q.breakable_empty
@@ -1779,9 +1967,20 @@ def format(q)
               q.text("end")
             end
             .if_flat do
-              q.format(statements)
-              q.text(" unless ")
-              q.format(predicate)
+              if statements.body.size == 1
+                q.format(statements)
+                q.text(" unless ")
+                q.format(predicate)
+              else
+                q.text("unless ")
+                q.format(predicate)
+                q.text(";")
+                q.breakable_space
+                q.format(statements)
+                q.text(";")
+                q.breakable_space
+                q.text("end")
+              end
             end
         end
       end