[
https://issues.apache.org/jira/browse/GROOVY-12552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18143309#comment-18143309
]
ASF GitHub Bot commented on GROOVY-12552:
-----------------------------------------
paulk-asert commented on PR #3081:
URL: https://github.com/apache/groovy/pull/3081#issuecomment-6812244164
Thanks James - the fix looks right to me, and I'm happy to merge it once it
has notes. Before writing them I checked how far back each part goes, and it's
a bit more mixed than the description says.
### Where the together/incremental difference comes from
I compiled your reproducer's sources exactly (package `example`, with
`Util`) the way `groovyc-repro.sh` does. For each version I checked which class
`Svc.rename`'s three name-accessor calls go to: `base.name = 'x'`,
`base.setName('y')` and `base.getName()`.
| Version | Compiled together | Incremental |
|---|---|---|
| 4.0.33, and 5.0.0-alpha-1 to alpha-8 | `Impl` `Named` `Named` | `Impl`
`Impl` `Impl` |
| 5.0.0-alpha-9 onwards (5.1.3, 6.0.0, master) | `Named` `Named` `Named` |
`Impl` `Impl` `Impl` |
So 4.0.33 is affected too, for the explicit `setName`/`getName` calls. Only
the assignment form `base.name = 'x'` changed in 5.0. A bisect between alpha-8
and alpha-9 puts that at `5c668fcb61` (GROOVY-11381). There, the setter lookup
for a property write moved from `findSetters` to `findMethodsWithGenerated`,
which also returns the interface's abstract setter. GROOVY-11381 was never
backported to 4.0.x. The explicit-call form goes back further: the GROOVY-10922
stub went to 2.5.22, 3.0.15 and 4.0.9 with the synthetic accessor added last.
### A behaviour change to note
When the interface's accessor has a wider type than the property, choosing
the class's accessor also changes the inferred type under `@CompileStatic`.
With `interface Named { Object getName() }`, `class Base implements Named {
String name }`, `class Impl extends Base {}` and a receiver of type `Impl`:
| `@CompileStatic` | 4.0.33 / 5.1.3 / 6.0.0 | With this PR |
|---|---|---|
| `i.getName().toUpperCase()` | compile error (`Object#toUpperCase`) |
compiles |
| `h(i.getName())` with `h(Object)` / `h(String)` | `h(Object)` |
`h(String)` |
| `h(i.name)` | `h(String)` | `h(String)` (unchanged) |
| `Holder<Number>` interface with an `Integer value` property,
`h(i.getValue())` with `h(Number)` / `h(Integer)` | `h(Number)` | `h(Integer)` |
I think the new behaviour is right: it matches the property form, a compiled
class, and dynamic Groovy. But a static overload choice can change with no
compile error, so it needs to be in the notes.
### Before merging
1. **`COMPATIBILITY.md`**, a Groovy 7 entry covering:
- The assignment form `obj.prop = v` calls the class's setter again when
compiled together. This is a regression since 5.0.0, from GROOVY-11381.
- The explicit `obj.getX()` / `obj.setX(v)` calls now prefer the class's
accessor over an interface's abstract one when compiled together. This is a
change from all earlier versions.
- Under `@CompileStatic`, where the interface's declared type is wider
than the property's, the inferred type, and with it the overload chosen, can
change, as in the table above.
2. **A test for the overload case**, e.g. the `h(Object)` / `h(String)` one.
`Groovy12552` compares bytecode; an assertion on the overload chosen would pin
down the visible behaviour.
3. **The PR description and JIRA:** please correct "4.0.33 is not affected"
to the above.
### Backports
Both commits cherry-pick cleanly onto `GROOVY_6_0_X` and `Groovy12552`
passes there, so I'm happy to take it to 6.0.1 with a matching 6.0.1
COMPATIBILITY entry.
I also tried your `GROOVY-12552-5_1_X` branch (af58670a55, 2f2b25932a)
against the current `GROOVY_5_1_X`:
- It cherry-picks cleanly. Your base, 27aceec5d2, is now 17 commits behind,
but there are no conflicts.
- With JDK 17, `Groovy12552` passes 3/3. When everything compiles together,
there's no `INVOKEINTERFACE p/Named`, two `INVOKEVIRTUAL p/Impl.setName` (the
`base.name = 'x'` assignment and the explicit `setName`) and `INVOKEVIRTUAL
p/Impl.getName`.
- The overload probe also changes there. With the backport, `h(i.getName())`
picks `h(String)`; on 5.1.3 it picks `h(Object)`.
So 5.1.x would get all of the fix, not just the regression half. The
property assignment and the explicit calls both go through the same
`findMethod` ordering. Moving the synthetic accessor first fixes the assignment
form (the GROOVY-11381 regression). It also changes which method explicit
`getX()`/`setX()` calls pick, and with it the inferred return type and static
overload choice wherever the interface declares a wider type than the property.
Fixing only the regression would need a separate change limited to the setter
path of property assignments. Since the full change alters behaviour that 5.1.x
users can see, I'd stop at 6.0.x. If we decide 5.1.x needs the assignment fix,
it should be that narrower change, with an addendum note in the 5.1.x release
notes (I can help with that).
Finally, to cover Daniel's suggestion, I've opened GROOVY-12760 for the
interface default-accessor case (compiled together gives `invokeinterface`,
incremental gives `invokevirtual`), so it's tracked separately from this PR.
Thanks again!
> STC: property access calls an interface's accessor instead of the property's
> own when the property's class is in the same compilation unit
> ------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12552
> URL: https://issues.apache.org/jira/browse/GROOVY-12552
> Project: Groovy
> Issue Type: Bug
> Components: Static Type Checker
> Affects Versions: 5.1.3
> Reporter: James Daugherty
> Priority: Major
>
> When an interface declares the setter and getter of a property that a class
> implements as a Groovy property, static compilation calls the interface's
> methods with {{invokeinterface}} when the class is compiled together with the
> caller, and the class's own accessors with {{invokevirtual}} when the class
> is already compiled. The behaviour at run time is the same, but the same
> source compiles to a different class file depending on what else is compiled.
> Groovy 4.0.33 calls the class's accessors in both cases.
> {code:groovy}
> interface Named {
> void setName(String name)
> String getName()
> }
> abstract class Base implements Named {
> String name
> }
> class Impl extends Base {}
> @groovy.transform.CompileStatic
> class Svc {
> String rename() {
> Base base = new Impl()
> base.name = 'x' // invokeinterface Named.setName when Base is in
> the unit, invokevirtual Impl.setName when it is compiled
> base.setName('y')
> base.getName()
> }
> }
> {code}
> Reproduced with 5.1.3 and master. A reproducer with a groovyc script and an
> incremental Gradle build:
> https://github.com/jdaugherty/groovy-property-accessor-interface-method
> *Cause*
> A property of a class in the compilation unit does not have its accessors yet
> when static type checking runs. {{StaticTypeCheckingVisitor.findMethod}} adds
> a synthetic accessor for the property when every method found is abstract
> (GROOVY-10922), but after the interface's method.
> {{StaticTypeCheckingSupport.removeCovariantsAndInterfaceEquivalents}} only
> drops an interface method that comes after the method of a class implementing
> it, so both stay. The receiver-distance tie-break in {{chooseBestMethods}}
> then ranks an interface implemented by the superclass at 1 and the superclass
> itself at 4, so the interface method is chosen whenever the receiver's
> inferred type is a subclass of the property's class ({{Impl}} here); with the
> receiver exactly {{Base}} the accessor is chosen already. A compiled class
> lists its real accessor first, and the interface method is dropped.
> *Proposed fix*
> Add the synthetic accessor first, where a compiled class has its accessor, so
> that it is preferred over the interface method the same way.
> Found while making incremental builds of a Grails application produce the
> same classes as a full build; see also GROOVY-12550 and GROOVY-12544.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
