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 21:39:54 UTC
[groovy] branch GROOVY-9791 updated (5fd8e1a -> 36b2a6a)
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 5fd8e1a GROOVY-9791: SC: not dynamic property for protected field from diff pack
add 0af2058 extract method for property access error and remove duplicate test cases
add 5080304 add missing license headers
add 103e302 fix dependency verification metadata for rat
add 34049aa GROOVY-9792: Bump Spotbugs/Spotbugs annotations versions
add f2d40b6 GROOVY-9793: Bump JUnit4 version to 4.13.1 (includes security fix)
add 28bdcf3 GROOVY-9794: Bump picocli version to 4.5.2
add 9009402 remove dead code
add 6a2ca07 remove recursion from ClassNodeUtils#getField and reuse in ASM classgen
new 36b2a6a 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 (5fd8e1a)
\
N -- N -- N refs/heads/GROOVY-9791 (36b2a6a)
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:
gradle/verification-metadata.xml | 30 ++++++++++++-
.../apache/groovy/ast/tools/ClassNodeUtils.java | 43 ++++++++++--------
.../groovy/classgen/AsmClassGenerator.java | 27 +++---------
.../classgen/asm/sc/StaticTypesCallSiteWriter.java | 41 +++++++++--------
src/test-resources/fail/String_03.groovy | 19 ++++++++
src/test-resources/fail/String_04.groovy | 19 ++++++++
.../asm/sc/StaticCompileFieldAccessTest.groovy | 51 +---------------------
versions.properties | 8 ++--
8 files changed, 126 insertions(+), 112 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 36b2a6aad187d79eb72919e782b3a24dcaf00879
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sun Oct 25 13:21:43 2020 -0500
GROOVY-9791: SC: not dynamic property for protected field from diff pack
---
.../groovy/classgen/AsmClassGenerator.java | 43 +++++++++++++---------
.../classgen/asm/sc/StaticTypesCallSiteWriter.java | 33 +----------------
.../sc/FieldsAndPropertiesStaticCompileTest.groovy | 9 ++---
3 files changed, 31 insertions(+), 54 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
index 022d44b..0fd7c0c 100644
--- a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
+++ b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
@@ -943,27 +943,36 @@ 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 fieldName, final boolean skipCurrent) {
return getField(current, fieldName, fieldNode ->
- (!skipCurrent || !current.equals(fieldNode.getDeclaringClass()))
- && isValidFieldNodeForByteCodeAccess(fieldNode, accessingNode)
+ (!skipCurrent || !current.equals(fieldNode.getDeclaringClass())) && isFieldDirectlyAccessible(fieldNode, accessingNode)
);
}
@@ -1138,7 +1147,7 @@ public class AsmClassGenerator extends ClassGenerator {
} else {
fieldNode = classNode.getDeclaredField(name);
- if (fieldNode == null && !isValidFieldNodeForByteCodeAccess(classNode.getField(name), classNode)) {
+ if (fieldNode == null && !isFieldDirectlyAccessible(getField(classNode, name), classNode)) {
// GROOVY-9501, GROOVY-9569, GROOVY-9650, GROOVY-9655, GROOVY-9665, GROOVY-9683, GROOVY-9695
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 8ff1407..df836a3 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;
@@ -550,7 +549,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();
@@ -595,36 +594,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')
}