Wrap parentheses when necessary

Author: kddnewton

bin/test

@@ -67,19 +67,16 @@ do_block.rb:4
 dyna_symbol.rb:0
 dyna_symbol.rb:4
 elsif.rb:0
-elsif.rb:2
 elsif.rb:3
 embdoc.rb:0
 embdoc.rb:1
 end_content.rb:0
 heredoc.rb:17
 heredoc.rb:18
 if.rb:3
-if.rb:4
 if.rb:7
 if.rb:10
 if.rb:13
-if_mod.rb:3
 ifop.rb:1
 ifop.rb:3
 label.rb:1
@@ -104,11 +101,7 @@ string_concat.rb:0
 string_embexpr.rb:5
 string_literal.rb:15
 string_literal.rb:16
-unless.rb:4
 unless.rb:7
 unless.rb:8
-unless_mod.rb:3
-until.rb:3
 when.rb:7
 when.rb:8
-while.rb:3

lib/syntax_tree/prism/formatter.rb

@@ -470,5 +470,56 @@ def format_write(operator_loc, value)
         end
       end
     end
+
+    # If you have a modifier statement (for instance a modifier if statement or
+    # a modifier while loop) there are times when you need to wrap the entire
+    # statement in parentheses. This occurs when you have something like:
+    #
+    #     foo[:foo] =
+    #       if bar?
+    #         baz
+    #       end
+    #
+    # Normally we would shorten this to an inline version, which would result in:
+    #
+    #     foo[:foo] = baz if bar?
+    #
+    # but this actually has different semantic meaning. The first example will
+    # result in a nil being inserted into the hash for the :foo key, whereas the
+    # second example will result in an empty hash because the if statement
+    # applies to the entire assignment.
+    #
+    # We can fix this in a couple of ways. We can use the then keyword, as in:
+    #
+    #     foo[:foo] = if bar? then baz end
+    #
+    # But this isn't used very often. We can also just leave it as is with the
+    # multi-line version, but for a short predicate and short value it looks
+    # verbose. The last option and the one used here is to add parentheses on
+    # both sides of the expression, as in:
+    #
+    #     foo[:foo] = (baz if bar?)
+    #
+    # This approach maintains the nice conciseness of the inline version, while
+    # keeping the correct semantic meaning.
+    def format_flat_parentheses
+      case parent&.type
+      when :arguments_node, 
+            :call_and_write_node, :call_or_write_node, :call_operator_write_node,
+            :class_variable_write_node, :class_variable_and_write_node, :class_variable_or_write_node, :class_variable_operator_write_node,
+            :constant_write_node, :constant_and_write_node, :constant_or_write_node, :constant_operator_write_node,
+            :constant_path_write_node, :constant_path_and_write_node, :constant_path_or_write_node, :constant_path_operator_write_node,
+            :global_variable_write_node, :global_variable_and_write_node, :global_variable_or_write_node, :global_variable_operator_write_node,
+            :instance_variable_write_node, :instance_variable_and_write_node, :instance_variable_or_write_node, :instance_variable_operator_write_node,
+            :local_variable_write_node, :local_variable_and_write_node, :local_variable_or_write_node, :local_variable_operator_write_node,
+            :multi_write_node,
+            :assoc_node, :call_node, :defined_node
+        text("(")
+        yield
+        text(")")
+      else
+        yield
+      end
+    end
   end
 end

lib/syntax_tree/prism/nodes.rb

@@ -1551,6 +1551,11 @@ def format(q)
               q.format(statements)
             end
           end
+
+          if consequent
+            q.breakable_force
+            q.format(consequent)
+          end
         end
       elsif !if_keyword_loc
         q.group do
@@ -1560,57 +1565,58 @@ def format(q)
           q.text(" : ")
           q.format(consequent.statements.body.first)
         end
-      elsif statements.nil? || predicate.contains_write?
+      elsif statements.nil?
         q.group do
-          q.loc(if_keyword_loc)
-          q.text(" ")
-          q.nest(3) { q.format(predicate) }
-
-          if statements
-            q.indent do
-              q.breakable_force
-              q.format(statements)
-            end
-          end
-
-          if consequent
-            q.breakable_force
-            q.format(consequent)
+          format_break(q)
+          q.break_parent
+        end
+      elsif predicate.contains_write? || statements.contains_write?
+        if end_keyword_loc
+          q.group do
+            format_break(q)
+            q.break_parent
           end
-
-          q.breakable_force
-          q.text("end")
+        else
+          q.group { format_flat(q) }
         end
       else
         q.group do
           q
-            .if_break do
-              q.group do
-                q.loc(if_keyword_loc)
-                q.text(" ")
-                q.nest(3) { q.format(predicate) }
-              end
+            .if_break { format_break(q) }
+            .if_flat { q.format_flat_parentheses { format_flat(q) } }
+        end
+      end
+    end
 
-              q.indent do
-                q.breakable_space
-                q.format(statements)
-              end
+    private
 
-              if consequent
-                q.breakable_space
-                q.format(consequent)
-              end
+    def format_break(q)
+      q.group do
+        q.loc(if_keyword_loc)
+        q.text(" ")
+        q.nest(3) { q.format(predicate) }
+      end
 
-              q.breakable_space
-              q.text("end")
-            end
-            .if_flat do
-              q.format(statements)
-              q.text(" if ")
-              q.format(predicate)
-            end
+      if statements
+        q.indent do
+          q.breakable_space
+          q.format(statements)
         end
       end
+
+      if consequent
+        q.breakable_space
+        q.format(consequent)
+      end
+
+      q.breakable_space
+      q.text("end")
+    end
+
+    def format_flat(q)
+      q.format(statements)
+      q.text(" if ")
+      q.format(predicate)
     end
   end
 
@@ -2939,55 +2945,58 @@ def format(q)
       statements = self.statements
       consequent = self.consequent
 
-      if !statements || consequent || predicate.contains_write?
+      if !statements || consequent
         q.group do
-          q.text("unless ")
-          q.nest(3) { q.format(predicate) }
-
-          if statements
-            q.indent do
-              q.breakable_force
-              q.format(statements)
-            end
-          end
-
-          if consequent
-            q.breakable_force
-            q.format(consequent)
+          format_break(q)
+          q.break_parent
+        end
+      elsif predicate.contains_write? || statements.contains_write?
+        if end_keyword_loc
+          q.group do
+            format_break(q)
+            q.break_parent
           end
-
-          q.breakable_force
-          q.text("end")
+        else
+          q.group { format_flat(q) }
         end
       else
         q.group do
           q
-            .if_break do
-              q.group do
-                q.text("unless ")
-                q.nest(3) { q.format(predicate) }
-              end
+            .if_break { format_break(q) }
+            .if_flat { q.format_flat_parentheses { format_flat(q) } }
+        end
+      end
+    end
 
-              q.indent do
-                q.breakable_space
-                q.format(statements)
-              end
+    private
 
-              if consequent
-                q.breakable_space
-                q.format(consequent)
-              end
+    def format_break(q)
+      q.group do
+        q.loc(keyword_loc)
+        q.text(" ")
+        q.nest(3) { q.format(predicate) }
+      end
 
-              q.breakable_space
-              q.text("end")
-            end
-            .if_flat do
-              q.format(statements)
-              q.text(" unless ")
-              q.format(predicate)
-            end
+      if statements
+        q.indent do
+          q.breakable_space
+          q.format(statements)
         end
       end
+
+      if consequent
+        q.breakable_space
+        q.format(consequent)
+      end
+
+      q.breakable_space
+      q.text("end")
+    end
+
+    def format_flat(q)
+      q.format(statements)
+      q.text(" unless ")
+      q.format(predicate)
     end
   end
 
@@ -3001,13 +3010,22 @@ class UntilNode
     #     until foo do bar end
     #     ^^^^^^^^^^^^^^^^^^^^
     def format(q)
-      if begin_modifier? || statements&.contains_write?
+      if begin_modifier?
         q.group { format_flat(q) }
-      elsif statements.nil? || keyword_loc.comments.any? || closing_loc&.comments&.any? || predicate.contains_write?
+      elsif statements.nil? || keyword_loc.comments.any? || closing_loc&.comments&.any?
         q.group do
           format_break(q)
           q.break_parent
         end
+      elsif predicate.contains_write? || statements.contains_write?
+        if closing_loc
+          q.group do
+            format_break(q)
+            q.break_parent
+          end
+        else
+          q.group { format_flat(q) }
+        end
       else
         q.group { q.if_break { format_break(q) }.if_flat { format_flat(q) } }
       end
@@ -3025,9 +3043,11 @@ def format_break(q)
     end
 
     def format_flat(q)
-      q.format(statements)
-      q.text(" until ")
-      q.format(predicate)
+      q.format_flat_parentheses do
+        q.format(statements)
+        q.text(" until ")
+        q.format(predicate)
+      end
     end
   end
 
@@ -3069,13 +3089,22 @@ class WhileNode
     #     while foo do bar end
     #     ^^^^^^^^^^^^^^^^^^^^
     def format(q)
-      if begin_modifier? || statements&.contains_write?
+      if begin_modifier?
         q.group { format_flat(q) }
-      elsif statements.nil? || keyword_loc.comments.any? || closing_loc&.comments&.any? || predicate.contains_write?
+      elsif statements.nil? || keyword_loc.comments.any? || closing_loc&.comments&.any?
         q.group do
           format_break(q)
           q.break_parent
         end
+      elsif predicate.contains_write? || statements.contains_write?
+        if closing_loc
+          q.group do
+            format_break(q)
+            q.break_parent
+          end
+        else
+          q.group { format_flat(q) }
+        end
       else
         q.group { q.if_break { format_break(q) }.if_flat { format_flat(q) } }
       end
@@ -3093,9 +3122,11 @@ def format_break(q)
     end
 
     def format_flat(q)
-      q.format(statements)
-      q.text(" while ")
-      q.format(predicate)
+      q.format_flat_parentheses do
+        q.format(statements)
+        q.text(" while ")
+        q.format(predicate)
+      end
     end
   end