pjfanning commented on PR #4006:
URL: https://github.com/apache/fory/pull/4006#issuecomment-5471056433
## AI review loop (AI_POLICY.md §5)
Model: Claude Opus 5 (`claude-opus-5[1m]`), via Claude Code, for both the
authoring and the two
reviewers. The reviewers ran as separate clean-context sessions with
different instructions:
- **Fory-guided reviewer** — instructed to read `AGENTS.md` and follow the
review workflow in
`.agents/ci-and-pr.md`, plus `.agents/languages/{scala,java}.md`.
- **Independent general reviewer** — explicitly forbidden from loading
`.agents/ci-and-pr.md` or
any copied Fory checklist, per the independent-review carve-out in
`AGENTS.md`.
Both were read-only: no edits, builds, tests, or pushes. Four rounds, each
on the diff as it stood.
Current PR head is `a86a870b4`.
### Round 1 — `e493fdb86`
Both reviewers independently found the same four issues:
| Finding | Resolution |
| --- | --- |
| GraalVM: the companion class, `MODULE$`, and `apply` were never
registered, so a nested case class would resolve on the JVM but fail in a
native image | Added `registerScalaCompanion`, following the existing
`registerScalaEnumerationOwner` precedent |
| A comment claimed an instance default's "bound invoker already lives in
the creator metadata" — false; `buildDefaultInvokers` unreflects at runtime for
both static and instance defaults | Corrected; the real reason to skip
`registerCreator` is that `creatorHandle` spreads an argument array over the
exact parameter count and cannot describe a receiver |
| Recognition forced the companion's `<clinit>` just to answer
`isCaseClass`, running user `object` bodies during type resolution | Split
recognition from binding: `Class.forName(..., initialize = false)`, `MODULE$`
read only once the model is committed |
| `receiver == null` conflated "static forwarders" with "null singleton" |
Explicit `staticForwarders` flag |
One suggestion was **rejected with evidence**: both reviewers proposed
hoisting the per-parameter
receiver fetch in generated code. Implementing it broke three tests —
generated code for an
`Expression` instance is emitted once at its first use site, and each use
here is a separate
missing-argument block, so a shared instance referenced a local declared in
a sibling block. Reverted
with a comment recording why.
The requested doubly-nested test then exposed a **pre-existing bug in
`fory-core`**:
`ReflectionUtils.getLiteralName` skipped its nested-Scala-object correction
for any canonical name
ending in `$`, which is every companion module class. A companion two levels
inside an object
produced `pkg.A$B$.C$` — Janino: `"pkg.A$B$" declares no member type "C$"`.
Fixed at the root.
### Round 2 — `ec34ed31b`
The independent reviewer found a **blocker**: `defaultsReceiver` had been
inserted into the public
full `JsonObjectModel` constructor, whose 15-argument form
`KotlinMetadataModels.kt:182` calls, so
`fory-json-kotlin` would not compile. The Fory-guided reviewer did not catch
this.
Resolved by making the full constructor private and restoring the previous
public signature, so no
Kotlin file is touched. Verified with `mvn -pl fory-json-kotlin compile` on
unmodified Kotlin
sources.
Also resolved this round: the `copy`/`productPrefix` marker was claiming
types with a *reachable*
companion but an unsupported primary constructor (varargs, non-public),
turning previously working
generic-model handling into a hard failure — now restricted to unreachable
companions; and the
workspace-path receiver local ran on every construction, so it was reverted
to a conditional fetch.
### Round 3 — `c0d49bae6`
No blockers from either reviewer. Both confirmed the round-2 fixes, and the
independent reviewer
re-enumerated every `JsonObjectModel`/`JsonCreatorInfo` call site to confirm
each binds
unambiguously.
The notable finding was about the tests: the two rejection tests both passed
for the wrong reason.
An outer-bound case class *also* has no reachable companion, so deleting the
`$outer` check would
have left both green. Asserting the messages then exposed a second layer —
the method-local test was
declared inside the suite *class*, so it captured an outer instance and hit
the outer branch,
never reaching the companion branch it claimed to cover. It now lives in a
method of an `object`.
Also fixed: a `LinkageError` from a companion that exists but cannot be
linked was reported as
"companion is not reachable, such as a case class declared in a method"; the
receiver invariant in
`JsonObjectModel.validate` was made total; `CompanionOwner` carries the
`MODULE$` `Field`.
### Round 4 — `637b5063f`
**Fory-guided reviewer: no actionable findings.** **Independent reviewer:
would approve**, with
four non-blocking notes.
The two disagreed on one point, and the independent reviewer was right: the
round-3 fix for
`LinkageError` handling had been an overcorrection. Rethrowing inside
`companionOwner` put a hard
failure into `isCaseClass`, which is consulted for every non-tuple `Product`
reaching this module —
including types it does not own and never claims. An unrelated `Product`
with a stale companion
could therefore fail resolution that previously succeeded through the core
object model, where
before this PR `isCaseClass` did no class loading at all.
Also from this round, and fixed in `a86a870b4`:
- A comment stated the wrong cause. It said the deeply nested canonical name
"names no resolvable
type"; javac resolves it, and the actual evidence was a Janino error
(`"pkg.A$B$" declares no member type "C$"`). The comment now names the
real failure.
- `getLiteralName` is a `fory-core` change affecting all serializer codegen,
and had only been
exercised through the JSON path. The `fory-scala` binary serializer suites
now cover it —
88 tests, including `SingleObjectSerializerTest`, which exercises exactly
`object A { object B { case class C } }`.
- The `$outer` rejection also fires for a case class enclosed by a trait;
message and doc updated.
- The native-image main additionally round-trips a nested case class with no
defaults (which still
needs the companion to match `apply`) and a doubly nested one.
**`a86a870b4` (the current head) contains these fixes and has not itself
been re-reviewed**, so this
is not a clean final review of the head.
**Artifacts:**
https://gist.github.com/pjfanning/6b9fcd687e22450399df72dabc368e99 — the
verbatim
final report of all eight review sessions, extracted from their transcripts,
with an index.
## Verification
| Suite | Scala 2.13.18 | Scala 3.3.8 |
| --- | --- | --- |
| `ScalaJsonSuite` | 20 passed | 20 passed |
| `ScalaJsonEnumerationSuite` | 4 passed | 4 passed |
| `ScalaJsonDerivationSuite` | n/a | 10 passed |
`fory-core` maven suite: 2294 passed, 0 failures, re-run because the
`ReflectionUtils` change feeds
every serializer's codegen; `fory-scala` binary serializer suites: 88
passed, for the same reason. `fory-json-kotlin` compiles against unmodified
Kotlin sources. Generated
readers were dumped with `FORY_CODE_DIR` and confirmed to contain the new
call in both codegen
paths, including a default that consumes a preceding constructor argument.
## Known gaps
- **The GraalVM path is unverified.** No GraalVM in the dev environment, and
nothing in CI runs the
Scala native-image mains — `ScalaJsonEnumerationNativeImageMain` and
`ScalaJsonNativeImageMain` are
referenced by no sbt task or workflow job (pre-existing; the
`graalvm_json` job builds the Java
main from `integration_tests/graalvm_tests`). `registerScalaCompanion` is
therefore checked by
inspection only. Both reviewers flagged this in every round. The harness
gained a nested case class
round-trip so the coverage exists if someone wires it up.
- **`mvn spotless:check` could not run locally** — google-java-format throws
`NoClassDefFoundError` under JDK 17 on an unrelated test file. Formatting
was checked by hand
against the surrounding style; CI's format job is the first real check.
## Behavior change
A case class declared inside a `class` or a method previously serialized and
silently decoded back
to an all-default instance. Both directions now raise
`UnsupportedJsonTypeException`. Writing a
value that can never be read back is the trap this PR fixes, so the write is
rejected with the read.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
