fix(core): generate legal, unique enum member names - #6
Conversation
Closes #5. Enum member names came out of a single sanitizing pass that could leave them illegal or colliding, and `dart analyze` rejected the result: - `-1` became `1` and `-2.5` became `25`, since the `value` prefix was only applied to names already starting with a digit, and the sign was stripped along with the other punctuation - `1.5` normalised onto integer `15`, because the decimal point was treated as a word separator - `new`, `class` and `default` were emitted as-is, as were `true` and `false` from boolean enums - `values` and `index` clash with members every Dart enum already has - values that sanitize down to nothing, like `日本` or `-`, produced empty identifiers - `Active`/`active` and `a-b`/`a_b` both reduced to one name Naming now runs in three passes: numbers are encoded so distinct values keep distinct names (`valueMinus1`, `value1Point5`), anything empty, digit-leading, reserved or enum-owned is prefixed, and a final pass suffixes whatever still collides. Values repeated after parsing - `[1.0, 1]`, `[0, -0]` - collapse onto one member rather than emitting two entries for the same @jsonvalue. Boolean enums also serialized as strings, the same defect #4 fixed for numbers, so they now emit bare literals with a `bool`-based extension. Their switch covers every case, so it gets no default clause. A null enum value in a string enum no longer emits `@JsonValue('null')`; it gets no member at all, matching what numeric enums already do.
qwlong
left a comment
There was a problem hiding this comment.
Review
Built the branch (420 tests passing) and put the generated output through a real toolchain — Dart 3.9.0, json_serializable 6.14.1, json_annotation 4.12.0 — rather than just reading the strings back.
The naming fix holds up completely
I tried to break legalizeEnumMemberName with everything the reserved-word list doesn't contain, generating string enums from each set and running dart analyze:
- built-in identifiers —
abstract as covariant deferred dynamic export external factory function get implements import interface library mixin operator part set static typedef - contextual keywords and modifiers —
await yield async sync on show hide of required late base sealed when extension Object/Enummembers —hashCode runtimeType toString noSuchMethod name- the PR's own targets — reserved words,
values/index, case collisions, separator collisions,日本,-,'',__,++
No issues found across all of it. So the list isn't under-inclusive: built-in identifiers and inherited members are legal as enum constants and genuinely don't need prefixing, while values and index — the two you did prefix — are exactly the ones that would break. That's the right cut, and numericEnumMemberName keeping the sign and decimal point is injective over JS numbers, so distinct values keep distinct names.
Dropping null members and collapsing repeated values are both correct, and match what I confirmed about $enumDecodeNullable on the previous PR.
The problem: @JsonValue doesn't accept double or bool
The names are now legal Dart, but for three of the value kinds this PR handles, the file still can't be built — json_serializable only accepts String, int, or null in a @JsonValue. Generating each kind and running build_runner:
| Enum values | build_runner |
|---|---|
[1, 2, 15, -1, 0] |
✅ builds |
['active', 'new', 'values', '日本', ''] |
✅ builds |
[1.5, 2.5] |
❌ The 'JsonValue' annotation on 'Dec?.value1Point5' does not have a value of type String, int, or null. |
[1e21] |
❌ same, on value1ePlus21 |
[true, false] |
❌ same, on Toggle?.valueTrue |
To be clear about attribution — none of these are regressions. I generated the same shapes off merged main: decimals already produced @JsonValue(1.5) and already failed this way, and booleans produced enum Toggle { true, false }, which doesn't compile at all. This PR moves booleans from one failure to a different one and fixes the naming half of decimals. But since it also adds the boolean branch together with tests that read as support, it'd ship a feature that cannot build. Details inline.
Worth noting these are invisible to the test suite by construction: every enum test asserts on the generated string, so "legal Dart" and "buildable Dart" are never actually checked. The test-generated-code job wouldn't catch them either — main still has || true on build_runner, dart analyze, and dart test (.github/workflows/ci.yml:77, :82, :87), and samples/petstore.yaml has no numeric or boolean enums. Same gap I flagged on #4; this PR is a concrete instance of what it lets through.
Small things
isBooleangates ontype === undefined || type === 'boolean', inheriting the body-vs-parameter asymmetry from #4:models.ts:246and:272still collapse a missing type to'string'for parameter enums, so an untypedenum: [true, false]behaves differently depending on where it sits. Same open item, not new here.uniqueEnumMemberNamecan produce mildly confusing names when suffixing meets numerics —[1, '1', 12]yieldsvalue1,value12,value122, where the second and third are easy to misread. Legal and unique, so purely cosmetic.
`@JsonValue` accepts String, int or null, so decimals, booleans and mixed value sets have no representation - build_runner refuses the file, whatever the member names are. Decimals already failed this way before this branch; booleans were failing on their names instead, and the boolean branch added here would have shipped a second way to fail. Those enums no longer become Dart enums. The schema keeps its scalar type and the allowed values stay a server-side constraint, the way a minimum/maximum would. A top-level one emits a typedef so `$ref`s to it still resolve, an inline one leaves the property scalar, and the boolean branch is gone with them. Integer and string enums are unchanged. Also from review: - `hashCode`, `runtimeType`, `toString` and `noSuchMethod` join the names an enum cannot declare - same conflicting_static_and_instance error as `values` and `index`, confirmed with dart analyze - the naming helpers move to getters/enum, where `enumValueToDartName` was a verbatim copy of the logic this branch is fixing, so there is one implementation rather than two - uniqueEnumMemberName documents that suffixes follow spec order, so reordering values renames members
|
Both reviews addressed in b852433. Reproduced the That is the whole class of the problem: legal member names were never going to be enough for decimals and booleans, since neither has a The fixValues json_serializable cannot express no longer become Dart enums. The schema keeps its scalar type and the allowed values stay a server-side constraint, the way a
A top-level one emits a typedef so The boolean branch is gone with them, which also settles P1 — no boolean enum is generated at all now, single-valued or otherwise, so there is no switch to be non-exhaustive. Added a test for VerificationGenerated a spec covering all of the above plus reserved words,
The restP2 — verified each one against
P3 — the naming helpers now live in P4 — the numeric assertions carry separators now ( P5 — documented on 424 tests pass. |
A parameter's Dart type is `{Method}{Path}{Param}Enum` by convention,
decided in endpoint-generator from nothing but the presence of `enum`.
Gating registration on whether the enum is representable left that name
and its import pointing at a file nobody writes:
import '../get_r_ratio_enum.f.dart'; // no such file
GetRRatioEnum? ratio, // no such class
Register every enum instead and let the top-level branch turn the
unrepresentable ones into typedefs, the way an array of unrepresentable
enum items already does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The forward-compatibility sentinel was found by member name, which de-duplication renames. `enum: ['Unknown']` sanitizes to a member spelled `unknown` that stands for a real value, so the sentinel was taken as already present and never added -- leaving fromValue(null) and the default arm decoding anything unrecognised as 'Unknown'. Look the sentinel up by the value it carries and, when the spec does not declare one, name the injected member through uniqueEnumMemberName. The template no longer hardcodes `unknown` either, since the sentinel can now be called something else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
generateEnum and getEnumData each turned a set of values into member names, and only generateEnum learned to drop nulls, collapse repeated values and de-duplicate names. Move the whole pipeline into buildEnumMembers and have both call it. Also read the scalar type off the values when a typedef'd enum declares none, so `enum: [true, false]` lands on bool rather than dynamic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
endpoint-generator built the type name by concatenation and referred to it as-is, while the declaration went through toDartClassName, which reads a run of capitals as an acronym. A header named `X-Flag` came out GetRXFlagEnum at the reference and GetRxFlagEnum at the declaration, so the generated package did not compile. Normalise the assembled name on both sides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Templates compile with escaping on, so a value reached @jsonvalue HTML-escaped: `a=b` became `a=b`, which compiles and then sends the wrong thing over the wire. Dart's own escaping was missing too - a bare `'` closes the literal and `$` starts an interpolation, so `$foo` did not compile at all, and `C:\path` silently lost its backslash. Put the values through a dartString helper that emits raw and escapes what Dart reads as syntax. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A static member of an extension is only reachable through the extension's
own name, so Status.fromValue('active') -- what a caller writes -- did not
compile. Move the declaration onto the enum and leave a forwarder behind
so StatusExtension.fromValue keeps working.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mapType answers String for any schema carrying an enum, so a response referencing `type: number, enum: [1.5, 2.5]` was declared Future<String> and decoded with `response.data as String`, which throws once a number arrives. Take the enum off before asking, the way the typedef path does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
qwlong
left a comment
There was a problem hiding this comment.
Re-review of the 8 commits since 095aff46
Rebuilt, ran the suite (437 passing), and put the output through freezed 3.x + json_serializable 6.14 on Dart 3.9. Everything I raised last time is resolved, and correctly.
First — a correction to my last review
I said the built-in identifiers and inherited members "genuinely don't need prefixing" and that the list wasn't under-inclusive. That was wrong, and my test didn't show what I claimed: sanitizeEnumMemberName lowercases the first segment, so feeding it 'runtimeType' produced runtimetype — my battery never generated the identifiers I said I'd cleared. Values like runtime_type and no_such_method do produce them, via ['runtime','type'] → 'runtime' + 'Type'.
Checking each one properly this time, one member per file:
| member | dart analyze |
|---|---|
runtimeType |
❌ conflicting_static_and_instance |
noSuchMethod |
❌ conflicting_static_and_instance |
index |
❌ conflicting_static_and_instance |
values |
❌ values_declaration_in_enum |
hashCode |
✅ legal |
toString |
✅ legal |
name |
✅ legal |
compareTo |
✅ legal |
So adding runtimeType and noSuchMethod fixes a real hole I'd waved through. Good catch.
The @JsonValue problem is fixed the right way
Decimals, exponents and booleans no longer produce enums that can't be built — they degrade to their scalar type, with the allowed values left as a server-side constraint:
typedef Ratio = double; // type: number, enum: [0.5, 1.5]
typedef Flag = bool; // type: boolean, enum: [true, false]and as properties, double? decimals, double? expo, bool? bools. Generating a model carrying integer, string, decimal, exponent, boolean, nullable-numeric and colliding-name enums side by side: build_runner writes its outputs and dart analyze reports No issues found. Last time the same battery failed on three of those kinds.
Degrading rather than falling back to quoted strings is the right call — a quoted "true" would have compiled while sending the wrong thing, which is the failure mode this whole series has been chasing.
The consequence of that got handled too
A parameter's Dart type is decided independently in endpoint-generator, so making unrepresentable enums vanish could have left GetXRatioEnum and its import pointing at a file nobody writes. Verified it doesn't:
// models/get_x_ratio_enum.f.dart
typedef GetXRatioEnum = double;
// models/params/list_x_params.f.dart
import '../get_x_ratio_enum.f.dart';
GetXRatioEnum? ratio,That package builds and analyzes clean as well. Registering every enum and letting the unrepresentable ones come out as typedefs is a tidier answer than special-casing the parameter path.
One small thing
ENUM_MEMBER_CONFLICTS is now over-inclusive by two — details inline. Cosmetic, since the result is still legal and unique.
| * fine - those come from an extension and from Comparable. | ||
| */ | ||
| const ENUM_MEMBER_CONFLICTS = new Set([ | ||
| 'values', 'index', 'hashCode', 'runtimeType', 'toString', 'noSuchMethod' |
There was a problem hiding this comment.
hashCode and toString don't actually conflict — Dart accepts both as enum constants:
enum E { hashCode } // No issues found!
enum E { toString } // No issues found!
enum E { runtimeType } // error - conflicting_static_and_instance
enum E { noSuchMethod } // error - conflicting_static_and_instanceSo enum: ['hash_code', 'to_string'] currently yields valueHashCode / valueToString where the unprefixed names would have compiled. Harmless — still legal, still unique — so this is only worth changing if you'd rather the common case read cleanly.
The comment above is the part I'd fix either way: "Declaring any of them is a conflicting_static_and_instance error" isn't true for those two, and it isn't the error values produces either (that one is values_declaration_in_enum). Since the next person to touch this set will reasonably trust that rationale rather than re-testing, an inaccurate one is worse than a slightly conservative list.
If you keep all six, something like "values and index are declared by every enum; runtimeType and noSuchMethod conflict with Object's instance members; hashCode and toString are legal but prefixed for consistency" would match what the compiler does.
There was a problem hiding this comment.
So is it okay to merge this PR?
Closes #5.
Problem
Enum member names came out of a single sanitizing pass, which could leave them illegal or colliding. Verified against
dart analyze— everything in the first two tables is a compile error, so the generated file does not build at all.Illegal identifiers
type: integer,enum: [-1]1type: number,enum: [-2.5]25enum: ["new", "class", "default"]new,class,defaultexpected_identifier_but_got_keywordtype: boolean,enum: [true, false]true,falseenum: ["日本", "-"]missing_identifierenum: ["values"]valuesvalues_declaration_in_enumenum: ["index"]indexconflicting_static_and_instanceThe
valueprefix was only applied to names already starting with a digit, so-1skipped it and then lost its sign to the punctuation pass. The decimal point was treated as a word separator, so1.5normalised onto integer15. Keywords and the members every enum already declares were never checked for.Collisions
enum: [1.5, 15]value15enum: ["Active", "active"]activeenum: ["a-b", "a_b"]aBenum: [1.0, 1]value1enum: [0, -0]value0Values with no
@JsonValuerepresentation@JsonValueacceptsString,intornull, so decimals and booleans cannot be Dart enums at all —build_runnerrejects the file whatever the member names are.type: booleancompounded it by emitting@JsonValue('true'), the string rather than the literal.A
nullenum value in a string enum emitted@JsonValue('null'), likewise the string.Change
Naming runs in three passes, because no single one covers the set:
-becomesMinus,.becomesPoint,+becomesPlus.-1isvalueMinus1,1.5isvalue1Point5,1e21isvalue1ePlus21.String(value)is canonical for JS numbers, so the mapping is injective. Values that already worked keep their names —200is stillvalue200.values/indexgets thevalueprefix.Active/activebecomeactive/active2.Layers 1 and 2 interact, which is why they land together: widening the prefix test alone turns
-1intovalue1, colliding with1.Enums json_serializable cannot express — decimals, booleans, mixed value types, integers beyond the safe range — no longer become Dart enums. The schema keeps its scalar type and the allowed values stay a server-side constraint, the way a
minimum/maximumwould. A top-level one emits a typedef so$refs still resolve; an inline one leaves the property scalar:Two smaller things in the same area:
[1.0, 1],[0, -0]— collapse onto one member. Two members sharing a@JsonValuewould make the generated map ambiguous.Two corrections to the above
Both were found by reviewing the first two commits against the real toolchain.
The representability gate was only on one side. A parameter's Dart type is
{Method}{Path}{Param}Enumby convention, decided inendpoint-generatorfrom nothing but the presence ofenum. Gating registration alone left that name and its import pointing at a file nobody writes:Every enum registers now, and the top-level branch turns the unrepresentable ones into typedefs — the same route an array of unrepresentable enum items was already taking.
De-duplication broke the
unknownsentinel, which was found by member name.enum: ['Unknown']sanitizes to a member spelledunknownthat stands for a real value, so the sentinel was taken as already present and never added, leavingfromValue(null)and the default arm decoding anything unrecognised as'Unknown'. It is found by the value it carries now, and the template no longer hardcodes the name — the injected member can end up calledunknown2.Neighbouring defects, fixed here too
These predate the PR. Each one is a generated file that does not compile, or that carries a value the spec never asked for:
$,',\in a value@JsonValue('$foo')did not compile;C:\patharrived asC:path&,=,>in a valuea=barrived asa=b— compiles, wrong over the wireGetRXFlagEnumat the reference against theGetRxFlagEnumthe model declaresfromValueStatus.fromValue('active')does not resolvetype: numbercame backFuture<String>andresponse.data as Stringthrewtypedynamicrather than the type its values haveTemplates compile with escaping on, which is where the HTML entities came from. Values go through a
dartStringhelper now that emits raw and escapes what Dart reads as syntax — a bare'closes the literal and$starts an interpolation.fromValuemoved onto the enum itself, with a forwarder left on the extension soStatusExtension.fromValuekeeps working.generateEnumandgetEnumDataeach turned a set of values into member names, and only one of them had learned to drop nulls, collapse repeats and de-duplicate. Both callbuildEnumMembersnow.Tests
16 cases in
models-enum.test.tsfor the original three groups — illegal names, collisions, values Dart cannot express — plus one for the null member. 13 more for the corrections and the neighbouring defects, each verified to fail without its fix.packages/core: 437 tests pass, and lint andtscreport exactly what they reported before.Generated one spec covering every shape above — reserved words, enum and Object members, case and separator collisions,
日本,-, quotes,$, backslashes, decimals, booleans, unrepresentable enums on query and header parameters, enum responses, and an object holding$refs to all of them — then ran the real toolchain over the output:build_runneris the gate that matters for@JsonValue:dart analyzealone does not check its types, which is how the decimal and boolean failures survived the first round. Neither gate catches a value that is merely wrong, so the output was also run:Notes
enum: ["value"]still generates a member namedvaluenext to the extension'sString get value. I expected that to conflict and it does not — enum members are static, the getter is an instance member, and the analyzer accepts it. Covered by a test so it does not get "fixed" later.The reserved-word list is deliberately narrower than
TypeMapper.DART_RESERVED_KEYWORDS. Checked 38 candidates againstdart analyze: built-in identifiers —static,dynamic,get,late,required,covariant,sealed,Function— are all legal enum members. Onlyvalues,indexand the Object members conflict. Reusing the wider list would prefix names that never needed it.A string enum response still comes back
Stringrather thanStatus. Only the part that threw at runtime is fixed here. Returning the enum means recovering the schema name from an inlined schema — generation dereferences, so the$refis gone by then — plus a decode branch in two template sets, and it changes a public return type. That belongs in its own change.The 3 pre-existing
no-unused-varslint errors inheaders-generator.*andgetters/body.tsare still there, untouched.