Compiler: left subexpression in += was incorrectly evaluated twice. F…

…ixes #3398
crystal-lang · Oct 10, 2016 · 9eab0f3 · 9eab0f3
1 parent 2be9f09
commit 9eab0f3
Showing 10 changed files with 417 additions and 125 deletions.
diff --git a/spec/compiler/codegen/op_assign_spec.cr b/spec/compiler/codegen/op_assign_spec.cr
@@ -0,0 +1,74 @@
+require "../../spec_helper"
+
+describe "Code gen: op assign" do
+  it "evaluates exps once (#3398)" do
+    run(%(
+      class Global
+        @@value = 0
+
+        def self.value
+          @@value
+        end
+
+        def self.value=(@@value)
+        end
+      end
+
+      class Foo
+        def bar=(bar)
+        end
+
+        def bar
+          0
+        end
+      end
+
+      def foo
+        Global.value += 1
+        Foo.new
+      end
+
+      foo.bar += 2
+
+      Global.value
+      )).to_i.should eq(1)
+  end
+
+  it "evaluates exps once, [] (#3398)" do
+    run(%(
+      class Global
+        @@value = 0
+
+        def self.value
+          @@value
+        end
+
+        def self.value=(@@value)
+        end
+      end
+
+      class Foo
+        def [](v)
+          0
+        end
+
+        def []=(k, v)
+        end
+      end
+
+      def foo
+        Global.value += 1
+        Foo.new
+      end
+
+      def bar
+        Global.value += 10
+        0
+      end
+
+      foo[bar] += 2
+
+      Global.value
+      )).to_i.should eq(11)
+  end
+end
diff --git a/spec/compiler/normalize/op_assign_spec.cr b/spec/compiler/normalize/op_assign_spec.cr
@@ -0,0 +1,67 @@
+require "../../spec_helper"
+
+describe "Normalize: op assign" do
+  it "normalizes var +=" do
+    assert_normalize "a = 1; a += 2", "a = 1\na = a + 2"
+  end
+
+  it "normalizes var ||=" do
+    assert_normalize "a = 1; a ||= 2", "a = 1\na || (a = 2)"
+  end
+
+  it "normalizes var &&=" do
+    assert_normalize "a = 1; a &&= 2", "a = 1\na && (a = 2)"
+  end
+
+  it "normalizes exp.value +=" do
+    assert_normalize "a.b += 1", "__temp_1 = a\n__temp_1.b = __temp_1.b + 1"
+  end
+
+  it "normalizes exp.value ||=" do
+    assert_normalize "a.b ||= 1", "__temp_1 = a\n__temp_1.b || (__temp_1.b = 1)"
+  end
+
+  it "normalizes exp.value &&=" do
+    assert_normalize "a.b &&= 1", "__temp_1 = a\n__temp_1.b && (__temp_1.b = 1)"
+  end
+
+  it "normalizes var.value +=" do
+    assert_normalize "a = 1; a.b += 2", "a = 1\na.b = a.b + 2"
+  end
+
+  it "normalizes @var.value +=" do
+    assert_normalize "@a.b += 2", "@a.b = @a.b + 2"
+  end
+
+  it "normalizes @@var.value +=" do
+    assert_normalize "@@a.b += 2", "@@a.b = @@a.b + 2"
+  end
+
+  it "normalizes exp[value] +=" do
+    assert_normalize "a[b, c] += 1", "__temp_1 = b\n__temp_2 = c\n__temp_3 = a\n__temp_3[__temp_1, __temp_2] = __temp_3[__temp_1, __temp_2] + 1"
+  end
+
+  it "normalizes exp[value] ||=" do
+    assert_normalize "a[b, c] ||= 1", "__temp_1 = b\n__temp_2 = c\n__temp_3 = a\n__temp_3[__temp_1, __temp_2]? || (__temp_3[__temp_1, __temp_2] = 1)"
+  end
+
+  it "normalizes exp[value] &&=" do
+    assert_normalize "a[b, c] &&= 1", "__temp_1 = b\n__temp_2 = c\n__temp_3 = a\n__temp_3[__temp_1, __temp_2]? && (__temp_3[__temp_1, __temp_2] = 1)"
+  end
+
+  it "normalizes exp[0] +=" do
+    assert_normalize "a[0] += 1", "__temp_2 = a\n__temp_2[0] = __temp_2[0] + 1"
+  end
+
+  it "normalizes var[0] +=" do
+    assert_normalize "a = 1; a[0] += 1", "a = 1\na[0] = a[0] + 1"
+  end
+
+  it "normalizes @var[0] +=" do
+    assert_normalize "@a[0] += 1", "@a[0] = @a[0] + 1"
+  end
+
+  it "normalizes @@var[0] +=" do
+    assert_normalize "@@a[0] += 1", "@@a[0] = @@a[0] + 1"
+  end
+end
diff --git a/spec/compiler/parser/parser_spec.cr b/spec/compiler/parser/parser_spec.cr
@@ -129,7 +129,7 @@ describe "Parser" do
   it_parses "a[0] = 1, 2", MultiAssign.new([Call.new("a".call, "[]", 0.int32)] of ASTNode, [1.int32, 2.int32] of ASTNode)
   it_parses "a[0], a[1] = 1, 2", MultiAssign.new([Call.new("a".call, "[]", 0.int32), Call.new("a".call, "[]", 1.int32)] of ASTNode, [1.int32, 2.int32] of ASTNode)
   it_parses "a.foo, a.bar = 1, 2", MultiAssign.new([Call.new("a".call, "foo"), Call.new("a".call, "bar")] of ASTNode, [1.int32, 2.int32] of ASTNode)
-  it_parses "x = 0; a, b = x += 1", [Assign.new("x".var, 0.int32), MultiAssign.new(["a".var, "b".var] of ASTNode, [Assign.new("x".var, Call.new("x".var, "+", 1.int32))] of ASTNode)] of ASTNode
+  it_parses "x = 0; a, b = x += 1", [Assign.new("x".var, 0.int32), MultiAssign.new(["a".var, "b".var] of ASTNode, [OpAssign.new("x".var, "+", 1.int32)] of ASTNode)] of ASTNode
   it_parses "a, b = 1, 2 if 3", If.new(3.int32, MultiAssign.new(["a".var, "b".var] of ASTNode, [1.int32, 2.int32] of ASTNode))
 
   it_parses "@a, b = 1, 2", MultiAssign.new(["@a".instance_var, "b".var] of ASTNode, [1.int32, 2.int32] of ASTNode)
@@ -364,7 +364,7 @@ describe "Parser" do
   it_parses "f.x = - 1", Call.new("f".call, "x=", [Call.new(1.int32, "-")] of ASTNode)
 
   ["+", "-", "*", "/", "%", "|", "&", "^", "**", "<<", ">>"].each do |op|
-    it_parses "f.x #{op}= 2", Call.new("f".call, "x=", Call.new(Call.new("f".call, "x"), op, 2.int32))
+    it_parses "f.x #{op}= 2", OpAssign.new(Call.new("f".call, "x"), op, 2.int32)
   end
 
   ["/", "<", "<=", "==", "!=", "=~", "!~", ">", ">=", "+", "-", "*", "/", "~", "%", "&", "|", "^", "**", "==="].each do |op|
@@ -386,16 +386,16 @@ describe "Parser" do
   end
 
   ["+", "-", "*", "/", "%", "|", "&", "^", "**", "<<", ">>"].each do |op|
-    it_parses "a = 1; a #{op}= 1", [Assign.new("a".var, 1.int32), Assign.new("a".var, Call.new("a".var, op, 1.int32))] of ASTNode
-    it_parses "a = 1; a #{op}=\n1", [Assign.new("a".var, 1.int32), Assign.new("a".var, Call.new("a".var, op, 1.int32))] of ASTNode
-    it_parses "a.b #{op}=\n1", Call.new("a".call, "b=", Call.new(Call.new("a".call, "b"), op, 1.int32))
+    it_parses "a = 1; a #{op}= 1", [Assign.new("a".var, 1.int32), OpAssign.new("a".var, op, 1.int32)]
+    it_parses "a = 1; a #{op}=\n1", [Assign.new("a".var, 1.int32), OpAssign.new("a".var, op, 1.int32)]
+    it_parses "a.b #{op}=\n1", OpAssign.new(Call.new("a".call, "b"), op, 1.int32)
   end
 
-  it_parses "a = 1; a &&= 1", [Assign.new("a".var, 1.int32), And.new("a".var, Assign.new("a".var, 1.int32))]
-  it_parses "a = 1; a ||= 1", [Assign.new("a".var, 1.int32), Or.new("a".var, Assign.new("a".var, 1.int32))]
+  it_parses "a = 1; a &&= 1", [Assign.new("a".var, 1.int32), OpAssign.new("a".var, "&&", 1.int32)]
+  it_parses "a = 1; a ||= 1", [Assign.new("a".var, 1.int32), OpAssign.new("a".var, "||", 1.int32)]
 
-  it_parses "a = 1; a[2] &&= 3", [Assign.new("a".var, 1.int32), And.new(Call.new("a".var, "[]?", 2.int32), Call.new("a".var, "[]=", 2.int32, 3.int32))]
-  it_parses "a = 1; a[2] ||= 3", [Assign.new("a".var, 1.int32), Or.new(Call.new("a".var, "[]?", 2.int32), Call.new("a".var, "[]=", 2.int32, 3.int32))]
+  it_parses "a = 1; a[2] &&= 3", [Assign.new("a".var, 1.int32), OpAssign.new(Call.new("a".var, "[]", 2.int32), "&&", 3.int32)]
+  it_parses "a = 1; a[2] ||= 3", [Assign.new("a".var, 1.int32), OpAssign.new(Call.new("a".var, "[]", 2.int32), "||", 3.int32)]
 
   it_parses "if foo; 1; end", If.new("foo".call, 1.int32)
   it_parses "if foo\n1\nend", If.new("foo".call, 1.int32)
@@ -531,12 +531,12 @@ describe "Parser" do
 
   it_parses "1 if 3", If.new(3.int32, 1.int32)
   it_parses "1 unless 3", Unless.new(3.int32, 1.int32)
-  it_parses "r = 1; r.x += 2", [Assign.new("r".var, 1.int32), Call.new("r".var, "x=", Call.new(Call.new("r".var, "x"), "+", 2.int32))] of ASTNode
+  it_parses "r = 1; r.x += 2", [Assign.new("r".var, 1.int32), OpAssign.new(Call.new("r".var, "x"), "+", 2.int32)] of ASTNode
 
   it_parses "foo if 3", If.new(3.int32, "foo".call)
   it_parses "foo unless 3", Unless.new(3.int32, "foo".call)
 
-  it_parses "a = 1; a += 10 if a += 20", [Assign.new("a".var, 1.int32), If.new(Assign.new("a".var, Call.new("a".var, "+", 20.int32)), Assign.new("a".var, Call.new("a".var, "+", 10.int32)))]
+  it_parses "a = 1; a += 10 if a += 20", [Assign.new("a".var, 1.int32), If.new(OpAssign.new("a".var, "+", 20.int32), OpAssign.new("a".var, "+", 10.int32))]
   it_parses "puts a if true", If.new(true.bool, Call.new(nil, "puts", "a".call))
   it_parses "puts ::foo", Call.new(nil, "puts", Call.new(nil, "foo", global: true))
 
@@ -1026,8 +1026,8 @@ describe "Parser" do
 
   it_parses "foo.bar(1).baz", Call.new(Call.new("foo".call, "bar", 1.int32), "baz")
 
-  it_parses "b.c ||= 1", Or.new(Call.new("b".call, "c"), Call.new("b".call, "c=", 1.int32))
-  it_parses "b.c &&= 1", And.new(Call.new("b".call, "c"), Call.new("b".call, "c=", 1.int32))
+  it_parses "b.c ||= 1", OpAssign.new(Call.new("b".call, "c"), "||", 1.int32)
+  it_parses "b.c &&= 1", OpAssign.new(Call.new("b".call, "c"), "&&", 1.int32)
 
   it_parses "a = 1; class Foo; @x = a; end", [Assign.new("a".var, 1.int32), ClassDef.new("Foo".path, Assign.new("@x".instance_var, "a".call))]
 

diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr
@@ -2951,7 +2951,7 @@ module Crystal
       end
     end
 
-    def visit(node : When | Unless | Until | MacroLiteral)
+    def visit(node : When | Unless | Until | MacroLiteral | OpAssign)
       raise "Bug: #{node.class_desc} node '#{node}' (#{node.location}) should have been eliminated in normalize"
     end
   end