You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by bl...@apache.org on 2017/12/23 14:51:49 UTC
groovy git commit: GROOVY-8835: enable this.@ and super.@ sirect
field access in more cases
Repository: groovy
Updated Branches:
refs/heads/master 1fe0acf07 -> e0dd85b6f
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/e0dd85b6
Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/e0dd85b6
Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/e0dd85b6
Branch: refs/heads/master
Commit: e0dd85b6ff4ccb848db7ac173eaf21f242947d63
Parents: 1fe0acf
Author: Jochen Theodorou <bl...@gmx.org>
Authored: Fri Nov 24 01:51:02 2017 +0100
Committer: Jochen Theodorou <bl...@gmx.org>
Committed: Sat Dec 23 15:51:13 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/e0dd85b6/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/e0dd85b6/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()
+ ''')
+ }
+
+
+}