From ab94d341b96317c73a98ad9c5ba358469cc1d553 Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Tue, 22 Aug 2017 17:48:09 -0400 Subject: [PATCH 1/4] [FIXED JENKINS-46358] Add support for StringGroovyMethods w/closures Note that this does *not* include `splitEachLine`, due to that requiring a private untranslated class. --- .../com/cloudbees/groovy/cps/tool/Driver.java | 4 +- .../cloudbees/groovy/cps/tool/Translator.java | 52 ++++++++-- .../groovy/cps/tool/translatable.txt | 23 +++++ .../com/cloudbees/groovy/cps/Continuable.java | 3 +- .../cps/CpsStringGroovyMethodsTest.groovy | 96 +++++++++++++++++++ 5 files changed, 166 insertions(+), 12 deletions(-) create mode 100644 lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy diff --git a/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Driver.java b/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Driver.java index 5de67ddbb..c810c357a 100644 --- a/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Driver.java +++ b/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Driver.java @@ -40,7 +40,9 @@ public void run(File dir) throws Exception { // classes to translate // TODO include other classes mentioned in DgmConverter if they have any applicable methods; and certainly StringGroovyMethods which has some Closure-based methods - List fileNames = asList("DefaultGroovyMethods", "DefaultGroovyStaticMethods"); + List fileNames = asList("DefaultGroovyMethods", + "DefaultGroovyStaticMethods", + "StringGroovyMethods"); List src = new ArrayList<>(); ZipArchive a = new ZipArchive((JavacFileManager) fileManager, new ZipFile(groovySrcJar)); diff --git a/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Translator.java b/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Translator.java index 97b30fd8b..7d1639128 100644 --- a/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Translator.java +++ b/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Translator.java @@ -95,7 +95,6 @@ import java.util.Optional; import java.util.Set; import java.util.TreeMap; -import java.util.TreeSet; import javax.annotation.Generated; import javax.lang.model.element.ElementKind; import javax.lang.model.element.Modifier; @@ -129,6 +128,7 @@ public class Translator { private final JClass $Builder; private final JClass $CatchExpression; private final DeclaredType closureType; + private final Map otherTranslated = new HashMap<>(); /** * To allow sibling calls to overloads to be resolved properly at runtime, write the actual implementation to an overload-proof private method. @@ -214,6 +214,9 @@ private static MethodLocation loc(String methodName) { JVar $methodName = m.param(String.class, "methodName"); m.body()._return(JExpr._new($MethodLocation).arg($output.dotclass()).arg($methodName)); }); + + // Record the fqcn we've already translated for possible use later. + otherTranslated.put(fqcn, $output); } /** @@ -333,20 +336,40 @@ public JExpression visitMethodInvocation(MethodInvocationTree mt, Void __) { if (ms instanceof MemberSelectTree) { MemberSelectTree mst = (MemberSelectTree) ms; - inv = $b.invoke("functionCall") - .arg(loc(mt)) - .arg(visit(mst.getExpression())) - .arg(n(mst.getIdentifier())); + // Make sure we translate any method calls to a transformed class too. + if (mst.getExpression() instanceof JCIdent && + !((JCIdent)mst.getExpression()).sym.toString().equals(fqcn) && + otherTranslated.containsKey(((JCIdent)mst.getExpression()).sym.toString())) { + inv = $b.invoke("functionCall") + .arg(loc(mt)) + .arg($b.invoke("constant").arg( + otherTranslated.get(((JCIdent)mst.getExpression()).sym.toString()).dotclass())) + .arg(n(mst.getIdentifier())); + + } else { + inv = $b.invoke("functionCall") + .arg(loc(mt)) + .arg(visit(mst.getExpression())) + .arg(n(mst.getIdentifier())); + } } else if (ms instanceof JCIdent) { // invocation without object selection, like foo(bar,zot) JCIdent it = (JCIdent) ms; if (!it.sym.owner.toString().equals(fqcn)) { - // static import - inv = $b.invoke("functionCall") - .arg(loc(mt)) - .arg($b.invoke("constant").arg(t(it.sym.owner.type).dotclass())) - .arg(n(it)); + if (otherTranslated.containsKey(it.sym.owner.toString())) { + // static import from transformed class + inv = $b.invoke("functionCall") + .arg(loc(mt)) + .arg($b.invoke("constant").arg(otherTranslated.get(it.sym.owner.toString()).dotclass())) + .arg(n(it)); + } else { + // static import from non-transformed class + inv = $b.invoke("functionCall") + .arg(loc(mt)) + .arg($b.invoke("constant").arg(t(it.sym.owner.type).dotclass())) + .arg(n(it)); + } } else { // invocation on this class String overloadResolved = mangledName((Symbol.MethodSymbol) it.sym); @@ -537,6 +560,15 @@ public JExpression visitAssignment(AssignmentTree at, Void __) { .arg(visit(at.getExpression())); } + @Override + public JExpression visitArrayType(ArrayTypeTree at, Void __) { + if (at.getType() instanceof IdentifierTree) { + return visitIdentifier((IdentifierTree) at.getType(), __); + } else { + return defaultAction(at, __); + } + } + @Override public JExpression visitNewArray(NewArrayTree nt, Void __) { if (nt.getInitializers()!=null) { diff --git a/dgm-builder/src/main/resources/com/cloudbees/groovy/cps/tool/translatable.txt b/dgm-builder/src/main/resources/com/cloudbees/groovy/cps/tool/translatable.txt index a14c62258..49e248b2e 100644 --- a/dgm-builder/src/main/resources/com/cloudbees/groovy/cps/tool/translatable.txt +++ b/dgm-builder/src/main/resources/com/cloudbees/groovy/cps/tool/translatable.txt @@ -167,3 +167,26 @@ org.codehaus.groovy.runtime.DefaultGroovyMethods.upto(long,java.lang.Number,groo org.codehaus.groovy.runtime.DefaultGroovyStaticMethods.createThread(java.lang.String,boolean,groovy.lang.Closure) org.codehaus.groovy.runtime.DefaultGroovyStaticMethods.sleep(java.lang.Object,long,groovy.lang.Closure) org.codehaus.groovy.runtime.DefaultGroovyStaticMethods.sleepImpl(long,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.eachMatch(T,java.lang.CharSequence,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.eachMatch(T,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.eachMatch(java.lang.String,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.eachMatch(java.lang.String,java.lang.String,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.find(java.lang.CharSequence,java.lang.CharSequence,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.find(java.lang.CharSequence,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.find(java.lang.String,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.find(java.lang.String,java.lang.String,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.findAll(java.lang.CharSequence,java.lang.CharSequence,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.findAll(java.lang.CharSequence,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.findAll(java.lang.String,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.findAll(java.lang.String,java.lang.String,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.getReplacement(java.util.regex.Matcher,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.replaceAll(java.lang.CharSequence,java.lang.CharSequence,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.replaceAll(java.lang.CharSequence,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.replaceAll(java.lang.String,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.replaceAll(java.lang.String,java.lang.String,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.replaceFirst(java.lang.CharSequence,java.lang.CharSequence,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.replaceFirst(java.lang.CharSequence,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.replaceFirst(java.lang.String,java.util.regex.Pattern,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.replaceFirst(java.lang.String,java.lang.String,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.takeWhile(java.lang.CharSequence,groovy.lang.Closure) +org.codehaus.groovy.runtime.StringGroovyMethods.takeWhile(groovy.lang.GString,groovy.lang.Closure) \ No newline at end of file diff --git a/lib/src/main/java/com/cloudbees/groovy/cps/Continuable.java b/lib/src/main/java/com/cloudbees/groovy/cps/Continuable.java index 3d5962c25..648eb0cff 100644 --- a/lib/src/main/java/com/cloudbees/groovy/cps/Continuable.java +++ b/lib/src/main/java/com/cloudbees/groovy/cps/Continuable.java @@ -32,7 +32,8 @@ public class Continuable implements Serializable { @SuppressWarnings("rawtypes") public static final List categories = ImmutableList.of( CpsDefaultGroovyMethods.class, - CpsDefaultGroovyStaticMethods.class); + CpsDefaultGroovyStaticMethods.class, + CpsStringGroovyMethods.class); /** * When the program resumes with a value (in particular an exception thrown), what environment diff --git a/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy b/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy new file mode 100644 index 000000000..847b848ba --- /dev/null +++ b/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy @@ -0,0 +1,96 @@ +package com.cloudbees.groovy.cps + +import org.junit.Ignore +import org.junit.Test + + +class CpsStringGroovyMethodsTest extends AbstractGroovyCpsTest { + @Test + void eachMatch() { + evalCPSAndSync(''' +int matchCount = 0 +"foobarfoooobar".eachMatch(~/foo/) { matchCount++ } +return matchCount +''', 2) + + evalCPSAndSync(''' +int matchCount = 0 +"foobarfoooobar".eachMatch('foo') { matchCount++ } +return matchCount +''', 2) + } + + @Test + void find() { + evalCPSAndSync(''' +return "foobar".find("oob") { it.reverse() } +''', "raboof") + + evalCPSAndSync(''' +return "foobar".find(~/oob/) { it.reverse() } +''', "raboof") + } + + @Test + void findAll() { + evalCPSAndSync(''' +return "foobarfoobarfoo".findAll("foo") { it.reverse() } +''', ['oof', 'oof', 'oof']) + + evalCPSAndSync(''' +return "foobarfoobarfoo".findAll(~/foo/) { it.reverse() } +''', ['oof', 'oof', 'oof']) + } + + @Test + void replaceAll() { + evalCPSAndSync(''' +return "foobarfoobarfoo".replaceAll("foo") { it.reverse() } +''', "oofbaroofbaroof") + + evalCPSAndSync(''' +return "foobarfoobarfoo".replaceAll(~/foo/) { it.reverse() } +''', "oofbaroofbaroof") + } + + @Test + void replaceFirst() { + evalCPSAndSync(''' +return "foobarfoobarfoo".replaceFirst("foo") { it.reverse() } +''', "oofbarfoobarfoo") + + evalCPSAndSync(''' +return "foobarfoobarfoo".replaceFirst(~/foo/) { it.reverse() } +''', "oofbarfoobarfoo") + } + + @Ignore("Waiting for StringGroovyMethods.LineIterable translation") + @Test + void splitEachLine() { + evalCPSAndSync(''' +return """ +abc|def +ghi|jkl +mno|pqr +""".splitEachLine("|") { it.reverse() } +''', "bob") + } + + @Test + void takeWhile() { + evalCPSAndSync(''' +return "Groovy".takeWhile{ it != 'v' } +''', "Groo") + } + + private void evalCPSAndSync(String script, Object value) { + evalCPS(script) == value + evalCPS(""" +@NonCPS +def someMethod() { + ${script} +} +someMethod() +""") == value + } +} From 77f3438401cf8005f07fb85f4b5fce345d13efe7 Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Tue, 22 Aug 2017 17:56:33 -0400 Subject: [PATCH 2/4] Adding a GString takeWhile test --- .../cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy b/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy index 847b848ba..f35650a9b 100644 --- a/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy +++ b/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy @@ -80,6 +80,11 @@ mno|pqr void takeWhile() { evalCPSAndSync(''' return "Groovy".takeWhile{ it != 'v' } +''', "Groo") + + evalCPSAndSync(''' +def vStr = 'v' +return "Groovy".takeWhile{ it != "${vStr}" } ''', "Groo") } From 522f56c624d1510c2e60106d987d13de65086af4 Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Tue, 22 Aug 2017 18:13:45 -0400 Subject: [PATCH 3/4] Oops, was testing the wrong thing --- .../cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy b/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy index f35650a9b..367665e49 100644 --- a/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy +++ b/lib/src/test/groovy/com/cloudbees/groovy/cps/CpsStringGroovyMethodsTest.groovy @@ -83,8 +83,8 @@ return "Groovy".takeWhile{ it != 'v' } ''', "Groo") evalCPSAndSync(''' -def vStr = 'v' -return "Groovy".takeWhile{ it != "${vStr}" } +def ovyStr = 'ovy' +return "Gro${ovyStr}".takeWhile{ it != "v" } ''', "Groo") } From d93226e1561859d54f43f717c91a3276ef3eeb6c Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Wed, 23 Aug 2017 09:50:53 -0400 Subject: [PATCH 4/4] Some additional comments --- .../java/com/cloudbees/groovy/cps/tool/Translator.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Translator.java b/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Translator.java index 7d1639128..28b63d52f 100644 --- a/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Translator.java +++ b/dgm-builder/src/main/java/com/cloudbees/groovy/cps/tool/Translator.java @@ -336,7 +336,10 @@ public JExpression visitMethodInvocation(MethodInvocationTree mt, Void __) { if (ms instanceof MemberSelectTree) { MemberSelectTree mst = (MemberSelectTree) ms; - // Make sure we translate any method calls to a transformed class too. + // If this is a call to a static method on another class, it may be an already-translated method, + // in which case, we need to use that translated method, not the original. So check if the expression + // is an identifier, that it's not the class we're in the process of translating, and if it's one + // of the other known translated classes. if (mst.getExpression() instanceof JCIdent && !((JCIdent)mst.getExpression()).sym.toString().equals(fqcn) && otherTranslated.containsKey(((JCIdent)mst.getExpression()).sym.toString())) { @@ -560,6 +563,9 @@ public JExpression visitAssignment(AssignmentTree at, Void __) { .arg(visit(at.getExpression())); } + /** + * This is needed to handle cases like {@code Object[].class}. + */ @Override public JExpression visitArrayType(ArrayTypeTree at, Void __) { if (at.getType() instanceof IdentifierTree) {