paulk-asert commented on PR #3008:
URL: https://github.com/apache/groovy/pull/3008#issuecomment-6356955487
AI suggestion, I haven't checked myself yet:
> The fix works for the reported case and `def x = Float; x.NaN`, but the
restructuring regresses a raw-`Class` receiver, which is the
`ClosuresStaticCompileTest.testClassClosureDelegateProperty` failure on every
JDK job:
>
> Foo.with { assert name == 'Foo' } // PR gives 'java.lang.Class'
>
> The delegate's property owner is raw `Class<T>` (no concrete type
argument). The new `isClassType(receiverType)` check sends it down the
class-receiver branch, and `classX(receiverType)` then becomes the literal
`Class.class`, so `getName()` runs on that. Substituting a class literal is
only valid when the type argument is concrete.
>
> The underlying bug looks narrower than the rewrite:
`makeGetPropertyWithGetter` only rejects an instance getter when `receiver
instanceof ClassExpression`, so a `Class`-valued receiver that isn't a literal
(cast, variable, `def`) lets the instance `isNaN()` beat the static field
`NaN`. The cast case additionally isn't flagged as a class receiver because the
checker records `Float` as the property owner with no inferred type.
>
> A ~20-line alternative on master: pass the existing `isClassReceiver` flag
into `makeGetPropertyWithGetter` and reject instance getters for it, the same
way as for a class literal, and in `getPropertyOwnerType` also set the flag
when the receiver's declared type is `Class<X>` with `X` the recorded owner:
>
> ```diff
> ---
a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
> +++
b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
> @@ -178,7 +178,7 @@ public class StaticTypesCallSiteWriter extends
CallSiteWriter {
> // type checker no longer admits either name unless a genuine
member backs it, and
> // rewriting an admitted member to size() produced silently wrong
values
>
> - if (makeGetPropertyWithGetter(receiver, receiverType,
propertyName, safe, implicitThis)) return;
> + if (makeGetPropertyWithGetter(receiver, receiverType,
propertyName, safe, implicitThis, isClassReceiver[0])) return;
>
> boolean isStaticProperty = (receiver instanceof ClassExpression
> && (receiverType.isDerivedFrom(receiver.getType()) ||
receiverType.implementsInterface(receiver.getType())));
> @@ -399,6 +399,15 @@ public class StaticTypesCallSiteWriter extends
CallSiteWriter {
> }
>
> private boolean makeGetPropertyWithGetter(final Expression receiver,
final ClassNode receiverType, final String propertyName, final boolean safe,
final boolean implicitThis) {
> + return makeGetPropertyWithGetter(receiver, receiverType,
propertyName, safe, implicitThis, false);
> + }
> +
> + /**
> + * @param classReceiver whether the receiver is a {@code Class} value
whose type
> + * argument is {@code receiverType}; like a class literal, it
has only the
> + * static accessors of that type
> + */
> + private boolean makeGetPropertyWithGetter(final Expression receiver,
final ClassNode receiverType, final String propertyName, final boolean safe,
final boolean implicitThis, final boolean classReceiver) {
> // check for an accessor method
> String getterName = "is" + capitalize(propertyName);
> MethodNode getterNode = receiverType.getGetterMethod(getterName);
> @@ -432,7 +441,7 @@ public class StaticTypesCallSiteWriter extends
CallSiteWriter {
> }
> }
> if (getterNode != null) {
> - if (!getterNode.isStatic() && receiver instanceof
ClassExpression && !isClassType(receiverType)) {
> + if (!getterNode.isStatic() && (classReceiver || receiver
instanceof ClassExpression) && !isClassType(receiverType)) { // GROOVY-12473
> return false;
> }
> if ((!getterNode.isPublic() || "class".equals(propertyName)
|| "empty".equals(propertyName)) && isOrImplements(receiverType, MAP_TYPE)) {
> @@ -458,20 +467,20 @@ public class StaticTypesCallSiteWriter extends
CallSiteWriter {
>
> // GROOVY-7149: check direct interfaces
> for (ClassNode node : receiverType.getInterfaces()) {
> - if (makeGetPropertyWithGetter(receiver, node, propertyName,
safe, implicitThis)) {
> + if (makeGetPropertyWithGetter(receiver, node, propertyName,
safe, implicitThis, classReceiver)) {
> return true;
> }
> }
> // check super class
> ClassNode superClass = receiverType.getSuperClass();
> if (superClass != null) {
> - if (makeGetPropertyWithGetter(receiver, superClass,
propertyName, safe, implicitThis)) {
> + if (makeGetPropertyWithGetter(receiver, superClass,
propertyName, safe, implicitThis, classReceiver)) {
> return true;
> }
> }
> // check outer class
> if (implicitThis && receiverType instanceof InnerClassNode &&
!receiverType.isStaticClass()) {
> - if (makeGetPropertyWithGetter(receiver,
receiverType.getOuterClass(), propertyName, safe, implicitThis)) {
> + if (makeGetPropertyWithGetter(receiver,
receiverType.getOuterClass(), propertyName, safe, implicitThis, classReceiver))
{
> return true;
> }
> }
> @@ -814,6 +823,9 @@ public class StaticTypesCallSiteWriter extends
CallSiteWriter {
> if (isClassClassNodeWrappingConcreteType(receiverType)) {
> if (isClassReceiver.length > 0) isClassReceiver[0] = true;
> receiverType = receiverType.getGenericsTypes()[0].getType();
> + } else if (isClassReceiver.length > 0 &&
isClassClassNodeWrappingConcreteType(receiver.getType())
> + &&
receiverType.equals(receiver.getType().getGenericsTypes()[0].getType())) {
> + isClassReceiver[0] = true; // GROOVY-12473: e.g. "(Float as
Class<Float>).NaN" records Float as owner
> }
> if (isPrimitiveType(receiverType)) {
> // GROOVY-6590: wrap primitive types
> ```
>
> With that, your 7 new tests and `testClassClosureDelegateProperty` all
pass, and the stc, asm.sc and bugs test packages ran 6161 tests with no
failures. A 16-case matrix of class-receiver shapes also matches dynamic
Groovy. If you'd rather keep the restructuring, restricting the class-literal
path to receivers with a concrete type argument should fix the regression.
>
> Separately: while checking this I found that SC never evaluates a receiver
expression for static member access (`pick().MAX_VALUE` doesn't call `pick()`);
that's pre-existing back to at least 4.0.32 and is now GROOVY-12479, so it
needn't be handled here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
