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 <[email protected]> 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') }
