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