fix: decimal build failure (#5) and opt-in serialization (#4)#6
Conversation
#5: decimal defines op_Increment/op_Decrement as real metadata operators (unlike int/double), so the generator emitted `new Id(++value._value)`, mutating the readonly _value field and failing with CS0191. Generate ++/-- by incrementing a local copy instead. Fixes any aliased type with metadata ++/-- (e.g. BigInteger). #4: add NewtypeOptions.Serializable, an opt-in flag that emits a nested System.Text.Json JsonConverter<T> and a TypeConverter (plus the [JsonConverter]/[TypeConverter] attributes). Newtypes then serialize as their underlying value; both converters delegate to T's own serializer, so any serializable T works for every newtype form. docs: README/NUGET serialization section and a lowercase-naming rationale (#2, closed as working-as-intended). Tests: decimal ++/-- regression, serialization round-trips, and generator-output assertions. 387 tests pass on net8/9/10.
WalkthroughAdds an opt-in ChangesSerializable Option and Decimal Fix
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant JsonSerializer as System.Text.Json
participant JsonConverter as NewtypeJsonConverter
participant TypeConverter as NewtypeTypeConverter
participant Underlying as Underlying T
Client->>JsonSerializer: Serialize(serialId)
JsonSerializer->>JsonConverter: Write(writer, value)
JsonConverter->>Underlying: writer.Write(value._value)
Underlying-->>JsonConverter: serialized underlying value
JsonConverter-->>JsonSerializer: bare JSON value written
JsonSerializer-->>Client: "{\"contract\":\"CTR-002\"}"
Client->>TypeConverter: ConvertFrom(string)
TypeConverter->>Underlying: parse string to T
Underlying-->>TypeConverter: T value
TypeConverter-->>Client: new SerialId(T value)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NewType.Tests/SerializationTests.cs`:
- Around line 99-105: The test NonSerializable_Newtype_HasNoCustomConverter uses
a negative assertion that only excludes one specific converter type, but Name
could still get its own generated converter and pass the test. Replace the
Assert.IsNotType check with a positive assertion that validates the expected
converter type for non-serializable newtypes. Assert that the converter for Name
matches the expected default converter type (not a custom generated one), which
properly validates that the serialization opt-in gate is enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cb1b379-6758-4372-b2ea-37c3266d918c
📒 Files selected for processing (10)
NewType.Generator/AliasAttributeSource.csNewType.Generator/AliasCodeGenerator.csNewType.Generator/AliasModel.csNewType.Generator/AliasModelExtractor.csNewType.Generator/NUGET.mdNewType.Tests/GeneratorTests/GeneratorOutputTests.csNewType.Tests/PrimitiveTests.csNewType.Tests/SerializationTests.csNewType.Tests/Types.csREADME.md
NonSerializable_Newtype_HasNoCustomConverter previously asserted the converter was not SerialId's converter — a different type's converter, which would pass even if Name got its own generated converter. Assert the exact default TypeConverter instead, which fails if any custom converter is generated, properly enforcing the Serializable opt-in gate.
Resolves the open issues in the repo.
#5 —
[newtype<decimal>]fails to build (CS0191)decimal(unlikeint/double) definesop_Increment/op_Decrementas real user-defined operators in metadata, so the generator emittednew Id(++value._value)/--value._value, mutating thereadonly _valuefield →CS0191.AppendUnaryOperatorsnow special-cases++/--to increment a local copy:This also covers any other aliased type with metadata
++/--(e.g.BigInteger).#4 — Serialization support (opt-in)
New
NewtypeOptions.Serializableflag. When set, the generator emits:System.Text.JsonJsonConverter<T>+[JsonConverter(...)]TypeConverter+[TypeConverter(...)]Both delegate the underlying value to
T's own serializer, so the newtype serializes as its underlying value and any serializableTworks — for every newtype form includingreadonly struct. Covers System.Text.Json, Newtonsoft.Json, ASP.NET model binding, and configuration binding.#2 — PascalCase attribute name
Closed as working-as-intended: the lowercase
newtype<T>is deliberate (mirrors the namespace, NuGet package id, and Haskell'snewtypekeyword; the class keeps theAttributesuffix internally). Rationale added to the README/NUGET docs.Tests
++/--regression + arithmetic/comparisonTypeConverter(exact-type assertion, so any generated converter would fail it)387 tests pass on net8.0, net9.0, and net10.0.
Closes #5
Closes #4