fix(scala): supported nested case class in fory-json-scala - #4006
Conversation
Scala 2 emits the `apply` and constructor-default static forwarders on a case class only for a top-level companion, so `fory-json-scala` did not recognize a case class declared inside an `object`. Such a type fell through to the generic Java object model, which discovered its fields for writing but had no creator for reading, so every property silently decoded to its default. Scala 3 emits those forwarders for any statically owned module and was unaffected. Resolve `apply` and `$lessinit$greater$default$N` from the companion singleton when the case class carries no static forwarders, keeping the static path as the fast path. `JsonObjectModel` now carries the receiver of instance constructor defaults, `JsonCreatorInfo` binds it into the default invoker, and both reader codegen paths invoke the default on that receiver. Reject, instead of silently decoding, a case class that cannot be reconstructed: one declared inside a class, which needs an outer instance, and one declared inside a method, whose companion is not reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses AI review findings on the previous commit. `ReflectionUtils.getLiteralName` skipped its nested-Scala-object correction whenever the canonical name ended with `$`, which is true of every companion module class. A companion declared two or more levels inside an object produced `a.B$Inner$.Depth$`, which names no resolvable type, so a generated reader for such a case class failed to compile. The correction now covers module classes. `ForyJsonGraalVMFeature` registers the companion class, its `MODULE$` field, and the `apply` and constructor-default methods that the Scala module queries when it rebuilds the object model at image runtime. Without this a nested case class resolved on the JVM but not in a native image. The comment justifying why an instance default skips `registerCreator` was wrong: no bound invoker is retained from build time; the real reason is that `creatorHandle` spreads an argument array over the exact parameter count and cannot describe a receiver. Recognition no longer initializes the companion: resolving the owner leaves the singleton unloaded, so deciding whether a type is a supported case class never runs a user object body, and `MODULE$` is read only once the model is built. `CompanionOwner` carries an explicit static-forwarder flag instead of inferring it from a null receiver, and the case-class marker also requires a declared `productPrefix` so fewer hand-written `Product` types are claimed. An instance default must now be declared by the created type's companion, and the model carries a receiver only when a default is actually bound to it. Tests cover the workspace codegen path through `@JsonUnwrapped`, a default that consumes a preceding constructor argument, a doubly nested companion, and the method-local rejection. The native-image main round-trips a nested case class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…itional Addresses the second and third AI review rounds. The receiver belongs only to the model shapes that can carry instance defaults. The full constructor is now private and its public form keeps its previous signature, so `fory-json-kotlin`, whose call site uses that shape, is untouched. `JsonObjectModel` also rejects a receiver that no default is bound to, so the invariant is total rather than checked per parameter. The unreconstructible-case-class claim applies only when the companion is unreachable. A reachable companion whose primary constructor this module does not support, such as a varargs or non-public one, keeps its previous handling instead of becoming a hard failure. A companion that exists but cannot be linked is no longer reported as an unreachable companion: only absence means the type has no companion, and a `LinkageError` now surfaces with the companion it failed to load. The workspace creator no longer hoists the defaults receiver into a local: that local ran on every construction, while the value is needed only on a missing argument. Both codegen paths now fetch it on the branch that uses it. GraalVM registration is narrowed to the members the Scala module looks up, and its helpers are named for the Scala companion they actually detect. Both rejection tests now assert their message. An outer-bound case class also has no reachable companion, so without that assertion either branch alone satisfied both tests, and the method-local case must be declared inside an object to reach the companion check at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the fourth AI review round. Recognition answers a predicate for every Product that reaches this module, including types it does not own and never claims. Reporting a companion that cannot link belongs to the owning path, so a `LinkageError` now fails only once `caseClassCodec` has committed to the type. Before this change an unrelated Product with a stale companion could fail resolution that previously succeeded through the core object model. The nested-module literal-name comment named the wrong cause: the mixed canonical name resolves for javac, and it is the generated-code compiler that mangles it into a name that resolves to nothing. The outer-instance rejection also fires for a case class enclosed by a trait, so its message and the doc say so. The native-image main also round-trips a nested case class with no defaults, which still needs the companion to match `apply`, and a doubly nested one, which additionally depends on the literal-name fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pjfanning
changed the title
AI review loop (AI_POLICY.md §5)Model: Claude Opus 5 (
Both were read-only: no edits, builds, tests, or pushes. Four rounds, each on the diff as it stood. Round 1 —
|
| 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. getLiteralNameis afory-corechange affecting all serializer codegen, and had only been
exercised through the JSON path. Thefory-scalabinary serializer suites now cover it —
88 tests, includingSingleObjectSerializerTest, which exercises exactly
object A { object B { case class C } }.- The
$outerrejection 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 matchapply) 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 —ScalaJsonEnumerationNativeImageMainandScalaJsonNativeImageMainare
referenced by no sbt task or workflow job (pre-existing; thegraalvm_jsonjob builds the Java
main fromintegration_tests/graalvm_tests).registerScalaCompanionis 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:checkcould not run locally — google-java-format throws
NoClassDefFoundErrorunder 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.
Addresses the fifth AI review round, where both reviewers independently found the same gap. Guarding only `Class.forName` was not enough to keep recognition free of linkage failures. `getField` and `getMethods` resolve member descriptors, so a companion or case class declaring a member whose type is absent at runtime threw a `NoClassDefFoundError` out of `isCaseClass` for a type this module does not own. Recognition now declines such a type; the owning path still reports it. Ambiguity stays loud, and the comment says so rather than promising blanket quiet. `CompanionOwner` keeps one piece of state: static forwarders are exactly the absence of a companion singleton. The literal-name comment named the wrong mechanism again. Verified against the compiled classes: a one-level companion is enclosed by the mirror class, so its canonical name ends with its only `$` and resolves; two or more levels put a `$`-terminated segment in the middle, which the generated-code compiler cannot resolve through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AI review loop: rounds 5 and 6Following the earlier summary, the loop ran two further rounds and has converged. Round 5 ( Round 6 ( Three optional round-6 nits were reviewed and deliberately not actioned, so that the clean result Artifacts updated — all twelve reviewer reports, verbatim, with a per-round outcome table: Verification added since the earlier comment: |
2 participants
Why?
Scala case classes defined inside Scala objects are not properly supported by fory-json-scala.
On Scala 2.13 such a type was not recognized as a case class, fell through to the generic Java
object model, and silently decoded every property to its default:
Recognition required a static
applyforwarder. Scala 2 emits those only for a top-level companion;Scala 3 emits them for any statically owned module, which is why Scala 3 was unaffected.
What does this PR do?
ScalaObjectModelsresolvesapplyand$lessinit$greater$default$Nfrom the companionsingleton when the case class carries no static forwarders. The static path stays the fast path.
JsonObjectModelcarries the receiver of instance constructor defaults,JsonCreatorInfobindsit into the default invoker, and both reader codegen paths invoke the default on that receiver.
ReflectionUtils.getLiteralNameskipped its nested-Scala-object correction for any canonical nameending in
$, which is every companion module class, so a companion two or more levels inside anobject produced a name the generated-code compiler cannot resolve. Fixed at the root.
ForyJsonGraalVMFeatureregisters the companion class, itsMODULE$field, and theapplyandconstructor-default methods the Scala module queries when it rebuilds the object model at image
runtime.
UnsupportedJsonTypeExceptioninsteadof silently decoding: one enclosed by a class or trait, which needs an outer instance, and one
declared in a method, whose companion is not reachable.
fory-json-kotlinis not touched: the publicJsonObjectModelconstructors it calls keep theirprevious signatures.
User-facing behavior change
A case class enclosed by a class, trait, or method previously serialized (the generic model
discovered its fields, including
$outer) and silently decoded back to an all-default instance.Both directions now raise
UnsupportedJsonTypeException. Writing a value that can never be readback is the trap this PR fixes, so the write is rejected with the read.
Related issues
AI Contribution Checklist
yesyes, I included the standardizedAI Usage Disclosureblock below.yes, I can explain and defend all important changes without AI help.yes, I reviewed AI-assisted code changes line by line before submission.yes, I completed line-by-line self-review first and fixed issues before requesting AI review.yes, I ran two fresh AI review agents on the current PR diff or current HEAD after the latest code changes: one Fory-guided reviewer usingAGENTS.mdand.agents/ci-and-pr.md, and one independent general reviewer in a separate clean-context session that was not pointed to.agents/ci-and-pr.mdor any copied Fory-specific review checklist. If the independent reviewer's tooling auto-loadedAGENTS.md, it followed the independent-review carve-out there.yes, I addressed all AI review comments and repeated the review loop until both ai reviewers reported no further actionable comments.yes, I attached screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes in this PR body.yes, I ran adequate human verification and recorded evidence (checks run locally or in CI, pass/fail summary, and confirmation I reviewed results).yes, I added/updated tests and specs where required.yes, I validated protocol/performance impacts with evidence when applicable.yes, I verified licensing and provenance compliance.AI review artifacts (verbatim final report of every review session, with an index):
https://gist.github.com/pjfanning/6b9fcd687e22450399df72dabc368e99
Verification
ScalaJsonSuiteScalaJsonEnumerationSuiteScalaJsonDerivationSuiteScala suites were run directly through the ScalaTest runner against locally built modules rather
than via
sbt +test: Scala 3.3.8 matches the cross-build, the 2.13 leg used 2.13.14 against therepo's 2.13.18 target. CI's cross-build is the authoritative check.
fory-coremaven suite: 2294 passed, 0 failures. Re-run becauseReflectionUtils.getLiteralNamefeeds every serializer's codegen, not only the JSON path.
fory-scalabinary serializer suites: 88 passed, for the same reason, includingSingleObjectSerializerTest, which exercisesobject A { object B { case class C } }.fory-jsonmaven suite: 964 passed, 0 failures.fory-json-kotlincompiles against unmodified Kotlin sources.FORY_CODE_DIRand confirmed to contain the new call in bothcodegen paths, including a default that consumes a preceding constructor argument.
prettier --write docs/json/scala.mdreports the file unchanged.Known gaps
nothing in the repo runs the Scala native-image mains —
ScalaJsonEnumerationNativeImageMainandScalaJsonNativeImageMainare referenced by no sbt task or workflow job (pre-existing; thegraalvm_jsonjob builds a Java main fromintegration_tests/graalvm_tests). The harness gainednested case-class round-trips so the coverage exists if it is wired up.
sbt +test,mvn spotless:checkandmvn checkstyle:checkwere not run locally. Spotlesscannot run in this environment: google-java-format throws
NoClassDefFoundErrorunder JDK 17, ona test file this PR does not touch. Formatting was checked by hand against the surrounding style
and every added line is within 100 columns; CI's format job is the first real check.
Does this PR introduce any user-facing change?
A case class enclosed by a class, trait, or method is now rejected rather than silently
round-tripping to defaults, as described above. No public API or wire-format change.
Benchmark