Modernize option defaults for the next major release#2952
Merged
Conversation
Klaxon has been unmaintained since ~2022 and our own fixture skip-lists cite its open bugs; the Jackson renderer is feature-complete and fully CI-covered. The kotlin/schema-kotlin fixtures now pin framework=klaxon explicitly so the Klaxon renderer keeps end-to-end coverage (kotlin-jackson and kotlinx fixtures cover the other frameworks). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
System.Text.Json is the built-in, Microsoft-recommended serializer; Newtonsoft.Json is in maintenance mode. The csharp, schema-csharp, schema-json-csharp, and graphql-csharp fixtures now pin framework=NewtonSoft explicitly so the Newtonsoft renderer keeps its end-to-end coverage (the SystemTextJson fixtures already exist). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
C++17 std::optional/std::variant have been universally available since ~2018; requiring Boost by default is a legacy accommodation. CI already compiles the fixture at -std=c++17. The quicktest options now exercise boost=true so the Boost code path keeps coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… edition-2018 - visibility now defaults to public: private fields on generated structs can neither be constructed nor read by consumers, and every modern Rust codegen emits pub fields. - derive-debug and derive-clone now default to true, per the Rust API guidelines (C-COMMON-TRAITS); all emitted field types are Clone, so this always compiles. - The vestigial edition-2018 option is deleted: it only toggled between 'use serde::...' and the pre-2018 'extern crate serde_derive;' prelude. The 2018+ form is now unconditional. Passing --edition-2018 is an error now (breaking). A quicktest combo pins the old defaults (visibility=private, no derives) so the legacy output shape keeps fixture coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ecosystem has turned on TypeScript enum syntax: Node's default type stripping and tsc --erasableSyntaxOnly both hard-error on it. Enums now render as unions of string literals by default; --no-prefer-unions restores the old output. Runtime typechecking of enum values is unaffected (the typeMap lists case values either way), so the schema fixtures' enum fail-samples still fail as required. Flow always rendered enums as string-literal unions and ignores this option; its output is unchanged. quicktests pin prefer-unions=false for typescript (covering the enum code path) and flow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
List<T> is the universal Java DTO idiom (Effective Java, OpenAPI Generator, jsonschema2pojo); raw arrays fight equals/hashCode and generics. The quicktest entries for the java, java-datetime-legacy, and java-lombok fixtures now pin array-type=array so the T[] code path keeps coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- final-props now defaults to true: Effective Dart and all current data-class idioms (freezed, @immutable culture) use final fields. A quicktest pins final-props=false so the mutable-property path keeps coverage. - The null-safety option is deleted: null-safe output is the only choice that compiles under Dart 3, so the non-null-safe branches were dead weight. Null-safe rendering is now unconditional; passing --null-safety/--no-null-safety is an error now (breaking). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- density=normal (one property per line, explicit CodingKeys) is what swift-format, SwiftFormat, and the Google Swift style guide produce; merged declarations read as a codegen quirk. Round-tripping is identical. The dense path stays covered by the existing density=dense quicktest; the now-redundant density=normal quicktest is dropped. - The swift-5-support option is deleted: the renderer never read it, so it has been a no-op for years. Passing --swift-5-support is an error now (breaking). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hand-written aeson records overwhelmingly use [a]; the Vector default was inherited from the Elm renderer and forces an extra import. The quicktest entry now pins array-type=array so the Vector code path keeps coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…alization
The emitted adl_serializer for optionals handled null by calling the
optional *factory* with no arguments. That is only correct in boost
mode, where the factory is boost::optional and produces an empty
optional. In boost-free mode std::make_optional<T>() wraps a
default-constructed T, and std::make_shared<T>() (both modes) points at
a default-constructed T — so a JSON null inside a union or array came
back as 0/""/{} on round-trip. Null now maps to an empty optType<T>().
This is a pre-existing bug in the boost=false option; flipping the
default to boost=false in this branch merely exposed it, because the
old {boost:"false"} quicktest never actually ran: quicktests without a
pinned input run against combinations[1-4].json, which are all in the
C++ fixture's skipJSON. The boost quicktests are therefore now pinned
to unions.json (nulls inside unions, where the boost and std code
paths differ) and pokedex.json so they really execute.
Verified locally: QUICKTEST=true FIXTURE=cplusplus passes end to end
(63 tests), with the boost=true quicktests compiled against Boost 1.84
headers; unions.json round-trips byte-identically in both modes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
In list mode, the union deserializer read JSON arrays with
jsonParser.readValueAs(new TypeReference<List>() {}) — a raw List, so
Jackson skipped element-type checking entirely and schema-invalid
inputs (e.g. a string array where List<Long> is expected) deserialized
successfully instead of being rejected. The TypeReference now carries
the full generic type (javaType instead of javaTypeWithoutGenerics),
e.g. TypeReference<List<Long>>.
This is a pre-existing bug in the array-type=list option; flipping the
default to list in this branch merely exposed it, via the
implicit-class-array-union.schema expected-failure sample in the
schema-java fixture (its .fail.union.json no longer failed).
Verified by running the generated code against Jackson 2.17: all five
valid samples round-trip, both fail samples now exit nonzero, and the
old code demonstrably accepted the bad input. A corpus-wide sweep of
all schema and JSON inputs in list mode shows every emitted
TypeReference now carries its element type; union-heavy inputs
(unions.json, combinations1-4, php-mixed-union, optional-union,
union-constructor-clash) round-trip within the harness' tolerances.
(Union members of map type still use raw readValueAs(Map.class) — that
looseness is mode-independent and pre-existing in both array and list
modes; left as a follow-up.)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # test/languages.ts
With #2694 merged, C# 8 output with nullable reference types becomes the out-of-box experience, pairing with the SystemTextJson default. Both csharp fixtures now pin csharp-version=5 and =6 in their quicktests (replacing the now-redundant =8 rows), so all three language-version code paths stay covered. Promoting v8 to the base schema-fixture runs put Newtonsoft v8 over the full schema corpus for the first time (quicktests only cover the JSON fixtures) and exposed a gap in #2694's NRT pragma set: the emitted constraint-check and string-transformer helpers produce CS8602, CS8604, and CS8625 warnings under '#nullable enable', which 'dotnet run' prints to stdout ahead of the JSON, breaking the fixture comparison. Those three codes are now suppressed alongside the existing CS8618/CS8601/CS8603(/CS8765) pragmas; a corpus-wide compile scan of every schema under both frameworks confirms no other warning codes remain. Suppressing CS8602 also fixes the exact issue behind three schema-csharp-SystemTextJson skips (minmaxlength, optional-constraints, optional-const-ref), so those schemas are un-skipped. Verified locally with dotnet SDK 8.0.423: QUICKTEST runs of csharp, csharp-SystemTextJson, schema-csharp, and schema-csharp-SystemTextJson all pass end to end (272 tests), with every quicktest row confirmed executing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…roperties For an optional property whose type is itself a nullable union (e.g. GraphQL's nullable String, which infers as string | null), propertyDefinition calls nullableCSType on the union, whose csType rendering already resolves the union through nullableCSType and appends '?'. At csharp-version 8 the outer call then appended a second one, emitting 'public string?? Name' — invalid C# (CS1519). The same path produced 'long??' for optional nullable value-type unions at any version. nullableCSType now unwraps a nullable union before rendering, so the annotation is applied exactly once. This is a pre-existing bug in the csharp-version=8 option: the GraphQL fixtures run base options only, so flipping the default to 8 put v8 GraphQL output in CI for the first time (caught by graphql-csharp on github5.graphql). Verified locally with dotnet SDK 8: a corpus-wide sweep (all GraphQL, schema, and JSON inputs under both frameworks, 242 generated files) contains no doubled '?' in any type position, a representative set compiles clean, and the full CI fixture set - csharp, schema-csharp, schema-json-csharp, graphql-csharp, csharp-SystemTextJson, schema-csharp-SystemTextJson - passes end to end (326 tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Modernizes the Elm target from Elm 0.18 to Elm 0.19 and brings the Elm fixtures back into CI.
Breaking: Elm 0.18 output is removed
The renderer now emits Elm 0.19 code only. There was never a version option, so nothing was deleted from the options surface — the generated code simply targets 0.19 now, and 0.18 projects can stay on older quicktype releases.
Generated-output changes
Jdec.succeed Fooinstead ofJpipe.decode Foo(decodewas removed in NoRedInk/elm-json-decode-pipeline 1.0; install instructions in the header comment updated accordingly).Json.Encode.list/arraytake the element encoder,Json.Encode.dictexists) makes the generatedmakeArrayEncoder/makeListEncoder/makeDictEncoderhelpers unnecessary — they are gone; onlymakeNullableEncoderremains.map/toList(ambiguous under 0.19 shadowing rules):import Dict exposing (Dict), plusimport Array exposing (Array)only with--array-type array;Listneeds no import.x,y,r,f,m,str,somethingElse) are now forbidden as generated global identifiers (caught bypriority/unions.json, which has a type namedx)._(an 0.19 syntax error, caught byblns-object.json).\u{XXXX}syntax instead of\uXXXX/\UXXXXXXXX(caught byidentifiers.json/nst-test-suite.json).array-typenow defaults tolistDefaults-audit recommendation #13, deferred until this modernization:
Listis the idiomatic Elm collection, and with the 0.19 encoder API thelistpath needs no helper at all.--array-type arraystill works and stays covered in CI by a quicktest entry (quickTestRendererOptions).Fixture revival
The driver in
test/fixtures/elm/is rewritten for 0.19:elm-package.json→elm.json(elm/core, elm/json, NoRedInk/elm-json-decode-pipeline 1.0.1).Main.elmusesPlatform.worker(Platform.programis gone) andJdec.errorToStringfor decode errors.runner.jsuses the 0.19 embedding API (Elm.Main.init()instead ofElm.Main.worker()).Warmup.elmmodule is compiled once by the setup command so all package dependencies are downloaded/built into the sharedELM_HOMEcache before per-sample compiles run in parallel (elm 0.19.1 can corrupt its package registry on concurrent cold-cache downloads). Thesysconfcpushack for the 0.18 compiler is gone.Skip lists (every entry re-verified against elm 0.19.1)
Un-skipped (fixed by the renderer changes above):
identifiers.json,simple-identifiers.json,blns-object.json,nst-test-suite.json,keywords.json.Still skipped, with verified reasons:
recursive.json,direct-recursive.json,bug427.json,bug790.json,list.jsonand the recursion schemas (union-list,list,ref-remote,mutually-recursive,postman-collection,vega-lite,simple-ref, plus newlyrecursive-union-flattening): Elm type aliases cannot be recursive.constructor.schema,keyword-unions.schema: compile and run now, but elm/json's field decoder uses the JSinoperator, which finds inheritedObject.prototypemembers — decoding{}yields a non-null"constructor".nested-intersection-union.schema: the generated decoder accepts invalid union members because all class properties decode viaJpipe.optional(same limitation as documented for C++).skipDiffViaSchema(schema round-trip changes inferred names/types); all 20 pre-existingskipDiffViaSchemaentries were re-checked and still differ.CI
- elm,schema-elmis re-enabled in the test matrix; the workflow's elm 0.19.1 install step already existed. Locally validated with elm 0.19.1: the full corpus (215 JSON tests incl. misc, 67 schema tests) passes for bothFIXTURE=elmandFIXTURE=schema-elm, as donpm run test:unitandnpm run lint.🤖 Generated with Claude Code