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