[
https://issues.apache.org/jira/browse/GROOVY-12489?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131908#comment-18131908
]
ASF GitHub Bot commented on GROOVY-12489:
-----------------------------------------
codeconsole commented on PR #3023:
URL: https://github.com/apache/groovy/pull/3023#issuecomment-6401187140
Pushed 528ca59e27. Replies in the order of the review:
1. Done in 8e5972cc73. The `GroovyObjectOverrides` class Javadoc states both
read criteria, where each is used, the one case where they differ (a synthetic
or `@Internal` implementation other than `GroovyObject`'s default), and the
single write criterion. `ScriptBytecodeAdapter.getProperty`, `PropertySelector`
and `AbstractCallSite` each name theirs and point there. Unifying them fails
`getPropertyMarkedInternalIsLeftToTheMetaClassByCallSites` and
`generatedGetPropertyIsAnOverrideButNotCustom`.
`defaultGetPropertyResolvesReadForTheSender` is unaffected, since the
`GroovyObject` default is `false` under both.
2. Comment added in 8e5972cc73, in
`AbstractCallSite.createGroovyObjectGetPropertySite` and the
`hasCustomGetProperty` Javadoc. For `NoSuchMethodException` the site is the
same `PogoGetPropertySite` as before. A `LinkageError` used to escape the read,
and now reaches the object's `getProperty` (9da6e65f14,
`UnreadableClassGetPropertySiteTest`).
3. Tests added in d9b089c3fd: `expandoOrMixinSetPropertyReceivesEveryWrite`
and `expandoOrMixinGetPropertyAnswersEveryRead` cover class and per-instance
expando closures and a mixin, after the sites have linked to the setter or
getter, for static-name, computed-name, `this` and `@TypeChecked` accesses,
with indy and classic. What keeps these off the fast path is the
`mci.getClass() != MetaClassImpl.class` check, which runs before both probes,
not `pickMethod`: for a mixed-in `setProperty`, `pickMethod` returns the
`GroovyObject` default. Disabling `pickMethod` alone breaks nothing; removing
the type check lets mixin writes through; removing both lets expando writes
through, and the set test fails for both. The `Selector` comment says so now
(034602f3c5). The classic and adapter paths intercept in every case.
4. Added in 1725ee8c3b:
`groovyObjectDefaultGetPropertyCarriesInternalAtRunTime`.
5. Stated in the class Javadoc in 8e5972cc73: writes have one criterion,
`overridesSetProperty`, shared by `ScriptBytecodeAdapter.setProperty`,
`setGroovyObjectProperty` and the indy write sites. No write path has set aside
a synthetic or `@Internal` `setProperty`, and classic compilation has no
property-write call sites; its writes go through `ScriptBytecodeAdapter`.
6. This PR adds no caller of these methods. The only one in main sources is
the indy write fallback, `IndyGuardsFiltersAndSignatures.setProperty`, whose
sender is the call site's lookup class; compiled code passes its class literal.
With no sender there is nothing for the metaclass to use, so the object's
method takes the access; the `setProperty` comment says so (8e5972cc73).
7. groovy-callsite declares `api project(':')`, so it always comes with the
core of the same version, and it already imports core internals
(`org.apache.groovy.internal.util.UncheckedThrow`). No package is split:
groovy-callsite holds only `org.codehaus.groovy.runtime.callsite`, which core
does not contain, and `org.apache.groovy.internal.metaclass` exists only in
core. There is no module-info; the automatic module names are
`org.apache.groovy` and `org.apache.groovy.callsite`, and under OSGi
groovy-callsite is a fragment of the `groovy` bundle (`Fragment-Host: groovy`)
that imports the package optionally. The class is `@Internal`, not
`@GroovyABI`. The Javadoc builds do not exclude internal packages, so it is
listed like `MetaClassConstant` in the same package.
Test gaps:
- EMC/mixin `setProperty` under indy: item 3, plus the read counterpart.
- `@Internal` retention: item 4.
- Interface extending `GroovyObject` with a default `getProperty`: added,
and the answer it locked was a bug. `!isDefault()` (GROOVY-8283, new in 5.0)
stood for "no getProperty specialization" (its TODO), and `SetPropertySelector`
already treated only `GroovyObject`'s own default as benign. Going around
another interface's default skipped it on every `obj.x = v` and untyped
`obj."$name"` read, while `obj.setProperty(...)` and `obj.x` reads reached it;
Groovy 4.0.33 reached it on every path, as a declared or trait method still
does. Fixed in 56d309f536: `overridesSetProperty` and `overridesGetProperty`
set aside only `GroovyObject`'s default.
`defaultOfAnotherInterfaceIsAnOverride`,
`defaultSetPropertyOfAnotherInterfaceReceivesEveryWrite` and
`defaultGetPropertyOfAnotherInterfaceAnswersEveryRead` cover an implementing
class, a subclass and a sub-interface, with indy and classic; d807afef7b adds
the inherited and sub-interface predicate cases and a `GroovyInterceptable`
control. An `@Internal` default of another interface is still left to the
metaclass by the call sites (`generatedGetPropertyIsAnOverrideButNotCustom`).
- Inherited synthetic `getProperty`: added in 1725ee8c3b;
`generatedGetPropertyIsAnOverrideButNotCustom` also covers a subclass
inheriting a synthetic `getProperty` and one inheriting an `@Internal` one.
Nits:
- No change.
- Done in d74021ddc8: one private factory builds the three `ClassValue`s and
holds the lookup and the fallback.
- No change.
> Dynamic property access fails or bypasses getProperty/setProperty when the
> receiver class has no reflection metadata (GraalVM native image)
> -------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12489
> URL: https://issues.apache.org/jira/browse/GROOVY-12489
> Project: Groovy
> Issue Type: Bug
> Components: groovy-runtime
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> A dynamic property access that carries a sender has to know whether the
> receiver overrides the {{setProperty}} or {{getProperty}} that
> {{GroovyObject}} provides. Groovy answers this with {{Class.getMethod}} on
> every access:
> * {{ScriptBytecodeAdapter.setProperty}}, {{setGroovyObjectProperty}} and
> {{getProperty}} let any exception from the lookup escape.
> * The invokedynamic {{SetPropertySelector}} treats a failed lookup as an
> override.
> * The invokedynamic {{PropertySelector}} goes to the meta-class instead,
> reading around the class's own {{getProperty}}.
> Reflection over a class can fail while the class itself works. In a GraalVM
> native image without reflection metadata for the class, {{getMethod}} throws
> {{NoSuchMethodException}}. As a result, every write that carries a sender to
> such an object fails, and so does every read through the adapter. Reads
> through invokedynamic sites are answered by the meta-class instead, with a
> {{MissingPropertyException}}, or, when the meta-class knows the property
> through a registered superclass, with the wrong value and no error.
> Applications such as Grails register every Groovy class for reflection only
> so these checks can be answered. On the JVM, a public method naming a type
> absent at run time makes the lookups throw {{NoClassDefFoundError}}. And
> where reflection works, every access pays for a method search and a
> {{Method}} copy.
> Proposed fix: cache each answer per class in a {{ClassValue}}, keeping the
> existing criteria, and when reflection cannot answer, treat the class as
> overriding so the access goes to the object's own method.
> PR: https://github.com/apache/groovy/pull/3023
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
