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.
--
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]
