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 2022/01/22 22:36:29 UTC
[groovy] branch master updated: GROOVY-6277: SC: prefer accessible field over inaccessible getter method
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new de88bf2 GROOVY-6277: SC: prefer accessible field over inaccessible getter method
de88bf2 is described below
commit de88bf281b6eef3265eb9bad5d77ada6b8da1794
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sat Jan 22 15:48:43 2022 -0600
GROOVY-6277: SC: prefer accessible field over inaccessible getter method
---
.../groovy/classgen/AsmClassGenerator.java | 37 +++++++++-------------
.../classgen/asm/sc/StaticTypesCallSiteWriter.java | 12 ++++---
.../transform/stc/StaticTypeCheckingVisitor.java | 26 ++++++++-------
.../stc/FieldsAndPropertiesSTCTest.groovy | 14 ++++++++
4 files changed, 51 insertions(+), 38 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
index 32c9ce5..d54dd1d 100644
--- a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
+++ b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
@@ -121,6 +121,7 @@ import org.objectweb.asm.util.TraceMethodVisitor;
import java.io.PrintWriter;
import java.io.Writer;
+import java.lang.reflect.Modifier;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
@@ -1021,36 +1022,28 @@ public class AsmClassGenerator extends ClassGenerator {
}
/**
- * @deprecated use {@link #isFieldDirectlyAccessible(FieldNode, ClassNode)} instead.
- */
- @Deprecated
- public static boolean isValidFieldNodeForByteCodeAccess(final FieldNode field, final ClassNode accessingClass) {
- return isFieldDirectlyAccessible(field, accessingClass);
- }
-
- /**
* 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;
+ public static boolean isFieldDirectlyAccessible(final FieldNode field, final ClassNode accessingClass) {
+ return field != null && isMemberDirectlyAccessible(field.getModifiers(), field.getDeclaringClass(), accessingClass);
+ }
- ClassNode declaringClass = field.getDeclaringClass();
+ public static boolean isMemberDirectlyAccessible(final int modifiers, final ClassNode declaringClass, final ClassNode accessingClass) {
+ // a public member is accessible from anywhere
+ if (Modifier.isPublic(modifiers)) return true;
- // any field is accessible from the declaring class
- if (clazz.equals(declaringClass)) return true;
+ // any member is accessible from the declaring class
+ if (accessingClass.equals(declaringClass)) return true;
- // a private field isn't accessible beyond the declaring class
- if (field.isPrivate()) return false;
+ // a private member isn't accessible beyond the declaring class
+ if (Modifier.isPrivate(modifiers)) return false;
- // a protected field is accessible from any subclass of the declaring class
- if (field.isProtected() && clazz.isDerivedFrom(declaringClass)) return true;
+ // a protected member is accessible from any subclass of the declaring class
+ if (Modifier.isProtected(modifiers) && accessingClass.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;
+ // a protected or package-private member is accessible from the declaring package
+ if (Objects.equals(accessingClass.getPackageName(), declaringClass.getPackageName())) return true;
return false;
}
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 016899f..f6d53cc 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
@@ -472,8 +472,8 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter {
return false;
}
- // GROOVY-5561: if two files are compiled in the same source unit
- // and that one references the other, the getters for properties have not been
+ // GROOVY-5561: if two files are compiled in the same source unit and
+ // one references the other, the getters for properties have not been
// generated by the compiler yet (generated by the Verifier)
PropertyNode propertyNode = receiverType.getProperty(propertyName);
if (getterNode == null && propertyNode != null) {
@@ -482,15 +482,17 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter {
getterName = prefix + capitalize(propertyName);
getterNode = new MethodNode(
getterName,
- ACC_PUBLIC,
+ ACC_PUBLIC | (propertyNode.isStatic() ? ACC_STATIC : 0),
propertyNode.getOriginType(),
Parameter.EMPTY_ARRAY,
ClassNode.EMPTY_ARRAY,
EmptyStatement.INSTANCE);
getterNode.setDeclaringClass(receiverType);
- if (propertyNode.isStatic()) getterNode.setModifiers(ACC_PUBLIC + ACC_STATIC);
}
if (getterNode != null) {
+ if (!AsmClassGenerator.isMemberDirectlyAccessible(getterNode.getModifiers(), getterNode.getDeclaringClass(), controller.getClassNode())) {
+ return false; // GROOVY-6277
+ }
MethodCallExpression call = callX(receiver, getterName);
call.setImplicitThis(implicitThis);
call.setMethodTarget(getterNode);
@@ -506,7 +508,7 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter {
}
}
- // check direct interfaces (GROOVY-7149)
+ // GROOVY-7149: check direct interfaces
for (ClassNode node : receiverType.getInterfaces()) {
if (makeGetPropertyWithGetter(receiver, node, propertyName, safe, implicitThis)) {
return true;
diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
index e42bf45..1426e5c 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -1484,7 +1484,10 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
ClassNode rawType = objectExpressionType.getPlainNodeReference();
inferDiamondType((ConstructorCallExpression) objectExpression, rawType);
}
- List<ClassNode> enclosingTypes = typeCheckingContext.getEnclosingClassNodes();
+ // enclosing excludes classes that skip STC
+ Set<ClassNode> enclosingTypes = new LinkedHashSet<>();
+ enclosingTypes.add(typeCheckingContext.getEnclosingClassNode());
+ enclosingTypes.addAll(enclosingTypes.iterator().next().getOuterClasses());
boolean staticOnlyAccess = isClassClassNodeWrappingConcreteType(objectExpressionType);
if (staticOnlyAccess && "this".equals(propertyName)) {
@@ -1577,7 +1580,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
// prefer explicit getter or setter over property if receiver is not 'this'
if (property == null || !enclosingTypes.contains(receiverType)) {
if (readMode) {
- if (getter != null) {
+ if (getter != null && hasAccessToMember(enclosingTypes.iterator().next(), getter.getDeclaringClass(), getter.getModifiers())) {
ClassNode returnType = inferReturnTypeGenerics(current, getter, ArgumentListExpression.EMPTY_ARGUMENTS);
storeInferredTypeForPropertyExpression(pexp, returnType);
storeTargetMethod(pexp, getter);
@@ -1696,14 +1699,16 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
return foundGetterOrSetter;
}
- private static boolean hasAccessToField(final ClassNode accessor, final FieldNode field) {
- if (field.isPublic() || accessor.equals(field.getDeclaringClass())) {
+ private static boolean hasAccessToMember(final ClassNode accessor, final ClassNode receiver, final int modifiers) {
+ if (Modifier.isPublic(modifiers)
+ || accessor.equals(receiver)
+ || accessor.getOuterClasses().contains(receiver)) {
return true;
}
- if (field.isProtected()) {
- return accessor.isDerivedFrom(field.getDeclaringClass());
+ if (Modifier.isProtected(modifiers)) {
+ return accessor.isDerivedFrom(receiver);
} else {
- return !field.isPrivate() && Objects.equals(accessor.getPackageName(), field.getDeclaringClass().getPackageName());
+ return !Modifier.isPrivate(modifiers) && Objects.equals(accessor.getPackageName(), receiver.getPackageName());
}
}
@@ -1815,7 +1820,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
private boolean storeField(final FieldNode field, final PropertyExpression expressionToStoreOn, final ClassNode receiver, final ClassCodeVisitorSupport visitor, final String delegationData, final boolean lhsOfAssignment) {
if (visitor != null) visitor.visitField(field);
checkOrMarkPrivateAccess(expressionToStoreOn, field, lhsOfAssignment);
- boolean accessible = hasAccessToField(isSuperExpression(expressionToStoreOn.getObjectExpression()) ? typeCheckingContext.getEnclosingClassNode() : receiver, field);
+ boolean accessible = hasAccessToMember(isSuperExpression(expressionToStoreOn.getObjectExpression()) ? typeCheckingContext.getEnclosingClassNode() : receiver, field.getDeclaringClass(), field.getModifiers());
if (expressionToStoreOn instanceof AttributeExpression) { // TODO: expand to include PropertyExpression
if (!accessible) {
@@ -3079,7 +3084,6 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
} else {
for (int i = 0, n = closureParams.length; i < n; i += 1) {
Parameter closureParam = closureParams[i];
- ClassNode declaredType = closureParam.getOriginType();
ClassNode inferredType = OBJECT_TYPE;
if (i < inferred.length - 1 || inferred.length == n) {
inferredType = inferred[i];
@@ -4097,9 +4101,9 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
// char c = (char) ...
} else if (sourceIsNull && isPrimitiveType(targetType) && !isPrimitiveBoolean(targetType)) {
return false;
- } else if ((expressionType.getModifiers() & Opcodes.ACC_FINAL) == 0 && targetType.isInterface()) {
+ } else if (!Modifier.isFinal(expressionType.getModifiers()) && targetType.isInterface()) {
return true;
- } else if ((targetType.getModifiers() & Opcodes.ACC_FINAL) == 0 && expressionType.isInterface()) {
+ } else if (!Modifier.isFinal(targetType.getModifiers()) && expressionType.isInterface()) {
return true;
} else if (!isAssignableTo(targetType, expressionType) && !implementsInterfaceOrIsSubclassOf(expressionType, targetType)) {
return false;
diff --git a/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy b/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
index c9d56cf..a4ca030 100644
--- a/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
+++ b/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
@@ -850,6 +850,20 @@ class FieldsAndPropertiesSTCTest extends StaticTypeCheckingTestCase {
'''
}
+ // GROOVY-6277
+ void testPublicFieldVersusPrivateGetter() {
+ assertScript '''
+ class Foo {
+ private getWho() { 'Foo' }
+ }
+ class Bar extends Foo {
+ public who = 'Bar'
+ }
+ String result = new Bar().who
+ assert result == 'Bar'
+ '''
+ }
+
// GROOVY-6610
void testPrivateStaticFieldAccessBeforeThis() {
assertScript '''