[
https://issues.apache.org/jira/browse/GROOVY-12138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095266#comment-18095266
]
ASF GitHub Bot commented on GROOVY-12138:
-----------------------------------------
paulk-asert commented on code in PR #2674:
URL: https://github.com/apache/groovy/pull/2674#discussion_r3558734384
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -441,6 +444,157 @@ public void setMetaClassCallHandleIfNeeded(boolean
standardMetaClass) {
}
}
+ /**
+ * Property-write based {@link Selector} (GROOVY-12138). Call sites have
the
+ * shape {@code (receiver, value)void}; {@code args[0]} is the receiver and
+ * {@code args[1]} the value being assigned, so the standard guard
machinery
+ * (receiver metaclass identity, switch point, argument classes) applies
+ * unchanged — the value-class guard triggers re-selection when the
assigned
+ * type changes.
+ * <p>
+ * The fast path covers the plain cases only: a non-static
+ * {@link MetaBeanProperty} whose setter accepts the runtime value type
+ * directly, or a writable field of matching type. Everything else — type
+ * coercion, {@code propertyMissing}, maps, static and expando properties —
+ * falls through to {@link MetaObjectProtocol#setProperty}, which is
exactly
+ * the path all writes took before this selector existed.
+ */
+ private static class SetPropertySelector extends MethodSelector {
+
+ public SetPropertySelector(CacheableCallSite callSite, Class<?>
sender, String propertyName, CallType callType, boolean safeNavigation, boolean
thisCall, boolean spreadCall, Object[] arguments) {
+ super(callSite, sender, propertyName, callType, safeNavigation,
thisCall, spreadCall, arguments);
+ }
+
+ /**
+ * Property writes are not routed through {@code invokeMethod}, thus
always returns false.
+ */
+ @Override
+ public boolean setInterceptor() {
+ return false;
+ }
+
+ /**
+ * Chooses the setter or field for a property write from the metaclass.
+ * The fast path is restricted to shapes whose resolution provably does
+ * not depend on the sender (see {@code MetaClassImpl#setProperty}'s
+ * field-vs-setter precedence, GROOVY-8283) or on receiver kind (maps,
+ * GROOVY-8065/GROOVY-11367; overridden {@code setProperty}). Anything
+ * else keeps the exact classic behavior via the adapter fallback in
+ * {@link #setMetaClassCallHandleIfNeeded}.
+ */
+ @Override
+ public void chooseMeta(MetaClassImpl mci) {
+ if (method != null || mci == null) return;
+ // ExpandoMetaClass (also produced by Class.mixin) overrides
+ // setProperty itself; only a plain MetaClassImpl resolution is
+ // safe to mirror here — mirroring the GET selector, which also
+ // keeps EMC off its fast metaclass path
+ if (mci.getClass() != MetaClassImpl.class) return;
+ Object receiver = getCorrectedReceiver();
+ if (receiver instanceof Class || receiver instanceof
java.util.Map) return;
+ if (GroovyCategorySupport.hasCategoryInCurrentThread()) return; //
categories can contribute setters
+
+ // a setProperty(String,Object) contributed by the class itself, a
+ // mixin, or an expando closure intercepts every write; only the
+ // GroovyObject interface default is the benign non-intercepting
+ // case (its behavior is the metaclass path this selector mirrors).
+ // Two complementary probes: reflection sees compiled overrides
+ // (including inherited ones), the metaclass sees mixin/expando
+ // contributions that reflection cannot.
+ if (receiver instanceof GroovyObject &&
hasOverriddenSetProperty(receiver.getClass())) return;
+ MetaMethod customSetProperty = mci.pickMethod("setProperty",
SET_PROPERTY_PARAMS);
+ if (customSetProperty != null
+ && customSetProperty.getDeclaringClass().getTheClass() !=
GroovyObject.class) {
+ return;
+ }
+
+ MetaProperty mp = mci.getMetaProperty(name);
+ Object value = args[1];
+ if (mp instanceof MetaBeanProperty mbp && !mp.isStatic()) {
+ MetaMethod setter = mbp.getSetter();
+ CachedField field = mbp.getField();
+ if (setter != null) {
+ // a non-private backing field can take precedence over the
+ // setter depending on the sender (GROOVY-8283): only the
+ // private-or-absent-field shape is sender-independent
+ if ((field == null || field.isPrivate()) &&
acceptsDirectly(setter, value)) {
+ method = setter;
+ if (LOG_ENABLED) LOG.info("direct setter selected for
property write: " + setter);
+ }
+ return;
+ }
+ if (field != null) {
+ setFieldWriteHandle(field, value);
+ }
+ } else if (mp instanceof CachedField cf && !mp.isStatic()) {
+ setFieldWriteHandle(cf, value);
+ }
+ // otherwise (no meta property, listeners, expando,
propertyMissing, ...): adapter path
+ }
+
+ private static final Class<?>[] SET_PROPERTY_PARAMS = {String.class,
Object.class};
Review Comment:
moved
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -441,6 +444,157 @@ public void setMetaClassCallHandleIfNeeded(boolean
standardMetaClass) {
}
}
+ /**
+ * Property-write based {@link Selector} (GROOVY-12138). Call sites have
the
+ * shape {@code (receiver, value)void}; {@code args[0]} is the receiver and
+ * {@code args[1]} the value being assigned, so the standard guard
machinery
+ * (receiver metaclass identity, switch point, argument classes) applies
+ * unchanged — the value-class guard triggers re-selection when the
assigned
+ * type changes.
+ * <p>
+ * The fast path covers the plain cases only: a non-static
+ * {@link MetaBeanProperty} whose setter accepts the runtime value type
+ * directly, or a writable field of matching type. Everything else — type
+ * coercion, {@code propertyMissing}, maps, static and expando properties —
+ * falls through to {@link MetaObjectProtocol#setProperty}, which is
exactly
+ * the path all writes took before this selector existed.
+ */
+ private static class SetPropertySelector extends MethodSelector {
+
+ public SetPropertySelector(CacheableCallSite callSite, Class<?>
sender, String propertyName, CallType callType, boolean safeNavigation, boolean
thisCall, boolean spreadCall, Object[] arguments) {
+ super(callSite, sender, propertyName, callType, safeNavigation,
thisCall, spreadCall, arguments);
+ }
+
+ /**
+ * Property writes are not routed through {@code invokeMethod}, thus
always returns false.
+ */
+ @Override
+ public boolean setInterceptor() {
+ return false;
+ }
+
+ /**
+ * Chooses the setter or field for a property write from the metaclass.
+ * The fast path is restricted to shapes whose resolution provably does
+ * not depend on the sender (see {@code MetaClassImpl#setProperty}'s
+ * field-vs-setter precedence, GROOVY-8283) or on receiver kind (maps,
+ * GROOVY-8065/GROOVY-11367; overridden {@code setProperty}). Anything
+ * else keeps the exact classic behavior via the adapter fallback in
+ * {@link #setMetaClassCallHandleIfNeeded}.
+ */
+ @Override
+ public void chooseMeta(MetaClassImpl mci) {
+ if (method != null || mci == null) return;
+ // ExpandoMetaClass (also produced by Class.mixin) overrides
+ // setProperty itself; only a plain MetaClassImpl resolution is
+ // safe to mirror here — mirroring the GET selector, which also
+ // keeps EMC off its fast metaclass path
+ if (mci.getClass() != MetaClassImpl.class) return;
+ Object receiver = getCorrectedReceiver();
+ if (receiver instanceof Class || receiver instanceof
java.util.Map) return;
+ if (GroovyCategorySupport.hasCategoryInCurrentThread()) return; //
categories can contribute setters
+
+ // a setProperty(String,Object) contributed by the class itself, a
+ // mixin, or an expando closure intercepts every write; only the
+ // GroovyObject interface default is the benign non-intercepting
+ // case (its behavior is the metaclass path this selector mirrors).
+ // Two complementary probes: reflection sees compiled overrides
+ // (including inherited ones), the metaclass sees mixin/expando
+ // contributions that reflection cannot.
+ if (receiver instanceof GroovyObject &&
hasOverriddenSetProperty(receiver.getClass())) return;
+ MetaMethod customSetProperty = mci.pickMethod("setProperty",
SET_PROPERTY_PARAMS);
+ if (customSetProperty != null
+ && customSetProperty.getDeclaringClass().getTheClass() !=
GroovyObject.class) {
+ return;
+ }
+
+ MetaProperty mp = mci.getMetaProperty(name);
+ Object value = args[1];
+ if (mp instanceof MetaBeanProperty mbp && !mp.isStatic()) {
+ MetaMethod setter = mbp.getSetter();
+ CachedField field = mbp.getField();
+ if (setter != null) {
+ // a non-private backing field can take precedence over the
+ // setter depending on the sender (GROOVY-8283): only the
+ // private-or-absent-field shape is sender-independent
+ if ((field == null || field.isPrivate()) &&
acceptsDirectly(setter, value)) {
+ method = setter;
+ if (LOG_ENABLED) LOG.info("direct setter selected for
property write: " + setter);
+ }
+ return;
+ }
+ if (field != null) {
+ setFieldWriteHandle(field, value);
+ }
+ } else if (mp instanceof CachedField cf && !mp.isStatic()) {
+ setFieldWriteHandle(cf, value);
+ }
+ // otherwise (no meta property, listeners, expando,
propertyMissing, ...): adapter path
+ }
+
+ private static final Class<?>[] SET_PROPERTY_PARAMS = {String.class,
Object.class};
+
+ /**
+ * Mirrors the receiver test of {@code
ScriptBytecodeAdapter#setProperty}:
+ * a {@code GroovyObject} whose {@code setProperty} is a real compiled
+ * override (not the interface default) intercepts all writes.
+ */
+ private static boolean hasOverriddenSetProperty(Class<?>
receiverClass) {
+ try {
+ return !receiverClass.getMethod("setProperty", String.class,
Object.class).isDefault();
+ } catch (ReflectiveOperationException ignore) {
+ return true; // cannot decide: stay on the adapter path
+ }
+ }
+
+ private boolean acceptsDirectly(MetaMethod setter, Object value) {
+ var parameterTypes = setter.getParameterTypes();
+ if (parameterTypes.length != 1 || setter.isVargsMethod()) return
false;
+ if (!Modifier.isPublic(setter.getModifiers())) return false;
+ Class<?> parameterType = parameterTypes[0].getTheClass();
+ if (value == null) return !parameterType.isPrimitive();
+ return TypeHelper.getWrapperClass(parameterType).isInstance(value);
+ }
+
+ private void setFieldWriteHandle(CachedField field, Object value) {
+ // only public fields are sender-independent; the rest go through
+ // the sender-aware adapter path
+ if (!Modifier.isPublic(field.getModifiers()) ||
Modifier.isFinal(field.getModifiers())) return;
+ Class<?> fieldType = field.getType();
+ boolean accepts = (value == null) ? !fieldType.isPrimitive()
+ :
TypeHelper.getWrapperClass(fieldType).isInstance(value);
+ if (!accepts) return; // needs coercion: adapter path
+ try {
+ // like the property-get field path: lookup for the sender,
then unreflect
+ @SuppressWarnings("removal")
+ MethodHandles.Lookup lookup = ((Java8)
VMPluginFactory.getPlugin()).newLookup(sender);
+ handle = field.asWriteAccessMethod(lookup);
+ if (LOG_ENABLED) LOG.info("direct field write handle set for
property write");
+ } catch (IllegalAccessException e) {
+ throw new GroovyBugError(e);
+ }
+ }
+
+ /**
+ * All remaining property writes go through the exact classic path:
+ * {@code ScriptBytecodeAdapter.setProperty(value, sender, receiver,
name)},
+ * bound with this call site's sender so sender-aware resolution
+ * (private members from inside the declaring class, precedence rules,
+ * maps, closure retry semantics) is preserved byte-for-byte.
+ */
+ @Override
+ public void setMetaClassCallHandleIfNeeded(boolean standardMetaClass) {
+ if (handle != null) return;
+ useMetaClass = true;
+ if (LOG_ENABLED) LOG.info("set adapter invocation path for
property set.");
+ // SBA_SET_PROPERTY: (Object value, Class sender, Object receiver,
String name)void
+ MethodHandle sba = MethodHandles.insertArguments(SBA_SET_PROPERTY,
1, sender);
+ sba = MethodHandles.insertArguments(sba, 2, name); // -> (Object
value, Object receiver)void
+ handle = MethodHandles.permuteArguments(sba,
+ MethodType.methodType(void.class, Object.class,
Object.class), 1, 0); // -> (receiver, value)void
Review Comment:
chamged
> emit invokedynamic call sites for property writes
> -------------------------------------------------
>
> Key: GROOVY-12138
> URL: https://issues.apache.org/jira/browse/GROOVY-12138
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
