blackdrag commented on code in PR #2674:
URL: https://github.com/apache/groovy/pull/2674#discussion_r3555761652
##########
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:
I would consider not using SBA directly here. Instead I would make a method
maybe even in the the selector, that has the arguments already in the right
order and allows you to use a single insertArguments to inject sender and name
and has the right remaining signature for the invocation. For example (name,
receiverClass, receiver, value). Then one insertArguments call would be enough.
You can still let that one call SBA... though I imagine we want to inline the
remaining logic there in the long run.
##########
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:
just for style, this should be before the methods. Nothing blocking in my
eyes
--
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]
