Skip to content

chore: simplify serialization codegen and fix RS2007 release tracking#7

Merged
thygrrr merged 1 commit into
mainfrom
chore/simplify-serialization-and-release-tracking
Jun 16, 2026
Merged

chore: simplify serialization codegen and fix RS2007 release tracking#7
thygrrr merged 1 commit into
mainfrom
chore/simplify-serialization-and-release-tracking

Conversation

@thygrrr

@thygrrr thygrrr commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Code-quality cleanup from a /simplify pass over the recently-merged work (serialization + constraints). No behavior change.

Serialization converter codegen (AliasCodeGenerator)

  • Dedup the string case. For [newtype<string>], typeof(T) is typeof(string), so the generated TypeConverter previously contained duplicate conditions (typeof(string) || typeof(string)) and unreachable is string / destinationType == typeof(string) branches. Now gated on AliasedTypeSpecialType == System_String to emit a single string arm.
  • Cache the inner converter. ConvertFrom/ConvertTo re-ran TypeDescriptor.GetConverter(typeof(T)) on every call. Now resolved once into a static readonly _inner field (emitted only for the non-string path that uses it).

AliasModelExtractor

  • ToLocationStruct called GetLineSpan() twice; now once via .Span.
  • Removed a no-op .Where(x => x is not null) over GetAttributes() (Roslyn never returns null elements).

Release tracking (AnalyzerReleases.Unshipped.md)

  • The rules table was written as a GitHub-Flavored-Markdown table (surrounding pipes + backtick-quoted category), which the RS2xxx release-tracking parser rejects → RS2007 warning, and the NEWTYPE001/NEWTYPE002 entries were silently discarded (not actually tracked).
  • Rewritten into the strict format (no surrounding pipes, bare Category). Category/Severity match the DiagnosticDescriptors, so the entries validate and RS2007 is gone.

Validation

  • Generated output confirmed: string newtype has no _inner field and a single typeof(string) term; int newtype has one cached _inner.
  • Full solution builds with 0 warnings, 0 errors.
  • 395/395 tests pass in Debug (net8.0/9.0/10.0).

Note: the *_Throws constraint tests fail under -c Release because the constraints feature wraps its validation throw in #if DEBUG (unless IncludeInRelease=true). That's pre-existing and unrelated to this PR — worth confirming CI runs them in Debug.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Summary by CodeRabbit

  • Bug Fixes

    • Improved type conversion behavior for aliased string types in generated TypeConverter classes, with better handling of conversion scenarios.
    • Enhanced performance of type conversion operations through optimized converter caching in generated code.
  • Refactor

    • Refined validation method extraction and location span computation logic.
  • Documentation

    • Updated release notes formatting.

Code-quality cleanup (no behavior change):

- AliasCodeGenerator (serialization converter codegen):
  - For string-backed newtypes, emit a single string arm instead of
    duplicate `typeof(string) || typeof(string)` conditions and
    unreachable `is string` / `destinationType == typeof(string)`
    branches (T == string makes typeof(T) == typeof(string)).
  - Cache the underlying TypeConverter in a `static readonly _inner`
    field instead of calling TypeDescriptor.GetConverter(typeof(T))
    on every ConvertFrom/ConvertTo (emitted only for the non-string
    path that uses it).

- AliasModelExtractor:
  - ToLocationStruct: call GetLineSpan() once via `.Span` instead of
    twice.
  - Drop a no-op `.Where(x => x is not null)` over GetAttributes()
    (Roslyn never returns null elements).

- AnalyzerReleases.Unshipped.md: rewrite the rules table into the
  strict analyzer-release-tracking format (no surrounding pipes, bare
  category) so NEWTYPE001/NEWTYPE002 are actually tracked and RS2007
  no longer warns.

Full solution builds with 0 warnings; 395/395 tests pass (Debug).
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 434e10a6-1930-4e54-8bf5-9cf004dacae7

📥 Commits

Reviewing files that changed from the base of the PR and between 03da871 and 9d9ab14.

📒 Files selected for processing (3)
  • NewType.Generator/AliasCodeGenerator.cs
  • NewType.Generator/AliasModelExtractor.cs
  • NewType.Generator/AnalyzerReleases.Unshipped.md

Walkthrough

The source generator's AppendSerializationConverters is updated to detect when the aliased type is string and branch ConvertFrom/ConvertTo accordingly, caching _inner only for non-string aliases. The model extractor drops a redundant null filter on attributes and compacts ToLocationStruct into a ternary. The unshipped release notes table is reformatted from Markdown pipe syntax to plain-text pipe-delimited layout.

Changes

TypeConverter string specialization and generator cleanup

Layer / File(s) Summary
Generated TypeConverter string specialization
NewType.Generator/AliasCodeGenerator.cs
Adds aliasedIsString flag; ConvertFrom constructs alias directly from string when aliased type is string, otherwise uses cached _inner; ConvertTo returns wrapped._value for string destination when aliased type is string, or delegates to _inner.ConvertToString for other types.
Model extractor cleanup
NewType.Generator/AliasModelExtractor.cs
Removes .Where(x => x is not null) guard from ExtractValidationMethod attribute iteration; ToLocationStruct rewritten as a compact ternary using GetLineSpan().Span.
Release notes reformatting
NewType.Generator/AnalyzerReleases.Unshipped.md
Replaces Markdown pipe-table with backticks with a plain-text pipe-delimited layout for NEWTYPE001 and NEWTYPE002 entries.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • outfox/newtype#6: Introduced the AppendSerializationConverters method in AliasCodeGenerator.cs that this PR extends with string-alias specialization.

Poem

🐇 Hop hop, the converter's bright,
When T is string, no inner in sight!
A ternary here, a null check gone,
The generator hops swiftly along.
Cleaner code under the warren's moon —
This rabbit ships a tidy tune! 🌙

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/simplify-serialization-and-release-tracking

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thygrrr thygrrr merged commit ea0d2db into main Jun 16, 2026
1 of 2 checks passed
@thygrrr thygrrr deleted the chore/simplify-serialization-and-release-tracking branch June 16, 2026 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant