[ 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)