blackdrag commented on PR #2770:
URL: https://github.com/apache/groovy/pull/2770#issuecomment-5222268882
I agree with most what you both said, but since this started to be a tiring
piece of work (5h of finding methods initial commits, compare with tags and so
on) I wanted first to get some feedback before proceeding.
> ## Annotation shape and docs
> ### Retention
[...}
> 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.
agreed will do. The primary purpose was information for the developer, not
for a tool, that is why I did not think of that.
> ### `since`
>
> A few small inconsistencies stood out:
>
> * Five places on `InvokerHelper` use `since="?"`.
yeah, I have to investigate further on those.
> * Version strings mix `1.0` / `1.0.0` and `2.0` / `2.0.0`.
the longer version should be always used, will change that.
> * Spacing varies: `since = "1.6.0"` vs `since="4.0.0"`.
ah well... ok
> * The empty default (`default ""`) leaves it unclear when `since` is
required.
I had to first to work a bit with it and then let it settle. I think now
since should be mandatory.
[...]
> ### 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`.
yeah, those have to go.
[...]
> 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.
if it is every public method in that file, then we can mark it on type level
using the earliest version and if there are later introduction we can use the
later version directly on those. But what if we have mixed in public methods
that are not used from bytecode? An internal marked class with bytecode ABI
markers on some of the public methods is still allowed to change those public
methods.
> ## 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
I think castToType for example shows the problem. I did not mark it, because
I did not verify its usage. I did not work through InvocationWriter for
example. And while that uses castToType, it is from SBC. I did another search
and could not find a usage by the compiler. So maybe it was in the past?
Tranditionally there is SBC.castToType that uses that now. So most likely the
method was never used directly by compiler produced bytecode. But can I be
sure? No. It means digging deep into the commit history and trying to find the
usage on an checkout of an old version most likely. The probability is high
though it was never used from the compiler directly. Thus the annotations go on
the box/unbox methods only, none on the class level.
[...]
> ### 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.
A once placed marker does not need an update. The
MethodCaller/multi-adapters help, but what if we decide to use a different
method in the future? The old entry point is potentially lost and forgotten,
then maybe removed and we broke older programs. Extracting StringDGM for
example broke older programs because some of the string methods in there where
used directly. That did happen in 3.0.0, and it took till 3.0.24 I think before
we fixed it. transform generated did proof to be more difficult actually. Only
following the callX usages did help, but sometimes the call is abstracted
through several levels and it gets difficult fast. indy bootstraps are another
good example. We broke that before. But it is only the bootstrap method we have
to consider here, not methods we produce method handles for. Unless the handle
is stored in the class bytecode, then we have to deal with handles as well. So
far the bootstrap method works like an isolation layer here. Adding new
bootstrap methods l
ike some of the Groovy 6 methods are a different story. There are also for
example for methods used by runtime generated classes. These are also not to be
considered.
[...]
> 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.
I tried to give those in the javadoc of the annotation, maybe needs to be
improved.
> ## 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.
The implies drop should maybe be extracted. But fact is we do not compile
1==>1 to 1.implies(1). We compile that by converting the numbers to boolean (in
a quite inefficient way btw.) and then handle it as boolean intrinsic. The path
I removed is never visited. In other words the optimization was never in
BinaryExpressionHelper. If I where wrong, there should have been failing tests.
Though I have the feeling we lack tests with implies and static compilation and
with anything but booleans as well.
> ## 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.
agreed I guess. Since the idea was well received so far I also have to add a
JIRA issue.
--
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]
