Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GROOVY-8283: field hides getter of super class (not interface) #1767

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

GROOVY-8283: field hides getter of super class (not interface) #1767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec44e38
GROOVY-8283: field hides getter or setter of super class (not interface)
eric-milles Oct 29, 2024
a69e055
GROOVY-8283: STC: field hides getter of super class
eric-milles Nov 21, 2024
9819280
GROOVY-8283: SC: field hides getter of super class
eric-milles Nov 22, 2024
a9a1fc4
GROOVY-8283: propagate sender class for `getProperty` and `setProperty`
eric-milles Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 29 additions & 14 deletions src/main/java/groovy/lang/MetaClassImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1824,18 +1824,18 @@ public Object getProperty(final Class sender, final Object object, final String
//----------------------------------------------------------------------
Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, name, useSuper, isStatic);
MetaMethod method = methodAndProperty.getV1();
MetaProperty prop = methodAndProperty.getV2();

if (method == null || isSpecialProperty(name)) {
if (method == null || isSpecialProperty(name) || isVisibleProperty(prop, method, sender)) {
//------------------------------------------------------------------
// public field
//------------------------------------------------------------------
MetaProperty mp = methodAndProperty.getV2();
if (mp != null && mp.isPublic()) {
if (prop != null && prop.isPublic()) {
try {
return mp.getProperty(object);
return prop.getProperty(object);
} catch (GroovyRuntimeException e) {
// can't access the field directly but there may be a getter
mp = null;
prop = null;
}
}

Expand All @@ -1849,9 +1849,9 @@ public Object getProperty(final Class sender, final Object object, final String
//------------------------------------------------------------------
// non-public field
//------------------------------------------------------------------
if (mp != null) {
if (prop != null) {
try {
return mp.getProperty(object);
return prop.getProperty(object);
} catch (GroovyRuntimeException e) {
}
}
Expand Down Expand Up @@ -1946,14 +1946,14 @@ public void setProperty(Object object, Object newValue) {
//----------------------------------------------------------------------
Tuple2<MetaMethod, MetaProperty> methodAndProperty = createMetaMethodAndMetaProperty(sender, name, useSuper, isStatic);
MetaMethod method = methodAndProperty.getV1();
MetaProperty prop = methodAndProperty.getV2();

if (method == null || isSpecialProperty(name)) {
if (method == null || isSpecialProperty(name) || isVisibleProperty(prop, method, sender)) {
//------------------------------------------------------------------
// public field
//------------------------------------------------------------------
MetaProperty mp = methodAndProperty.getV2();
if (mp != null && mp.isPublic()) {
return mp;
if (prop != null && prop.isPublic()) {
return prop;
}

//------------------------------------------------------------------
Expand All @@ -1976,8 +1976,8 @@ public void setProperty(Object object, Object newValue) {
//------------------------------------------------------------------
// non-public field
//------------------------------------------------------------------
if (mp != null) {
return mp;
if (prop != null) {
return prop;
}
}

Expand Down Expand Up @@ -2112,6 +2112,21 @@ private boolean isSpecialProperty(final String name) {
return "class".equals(name) || (isMap && ("empty".equals(name)));
}

private boolean isVisibleProperty(final MetaProperty field, final MetaMethod method, final Class<?> sender) {
if (!(field instanceof CachedField)) return false;

if (field.isPrivate()) return false;

Class<?> owner = ((CachedField) field).getDeclaringClass();
// ensure access originates within the type hierarchy of the field owner
if (owner.equals(sender) || !owner.isAssignableFrom(sender)) return false;

if (!field.isPublic() && !field.isProtected() && !inSamePackage(owner, sender)) return false;

// GROOVY-8283: non-private field that hides class access method
return !owner.isAssignableFrom(method.getDeclaringClass().getTheClass()) && !method.getDeclaringClass().isInterface();
}

private Tuple2<MetaMethod, MetaProperty> createMetaMethodAndMetaProperty(final Class sender, final String name, final boolean useSuper, final boolean isStatic) {
MetaMethod method = null;
MetaProperty mp = getMetaProperty(sender, name, useSuper, isStatic);
Expand Down Expand Up @@ -2772,7 +2787,7 @@ public void setProperty(final Class sender, final Object object, final String na
//----------------------------------------------------------------------
// field
//----------------------------------------------------------------------
if (method == null && field != null
if (field != null && (method == null || isVisibleProperty(field, method, sender))
&& (!isMap || isStatic // GROOVY-8065
|| field.isPublic())) { // GROOVY-11367
if (!field.isFinal()) {
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@

import static org.apache.groovy.ast.tools.ClassNodeUtils.getField;
import static org.apache.groovy.ast.tools.ClassNodeUtils.getMethod;
import static org.apache.groovy.ast.tools.ClassNodeUtils.isSubtype;
import static org.apache.groovy.ast.tools.ExpressionUtils.isThisExpression;
import static org.apache.groovy.util.BeanUtils.capitalize;
import static org.codehaus.groovy.ast.ClassHelper.CLASS_Type;
Expand Down Expand Up @@ -454,6 +455,12 @@ private boolean makeGetPropertyWithGetter(final Expression receiver, final Class
if (!AsmClassGenerator.isMemberDirectlyAccessible(getterNode.getModifiers(), getterNode.getDeclaringClass(), controller.getClassNode())) {
return false; // GROOVY-6277
}
FieldNode fieldNode = getField(receiverType, propertyName);
if (fieldNode != null && !getterNode.getDeclaringClass().equals(fieldNode.getDeclaringClass())
&& isSubtype(getterNode.getDeclaringClass(), fieldNode.getDeclaringClass()) // field found before getter (starting from receiver type)
&& AsmClassGenerator.isMemberDirectlyAccessible(fieldNode.getModifiers(), fieldNode.getDeclaringClass(), controller.getClassNode())) {
return false; // GROOVY-8283
}
MethodCallExpression call = callX(receiver, getterName);
call.setImplicitThis(implicitThis);
call.setMethodTarget(getterNode);
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,8 @@ public static void setPropertyOnSuperSpreadSafe(Object messageArgument, Class se

public static Object getProperty(Class senderClass, Object receiver, String messageName) throws Throwable {
try {
if (receiver instanceof GroovyObject) {
if (receiver instanceof GroovyObject && !receiver.getClass().getMethod("getProperty", String.class).isDefault()) {
// TODO: instead of checking for no getProperty specialization, pass senderClass in ThreadLocal or something
var groovyObject = (GroovyObject) receiver;
return groovyObject.getProperty(messageName);
} else {
Expand Down Expand Up @@ -499,7 +500,7 @@ public static Object getPropertySpreadSafe(Class senderClass, Object receiver, S

public static void setProperty(Object messageArgument, Class senderClass, Object receiver, String messageName) throws Throwable {
try {
if (receiver instanceof GroovyObject) {
if (receiver instanceof GroovyObject && !receiver.getClass().getMethod("setProperty", String.class, Object.class).isDefault()) {
var groovyObject = (GroovyObject) receiver;
groovyObject.setProperty(messageName, messageArgument);
} else {
Expand Down
18 changes: 13 additions & 5 deletions src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1664,11 +1664,13 @@ && hasAccessToMember(typeCheckingContext.getEnclosingClassNode(), method.getDecl
else if (field != null && enclosingTypes.contains(current) && storeField(field, pexp, receiverType, visitor, receiver.getData(), !readMode)) {
return true;
}
// GROOVY-8283: accessible field shadows getter of super class; GROOVY-11381: inaccessible field stops the loop, so search supers for getter
boolean checkUp = field != null && !hasAccessToMember(typeCheckingContext.getEnclosingClassNode(), field.getDeclaringClass(), field.getModifiers());

MethodNode getter = current.getGetterMethod(isserName);
MethodNode getter = getGetterMethod(current, isserName, checkUp);
getter = allowStaticAccessToMember(getter, staticOnly);
if (getter == null) {
getter = current.getGetterMethod(getterName);
getter = getGetterMethod(current, getterName, checkUp);
getter = allowStaticAccessToMember(getter, staticOnly);
}
if (getter != null && ((publicOnly && (!getter.isPublic() || "class".equals(propertyName) || "empty".equals(propertyName)))
Expand All @@ -1681,8 +1683,8 @@ else if (field != null && enclosingTypes.contains(current) && storeField(field,

PropertyNode property = current.getProperty(propertyName);
property = allowStaticAccessToMember(property, staticOnly);
// prefer explicit getter or setter over property if receiver is not 'this'
if (property == null || !enclosingTypes.contains(receiverType)) {
// prefer explicit getter/setter for out-of-scope references
if (property == null || !enclosingTypes.contains(current)) {
if (readMode) {
if (getter != null) {
ClassNode returnType = inferReturnTypeGenerics(receiverType, getter, ArgumentListExpression.EMPTY_ARGUMENTS);
Expand Down Expand Up @@ -1828,6 +1830,12 @@ else if (field != null && enclosingTypes.contains(current) && storeField(field,
return foundGetterOrSetter;
}

private static MethodNode getGetterMethod(final ClassNode classNode, final String getterName, final boolean searchSupers) {
MethodNode getter = classNode.getGetterMethod(getterName, searchSupers);
if (getter != null && (getter.getModifiers() & Opcodes.ACC_BRIDGE) != 0) getter = null; // GROOVY-11341
return getter;
}

private static boolean hasAccessToMember(final ClassNode accessor, final ClassNode receiver, final int modifiers) {
if (Modifier.isPublic(modifiers)
|| accessor.equals(receiver)
Expand Down Expand Up @@ -2714,7 +2722,7 @@ private static MethodNode findPropertyMethod(final ClassNode type, final String
node.setDeclaringClass(pn.getDeclaringClass());
node.setSynthetic(true);
return node;
} else if (name.equals(pn.getSetterNameOrDefault()) && !Modifier.isFinal(pn.getModifiers())) {
} else if (name.equals(pn.getSetterNameOrDefault()) && !pn.isFinal()) {
MethodNode node = new MethodNode(name, Opcodes.ACC_PUBLIC | (pn.isStatic() ? Opcodes.ACC_STATIC : 0), VOID_TYPE, new Parameter[]{new Parameter(pn.getType(), pn.getName())}, ClassNode.EMPTY_ARRAY, null);
node.setDeclaringClass(pn.getDeclaringClass());
node.setSynthetic(true);
Expand Down
Loading
Loading