daniellansun commented on PR #2770:
URL: https://github.com/apache/groovy/pull/2770#issuecomment-5219797463
Just a few thoughts after reading through the change. The idea makes a lot
of sense to me — I only have some questions about how we keep the marker
consistent and easy to maintain.
---
## What works well
The problem is real: things the compiler (or transforms) call directly in
the runtime are a compatibility surface that’s easy to miss. Classic call
sites, `ScriptBytecodeAdapter`, indy bootstraps, transform helpers,
GINQ/contracts/macro runtimes — all of that can outlive the compiler that
produced the bytecode.
A few things in this commit already feel right:
- Putting `@GroovyABI` on the `ScriptBytecodeAdapter` *class*, then
overriding only the newer methods (`compoundAssign`, `packedClosure`), is a
nice pattern.
- Touching the subprojects (callsite, contracts, ginq, macro) and not only
groovy-core matches where those calls actually land.
- Marking `CallSite` / `CallSiteArray` fits what we already say in
`COMPATIBILITY.md` about classic linkage for Groovy 4/5 bytecode on 6.
---
## Annotation shape and docs
### Retention
```java
@Retention(SOURCE)
public @interface GroovyABI {
String since() default "";
}
```
With `SOURCE` retention the marker never makes it into the published jars,
so japicmp (and anything else that only sees class files) can’t use it. That
might be exactly what you want if this is mainly for people reading the source
— if so, saying that somewhere would help.
If we later want a small check like “don’t drop a compiler-referenced member
by accident,” `CLASS` or `RUNTIME` would make that easier (closer to how
`@Incubating` works).
It might also be worth a short note in `COMPATIBILITY.md` so `@GroovyABI`
sits next to Public API / `@Internal` / the binary-compat check, rather than
living only in the annotation Javadoc.
### `since`
A few small inconsistencies stood out:
- Five places on `InvokerHelper` use `since="?"`.
- Version strings mix `1.0` / `1.0.0` and `2.0` / `2.0.0`.
- Spacing varies: `since = "1.6.0"` vs `since="4.0.0"`.
- The empty default (`default ""`) leaves it unclear when `since` is
required.
I’d lean toward one format (`x.y.z`), no `?` in the tree (either fill it in
or leave `since` off until we know), and a short rule for how this relates to
existing `@since` in Javadoc when both are present.
### Policy vs what’s annotated
The Javadoc says `groovy.*` isn’t annotated because it’s already public API,
but we do annotate members on `Closure`, `Reference`, and `Awaitable`.
Either story is fine:
- leave `groovy.*` alone and reserve `@GroovyABI` for internal/runtime types
the compiler still hard-wires, or
- use `@GroovyABI` even on public types when the *compiler* depends on that
specific member.
I’d just pick one and make the text match the tree. Small Javadoc nits while
we’re there: the notes use bare `<li>` without a list wrapper, and a `@since`
on the annotation type itself would match the usual style.
---
## Class-level vs every method
We already allow `TYPE`, and `ScriptBytecodeAdapter` uses that well.
Elsewhere we stamp the same `since` on every overload:
| Area | What happens |
|---|---|
| `Maps.of(...)` | ~100 identical annotations on a huge file |
| `CallSite` | every `call*` / `callSafe*` / … overload |
| `HashCodeHelper.updateHash` | every overload |
| DTT box/unbox | every method; the class itself isn’t marked |
Would it make sense to default to the type, and only mark members when
`since` differs or when only part of the type is ABI? That would cut a lot of
noise without losing the refinement pattern you already use on
`ScriptBytecodeAdapter`. `Maps` is the clearest case — the per-overload marks
don’t really say more than one class-level annotation would.
---
## How complete does the inventory need to be?
A partial list is still useful as a start. The risk is that people read “no
annotation” as “safe to change.” A few busy places show the gap:
| Surface | Role | Currently |
|---|---|---|
| `ScriptBytecodeAdapter` | main classic bridge | class-level — good |
| `DefaultTypeTransformation` | box/unbox **and** casts (`castToType`, etc.)
| box/unbox only |
| `NumberMath` | arithmetic / compare as well as bitwise | mostly bitwise /
shift / mod |
| `InvokerHelper` | property / invoke helpers | a subset; several with `?` |
| `CallSiteArray` | classic linkage (ctor + `array` / `owner`) | constructor
only |
Also, two annotations sit on **private** methods, which outside bytecode
can’t call:
- `AssertionRenderer.render()` (private) — the public static `render(String,
ValueRecorder)` is probably what we mean
- `FieldValues.findField(...)` (private helper)
Those look accidental; moving them to the public entry points (or dropping
them) would keep the meaning clear.
### Looking a bit further ahead
Keeping hundreds of hand-placed marks up to date will be hard. A lot of this
surface is already “registered” in the compiler via `MethodCaller` /
multi-adapters, indy bootstraps, and transform-generated calls. Even a simple
test that checks known registration points against `@GroovyABI` would help
later. No need to build that now — just something to keep in mind so the first
cut doesn’t fight that idea.
For v1, two shapes both seem reasonable:
1. **Narrow but complete** — e.g. call sites + `ScriptBytecodeAdapter` +
indy bootstraps, fully covered; or
2. **Wide pass** like now, but with type-level defaults, no `?`, no private
members, and a clear rule for what belongs (always-emitted bridges vs
occasional DGM / static-compile paths).
Right now a few DGM / `StringGroovyMethods` / `NumberMath` bits sit next to
the core bridges without saying why those and not others. One sentence of
guidance would go a long way so we don’t slowly mark half the GDK.
---
## A few edits that aren’t about `@GroovyABI`
These are easy to miss in the same commit:
| File | Change |
|---|---|
| `BinaryExpressionHelper` | drops an unused assignment after boxing |
| `ClosureWriter` | unused `callX` import |
| `StaticTypesCallSiteWriter` | whitespace on `"or"`, and **drops `case
"implies":`** |
The first two are harmless. The `implies` case changes STC dispatch for
number×number `implies` (it no longer takes the optimized path). Even if that
path is rare, it might be clearer in its own change — or with a short note and
a test if we keep it here. Splitting pure inventory from compiler behaviour
also makes history and reverts easier.
---
## Optional next steps
Nothing urgent — more a suggested order if we iterate:
1. Spell out the contract briefly in `COMPATIBILITY.md` (what it means,
class vs member, version format, how it relates to public API).
2. Tighten the annotation (retention, `since` rules, Javadoc, `groovy.*`
wording).
3. Prefer type-level marks when everything shares the same `since`.
4. Clean the inventory: resolve/drop `?`, fix private members, either finish
the core bridges or document an explicit v1 subset.
5. Split (or call out) the unrelated compiler cleanups and the `implies`
change.
6. Later: a lightweight check against known compiler registration points.
---
## Closing
I like the direction. Making the compiler↔runtime surface visible will help
future work. The main things I’d still like to settle are: who the annotation
is for (humans only vs tooling), leaning on type-level marks so the inventory
stays readable, and making the marked set either complete for a stated scope or
clearly “work in progress,” so it stays a trustworthy signal.
--
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]
