[ 
https://issues.apache.org/jira/browse/GROOVY-8283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18008100#comment-18008100
 ] 

ASF GitHub Bot commented on GROOVY-8283:
----------------------------------------

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")
   ```





> Field shadowing not considered in STC
> -------------------------------------
>
>                 Key: GROOVY-8283
>                 URL: https://issues.apache.org/jira/browse/GROOVY-8283
>             Project: Groovy
>          Issue Type: Bug
>          Components: Static compilation, Static Type Checker
>    Affects Versions: 2.4.12
>            Reporter: Daniil Ovchinnikov
>            Assignee: Eric Milles
>            Priority: Major
>              Labels: breaking
>
> {code}
> import groovy.transform.CompileStatic
> @CompileStatic class A {}
> @CompileStatic class B {}
> @CompileStatic class Parent {
>   protected A ff = new A()
>   A getFf() { ff }
> }
> @CompileStatic class Child extends Parent {
>   protected B ff = new B()
> }
> @CompileStatic class Usage extends Child {
>   def test() {
>     println ff        // A@id
>     println getFf()   // A@id
>     println this.@ff  // B@id
>   }
>   def test2() {
>     I.wantsB(ff)        // 
> ScriptBytecodeAdapter.castToType(((Usage)this).getFf(), B.class)) is 
> generated (wrong)
>     I.wantsB(getFf())   // [STC] - Cannot find matching method I#wantsB(A)
>     I.wantsB(this.@ff)  // [STC] - Cannot find matching method I#wantsB(A) 
> (wrong)
>   }
> }
> @CompileStatic class I {
>   static void wantsB(B b) {}
> }
> new Usage().test()
> {code}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to