Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 2068A200D2F for ; Wed, 1 Nov 2017 10:18:15 +0100 (CET) Received: by cust-asf.ponee.io (Postfix) id 1DE68160BE6; Wed, 1 Nov 2017 09:18:15 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 3130A160BEA for ; Wed, 1 Nov 2017 10:18:14 +0100 (CET) Received: (qmail 18404 invoked by uid 500); 1 Nov 2017 09:18:13 -0000 Mailing-List: contact commits-help@groovy.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@groovy.apache.org Delivered-To: mailing list commits@groovy.apache.org Received: (qmail 18381 invoked by uid 99); 1 Nov 2017 09:18:13 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 01 Nov 2017 09:18:13 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 5E334DFAEB; Wed, 1 Nov 2017 09:18:09 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit From: paulk@apache.org To: commits@groovy.apache.org Date: Wed, 01 Nov 2017 09:18:12 -0000 Message-Id: In-Reply-To: <93e17814d7b14cc690d18dc88f4f095a@git.apache.org> References: <93e17814d7b14cc690d18dc88f4f095a@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [4/4] groovy git commit: GROOVY-8112: NPE in Groovy compiler when referencing @Field in aic (closes #622) archived-at: Wed, 01 Nov 2017 09:18:15 -0000 GROOVY-8112: NPE in Groovy compiler when referencing @Field in aic (closes #622) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/ed69389a Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/ed69389a Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/ed69389a Branch: refs/heads/GROOVY_2_4_X Commit: ed69389a62c413f71d78eb26bbaab364a9c2318c Parents: a3acf47 Author: paulk Authored: Mon Oct 30 08:47:28 2017 +1000 Committer: paulk Committed: Wed Nov 1 19:17:53 2017 +1000 ---------------------------------------------------------------------- .../transform/FieldASTTransformation.java | 104 +++++++++++++++---- .../groovy/transform/FieldTransformTest.groovy | 39 ++++++- 2 files changed, 122 insertions(+), 21 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/ed69389a/src/main/org/codehaus/groovy/transform/FieldASTTransformation.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/transform/FieldASTTransformation.java b/src/main/org/codehaus/groovy/transform/FieldASTTransformation.java index 0ef343a..eb2a49c 100644 --- a/src/main/org/codehaus/groovy/transform/FieldASTTransformation.java +++ b/src/main/org/codehaus/groovy/transform/FieldASTTransformation.java @@ -26,15 +26,19 @@ import org.codehaus.groovy.ast.AnnotatedNode; import org.codehaus.groovy.ast.AnnotationNode; import org.codehaus.groovy.ast.ClassCodeExpressionTransformer; import org.codehaus.groovy.ast.ClassNode; +import org.codehaus.groovy.ast.ConstructorNode; import org.codehaus.groovy.ast.FieldNode; import org.codehaus.groovy.ast.MethodNode; +import org.codehaus.groovy.ast.Parameter; import org.codehaus.groovy.ast.Variable; import org.codehaus.groovy.ast.VariableScope; -import org.codehaus.groovy.ast.expr.BinaryExpression; +import org.codehaus.groovy.ast.expr.ArgumentListExpression; import org.codehaus.groovy.ast.expr.ClosureExpression; import org.codehaus.groovy.ast.expr.ConstantExpression; +import org.codehaus.groovy.ast.expr.ConstructorCallExpression; import org.codehaus.groovy.ast.expr.DeclarationExpression; import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.TupleExpression; import org.codehaus.groovy.ast.expr.VariableExpression; import org.codehaus.groovy.ast.stmt.ExpressionStatement; import org.codehaus.groovy.classgen.VariableScopeVisitor; @@ -42,6 +46,7 @@ import org.codehaus.groovy.control.CompilePhase; import org.codehaus.groovy.control.SourceUnit; import org.objectweb.asm.Opcodes; +import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; import java.util.List; @@ -50,9 +55,6 @@ import static org.codehaus.groovy.ast.ClassHelper.make; /** * Handles transformation for the @Field annotation. - * - * @author Paul King - * @author Cedric Champeau */ @GroovyASTTransformation(phase = CompilePhase.SEMANTIC_ANALYSIS) public class FieldASTTransformation extends ClassCodeExpressionTransformer implements ASTTransformation, Opcodes { @@ -68,6 +70,7 @@ public class FieldASTTransformation extends ClassCodeExpressionTransformer imple private String variableName; private FieldNode fieldNode; private ClosureExpression currentClosure; + private ConstructorCallExpression currentAIC; public void visit(ASTNode[] nodes, SourceUnit source) { sourceUnit = source; @@ -153,23 +156,76 @@ public class FieldASTTransformation extends ClassCodeExpressionTransformer imple } else if (insideScriptBody && expr instanceof VariableExpression && currentClosure != null) { VariableExpression ve = (VariableExpression) expr; if (ve.getName().equals(variableName)) { - // we may only check the variable name because the Groovy compiler - // already fails if a variable with the same name already exists in the scope. - // this means that a closure cannot shadow a class variable - ve.setAccessedVariable(fieldNode); - final VariableScope variableScope = currentClosure.getVariableScope(); - final Iterator iterator = variableScope.getReferencedLocalVariablesIterator(); - while (iterator.hasNext()) { - Variable next = iterator.next(); - if (next.getName().equals(variableName)) iterator.remove(); - } - variableScope.putReferencedClassVariable(fieldNode); + adjustToClassVar(ve); return ve; } + } else if (currentAIC != null && expr instanceof ArgumentListExpression) { + // if a match is found, the compiler will have already set up aic constructor to hav + // an argument which isn't needed since we'll be accessing the field; we must undo it + Expression skip = null; + List origArgList = ((ArgumentListExpression) expr).getExpressions(); + for (int i = 0; i < origArgList.size(); i++) { + Expression arg = origArgList.get(i); + if (matchesCandidate(arg)) { + skip = arg; + adjustConstructorAndFields(i, currentAIC.getType()); + break; + } + } + if (skip != null) { + return adjustedArgList(skip, origArgList); + } } return expr.transformExpression(this); } + private boolean matchesCandidate(Expression arg) { + return arg instanceof VariableExpression && ((VariableExpression) arg).getAccessedVariable() == candidate.getVariableExpression().getAccessedVariable(); + } + + private Expression adjustedArgList(Expression skip, List origArgs) { + List newArgs = new ArrayList(origArgs.size() - 1); + for (Expression origArg : origArgs) { + if (skip != origArg) { + newArgs.add(origArg); + } + } + return new ArgumentListExpression(newArgs); + } + + private void adjustConstructorAndFields(int skipIndex, ClassNode type) { + List constructors = type.getDeclaredConstructors(); + if (constructors.size() == 1) { + ConstructorNode constructor = constructors.get(0); + Parameter[] params = constructor.getParameters(); + Parameter[] newParams = new Parameter[params.length - 1]; + int to = 0; + for (int from = 0; from < params.length; from++) { + if (from != skipIndex) { + newParams[to++] = params[from]; + } + } + type.removeConstructor(constructor); + // code doesn't mention the removed param at this point, okay to leave as is + type.addConstructor(constructor.getModifiers(), newParams, constructor.getExceptions(), constructor.getCode()); + type.removeField(variableName); + } + } + + private void adjustToClassVar(VariableExpression expr) { + // we only need to check the variable name because the Groovy compiler + // already fails if a variable with the same name already exists in the scope. + // this means that a closure cannot shadow a class variable + expr.setAccessedVariable(fieldNode); + final VariableScope variableScope = currentClosure.getVariableScope(); + final Iterator iterator = variableScope.getReferencedLocalVariablesIterator(); + while (iterator.hasNext()) { + Variable next = iterator.next(); + if (next.getName().equals(variableName)) iterator.remove(); + } + variableScope.putReferencedClassVariable(fieldNode); + } + @Override public void visitClosureExpression(final ClosureExpression expression) { ClosureExpression old = currentClosure; @@ -179,6 +235,20 @@ public class FieldASTTransformation extends ClassCodeExpressionTransformer imple } @Override + public void visitConstructorCallExpression(final ConstructorCallExpression cce) { + if (!insideScriptBody || !cce.isUsingAnonymousInnerClass()) return; + ConstructorCallExpression old = currentAIC; + currentAIC = cce; + Expression newArgs = transform(cce.getArguments()); + if (cce.getArguments() instanceof TupleExpression && newArgs instanceof TupleExpression) { + List argList = ((TupleExpression) cce.getArguments()).getExpressions(); + argList.clear(); + argList.addAll(((TupleExpression) newArgs).getExpressions()); + } + currentAIC = old; + } + + @Override public void visitMethod(MethodNode node) { Boolean oldInsideScriptBody = insideScriptBody; if (node.isScriptBody()) insideScriptBody = true; @@ -189,9 +259,7 @@ public class FieldASTTransformation extends ClassCodeExpressionTransformer imple @Override public void visitExpressionStatement(ExpressionStatement es) { Expression exp = es.getExpression(); - if (exp instanceof BinaryExpression) { - exp.visit(this); - } + exp.visit(this); super.visitExpressionStatement(es); } http://git-wip-us.apache.org/repos/asf/groovy/blob/ed69389a/src/test/org/codehaus/groovy/transform/FieldTransformTest.groovy ---------------------------------------------------------------------- diff --git a/src/test/org/codehaus/groovy/transform/FieldTransformTest.groovy b/src/test/org/codehaus/groovy/transform/FieldTransformTest.groovy index 2821f6e..6575720 100644 --- a/src/test/org/codehaus/groovy/transform/FieldTransformTest.groovy +++ b/src/test/org/codehaus/groovy/transform/FieldTransformTest.groovy @@ -21,8 +21,7 @@ package org.codehaus.groovy.transform import gls.CompilableTestSupport /** - * @author Paul King - * @author C�dric Champeau + * Tests for the {@code @Field} transformation */ class FieldTransformTest extends CompilableTestSupport { @@ -226,4 +225,38 @@ class FieldTransformTest extends CompilableTestSupport { assert foo + bar + baz == 'foobarbaz' ''' } -} \ No newline at end of file + + void testAnonymousInnerClassReferencesToField() { + // GROOVY-8112 + assertScript ''' + @groovy.transform.Field + StringBuilder logger = new StringBuilder() + logger.append('a') + ['b'].each { + logger.append(it) + new Object() { + String toString() { + logger.append('c') + ['d'].each { logger.append(it) } + } + }.toString() + } + Closure c = { logger.append('e') } + c() + // control: worked previously, make sure we didn't break + def method() { + logger.append('f') + ['g'].each { + logger.append(it) + new Object() { + String toString() { + logger.append('h') + } + }.toString() + } + } + method() + assert logger.toString() == 'abcdefgh' + ''' + } +}