Do not desugar Foo::Bar {||,&&,+}= baz as it is incorrect without a temporary variable

* See #1329 (comment) for details.

Author: eregon

lib/yarp/desugar_visitor.rb

@@ -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
-    #
-    # defined?(Foo::Bar) ? 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

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")
@@ -55,5 +55,10 @@ def assert_desugars(expected, source)
       ast = YARP.parse(source).value.accept(DesugarVisitor.new)
       assert_equal expected, ast_inspect(ast.statements.body.last)
     end
+
+    def assert_not_desugared(source, reason)
+      ast = YARP.parse(source).value
+      assert_equal_nodes(ast, ast.accept(YARP::DesugarVisitor.new))
+    end
   end
 end