Merge pull request #45 from jglick/fieldViaGetter-JENKINS-31484

[JENKINS-31484] Translate field accesses to AttributeAccessBlock rather than PropertyAccessBlock
cloudbees · Jan 31, 2017 · 97b942e · 97b942e
2 parents c0d9869 + 496195a
commit 97b942e
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 11 deletions.
diff --git a/src/main/groovy/com/cloudbees/groovy/cps/CpsTransformer.groovy b/src/main/groovy/com/cloudbees/groovy/cps/CpsTransformer.groovy
@@ -184,7 +184,10 @@ class CpsTransformer extends CompilationCustomizer implements GroovyCodeVisitor
         Expression body;
 
         // transform the body
-        parent = { e -> body=e }
+        parent = {
+            e -> body = e
+            // println "in $classNode.name transformed $m.typeDescriptor to $e.text"
+        }
         visitWithSafepoint(m.code)
 
         def params = new ListExpression();
@@ -725,11 +728,21 @@ class CpsTransformer extends CompilationCustomizer implements GroovyCodeVisitor
 
     void visitPropertyExpression(PropertyExpression exp) {
         // TODO: spread
-        makeNode("property") {
-            loc(exp)
-            visit(exp.objectExpression)
-            visit(exp.property)
-            literal(exp.safe)
+        if (exp.objectExpression instanceof VariableExpression && exp.objectExpression.thisExpression &&
+                exp.property instanceof ConstantExpression && classNode.getSetterMethod('set' + Verifier.capitalize(exp.property.value), false) != null) {
+            makeNode("attribute") {
+                loc(exp)
+                visit(exp.objectExpression)
+                visit(exp.property)
+                literal(exp.safe)
+            }
+        } else {
+            makeNode("property") {
+                loc(exp)
+                visit(exp.objectExpression)
+                visit(exp.property)
+                literal(exp.safe)
+            }
         }
     }
 
@@ -788,10 +801,19 @@ class CpsTransformer extends CompilationCustomizer implements GroovyCodeVisitor
         if (ref instanceof DynamicVariable
         ||  ref instanceof PropertyNode
         ||  ref instanceof FieldNode) {
-            makeNode("property") {
-                loc(exp)
-                makeNode("javaThis_")
-                literal(exp.name)
+            if (ref instanceof FieldNode && classNode.getGetterMethod('get' + Verifier.capitalize(exp.name)) != null) {
+                makeNode("attribute") {
+                    loc(exp)
+                    makeNode("javaThis_")
+                    visit(new ConstantExpression(exp.name))
+                    literal(false)
+                }
+            } else {
+                makeNode("property") {
+                    loc(exp)
+                    makeNode("javaThis_")
+                    literal(exp.name)
+                }
             }
         } else
         if (exp.name=="this") {

diff --git a/src/main/java/com/cloudbees/groovy/cps/Next.java b/src/main/java/com/cloudbees/groovy/cps/Next.java
@@ -1,6 +1,8 @@
 package com.cloudbees.groovy.cps;
 
 import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * Remaining computation to execute. To work around the lack of tail-call optimization.
@@ -51,6 +53,20 @@ public Next run() {
         return n;
     }
 
+    public Outcome run(int max) {
+        List<String> functions = new ArrayList<String>();
+        Next n = this;
+        while(n.yield==null) {
+            functions.add(n.f.getClass().getCanonicalName());
+            if (--max == 0) {
+                int len = functions.size();
+                throw new AssertionError("Did not terminate; ran " + len + " steps ending with: " + functions.subList(len - 20, len));
+            }
+            n = n.step();
+        }
+        return n.yield;
+    }
+
     /**
      * Executes one step
      */

diff --git a/src/test/groovy/com/cloudbees/groovy/cps/AbstractGroovyCpsTest.groovy b/src/test/groovy/com/cloudbees/groovy/cps/AbstractGroovyCpsTest.groovy
@@ -52,7 +52,7 @@ abstract class AbstractGroovyCpsTest extends Assert {
     }
 
     Object evalCPSonly(String script) {
-        return parseCps(script).invoke(null, null, Continuation.HALT).run().yield.replay()
+        return parseCps(script).invoke(null, null, Continuation.HALT).run(10000).replay()
     }
 
     CpsCallableInvocation parseCps(String script) {

diff --git a/src/test/groovy/com/cloudbees/groovy/cps/CpsTransformerTest.groovy b/src/test/groovy/com/cloudbees/groovy/cps/CpsTransformerTest.groovy
@@ -612,6 +612,29 @@ class CpsTransformerTest extends AbstractGroovyCpsTest {
         ''') == [-3,2];
     }
 
+    @Test
+    void fieldDirect() {
+        assert evalCPS('class C {private int x = 33}; new C().x') == 33
+    }
+
+    @Issue("JENKINS-31484")
+    @Test
+    void fieldViaGetter() {
+        assert evalCPS('class C {private int x = 33; int getX() {2 * this.@x}}; new C().x') == 66
+        assert evalCPS('class C {private int x = 33; int getX() {2 * x}}; new C().x') == 66
+    }
+
+    @Issue("JENKINS-31484")
+    @Test
+    void fieldViaSetter() {
+        assert evalCPS('class C {private int x = 0; int getX() {2 * x}; void setX(int x) {this.@x = x / 3}}; C c = new C(); c.x = 33; c.x') == 22
+        assert evalCPS('class C {private int x = 0; int getX() {2 * x}; void setX(int x) {this.x = x / 3}}; C c = new C(); c.x = 33; c.x') == 22
+    }
+
+    @Test
+    void nonField() {
+        assert evalCPS('class C extends HashMap {def read() {x * 2}; def write(x) {this.x = x / 3}}; C c = new C(); c.write(33); c.read()') == 22
+    }
 
     @Test
     void method_pointer() {