Repository: groovy Updated Branches: refs/heads/GROOVY_2_5_X fa04a25ad -> bcff55f99
GROOVY-8835: enable this.@ and super.@ sirect field access in more cases Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/bcff55f9 Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/bcff55f9 Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/bcff55f9 Branch: refs/heads/GROOVY_2_5_X Commit: bcff55f998a975a9e1f1a0e142d3dc3c791f5c0c Parents: fa04a25 Author: Jochen Theodorou <[email protected]> Authored: Fri Nov 24 01:51:02 2017 +0100 Committer: Jochen Theodorou <[email protected]> Committed: Sat Dec 23 15:53:01 2017 +0100 ---------------------------------------------------------------------- .../groovy/classgen/AsmClassGenerator.java | 35 +++ src/test/groovy/bugs/Groovy4418Bug.groovy | 238 +++++++++++++++---- 2 files changed, 221 insertions(+), 52 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/bcff55f9/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java index 9fb9a81..208ab46 100644 --- a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java +++ b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java @@ -890,11 +890,29 @@ public class AsmClassGenerator extends ClassGenerator { return name; } + private FieldNode getDeclaredFieldOfCurrentClassOrAccessibleFieldOfSuper(ClassNode current, String name, boolean skipCurrent) { + if (!skipCurrent) { + FieldNode currentClassField = current.getDeclaredField(name); + if (currentClassField != null) return currentClassField; + } + for (ClassNode node = current.getSuperClass(); node!=null; node = node.getSuperClass()) { + FieldNode fn = node.getDeclaredField(name); + if (fn != null && (fn.isPublic() || fn.isProtected())) return fn; + } + return null; + } + private void visitAttributeOrProperty(PropertyExpression expression, MethodCallerMultiAdapter adapter) { MethodVisitor mv = controller.getMethodVisitor(); Expression objectExpression = expression.getObjectExpression(); ClassNode classNode = controller.getClassNode(); + + //TODO (blackdrag): this if branch needs a rework. There should be no direct method calls be produced, the + // handling of this/super could be much simplified (see visitAttributeExpression), the field accessibility check + // could be moved directly into the search, which would also no longer require the GroovyBugError then + // the outer class field access seems to be without any tests (if there are tests for that, then the code + // here is dead code) if (isThisOrSuper(objectExpression)) { // let's use the field expression if it's available String name = expression.getPropertyAsString(); @@ -1058,6 +1076,23 @@ public class AsmClassGenerator extends ClassGenerator { public void visitAttributeExpression(AttributeExpression expression) { Expression objectExpression = expression.getObjectExpression(); + ClassNode classNode = controller.getClassNode(); + // TODO: checking for isThisOrSuper is enough for AttributeExpression, but if this is moved into + // visitAttributeOrProperty to handle attributes and properties equally, then the extended check should be done + if (isThisOrSuper(objectExpression) /*&& + !(expression.isImplicitThis() && controller.isInClosure()) */ + ) { + // let's use the field expression if it's available + String name = expression.getPropertyAsString(); + if (name != null) { + FieldNode field = getDeclaredFieldOfCurrentClassOrAccessibleFieldOfSuper(classNode, name, isSuperExpression(objectExpression)); + if (field != null) { + visitFieldExpression(new FieldExpression(field)); + return; + } + } + } + MethodCallerMultiAdapter adapter; OperandStack operandStack = controller.getOperandStack(); int mark = operandStack.getStackLength()-1; http://git-wip-us.apache.org/repos/asf/groovy/blob/bcff55f9/src/test/groovy/bugs/Groovy4418Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/groovy/bugs/Groovy4418Bug.groovy b/src/test/groovy/bugs/Groovy4418Bug.groovy index 234ec4b..3e8a93c 100644 --- a/src/test/groovy/bugs/Groovy4418Bug.groovy +++ b/src/test/groovy/bugs/Groovy4418Bug.groovy @@ -1,52 +1,186 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package groovy.bugs - -import gls.CompilableTestSupport - -class Groovy4418Bug extends CompilableTestSupport { - void testStaticFieldAccess() { - assertScript ''' - class Base { - static String field = 'foo' - } - class Subclass extends Base { - static method() { - field - } - } - assert new Subclass().method() == 'foo' - ''' - } - - // additional test for GROOVY-6183 - void testStaticAttributeAccess() { - assertScript ''' - class A { - static protected int x - public static void reset() { this.@x = 2 } - } - assert A.x == 0 - assert A.@x == 0 - A.reset() - assert A.x == 2 - assert A.@x == 2 - ''' - } -} +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy.bugs + +import gls.CompilableTestSupport + +class Groovy4418Bug extends CompilableTestSupport { + void testStaticFieldAccess() { + assertScript ''' + class Base { + static String field = 'foo' + } + class Subclass extends Base { + static method() { + field + } + } + assert new Subclass().method() == 'foo' + ''' + } + + // additional test for GROOVY-6183 + void testStaticAttributeAccess() { + assertScript ''' + class A { + static protected int x + public static void reset() { this.@x = 2 } + } + assert A.x == 0 + assert A.@x == 0 + A.reset() + assert A.x == 2 + assert A.@x == 2 + ''' + } + + // GROOVY-8385 + void testParentClassStaticAttributeSetAccessShouldNotCallSetter() { + assertScript ''' + class A { + static protected p + static setP(){def val} + static getP(){this.@p} + } + class B extends A { + def m(){this.@p = 1} + } + def x = new B() + assert A.@p == null + x.m() + assert A.@p == 1 + ''' + + assertScript ''' + class A { + static protected p + static setP(){def val} + static getP(){this.@p} + } + class B extends A { + def m(){super.@p = 1} + } + def x = new B() + assert A.@p == null + x.m() + assert A.@p == 1 + ''' + + assertScript ''' + class A { + static protected p + static setP(){def val} + static getP(){this.@p} + } + class AA extends A {} + class B extends AA { + def m(){super.@p = 1} + } + def x = new B() + assert A.@p == null + x.m() + assert A.@p == 1 + ''' + } + + // GROOVY-8385 + void testParentClassNonStaticAttributeSetAccessShouldNotCallSetter() { + assertScript ''' + class A { + protected p + void setP(def val){} + def getP(){p} + } + class B extends A { + def m(){this.@p = 1} + } + def x = new B() + assert x.@p == null + x.m() + assert x.@p == 1 + ''' + + assertScript ''' + class A { + protected p + void setP(def val){} + def getP(){p} + } + class B extends A { + def m(){super.@p = 1} + } + def x = new B() + assert x.@p == null + x.m() + assert x.@p == 1 + ''' + + assertScript ''' + class A { + protected p + void setP(def val){} + def getP(){p} + } + class AA extends A {} + class B extends AA { + def m(){super.@p = 1} + } + def x = new B() + assert x.@p == null + x.m() + assert x.@p == 1 + ''' + } + + // GROOVY-8385 + void testParentClassPrivateStaticAttributeSetAccessShouldCallSetter() { + shouldFail(MissingFieldException, ''' + class A { + static private p + static setP(){def val} + static getP(){this.@p} + } + class B extends A { + def m(){this.@p = 1} + } + def x = new B() + assert A.@p == null + x.m() + ''') + } + + // GROOVY-8385 + void testParentClassPrivateNonStaticAttributeSetAccessShouldNotCallSetter() { + shouldFail(MissingFieldException, ''' + class A { + private p + void setP(def val){} + def getP(){p} + } + class B extends A { + def m(){this.@p = 1} + } + def x = new B() + assert x.@p == null + x.m() + ''') + } + + +}
