From ec44e3874762c47910c511509aaf49bbdc5da873 Mon Sep 17 00:00:00 2001 From: Eric Milles Date: Tue, 29 Oct 2024 08:48:40 -0500 Subject: [PATCH 1/4] GROOVY-8283: field hides getter or setter of super class (not interface) --- src/main/java/groovy/lang/MetaClassImpl.java | 43 ++++-- src/test/groovy/bugs/Groovy8283.groovy | 132 +++++++++++++++++++ 2 files changed, 161 insertions(+), 14 deletions(-) create mode 100644 src/test/groovy/bugs/Groovy8283.groovy diff --git a/src/main/java/groovy/lang/MetaClassImpl.java b/src/main/java/groovy/lang/MetaClassImpl.java index 427d4b4e608..09c127fe9e4 100644 --- a/src/main/java/groovy/lang/MetaClassImpl.java +++ b/src/main/java/groovy/lang/MetaClassImpl.java @@ -1824,18 +1824,18 @@ public Object getProperty(final Class sender, final Object object, final String //---------------------------------------------------------------------- Tuple2 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; } } @@ -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) { } } @@ -1946,14 +1946,14 @@ public void setProperty(Object object, Object newValue) { //---------------------------------------------------------------------- Tuple2 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; } //------------------------------------------------------------------ @@ -1976,8 +1976,8 @@ public void setProperty(Object object, Object newValue) { //------------------------------------------------------------------ // non-public field //------------------------------------------------------------------ - if (mp != null) { - return mp; + if (prop != null) { + return prop; } } @@ -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 createMetaMethodAndMetaProperty(final Class sender, final String name, final boolean useSuper, final boolean isStatic) { MetaMethod method = null; MetaProperty mp = getMetaProperty(sender, name, useSuper, isStatic); @@ -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()) { diff --git a/src/test/groovy/bugs/Groovy8283.groovy b/src/test/groovy/bugs/Groovy8283.groovy new file mode 100644 index 00000000000..ec99829e2dd --- /dev/null +++ b/src/test/groovy/bugs/Groovy8283.groovy @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy.bugs + +import org.junit.Test + +import static groovy.test.GroovyAssert.assertScript + +final class Groovy8283 { + + @Test + void testReadFieldPropertyShadowing() { + def shell = new GroovyShell() + shell.parse '''package p + class A {} + class B {} + class C { + protected A foo = new A() + A getFoo() { return foo } + } + class D extends C { + protected B foo = new B() // hides A#foo; should hide A#getFoo in subclasses + } + ''' + assertScript shell, '''import p.* + class E extends D { + void test() { + assert foo.class == B + assert this.foo.class == B + assert this.@foo.class == B + assert this.getFoo().getClass() == A + + def that = new E() + assert that.foo.class == B + assert that.@foo.class == B + assert that.getFoo().getClass() == A + } + } + + new E().test() + assert new E().foo.class == A // not the field from this perspective + ''' + } + + @Test + void testWriteFieldPropertyShadowing() { + def shell = new GroovyShell() + shell.parse '''package p + class A {} + class B {} + class C { + boolean setter + protected A foo = new A() + A getFooA() { return this.@foo } + void setFoo(A a) { setter = true; this.@foo = a } + } + class D extends C { + protected B foo = new B() // hides A#foo; should hide A#setFoo in subclasses + B getFooB() { return this.@foo } + } + ''' + assertScript shell, '''import p.* + class E extends D { + void test1() { + foo = null + assert !setter + assert fooA != null + assert fooB == null + } + void test2() { + this.foo = null + assert !setter + assert fooA != null + assert fooB == null + } + void test3() { + this.@foo = null + assert !setter + assert fooA != null + assert fooB == null + } + void test4() { + this.setFoo(null) + assert setter + assert fooA == null + assert fooB != null + } + void test5() { + def that = new E() + that.foo = null + assert !that.setter + assert that.fooA != null + assert that.fooB == null + + that = new E() + that.@foo = null + assert !that.setter + assert that.fooA != null + assert that.fooB == null + + that = new E() + that.setFoo(null) + assert that.setter + assert that.fooA == null + assert that.fooB != null + } + } + + new E().test1() + new E().test2() + new E().test3() + new E().test4() + new E().test5() + ''' + } +} From a69e055fce2551923f4f300a627a5dcafcfe4065 Mon Sep 17 00:00:00 2001 From: Eric Milles Date: Thu, 21 Nov 2024 13:44:14 -0600 Subject: [PATCH 2/4] GROOVY-8283: STC: field hides getter of super class --- .../stc/StaticTypeCheckingVisitor.java | 18 +++++--- src/test/groovy/bugs/Groovy8283.groovy | 44 +++++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java index f0cdc32950b..131a4c6cfac 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -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))) @@ -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); @@ -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) @@ -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); diff --git a/src/test/groovy/bugs/Groovy8283.groovy b/src/test/groovy/bugs/Groovy8283.groovy index ec99829e2dd..9e75a720b2f 100644 --- a/src/test/groovy/bugs/Groovy8283.groovy +++ b/src/test/groovy/bugs/Groovy8283.groovy @@ -56,6 +56,50 @@ final class Groovy8283 { new E().test() assert new E().foo.class == A // not the field from this perspective ''' + assertScript shell, '''import p.* + class E extends D { + @groovy.transform.ASTTest(phase=org.codehaus.groovy.control.CompilePhase.INSTRUCTION_SELECTION, value={ + def typeof = { label -> lookup(label)[0].getExpression().getNodeMetaData(org.codehaus.groovy.transform.stc.StaticTypesMarker.INFERRED_TYPE).toString(false) } + + assert typeof('implicit' ) == 'p.B' + assert typeof('explicit' ) == 'p.B' + assert typeof('attribute' ) == 'p.B' + assert typeof('methodCall' ) == 'p.A' + + assert typeof('property' ) == 'p.B' + assert typeof('attribute2' ) == 'p.B' + assert typeof('methodCall2') == 'p.A' + }) + @groovy.transform.TypeChecked + void test() { + implicit: + def a = foo + explicit: + def b = this.foo + attribute: + def c = this.@foo + methodCall: + def d = this.getFoo() + + def that = new E() + property: + def x = that.foo + attribute2: + def y = that.@foo + methodCall2: + def z = that.getFoo() + } + } + + @groovy.transform.TypeChecked + void test() { + @groovy.transform.ASTTest(phase=org.codehaus.groovy.control.CompilePhase.INSTRUCTION_SELECTION, value={ + def type = node.getNodeMetaData(org.codehaus.groovy.transform.stc.StaticTypesMarker.INFERRED_TYPE) + assert type.toString(false) == 'p.A' + }) + def a = new E().foo + } + ''' } @Test From 981928053477e7d7790e0b6000e7a4dd5b45b583 Mon Sep 17 00:00:00 2001 From: Eric Milles Date: Fri, 22 Nov 2024 12:02:46 -0600 Subject: [PATCH 3/4] GROOVY-8283: SC: field hides getter of super class --- .../asm/sc/StaticTypesCallSiteWriter.java | 7 +++ src/test/groovy/bugs/Groovy8283.groovy | 54 ++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java index cd641ceb407..5a014a4f257 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java @@ -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; @@ -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); diff --git a/src/test/groovy/bugs/Groovy8283.groovy b/src/test/groovy/bugs/Groovy8283.groovy index 9e75a720b2f..fc34722f4a7 100644 --- a/src/test/groovy/bugs/Groovy8283.groovy +++ b/src/test/groovy/bugs/Groovy8283.groovy @@ -35,7 +35,7 @@ final class Groovy8283 { A getFoo() { return foo } } class D extends C { - protected B foo = new B() // hides A#foo; should hide A#getFoo in subclasses + protected B foo = new B() } ''' assertScript shell, '''import p.* @@ -56,6 +56,22 @@ final class Groovy8283 { new E().test() assert new E().foo.class == A // not the field from this perspective ''' + } + + @Test + void testReadFieldPropertyShadowing2() { + def shell = new GroovyShell() + shell.parse '''package p + class A {} + class B {} + class C { + protected A foo = new A() + A getFoo() { return foo } + } + class D extends C { + protected B foo = new B() + } + ''' assertScript shell, '''import p.* class E extends D { @groovy.transform.ASTTest(phase=org.codehaus.groovy.control.CompilePhase.INSTRUCTION_SELECTION, value={ @@ -102,6 +118,42 @@ final class Groovy8283 { ''' } + @Test + void testReadFieldPropertyShadowing3() { + def shell = GroovyShell.withConfig { + ast(groovy.transform.CompileStatic) + } + shell.parse '''package p + class A {} + class B {} + class C { + protected A foo = new A() + A getFoo() { return foo } + } + class D extends C { + protected B foo = new B() + } + ''' + assertScript shell, '''import p.* + class E extends D { + void test() { + assert foo.class == B + assert this.foo.class == B + assert this.@foo.class == B + assert this.getFoo().getClass() == A + + def that = new E() + assert that.foo.class == B + assert that.@foo.class == B + assert that.getFoo().getClass() == A + } + } + + new E().test() + assert new E().foo.class == A // not the field from this perspective + ''' + } + @Test void testWriteFieldPropertyShadowing() { def shell = new GroovyShell() From a9a1fc44774a3e311d9c7a709e899c03affbcbaf Mon Sep 17 00:00:00 2001 From: Eric Milles Date: Mon, 9 Dec 2024 12:31:11 -0600 Subject: [PATCH 4/4] GROOVY-8283: propagate sender class for `getProperty` and `setProperty` --- .../org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java | 5 +++-- src/test/groovy/bugs/Groovy8283.groovy | 6 ++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java b/src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java index d8d6020c707..749636e3436 100644 --- a/src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java +++ b/src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java @@ -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 { @@ -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 { diff --git a/src/test/groovy/bugs/Groovy8283.groovy b/src/test/groovy/bugs/Groovy8283.groovy index fc34722f4a7..2d7f8b3dbb7 100644 --- a/src/test/groovy/bugs/Groovy8283.groovy +++ b/src/test/groovy/bugs/Groovy8283.groovy @@ -223,6 +223,12 @@ final class Groovy8283 { new E().test3() new E().test4() new E().test5() + + def e = new E() + e.foo = null // not the field from this perspective + assert e.setter + assert e.fooA == null + assert e.fooB != null ''' } }