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 2020/10/25 17:00:07 UTC
[groovy] branch GROOVY-9791 updated (b0f80d0 -> 1b08370)
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a change to branch GROOVY-9791
in repository https://gitbox.apache.org/repos/asf/groovy.git.
discard b0f80d0 GROOVY-9791: SC: not dynamic property for protected field from diff pack
new 1b08370 GROOVY-9791: SC: not dynamic property for protected field from diff pack
This update added new revisions after undoing existing revisions.
That is to say, some revisions that were in the old version of the
branch are not in the new version. This situation occurs
when a user --force pushes a change and generates a repository
containing something like this:
* -- * -- B -- O -- O -- O (b0f80d0)
\
N -- N -- N refs/heads/GROOVY-9791 (1b08370)
You should already have received notification emails for all of the O
revisions, and so the following emails describe only the N revisions
from the common base, B.
Any revisions marked "omit" are not gone; other references still
refer to them. Any revisions marked "discard" are gone forever.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../groovy/classgen/AsmClassGenerator.java | 47 +++++++++++++---------
.../classgen/asm/sc/StaticTypesCallSiteWriter.java | 27 +------------
2 files changed, 29 insertions(+), 45 deletions(-)
[groovy] 01/01: GROOVY-9791: SC: not dynamic property for protected
field from diff pack
Posted by em...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY-9791
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 1b08370f888dbeb36f70b0f6cc2ce0fc0942a66a
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Fri Oct 23 14:00:23 2020 -0500
GROOVY-9791: SC: not dynamic property for protected field from diff pack
---
.../groovy/classgen/AsmClassGenerator.java | 47 +++++++++++++---------
.../classgen/asm/sc/StaticTypesCallSiteWriter.java | 33 +--------------
.../sc/FieldsAndPropertiesStaticCompileTest.groovy | 9 ++---
3 files changed, 33 insertions(+), 56 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
index 6d41bd3..246509e 100644
--- a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
+++ b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
@@ -109,7 +109,6 @@ import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.FieldVisitor;
import org.objectweb.asm.Label;
import org.objectweb.asm.MethodVisitor;
-import org.objectweb.asm.Opcodes;
import org.objectweb.asm.Type;
import org.objectweb.asm.util.TraceMethodVisitor;
@@ -943,31 +942,41 @@ public class AsmClassGenerator extends ClassGenerator {
return name;
}
- private static boolean isValidFieldNodeForByteCodeAccess(final FieldNode fn, final ClassNode accessingNode) {
- if (fn == null) return false;
- ClassNode declaringClass = fn.getDeclaringClass();
- // same class is always allowed access
- if (fn.isPublic() || declaringClass.equals(accessingNode)) return true;
- boolean samePackages = Objects.equals(declaringClass.getPackageName(), accessingNode.getPackageName());
- // protected means same class or same package, or subclass
- if (fn.isProtected() && (samePackages || accessingNode.isDerivedFrom(declaringClass))) {
- return true;
- }
- if (!fn.isPrivate()) {
- // package private is the only modifier left. It means same package is allowed, subclass not, same class is
- return samePackages;
- }
+ /**
+ * Determines if the given class can directly access the given field (via
+ * {@code GETFIELD}, {@code GETSTATIC}, etc. bytecode instructions).
+ */
+ public static boolean isFieldDirectlyAccessible(final FieldNode field, final ClassNode clazz) {
+ if (field == null) return false;
+
+ // a public field is accessible from anywhere
+ if (field.isPublic()) return true;
+
+ ClassNode declaringClass = field.getDeclaringClass();
+
+ // any field is accessible from the declaring class
+ if (clazz.equals(declaringClass)) return true;
+
+ // a private field isn't accessible beyond the declaring class
+ if (field.isPrivate()) return false;
+
+ // a protected field is accessible from any subclass of the declaring class
+ if (field.isProtected() && clazz.isDerivedFrom(declaringClass)) return true;
+
+ // a protected or package-private field is accessible from the declaring package
+ if (Objects.equals(clazz.getPackageName(), declaringClass.getPackageName())) return true;
+
return false;
}
public static FieldNode getDeclaredFieldOfCurrentClassOrAccessibleFieldOfSuper(final ClassNode accessingNode, final ClassNode current, final String name, final boolean skipCurrent) {
if (!skipCurrent) {
FieldNode currentClassField = current.getDeclaredField(name);
- if (isValidFieldNodeForByteCodeAccess(currentClassField, accessingNode)) return currentClassField;
+ if (isFieldDirectlyAccessible(currentClassField, accessingNode)) return currentClassField;
}
for (ClassNode node = current.getSuperClass(); node != null; node = node.getSuperClass()) {
FieldNode fn = node.getDeclaredField(name);
- if (isValidFieldNodeForByteCodeAccess(fn, accessingNode)) return fn;
+ if (isFieldDirectlyAccessible(fn, accessingNode)) return fn;
}
return null;
}
@@ -1143,8 +1152,8 @@ public class AsmClassGenerator extends ClassGenerator {
} else {
fieldNode = classNode.getDeclaredField(name);
- if (fieldNode == null && !isValidFieldNodeForByteCodeAccess(classNode.getField(name), classNode)) {
- // GROOVY-9501, GROOVY-9569
+ if (fieldNode == null && !isFieldDirectlyAccessible(classNode.getField(name), classNode)) {
+ // GROOVY-9501, GROOVY-9569, GROOVY-9650, GROOVY-9655, GROOVY-9665, GROOVY-9683
if (checkStaticOuterField(expression, name)) return;
}
}
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
index 9a99948..5ddf97a 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
@@ -53,7 +53,6 @@ import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;
-import java.util.Objects;
import static org.apache.groovy.ast.tools.ClassNodeUtils.getField;
import static org.apache.groovy.ast.tools.ExpressionUtils.isThisExpression;
@@ -551,7 +550,7 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter {
boolean makeGetField(final Expression receiver, final ClassNode receiverType, final String fieldName, final boolean safe, final boolean implicitThis) {
FieldNode field = getField(receiverType, fieldName); // GROOVY-7039: include interface constants
- if (field != null && isDirectAccessAllowed(field, controller.getClassNode())) {
+ if (field != null && AsmClassGenerator.isFieldDirectlyAccessible(field, controller.getClassNode())) {
CompileStack compileStack = controller.getCompileStack();
MethodVisitor mv = controller.getMethodVisitor();
ClassNode replacementType = field.getOriginType();
@@ -596,36 +595,6 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter {
return false;
}
- /**
- * Direct access is allowed from the declaring class of the field and sometimes from inner and peer types.
- *
- * @return {@code true} if GETFIELD or GETSTATIC is safe for given field and receiver
- */
- private static boolean isDirectAccessAllowed(final FieldNode field, final ClassNode receiver) {
- // first, direct access from anywhere for public fields
- if (field.isPublic()) return true;
-
- ClassNode declaringType = field.getDeclaringClass().redirect(), receiverType = receiver.redirect();
-
- // next, direct access from within the declaring class
- if (receiverType.equals(declaringType)) return true;
-
- if (field.isPrivate()) return false;
-
- // next, direct access from within the declaring package
- if (Objects.equals(receiver.getPackageName(), declaringType.getPackageName())) return true;
-
- // last, inner class access to outer class fields
- receiverType = receiverType.getOuterClass();
- while (receiverType != null) {
- if (receiverType.equals(declaringType)) {
- return true;
- }
- receiverType = receiverType.getOuterClass();
- }
- return false;
- }
-
@Override
public void makeSiteEntry() {
}
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
index 396a958..2fffda8 100644
--- a/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
@@ -18,7 +18,6 @@
*/
package org.codehaus.groovy.classgen.asm.sc
-import groovy.test.NotYetImplemented
import groovy.transform.stc.FieldsAndPropertiesSTCTest
final class FieldsAndPropertiesStaticCompileTest extends FieldsAndPropertiesSTCTest implements StaticCompilationTestSupport {
@@ -263,7 +262,7 @@ final class FieldsAndPropertiesStaticCompileTest extends FieldsAndPropertiesSTCT
}
}
- @NotYetImplemented
+ // GROOVY-9791
void testReadFieldFromSuperClass2() {
assertScript '''
package p
@@ -281,11 +280,11 @@ final class FieldsAndPropertiesStaticCompileTest extends FieldsAndPropertiesSTCT
assert new B().m() == 0
'''
def b = astTrees['B'][1]
- assert b.contains('GETFIELD A.x')
+ assert b.contains('GETFIELD p/A.x')
assert !b.contains('INVOKEINTERFACE groovy/lang/GroovyObject.getProperty')
}
- @NotYetImplemented
+ // GROOVY-9791
void testReadFieldFromSuperClass3() {
assertScript '''
package p
@@ -303,7 +302,7 @@ final class FieldsAndPropertiesStaticCompileTest extends FieldsAndPropertiesSTCT
assert B.m() == 0
'''
def b = astTrees['B'][1]
- assert b.contains('GETFIELD A.x')
+ assert b.contains('GETSTATIC p/A.x')
assert !b.contains('INVOKESTATIC org/codehaus/groovy/runtime/ScriptBytecodeAdapter.getGroovyObjectProperty')
}