[
https://issues.apache.org/jira/browse/GROOVY-12314?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12314:
-------------------------------
Description:
Found while investigating GROOVY-12305 (see also GROOVY-12290, GROOVY-9967): a
family of edge cases where property access that resolves to a *field* behaves
differently under dynamic Groovy, {{@TypeChecked}} and {{@CompileStatic}} —
different values, different failure modes, or silently wrong values. Each item
is individually minor; taken together they justify aligning the three modes
even though some behavior changes result.
h3. Current behavior (measured on 4.0.33, 5.1.1, 6.0.0-beta-3 and master)
*A. Non-public fields of foreign (JDK) classes*
{code:groovy}
def list = [1, 2]
println list.modCount // protected field inherited from
java.util.AbstractList
println list.elementData // package-private field of java.util.ArrayList
println list.@modCount // attribute access
{code}
||access (dynamic, no add-opens)||4.0.33||5.1.1||beta-3 / master||
|{{list.modCount}} (protected)|MissingPropertyException|*GroovyBugError* "BUG!
UNCAUGHT EXCEPTION: member is protected"|*GroovyBugError*|
|{{list.elementData}}
(package-private)|MissingPropertyException|MissingPropertyException|MissingPropertyException|
|{{list.@modCount}}|MissingFieldException|raw IllegalAccessException|raw
IllegalAccessException|
The GroovyBugError is a 4→5 regression:
{{Selector$PropertySelector.chooseMeta}} wraps the refused {{MethodHandles}}
lookup in GroovyBugError (vmplugin/v8/Selector.java, GROOVY-9144/9596 code). It
is not catchable as MissingPropertyException and presents to the user as an
internal bug. The {{.@}} error-shape change (MissingFieldException → raw
IllegalAccessException escaping from {{CachedField.getProperty}}) is likewise a
4→5 regression.
The static modes for the same expressions (master):
* {{@TypeChecked}}: compiles — {{storeField}} admits the fields
({{isFieldAccessible}}'s exact-receiver leniency still covers package-private,
and {{storeField}} deliberately proceeds for inaccessible protected fields) —
then fails at runtime as above, despite the checker having typed the expression
via the field ({{int}} / {{Object[]}}).
* {{@CompileStatic}}: classgen error {{Access to E#modCount is forbidden @ line
-1, column -1}} — unresolved type parameter as the receiver name and no source
position ({{StaticTypesCallSiteWriter#addPropertyAccessError}}).
With {{--add-opens java.base/java.util=ALL-UNNAMED}}, every spelling above
works and prints the field value, on all versions.
*B. Collection {{size}}/{{length}} classgen shortcut*
{{StaticTypesCallSiteWriter#makeGetPropertySite}} rewrites {{size}}/{{length}}
on Collection receivers to {{size()}} *before* getter/map-rule/field lookup.
Measured under {{@CompileStatic}} (the dynamic column is identical on all four
versions):
{code:groovy}
class C { int getSize() { 999 } } // Groovy class
// JColl: Java class extends ArrayList<Object> with public int size = 42,
public int length = 99
{code}
||scenario (CS)||4.0.33||5.1.1||beta-3 / master||dynamic (all versions)||
|{{List l = \[1,2\]; l.size}}|2|2|STC error|MissingPropertyException|
|{{l.length}}|STC error|STC error|STC error|MissingPropertyException|
|Groovy class with {{getSize()}}: {{c.size}}|999|999|999|999|
|Groovy class, public field {{size=42}}: {{d.size}}|42|42|42|42|
|{{List<C> l; l.size}} (element {{getSize()}})|*2*|*2*|\[999, 999\]|\[999,
999\]|
|Java class, public field {{size=42}}: {{j.size}}|*1*|*1*|*1*|42|
|Java class, public field {{length=99}}: {{j.length}}|*1*|*1*|*1*|99|
Notes:
* The shortcut's only mainstream feeder was STC resolving {{l.size}} to
ArrayList's *private* {{int size}} field via the exact-receiver leniency —
closed by GROOVY-12290 — which is why rows 1 and 5 changed in 6.0.0-beta-3
(aligning with dynamic semantics).
* Row 5 on 4.x/5.x is a three-way divergence: dynamic gives \[999, 999\];
{{@TypeChecked}} returns \[999, 999\] at runtime while statically typing the
expression {{int}} (so {{int n = l.size}} type-checks cleanly then throws
GroovyCastException); {{@CompileStatic}} gives 2.
* Rows 6–7 are silently wrong values on *every* version: STC legitimately
admits the public field, but the shortcut hijacks the access to {{size()}}.
Groovy-class receivers are immune because {{makeGroovyObjectGetPropertySite}}
has no such shortcut — the result differs depending on whether the receiver
class is written in Java or Groovy.
h3. Proposed changes (one ticket, likely several commits)
# *STC*: extend the GROOVY-12290 accessibility rule so plain property syntax no
longer admits package-private or protected fields that the enclosing class
could not access by Java rules (including dropping the inaccessible-protected
fall-through in {{storeField}}). Kept: same-nest, same-package package-private,
protected from subclasses, closure bodies, delegates, and {{.@}} — the escape
hatches GROOVY-12290 preserved.
# *Runtime (indy)*: {{PropertySelector.chooseMeta}} — when the sender lookup
refuses access (IllegalAccessException), fall back to the normal
missing-property handling instead of throwing GroovyBugError. Restores the
graceful 4.x contract and keeps {{--add-opens}} users working. This part is a
standalone 5.x regression fix and could be back-ported to GROOVY_5_0_X
independently of the rest.
# *Runtime polish*: attribute access ({{.@}}) that reaches an inaccessible
field should throw MissingFieldException (with the IllegalAccessException as
cause) as 4.x did, instead of leaking the raw IllegalAccessException from
{{CachedField.getProperty}}.
# *Classgen*: remove the Collection {{size}}/{{length}} shortcut.
Post-GROOVY-12290 it is vestigial — no remaining STC-admitted access needs it,
and every reachable case produces wrong values. The Java-public-field scenarios
then resolve via {{makeGetField}}, matching dynamic behavior and Groovy-class
receivers.
# *Classgen polish*: {{addPropertyAccessError}} should print a resolved
receiver type name and carry a real source position (currently "E" and line -1)
— safety-net quality, since item 1 removes its known trigger.
h3. Resulting behavior (target)
||scenario (no add-opens)||dynamic||@TypeChecked||@CompileStatic||
|{{list.size}} / {{list.length}}|MissingPropertyException|STC error|STC error|
|{{list.modCount}} (protected)|MissingPropertyException (was BUG!)|STC error
(was compiles→BUG!)|STC error (was line -1 classgen error)|
|{{list.elementData}} (package-private)|MissingPropertyException|STC error (was
compiles→runtime MPE)|STC error (was line -1 classgen error)|
|Groovy {{getSize()}} / Groovy public field / element spread|999 / 42 / \[999,
999\]|999 / 42 / \[999, 999\]|999 / 42 / \[999, 999\]|
|Java public field {{size}}/{{length}}|42 / 99|42 / 99|42 / 99 (was 1)|
|{{list.@modCount}}|MissingFieldException (was raw
IAE)|MissingFieldException|MissingFieldException|
||with --add-opens||dynamic||@TypeChecked||@CompileStatic||
|{{list.modCount}}|field value (kept)|STC error|STC error|
|{{list.@modCount}}|field value|field value|field value|
All three modes then agree on every row: the same value, or the static modes
rejecting at compile time exactly what dynamic would fail at runtime. All
failures are well-formed — catchable
MissingPropertyException/MissingFieldException at runtime, positioned STC
errors at compile time; no GroovyBugError, no "line -1" classgen errors. The
one deliberate asymmetry: with {{--add-opens}}, dynamic property syntax can
still read a protected field, while the static modes require the explicit
{{.@}} spelling.
h3. Breaking changes (accepted as the price of alignment)
* {{@TypeChecked}}/{{@CompileStatic}} code reading package-private/protected
foreign fields via property syntax stops compiling (today it fails at runtime
or with a malformed classgen error anyway; {{.@}} remains available).
* {{@CompileStatic}} on a Java Collection class with a public
{{size}}/{{length}} field changes from the element count to the field value
(bug fix, but observable).
* Already shipped in 6.0.0-beta-3 via GROOVY-12290, noted here for the
migration notes: {{list.size}} under {{@CompileStatic}} is now a compile error;
on 4.x/5.x it compiled and returned the element count.
> Align field-backed property access across compilation modes
> -----------------------------------------------------------
>
> Key: GROOVY-12314
> URL: https://issues.apache.org/jira/browse/GROOVY-12314
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Found while investigating GROOVY-12305 (see also GROOVY-12290, GROOVY-9967):
> a family of edge cases where property access that resolves to a *field*
> behaves differently under dynamic Groovy, {{@TypeChecked}} and
> {{@CompileStatic}} — different values, different failure modes, or silently
> wrong values. Each item is individually minor; taken together they justify
> aligning the three modes even though some behavior changes result.
> h3. Current behavior (measured on 4.0.33, 5.1.1, 6.0.0-beta-3 and master)
> *A. Non-public fields of foreign (JDK) classes*
> {code:groovy}
> def list = [1, 2]
> println list.modCount // protected field inherited from
> java.util.AbstractList
> println list.elementData // package-private field of java.util.ArrayList
> println list.@modCount // attribute access
> {code}
> ||access (dynamic, no add-opens)||4.0.33||5.1.1||beta-3 / master||
> |{{list.modCount}} (protected)|MissingPropertyException|*GroovyBugError*
> "BUG! UNCAUGHT EXCEPTION: member is protected"|*GroovyBugError*|
> |{{list.elementData}}
> (package-private)|MissingPropertyException|MissingPropertyException|MissingPropertyException|
> |{{list.@modCount}}|MissingFieldException|raw IllegalAccessException|raw
> IllegalAccessException|
> The GroovyBugError is a 4→5 regression:
> {{Selector$PropertySelector.chooseMeta}} wraps the refused {{MethodHandles}}
> lookup in GroovyBugError (vmplugin/v8/Selector.java, GROOVY-9144/9596 code).
> It is not catchable as MissingPropertyException and presents to the user as
> an internal bug. The {{.@}} error-shape change (MissingFieldException → raw
> IllegalAccessException escaping from {{CachedField.getProperty}}) is likewise
> a 4→5 regression.
> The static modes for the same expressions (master):
> * {{@TypeChecked}}: compiles — {{storeField}} admits the fields
> ({{isFieldAccessible}}'s exact-receiver leniency still covers
> package-private, and {{storeField}} deliberately proceeds for inaccessible
> protected fields) — then fails at runtime as above, despite the checker
> having typed the expression via the field ({{int}} / {{Object[]}}).
> * {{@CompileStatic}}: classgen error {{Access to E#modCount is forbidden @
> line -1, column -1}} — unresolved type parameter as the receiver name and no
> source position ({{StaticTypesCallSiteWriter#addPropertyAccessError}}).
> With {{--add-opens java.base/java.util=ALL-UNNAMED}}, every spelling above
> works and prints the field value, on all versions.
> *B. Collection {{size}}/{{length}} classgen shortcut*
> {{StaticTypesCallSiteWriter#makeGetPropertySite}} rewrites
> {{size}}/{{length}} on Collection receivers to {{size()}} *before*
> getter/map-rule/field lookup. Measured under {{@CompileStatic}} (the dynamic
> column is identical on all four versions):
> {code:groovy}
> class C { int getSize() { 999 } } // Groovy class
> // JColl: Java class extends ArrayList<Object> with public int size = 42,
> public int length = 99
> {code}
> ||scenario (CS)||4.0.33||5.1.1||beta-3 / master||dynamic (all versions)||
> |{{List l = \[1,2\]; l.size}}|2|2|STC error|MissingPropertyException|
> |{{l.length}}|STC error|STC error|STC error|MissingPropertyException|
> |Groovy class with {{getSize()}}: {{c.size}}|999|999|999|999|
> |Groovy class, public field {{size=42}}: {{d.size}}|42|42|42|42|
> |{{List<C> l; l.size}} (element {{getSize()}})|*2*|*2*|\[999, 999\]|\[999,
> 999\]|
> |Java class, public field {{size=42}}: {{j.size}}|*1*|*1*|*1*|42|
> |Java class, public field {{length=99}}: {{j.length}}|*1*|*1*|*1*|99|
> Notes:
> * The shortcut's only mainstream feeder was STC resolving {{l.size}} to
> ArrayList's *private* {{int size}} field via the exact-receiver leniency —
> closed by GROOVY-12290 — which is why rows 1 and 5 changed in 6.0.0-beta-3
> (aligning with dynamic semantics).
> * Row 5 on 4.x/5.x is a three-way divergence: dynamic gives \[999, 999\];
> {{@TypeChecked}} returns \[999, 999\] at runtime while statically typing the
> expression {{int}} (so {{int n = l.size}} type-checks cleanly then throws
> GroovyCastException); {{@CompileStatic}} gives 2.
> * Rows 6–7 are silently wrong values on *every* version: STC legitimately
> admits the public field, but the shortcut hijacks the access to {{size()}}.
> Groovy-class receivers are immune because {{makeGroovyObjectGetPropertySite}}
> has no such shortcut — the result differs depending on whether the receiver
> class is written in Java or Groovy.
> h3. Proposed changes (one ticket, likely several commits)
> # *STC*: extend the GROOVY-12290 accessibility rule so plain property syntax
> no longer admits package-private or protected fields that the enclosing class
> could not access by Java rules (including dropping the inaccessible-protected
> fall-through in {{storeField}}). Kept: same-nest, same-package
> package-private, protected from subclasses, closure bodies, delegates, and
> {{.@}} — the escape hatches GROOVY-12290 preserved.
> # *Runtime (indy)*: {{PropertySelector.chooseMeta}} — when the sender lookup
> refuses access (IllegalAccessException), fall back to the normal
> missing-property handling instead of throwing GroovyBugError. Restores the
> graceful 4.x contract and keeps {{--add-opens}} users working. This part is a
> standalone 5.x regression fix and could be back-ported to GROOVY_5_0_X
> independently of the rest.
> # *Runtime polish*: attribute access ({{.@}}) that reaches an inaccessible
> field should throw MissingFieldException (with the IllegalAccessException as
> cause) as 4.x did, instead of leaking the raw IllegalAccessException from
> {{CachedField.getProperty}}.
> # *Classgen*: remove the Collection {{size}}/{{length}} shortcut.
> Post-GROOVY-12290 it is vestigial — no remaining STC-admitted access needs
> it, and every reachable case produces wrong values. The Java-public-field
> scenarios then resolve via {{makeGetField}}, matching dynamic behavior and
> Groovy-class receivers.
> # *Classgen polish*: {{addPropertyAccessError}} should print a resolved
> receiver type name and carry a real source position (currently "E" and line
> -1) — safety-net quality, since item 1 removes its known trigger.
> h3. Resulting behavior (target)
> ||scenario (no add-opens)||dynamic||@TypeChecked||@CompileStatic||
> |{{list.size}} / {{list.length}}|MissingPropertyException|STC error|STC error|
> |{{list.modCount}} (protected)|MissingPropertyException (was BUG!)|STC error
> (was compiles→BUG!)|STC error (was line -1 classgen error)|
> |{{list.elementData}} (package-private)|MissingPropertyException|STC error
> (was compiles→runtime MPE)|STC error (was line -1 classgen error)|
> |Groovy {{getSize()}} / Groovy public field / element spread|999 / 42 /
> \[999, 999\]|999 / 42 / \[999, 999\]|999 / 42 / \[999, 999\]|
> |Java public field {{size}}/{{length}}|42 / 99|42 / 99|42 / 99 (was 1)|
> |{{list.@modCount}}|MissingFieldException (was raw
> IAE)|MissingFieldException|MissingFieldException|
> ||with --add-opens||dynamic||@TypeChecked||@CompileStatic||
> |{{list.modCount}}|field value (kept)|STC error|STC error|
> |{{list.@modCount}}|field value|field value|field value|
> All three modes then agree on every row: the same value, or the static modes
> rejecting at compile time exactly what dynamic would fail at runtime. All
> failures are well-formed — catchable
> MissingPropertyException/MissingFieldException at runtime, positioned STC
> errors at compile time; no GroovyBugError, no "line -1" classgen errors. The
> one deliberate asymmetry: with {{--add-opens}}, dynamic property syntax can
> still read a protected field, while the static modes require the explicit
> {{.@}} spelling.
> h3. Breaking changes (accepted as the price of alignment)
> * {{@TypeChecked}}/{{@CompileStatic}} code reading package-private/protected
> foreign fields via property syntax stops compiling (today it fails at runtime
> or with a malformed classgen error anyway; {{.@}} remains available).
> * {{@CompileStatic}} on a Java Collection class with a public
> {{size}}/{{length}} field changes from the element count to the field value
> (bug fix, but observable).
> * Already shipped in 6.0.0-beta-3 via GROOVY-12290, noted here for the
> migration notes: {{list.size}} under {{@CompileStatic}} is now a compile
> error; on 4.x/5.x it compiled and returned the element count.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
