Merge pull request #1369 from eregon/desugaring-fixes

Desugaring fixes

bin/parse

@@ -16,4 +16,5 @@ else
 end
 
 result.mark_newlines if ENV['MARK_NEWLINES']
+result = result.value.accept(YARP::DesugarVisitor.new) if ENV['DESUGAR']
 pp result

config.yml

@@ -1221,10 +1221,10 @@ nodes:
     fields:
       - name: name_loc
         type: location
-      - name: operator_loc
-        type: location
       - name: value
         type: node
+      - name: operator_loc
+        type: location
     comment: |
       Represents writing to a global variable.
 
@@ -1595,10 +1595,10 @@ nodes:
         type: constant
       - name: depth
         type: uint32
-      - name: value
-        type: node
       - name: name_loc
         type: location
+      - name: value
+        type: node
       - name: operator_loc
         type: location
     comment: |

lib/yarp/desugar_visitor.rb

@@ -42,7 +42,7 @@ def visit_constant_and_write_node(node)
     #
     # becomes
     #
-    # Foo || Foo = bar
+    # defined?(Foo) ? Foo : Foo = bar
     def visit_constant_or_write_node(node)
       desugar_or_write_defined_node(node, ConstantReadNode, ConstantWriteNode)
     end
@@ -56,69 +56,6 @@ def visit_constant_operator_write_node(node)
       desugar_operator_write_node(node, ConstantReadNode, ConstantWriteNode)
     end
 
-    # Foo::Bar &&= baz
-    #
-    # becomes
-    #
-    # Foo::Bar && Foo::Bar = baz
-    def visit_constant_path_and_write_node(node)
-      AndNode.new(
-        node.target,
-        ConstantPathWriteNode.new(node.target, node.value, node.operator_loc, node.location),
-        node.operator_loc,
-        node.location
-      )
-    end
-
-    # Foo::Bar ||= baz
-    #
-    # becomes
-    #
-    # Foo::Bar || Foo::Bar = baz
-    def visit_constant_path_or_write_node(node)
-      IfNode.new(
-        node.operator_loc,
-        DefinedNode.new(nil, node.target, nil, node.operator_loc, node.target.location),
-        StatementsNode.new([node.target], node.location),
-        ElseNode.new(
-          node.operator_loc,
-          StatementsNode.new(
-            [ConstantPathWriteNode.new(node.target, node.value, node.operator_loc, node.location)],
-            node.location
-          ),
-          node.operator_loc,
-          node.location
-        ),
-        node.operator_loc,
-        node.location
-      )
-    end
-
-    # Foo::Bar += baz
-    #
-    # becomes
-    #
-    # Foo::Bar = Foo::Bar + baz
-    def visit_constant_path_operator_write_node(node)
-      ConstantPathWriteNode.new(
-        node.target,
-        CallNode.new(
-          node.target,
-          nil,
-          node.operator_loc.copy(length: node.operator_loc.length - 1),
-          nil,
-          ArgumentsNode.new([node.value], node.value.location),
-          nil,
-          nil,
-          0,
-          node.operator_loc.slice.chomp("="),
-          node.location
-        ),
-        node.operator_loc.copy(start_offset: node.operator_loc.end_offset - 1, length: 1),
-        node.location
-      )
-    end
-
     # $foo &&= bar
     #
     # becomes
@@ -132,7 +69,7 @@ def visit_global_variable_and_write_node(node)
     #
     # becomes
     #
-    # $foo || $foo = bar
+    # defined?($foo) ? $foo : $foo = bar
     def visit_global_variable_or_write_node(node)
       desugar_or_write_defined_node(node, GlobalVariableReadNode, GlobalVariableWriteNode)
     end
@@ -244,7 +181,7 @@ def desugar_or_write_node(node, read_class, write_class, arguments: [])
       )
     end
 
-    # Don't desugar `x ||= y` to `defined?(x) ? x : x = y`
+    # Desugar `x ||= y` to `defined?(x) ? x : x = y`
     def desugar_or_write_defined_node(node, read_class, write_class, arguments: [])
       IfNode.new(
         node.operator_loc,

test/yarp/desugar_visitor_test.rb

@@ -6,7 +6,7 @@ module YARP
   class DesugarVisitorTest < TestCase
     def test_and_write
       assert_desugars("(AndNode (ClassVariableReadNode) (ClassVariableWriteNode (CallNode)))", "@@foo &&= bar")
-      assert_desugars("(AndNode (ConstantPathNode (ConstantReadNode) (ConstantReadNode)) (ConstantPathWriteNode (ConstantPathNode (ConstantReadNode) (ConstantReadNode)) (CallNode)))", "Foo::Bar &&= baz")
+      assert_not_desugared("Foo::Bar &&= baz", "Desugaring would execute Foo twice or need temporary variables")
       assert_desugars("(AndNode (ConstantReadNode) (ConstantWriteNode (CallNode)))", "Foo &&= bar")
       assert_desugars("(AndNode (GlobalVariableReadNode) (GlobalVariableWriteNode (CallNode)))", "$foo &&= bar")
       assert_desugars("(AndNode (InstanceVariableReadNode) (InstanceVariableWriteNode (CallNode)))", "@foo &&= bar")
@@ -16,7 +16,7 @@ def test_and_write
 
     def test_or_write
       assert_desugars("(IfNode (DefinedNode (ClassVariableReadNode)) (StatementsNode (ClassVariableReadNode)) (ElseNode (StatementsNode (ClassVariableWriteNode (CallNode)))))", "@@foo ||= bar")
-      assert_desugars("(IfNode (DefinedNode (ConstantPathNode (ConstantReadNode) (ConstantReadNode))) (StatementsNode (ConstantPathNode (ConstantReadNode) (ConstantReadNode))) (ElseNode (StatementsNode (ConstantPathWriteNode (ConstantPathNode (ConstantReadNode) (ConstantReadNode)) (CallNode)))))", "Foo::Bar ||= baz")
+      assert_not_desugared("Foo::Bar ||= baz", "Desugaring would execute Foo twice or need temporary variables")
       assert_desugars("(IfNode (DefinedNode (ConstantReadNode)) (StatementsNode (ConstantReadNode)) (ElseNode (StatementsNode (ConstantWriteNode (CallNode)))))", "Foo ||= bar")
       assert_desugars("(IfNode (DefinedNode (GlobalVariableReadNode)) (StatementsNode (GlobalVariableReadNode)) (ElseNode (StatementsNode (GlobalVariableWriteNode (CallNode)))))", "$foo ||= bar")
       assert_desugars("(OrNode (InstanceVariableReadNode) (InstanceVariableWriteNode (CallNode)))", "@foo ||= bar")
@@ -26,7 +26,7 @@ def test_or_write
 
     def test_operator_write
       assert_desugars("(ClassVariableWriteNode (CallNode (ClassVariableReadNode) (ArgumentsNode (CallNode))))", "@@foo += bar")
-      assert_desugars("(ConstantPathWriteNode (ConstantPathNode (ConstantReadNode) (ConstantReadNode)) (CallNode (ConstantPathNode (ConstantReadNode) (ConstantReadNode)) (ArgumentsNode (CallNode))))", "Foo::Bar += baz")
+      assert_not_desugared("Foo::Bar += baz", "Desugaring would execute Foo twice or need temporary variables")
       assert_desugars("(ConstantWriteNode (CallNode (ConstantReadNode) (ArgumentsNode (CallNode))))", "Foo += bar")
       assert_desugars("(GlobalVariableWriteNode (CallNode (GlobalVariableReadNode) (ArgumentsNode (CallNode))))", "$foo += bar")
       assert_desugars("(InstanceVariableWriteNode (CallNode (InstanceVariableReadNode) (ArgumentsNode (CallNode))))", "@foo += bar")
@@ -51,9 +51,36 @@ def ast_inspect(node)
       "(#{parts.join(" ")})"
     end
 
+    # Ensure every node is only present once in the AST.
+    # If the same node is present twice it would most likely indicate it is executed twice, which is invalid semantically.
+    # This also acts as a sanity check that Node#child_nodes returns only nodes or nil (which caught a couple bugs).
+    class EnsureEveryNodeOnceInAST < Visitor
+      def initialize
+        @all_nodes = {}.compare_by_identity
+      end
+
+      def visit(node)
+        if node
+          if @all_nodes.include?(node)
+            raise "#{node.inspect} is present multiple times in the desugared AST and likely executed multiple times"
+          else
+            @all_nodes[node] = true
+          end
+        end
+        super(node)
+      end
+    end
+
     def assert_desugars(expected, source)
       ast = YARP.parse(source).value.accept(DesugarVisitor.new)
       assert_equal expected, ast_inspect(ast.statements.body.last)
+
+      ast.accept(EnsureEveryNodeOnceInAST.new)
+    end
+
+    def assert_not_desugared(source, reason)
+      ast = YARP.parse(source).value
+      assert_equal_nodes(ast, ast.accept(DesugarVisitor.new))
     end
   end
 end

test/yarp/errors_test.rb

@@ -513,16 +513,16 @@ def test_cannot_assign_to_a_reserved_numbered_parameter
       expected = BeginNode(
         Location(),
         StatementsNode([
-          LocalVariableWriteNode(:_1, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_2, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_3, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_4, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_5, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_6, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_7, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_8, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_9, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location()),
-          LocalVariableWriteNode(:_10, 0, SymbolNode(Location(), Location(), nil, "a"), Location(), Location())
+          LocalVariableWriteNode(:_1, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_2, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_3, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_4, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_5, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_6, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_7, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_8, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_9, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location()),
+          LocalVariableWriteNode(:_10, 0, Location(), SymbolNode(Location(), Location(), nil, "a"), Location())
         ]),
         nil,
         nil,
@@ -1003,8 +1003,8 @@ def test_dont_allow_setting_to_back_and_nth_reference
       expected = BeginNode(
         Location(),
         StatementsNode([
-          GlobalVariableWriteNode(Location(), Location(), NilNode()),
-          GlobalVariableWriteNode(Location(), Location(), NilNode())
+          GlobalVariableWriteNode(Location(), NilNode(), Location()),
+          GlobalVariableWriteNode(Location(), NilNode(), Location())
         ]),
         nil,
         nil,