You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by em...@apache.org on 2023/01/18 18:27:06 UTC
[groovy] branch GROOVY_4_0_X updated: GROOVY-10722, GROOVY-10840: `null` or array for array-based constructor
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY_4_0_X
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/GROOVY_4_0_X by this push:
new 04600b6fe5 GROOVY-10722, GROOVY-10840: `null` or array for array-based constructor
04600b6fe5 is described below
commit 04600b6fe53c237dc78b60921f9f000a9db32934
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Wed Jan 18 09:49:12 2023 -0600
GROOVY-10722, GROOVY-10840: `null` or array for array-based constructor
---
.../options/ImmutablePropertyHandler.java | 18 ++++++------
.../codehaus/groovy/ast/tools/GeneralUtils.java | 27 +++++++++++-------
.../groovy/classgen/asm/InvocationWriter.java | 23 +++++++++------
src/test/gls/innerClass/InnerClassTest.groovy | 26 ++++++++++++++++-
.../bugs/DirectMethodCallWithVargsTest.groovy | 33 +++++++++++++++++++---
.../console/ui/AstNodeToScriptAdapterTest.groovy | 8 ++++--
6 files changed, 99 insertions(+), 36 deletions(-)
diff --git a/src/main/java/groovy/transform/options/ImmutablePropertyHandler.java b/src/main/java/groovy/transform/options/ImmutablePropertyHandler.java
index 4fe8b11569..c8bcabe59f 100644
--- a/src/main/java/groovy/transform/options/ImmutablePropertyHandler.java
+++ b/src/main/java/groovy/transform/options/ImmutablePropertyHandler.java
@@ -65,14 +65,14 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.castX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.classList2args;
import static org.codehaus.groovy.ast.tools.GeneralUtils.constX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.ctorX;
-import static org.codehaus.groovy.ast.tools.GeneralUtils.equalsNullX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.findArg;
import static org.codehaus.groovy.ast.tools.GeneralUtils.ifElseS;
import static org.codehaus.groovy.ast.tools.GeneralUtils.ifS;
import static org.codehaus.groovy.ast.tools.GeneralUtils.isInstanceOfX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.isNullX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
import static org.codehaus.groovy.ast.tools.GeneralUtils.list2args;
-import static org.codehaus.groovy.ast.tools.GeneralUtils.notX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.notNullX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.propX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.safeExpression;
import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt;
@@ -124,7 +124,7 @@ public class ImmutablePropertyHandler extends PropertyHandler {
public boolean validateProperties(final AbstractASTTransformation xform, final BlockStatement body, final ClassNode cNode, final List<PropertyNode> props) {
if (xform instanceof MapConstructorASTTransformation) {
VariableExpression namedArgs = varX("args");
- body.addStatement(ifS(equalsNullX(namedArgs), assignS(namedArgs, new MapExpression())));
+ body.addStatement(ifS(isNullX(namedArgs), assignS(namedArgs, new MapExpression())));
boolean pojo = !cNode.getAnnotations(POJO_TYPE).isEmpty();
body.addStatement(checkPropNamesS(namedArgs, pojo, props));
}
@@ -209,7 +209,7 @@ public class ImmutablePropertyHandler extends PropertyHandler {
Expression param = getParam(fNode, namedArgsMap != null);
Statement assignStmt = assignS(fieldExpr, castX(fType, param));
if (shouldNullCheck) {
- assignStmt = ifElseS(equalsNullX(param), NullCheckASTTransformation.makeThrowStmt(fNode.getName()), assignStmt);
+ assignStmt = ifElseS(isNullX(param), NullCheckASTTransformation.makeThrowStmt(fNode.getName()), assignStmt);
}
Expression initExpr = fNode.getInitialValueExpression();
Statement assignInit;
@@ -243,7 +243,7 @@ public class ImmutablePropertyHandler extends PropertyHandler {
Expression param = getParam(fNode, namedArgsMap != null);
Statement assignStmt = assignS(fieldExpr, createCheckImmutable(fNode, param, knownImmutables, knownImmutableClasses));
assignStmt = ifElseS(
- equalsNullX(param),
+ isNullX(param),
shouldNullCheck ? NullCheckASTTransformation.makeThrowStmt(fNode.getName()) : assignNullS(fieldExpr),
assignStmt);
Expression initExpr = fNode.getInitialValueExpression();
@@ -271,7 +271,7 @@ public class ImmutablePropertyHandler extends PropertyHandler {
assignS(fieldExpr, cloneCollectionExpr(cloneArrayOrCloneableExpr(param, fieldType), fieldType)),
assignS(fieldExpr, cloneCollectionExpr(param, fieldType)));
assignStmt = ifElseS(
- equalsNullX(param),
+ isNullX(param),
shouldNullCheck ? NullCheckASTTransformation.makeThrowStmt(fNode.getName()) : assignNullS(fieldExpr),
assignStmt);
Expression initExpr = fNode.getInitialValueExpression();
@@ -290,7 +290,7 @@ public class ImmutablePropertyHandler extends PropertyHandler {
final Expression param = getParam(fNode, namedArgsMap != null);
Statement assignStmt = assignS(fieldExpr, cloneArrayOrCloneableExpr(param, fieldType));
assignStmt = ifElseS(
- equalsNullX(param),
+ isNullX(param),
shouldNullCheck ? NullCheckASTTransformation.makeThrowStmt(fNode.getName()) : assignNullS(fieldExpr),
assignStmt);
final Statement assignInit;
@@ -312,7 +312,7 @@ public class ImmutablePropertyHandler extends PropertyHandler {
final Expression param = getParam(fNode, namedArgsMap != null);
Statement assignStmt = assignS(fieldExpr, cloneDateExpr(param));
assignStmt = ifElseS(
- equalsNullX(param),
+ isNullX(param),
shouldNullCheck ? NullCheckASTTransformation.makeThrowStmt(fNode.getName()) : assignNullS(fieldExpr),
assignStmt);
final Statement assignInit;
@@ -333,7 +333,7 @@ public class ImmutablePropertyHandler extends PropertyHandler {
String name = fNode.getName();
Expression value = findArg(name);
return ifS(
- notX(equalsNullX(value)),
+ notNullX(value),
throwS(
ctorX(READONLYEXCEPTION_TYPE, args(constX(name), constX(cNode.getName())))
)
diff --git a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
index 06edc9e3ee..ca8b35c821 100644
--- a/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
+++ b/src/main/java/org/codehaus/groovy/ast/tools/GeneralUtils.java
@@ -371,12 +371,12 @@ public class GeneralUtils {
return new MapEntryExpression(key, value);
}
- public static BinaryExpression eqX(final Expression lhv, final Expression rhv) {
- return binX(lhv, EQ, rhv);
+ public static BinaryExpression eqX(final Expression left, final Expression right) {
+ return binX(left, EQ, right);
}
- public static BooleanExpression equalsNullX(final Expression argExpr) {
- return boolX(eqX(argExpr, nullX()));
+ public static BooleanExpression equalsNullX(final Expression expr) {
+ return boolX(eqX(nullX(), expr));
}
public static FieldExpression fieldX(final FieldNode fieldNode) {
@@ -660,8 +660,15 @@ public class GeneralUtils {
return binX(target, INDEX, value);
}
- public static BooleanExpression isInstanceOfX(final Expression objectExpression, final ClassNode cNode) {
- return boolX(binX(objectExpression, INSTANCEOF, classX(cNode)));
+ public static BooleanExpression isInstanceOfX(final Expression expr, final ClassNode type) {
+ return boolX(binX(expr, INSTANCEOF, classX(type)));
+ }
+
+ /**
+ * @since 4.0.8
+ */
+ public static BooleanExpression isNullOrInstanceOfX(final Expression expr, final ClassNode type) {
+ return boolX(orX(binX(nullX(), Token.newSymbol(Types.COMPARE_IDENTICAL, -1, -1), expr), binX(expr, INSTANCEOF, classX(type))));
}
/**
@@ -739,8 +746,8 @@ public class GeneralUtils {
return binX(lhv, NOT_IDENTICAL, rhv);
}
- public static BooleanExpression notNullX(final Expression argExpr) {
- return boolX(binX(argExpr, NE, nullX()));
+ public static BooleanExpression notNullX(final Expression expr) {
+ return boolX(binX(nullX(), NE, expr));
}
public static NotExpression notX(final Expression expr) {
@@ -802,7 +809,7 @@ public class GeneralUtils {
}
public static Statement safeExpression(final Expression fieldExpr, final Expression expression) {
- return new IfStatement(equalsNullX(fieldExpr), stmt(fieldExpr), stmt(expression));
+ return new IfStatement(isNullX(fieldExpr), stmt(fieldExpr), stmt(expression));
}
public static BooleanExpression sameX(final Expression self, final Expression other) {
@@ -945,7 +952,7 @@ public class GeneralUtils {
}
fNode.setInitialValueExpression(null);
Expression value = findArg(name);
- return ifElseS(equalsNullX(value), assignInit, assignS(fieldExpr, castX(fType, value)));
+ return ifElseS(isNullX(value), assignInit, assignS(fieldExpr, castX(fType, value)));
}
/**
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/InvocationWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/InvocationWriter.java
index 7032a92da1..7c0816bdd0 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/InvocationWriter.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/InvocationWriter.java
@@ -60,7 +60,11 @@ import static org.codehaus.groovy.ast.ClassHelper.isObjectType;
import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveType;
import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveVoid;
import static org.codehaus.groovy.ast.ClassHelper.isStringType;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.castX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.isNullOrInstanceOfX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.ternaryX;
import static org.codehaus.groovy.ast.tools.ParameterUtils.isVargs;
+import static org.codehaus.groovy.runtime.DefaultGroovyMethods.last;
import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.isClassClassNodeWrappingConcreteType;
import static org.objectweb.asm.Opcodes.AALOAD;
import static org.objectweb.asm.Opcodes.ACC_FINAL;
@@ -247,17 +251,16 @@ public class InvocationWriter {
int nthParameter = parameters.length - 1;
ClassNode lastType = parameters[nthParameter].getOriginType();
- AsmClassGenerator acg = controller.getAcg();
OperandStack operandStack = controller.getOperandStack();
- int stackLen = operandStack.getStackLength() + arguments.size();
+ int expected = operandStack.getStackLength() + arguments.size();
boolean varg = lastType.isArray() && (
arguments.size() > parameters.length
|| arguments.size() == parameters.length - 1
- || !isArray(arguments.get(arguments.size() - 1)));
+ || !arguments.isEmpty() && !isArray(last(arguments)));
for (int i = 0, n = varg ? nthParameter : arguments.size(); i < n; i += 1) {
Expression argument = arguments.get(i);
- argument.visit(acg);
+ argument.visit(controller.getAcg());
if (!isNullConstant(argument)) {
operandStack.doGroovyCast(parameters[i].getType());
}
@@ -265,14 +268,16 @@ public class InvocationWriter {
if (varg) {
// last arguments wrapped in an array
- List<Expression> lastArgs = new ArrayList<>();
- for (int i = nthParameter, n = arguments.size(); i < n; i += 1) {
- lastArgs.add(arguments.get(i));
+ List<Expression> lastArgs = arguments.subList(nthParameter, arguments.size());
+ Expression array = new ArrayExpression(lastType.getComponentType(), lastArgs);
+ if (lastArgs.size() == 1) { // GROOVY-10722: disambiguate array and null cases
+ Expression lastExpr = lastArgs.get(0); // TODO: cache non-trivial expression value
+ array = ternaryX(isNullOrInstanceOfX(lastExpr, lastType), castX(lastType, lastExpr), array);
}
- new ArrayExpression(lastType.getComponentType(), lastArgs).visit(acg);
+ array.visit(controller.getAcg());
// adjust stack length
- while (operandStack.getStackLength() < stackLen) {
+ while (operandStack.getStackLength() < expected) {
operandStack.push(ClassHelper.OBJECT_TYPE);
}
if (arguments.size() == nthParameter) {
diff --git a/src/test/gls/innerClass/InnerClassTest.groovy b/src/test/gls/innerClass/InnerClassTest.groovy
index 476bb45912..f21681560e 100644
--- a/src/test/gls/innerClass/InnerClassTest.groovy
+++ b/src/test/gls/innerClass/InnerClassTest.groovy
@@ -45,7 +45,20 @@ final class InnerClassTest {
'''
}
- @Test // GROOVY-7370
+ @Test // GROOVY-10840
+ void testArrayAIC() {
+ assertScript '''
+ class BAIS extends ByteArrayInputStream {
+ BAIS(String input) {
+ super(input.bytes)
+ }
+ }
+
+ assert new BAIS('input').available() >= 5
+ '''
+ }
+
+ @Test // GROOVY-7370, GROOVY-10722
void testVargsAIC() {
String pogo = '''
class C {
@@ -70,6 +83,17 @@ final class InnerClassTest {
def c = new C('x','y') { }
assert c.strings.length == 2
'''
+
+ assertScript pogo + '''
+ def c = new C(null) { }
+ assert c.strings == null
+ '''
+
+ assertScript pogo + '''
+ def a = new String[0]
+ def c = new C( a ) { }
+ assert c.strings.length == 0
+ '''
}
@Test
diff --git a/src/test/groovy/bugs/DirectMethodCallWithVargsTest.groovy b/src/test/groovy/bugs/DirectMethodCallWithVargsTest.groovy
index 1abd390f12..58ca8fad12 100644
--- a/src/test/groovy/bugs/DirectMethodCallWithVargsTest.groovy
+++ b/src/test/groovy/bugs/DirectMethodCallWithVargsTest.groovy
@@ -32,18 +32,23 @@ final class DirectMethodCallWithVargsTest {
void testDirectMethodCallWithVargs() {
assertScript shell, '''
def foo(String... args) {
- (args as List).join(',')
+ args.join(',')
}
assert foo() == ''
assert foo('1') == '1'
assert foo('1','2','3') == '1,2,3'
assert foo('1','2','3','4') == '1,2,3,4'
+ assert foo(new String[]{'1','2','3','4'}) == '1,2,3,4'
def a = '1'
def b = '2'
def c = '3'
assert foo(a,b,c) == '1,2,3'
+
+ shouldFail(NullPointerException) {
+ foo(null)
+ }
'''
}
@@ -51,13 +56,19 @@ final class DirectMethodCallWithVargsTest {
void testDirectMethodCallWithPrimitiveVargs() {
assertScript shell, '''
def foo(int... args) {
- (args as List).join(',')
+ args.join(',')
}
assert foo() == ''
+ assert foo(0) == '0'
assert foo(1) == '1'
assert foo(1,2,3) == '1,2,3'
assert foo(1,2,3,4) == '1,2,3,4'
+ assert foo(new int[]{1,2,3,4}) == '1,2,3,4'
+
+ shouldFail(NullPointerException) {
+ foo(null)
+ }
'''
}
@@ -65,18 +76,25 @@ final class DirectMethodCallWithVargsTest {
void testDirectMethodCallWithArgumentAndVargs() {
assertScript shell, '''
def foo(String prefix, String... args) {
- prefix+(args as List).join(',')
+ '' + prefix + args.join(',')
}
assert foo('A') == 'A'
+ assert foo(null) == 'null'
assert foo('A','1') == 'A1'
assert foo('A','1','2','3') == 'A1,2,3'
assert foo('A','1','2','3','4') == 'A1,2,3,4'
+ assert foo('A','1','2','3','4') == 'A1,2,3,4'
+ assert foo('A',new String[]{'1','2','3','4'}) == 'A1,2,3,4'
def a = '1'
def b = '2'
def c = '3'
assert foo('A',a,b,c) == 'A1,2,3'
+
+ shouldFail(NullPointerException) {
+ foo('A', null)
+ }
'''
}
@@ -84,19 +102,26 @@ final class DirectMethodCallWithVargsTest {
void testDirectMethodCallWithArgumentAndPrimitiveVargs() {
assertScript shell, '''
def foo(int prefix, int... args) {
- "$prefix"+(args as List).join(',')
+ '' + prefix + args.join(',')
}
assert foo(1) == '1'
+ assert foo(1,0) == '10'
assert foo(1,1) == '11'
assert foo(1,1,2,3) == '11,2,3'
assert foo(1,1,2,3,4) == '11,2,3,4'
+ assert foo(0,new int[]{1,2,3,4}) == '01,2,3,4'
+
+ shouldFail(NullPointerException) {
+ foo(1, null)
+ }
'''
}
//--------------------------------------------------------------------------
private final GroovyShell shell = GroovyShell.withConfig {
+ imports{ staticMember 'groovy.test.GroovyAssert','shouldFail' }
inline(phase: 'CANONICALIZATION') { sourceUnit, x, classNode ->
def visitor = new MethodCallVisitor(sourceUnit)
classNode.methods.each(visitor.&acceptMethod)
diff --git a/subprojects/groovy-console/src/test/groovy/groovy/console/ui/AstNodeToScriptAdapterTest.groovy b/subprojects/groovy-console/src/test/groovy/groovy/console/ui/AstNodeToScriptAdapterTest.groovy
index 7da2ede8ba..be206753af 100644
--- a/subprojects/groovy-console/src/test/groovy/groovy/console/ui/AstNodeToScriptAdapterTest.groovy
+++ b/subprojects/groovy-console/src/test/groovy/groovy/console/ui/AstNodeToScriptAdapterTest.groovy
@@ -622,14 +622,16 @@ final class AstNodeToScriptAdapterTest extends GroovyTestCase {
}
void testLazyAnnotation() {
- String script = '''class Event {
+ String script = '''
+ class Event {
@Lazy ArrayList speakers
- }'''
+ }
+ '''
String result = compileToScript(script, CompilePhase.CANONICALIZATION)
assert result =~ /Lazy\s*private java\.util\.ArrayList .*speakers /
assert result.contains('public java.util.ArrayList getSpeakers() {')
- assert result.contains('if ($speakers != null) {')
+ assert result.contains('if ($speakers != null') || result.contains('if (null != $speakers')
assert result.contains('$speakers = new java.util.ArrayList()')
}