paulk-asert opened a new pull request, #3210:
URL: https://github.com/apache/groovy/pull/3210
## Summary
This brings together six closure-dispatch PRs (#3104, #3185, #3188, #3192,
#3196, #3201) and adds three follow-on commits. Together they make
implicit-this method calls and property access in a closure body behave the
same way:
- across all resolve strategies;
- for methods and properties alike;
- on both runtime dispatch paths: the closure's own `ClosureMetaClass`, and
`MetaClassImpl`, used once a closure has an `ExpandoMetaClass`.
They also stop a closure's metaclass from exposing, or overwriting, the
private fields of `groovy.lang.Closure`. This PR supersedes the six original
PRs.
| Issue | Change
|
| ---------------------------------------- |
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
| GROOVY-12578 / GROOVY-12664 (duplicates) | A closure's own method (`with`,
`tap`, `toString`…) returns its result when neither delegate nor owner can take
the call. It previously ran, threw `MissingMethodException`, and indy then ran
it a second time. This fixes a 6.0.0 regression from GROOVY-11858. |
| GROOVY-12667 | `OWNER_FIRST`/`DELEGATE_FIRST`
method calls skip a null owner or delegate rather than throwing
`NullPointerException`, in `ClosureMetaClass` and in `MetaClassImpl`'s closure
dispatch. This restores Groovy 3's behaviour for method calls.
|
| GROOVY-12671 | Property access skips a null
owner or delegate the same way; with no candidate it throws
`MissingPropertyException` instead of `NullPointerException`. This also fixes
`setPropertyTryThese`, which tested `o1 != null` where it meant `o2`.
|
| GROOVY-12675 | `closure.metaClass.properties`
no longer lists `Closure`'s private members, so a `MissingPropertyException` no
longer suggests "Possible solutions: bcw".
|
| GROOVY-12680 | A closure's metaclass no longer
reads or writes `Closure`'s private fields (`bcw`, `thisType`, `mopPerturbed`,
`serialVersionUID`…). Overwriting `thisType` was possible from 5.x and broke
the closure's access to private fields of its enclosing class. |
## Behaviour after this PR
When a closure's owner or delegate is null (after `dehydrate()`, after
`rehydrate` with a null argument, or after `delegate = null`):
- **The other candidate answers.** For example, under `OWNER_FIRST` with a
null owner, the delegate's method or property is used.
- **With no usable candidate,** a method of the closure itself runs once, on
the closure. Any other method throws `MissingMethodException`; a property
throws `MissingPropertyException`.
- **The same result on both dispatch paths.** A closure dispatches through
`MetaClassImpl` instead of `ClosureMetaClass` once it has an
`ExpandoMetaClass`: after `ExpandoMetaClass.enableGlobally()`, or after adding
to a single closure's metaclass. Either way the outcome is the same.
A closure's metaclass treats `Closure`'s private fields as any metaclass
treats a superclass's private fields:
- `getProperties()` doesn't list them;
- `getMetaProperty`/`hasProperty` return null;
- `getProperty`/`setProperty` throw `MissingPropertyException`.
`closure.properties` and `Closure`'s public properties are unchanged.
## Consistency, measured
A probe ran the same matrix on 3.0.25, 4.0.33, 5.1.3, 6.0.0, master and this
branch: 5 resolve strategies × 5 owner/delegate combinations × 7 closure-body
operations, in dynamic, `ExpandoMetaClass` and `@CompileStatic` modes.
- **Dispatch paths:** outcome-kind disagreements between `ClosureMetaClass`
and `MetaClassImpl` go from 16 on master to **0**. Of the 9 differences left in
full result strings, 8 are the two pre-existing issues below and 1 is message
wording.
- **Property access** now behaves identically on both paths.
- **Packed closures (GEP-27)** aren't affected. A closure that can be
rehydrated escapes its method, and escaping closures are never packed.
## Breaking changes
These are documented in `COMPATIBILITY.md` under two Groovy 7 entries.
- **`NullPointerException` becomes `MissingMethodException` or
`MissingPropertyException`** for a closure with a null owner or delegate. This
only changes paths that already failed.
- **`toString()`/`with` in a closure whose owner or delegate is null run on
the remaining candidate or on the closure.** In Groovy 6 they ran against
`null`; in 3.0–5.1 they ran on the closure.
- **`Closure`'s private fields are no longer reachable through a closure's
metaclass.** `serialVersionUID` and `bcw` were readable in every version.
Nothing that ran without an exception behaves differently, apart from those
closure methods in the null cases.
## Commits
1. GROOVY-12578: return the result of a closure's own method when the
delegate and owner lack it (#3104)
2. GROOVY-12664: the same fix, adding its test only (#3185)
3. GROOVY-12667: skip a null delegate or owner in `DELEGATE_FIRST` and
`OWNER_FIRST` method dispatch (#3188)
4. GROOVY-12671: skip a null owner or delegate when resolving closure
properties (#3192)
5. GROOVY-12675: do not list private members of `Closure` as closure
properties (#3196)
6. GROOVY-12680: do not resolve private members of `Closure` through a
closure's metaclass (#3201)
7. GROOVY-12680: share the private-member check of a closure's metaclass.
One predicate is now used by the property list and every lookup; a new test
pins that they agree for every private field of `Closure` without a public
accessor, derived by reflection.
8. GROOVY-12667: skip a null owner in the closure dispatch of
`MetaClassImpl`. The `ExpandoMetaClass` path threw `NullPointerException` from
`lookupObjectMetaClass(null)`. A new test runs each null case on both dispatch
paths and asserts which path it's on.
Plus the `COMPATIBILITY.md` entries.
## Testing
- **New tests:** every PR's tests, plus the two added above. Each was
confirmed to fail without its fix.
- **Wider run:** closure, delegation, metaclass, `ExpandoMetaClass`,
memoize/curry/trampoline, `gls.invocation` and the `bugs` packages. That's 958
classes and 5,366 tests, with 0 failures.
- **`lintGate`:** green.
- **Not run:** the full `:test` suite.
## Backport
Commits 1–2 (GROOVY-12578/12664) fix a 6.0.0 regression and suit
`GROOVY_6_0_X`; they apply first and cherry-pick on their own. The rest change
exception types and belong in 7.0 only.
## Not in this PR (pre-existing, to be filed separately)
- **`OWNER_FIRST` misroute under `ExpandoMetaClass`.** `MetaClassImpl`'s
closure dispatch calls the delegate's method first under `OWNER_FIRST`, in
every version since at least 3.0, because the helper it shares with
`DELEGATE_FIRST` always tries the delegate first. Fixing it changes results of
code that works today, especially builder-style DSLs in applications that
enable `ExpandoMetaClass` globally, so it needs its own issue and compatibility
note.
- **`*_ONLY` strategies only consider a delegate or owner that is a
`GroovyObject`.** So a `Map` delegate works under `DELEGATE_FIRST` but not
under `DELEGATE_ONLY`.
- **`@CompileStatic` closures don't consistently honour the resolve
strategy.** Property reads go to the owner, but writes follow the strategy
(since 5.x).
GROOVY-12664 and GROOVY-12578 describe the same defect; one can be closed as
a duplicate.
--
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]
