Copilot commented on code in PR #1767:
URL: https://github.com/apache/groovy/pull/1767#discussion_r2216334564
##########
src/main/java/groovy/lang/MetaClassImpl.java:
##########
@@ -2089,10 +2091,25 @@ public void setProperty(final Object object, final
Object newValue) {
* Object#getClass and Map#isEmpty
*/
private boolean isSpecialProperty(final String name) {
- return "class".equals(name) || (isMap && ("empty".equals(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();
Review Comment:
[nitpick] This complex boolean expression combining multiple conditions
should be broken down into intermediate variables with descriptive names to
improve readability and maintainability.
```suggestion
boolean isNotAssignableFromOwner =
!owner.isAssignableFrom(method.getDeclaringClass().getTheClass());
boolean isNotInterface = !method.getDeclaringClass().isInterface();
return isNotAssignableFromOwner && isNotInterface;
```
##########
src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java:
##########
@@ -468,7 +468,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
Review Comment:
This TODO comment indicates incomplete implementation. Consider creating a
proper issue tracker entry and referencing it here, or implement the suggested
solution.
```suggestion
// TODO: [ISSUE-1234] Instead of checking for no getProperty
specialization, pass senderClass in ThreadLocal or another appropriate
mechanism.
```
##########
src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java:
##########
@@ -540,14 +541,19 @@ public static void setPropertySpreadSafe(Object
messageArgument, Class senderCla
// --------------------------------------------------------
public static Object getGroovyObjectProperty(Class senderClass,
GroovyObject receiver, String messageName) throws Throwable {
+ Class<?> receiverClass = receiver.getClass();
try {
- return receiver.getProperty(messageName);
- } catch (GroovyRuntimeException gre) {
- if (gre instanceof MissingPropertyException &&
senderClass!=receiver.getClass() && senderClass.isInstance(receiver)) {
- return receiver.getMetaClass().getProperty(senderClass,
receiver, messageName, false, false);
+// if (!receiverClass.getMethod("getProperty",
String.class).isDefault()) {
+ return receiver.getProperty(messageName);
+// }
Review Comment:
Commented-out code should be removed to avoid confusion and maintain code
cleanliness.
##########
src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java:
##########
@@ -540,14 +541,19 @@ public static void setPropertySpreadSafe(Object
messageArgument, Class senderCla
// --------------------------------------------------------
public static Object getGroovyObjectProperty(Class senderClass,
GroovyObject receiver, String messageName) throws Throwable {
+ Class<?> receiverClass = receiver.getClass();
try {
- return receiver.getProperty(messageName);
- } catch (GroovyRuntimeException gre) {
- if (gre instanceof MissingPropertyException &&
senderClass!=receiver.getClass() && senderClass.isInstance(receiver)) {
- return receiver.getMetaClass().getProperty(senderClass,
receiver, messageName, false, false);
+// if (!receiverClass.getMethod("getProperty",
String.class).isDefault()) {
Review Comment:
Commented-out code should be removed to avoid confusion and maintain code
cleanliness.
```suggestion
```
##########
src/main/java/groovy/lang/MetaClassImpl.java:
##########
@@ -2089,10 +2091,25 @@ public void setProperty(final Object object, final
Object newValue) {
* Object#getClass and Map#isEmpty
*/
private boolean isSpecialProperty(final String name) {
- return "class".equals(name) || (isMap && ("empty".equals(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
Review Comment:
[nitpick] The comment should clarify what 'class access method' means in
this context. It appears to refer to getter methods, which should be explicitly
stated.
```suggestion
// GROOVY-8283: non-private field that hides a getter method
(referred to here as a "class access method")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@groovy.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
