Skip to content

Write codegen outputs as explicit UTF-8 - #1720

Merged
angularsen merged 2 commits into
masterfrom
agl-codex/codegen-explicit-utf8
Aug 1, 2026
Merged

Write codegen outputs as explicit UTF-8#1720
angularsen merged 2 commits into
masterfrom
agl-codex/codegen-explicit-utf8

Conversation

@angularsen

Copy link
Copy Markdown
Owner

Motivation

CodeGen currently relies on default file encodings from File.WriteAllText, File.CreateText, StreamWriter, and related read helpers. Those defaults can differ by runtime/API history and make generated output encoding/BOM behavior less obvious than it should be.

We want generated output to be stable across platforms and avoid review noise from UTF-8 BOM differences.

Changes

  • Add a small CodeGenFile helper that reads text as UTF-8 and detects an existing BOM, but writes generated/codegen-normalized files as explicit UTF-8 without BOM.
  • Use the helper for generated C# files, generated resource text files, normalized relation JSON, unit enum allocation JSON, quantity JSON reads, and codegen edit helpers.

Validation

  • dotnet build CodeGen\CodeGen.csproj
  • generate-code.bat
  • Scanned generated/codegen-normalized outputs and found no UTF-8 BOM.
  • Running code generation after the change produced no generated-file diffs.

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review

Reviewed the diff for #1720. This is a scoped, CodeGen-internal change (no GeneratedCode/* output changed, no unit definitions touched), so most of the usual checklist items don't apply here — no breaking changes, no new quantities/units, and no generated-code diffs to inspect.

Code quality

  • CodeGenFile mirrors the System.IO.File surface (ReadAllText/WriteAllText/OpenText/CreateText), which makes the call-site diffs essentially mechanical and easy to review. Good approach for this kind of cross-cutting change.
  • The fix is real, not just cosmetic: File.CreateText(...) and new StreamWriter(path) default to Encoding.UTF8, which does emit a BOM in .NET — that's the actual bug being fixed (visible in UnitsNetGenerator.GenerateResourceFiles for .restext files and in FileInfoExtensions.EditFile). File.WriteAllText(path, contents) without an explicit encoding is already BOM-free by default in modern .NET, so those call-site swaps are more about explicitness/consistency than fixing an actual BOM bug — worth calling out in the PR description in case anyone wonders why so many call sites changed for what sounds like one bug.
  • Minor nit: Utf8NoBom is declared internal but is only referenced from within CodeGenFile itself — could be private.
  • FileInfoExtensions.EditFile (touched by this PR) doesn't appear to be called anywhere in the repo currently — pre-existing dead code, not introduced by this PR, just flagging in case it's worth pruning separately.

Correctness

  • ReadAllText/OpenText pass Utf8NoBom as the encoding but still get BOM detection (detectEncodingFromByteOrderMarks: true, implicit for File.ReadAllText(path, encoding)), so reading a file that still has a BOM (e.g., not yet regenerated) will correctly detect and strip it rather than misinterpreting the BOM bytes as content. Good attention to detail — this matches the PR description's claim about detecting existing BOMs on read while normalizing to no-BOM on write.

Test coverage

  • No dedicated tests, but that matches the existing pattern for the CodeGen project (no test project exists for it today), so this isn't a regression. Given the class is small and isolated, a couple of quick round-trip unit tests (write via CodeGenFile.WriteAllText, assert no BOM byte / assert ReadAllText round-trips a BOM'd file) could be a nice, low-cost addition if a CodeGen test project ever gets introduced — not blocking here.

Other

  • PR description states codegen was re-run and produced no diffs, and that generated outputs were scanned for BOM — good validation story for a change like this.

Overall this looks like a clean, well-motivated, low-risk internal tooling improvement.

@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98%. Comparing base (e6ccd72) to head (537a299).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1720   +/-   ##
======================================
  Coverage      98%     98%           
======================================
  Files         514     514           
  Lines       24141   24141           
======================================
  Hits        23748   23748           
  Misses        393     393           
Flag Coverage Δ
net48 97% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@angularsen
angularsen marked this pull request as ready for review August 1, 2026 22:07
@angularsen
angularsen merged commit 29b0424 into master Aug 1, 2026
3 checks passed
@angularsen
angularsen deleted the agl-codex/codegen-explicit-utf8 branch August 1, 2026 22:10
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review: Write codegen outputs as explicit UTF-8

Scope check: this only touches CodeGen/ (build-time tool), no changes under */GeneratedCode/, Common/UnitDefinitions/, or the shipped UnitsNet* packages.

Breaking changes

None. CodeGen isn't shipped in any NuGet package, so this can't affect library consumers.

New quantities or units

N/A — not touched by this PR.

Generated code

No generated files changed, and that's expected rather than a gap: CodeGen.csproj targets net10.0, and on modern .NET, File.WriteAllText, File.CreateText, and the no-encoding StreamWriter(path)/StreamWriter(path, append) constructors already default to UTF‑8 without a BOM (confirmed via MS Learn remarks on StreamWriter(String, Boolean), and empirically — UnitsNet/GeneratedCode/Quantities/Length.g.cs starts with 2f 2f 2d i.e. //-, no EF BB BF). So for quantities like Length (ILinearQuantity), Temperature (IAffineQuantity), or Level (ILogarithmicQuantity), output is byte-for-byte identical before/after — matches the PR's own "no generated-file diffs" note. Worth softening the PR description slightly: this isn't fixing an active BOM leak today, it's making the encoding intent explicit/defensive against a future call site accidentally using a BOM-emitting encoding (e.g. new StreamWriter(path, false, Encoding.UTF8) or new UTF8Encoding(true)). Still a reasonable change for clarity/consistency.

Potential bug: encoding switched from "throw on invalid bytes" to "silently replace"

CodeGenFile.Utf8NoBom is built with:

new UTF8Encoding(encoderShouldEmitUTF8Identifier: false)

Per the UTF8Encoding(bool) docs, this constructor does not throw on invalid byte sequences (replaces them instead). That's a behavior change from the encoding that File.ReadAllText(path) / File.WriteAllText(path, text) use internally without an explicit encoding, which (per the StreamWriter(String, Boolean) remarks) does throw on invalid bytes.

Practical effect: QuantityJsonFilesParser.ParseQuantity, QuantityRelationsParser.ParseRelations, and UnitEnumValueAllocator.ReadFromFile now read Common/UnitDefinitions/*.json and the unit-enum-allocation JSON via CodeGenFile.ReadAllText. If one of those files ever had a mis-encoded/mojibake byte sequence, codegen previously would have failed loudly; now it'll silently substitute U+FFFD and could generate subtly corrupted code/text without any error. Low likelihood, but easy to fix — use the 2-arg ctor to keep strictness while still controlling the BOM:

internal static readonly Encoding Utf8NoBom = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true);

Style

Using-directive ordering nit in CodeGen/Helpers/UnitEnumValueAllocation/UnitEnumValueAllocator.csusing CodeGen.Helpers; is inserted after using CodeGen.JsonTypes;, breaking alphabetical order (it should come before CodeGen.JsonTypes). The other four files in this diff all keep correct alphabetical ordering, so this one looks like an oversight.

Code quality

CodeGenFile is a clean, small, well-documented (if terse) wrapper, and all call sites in CodeGen/ were converted consistently — I didn't find any stray File.ReadAllText/WriteAllText/CreateText/new StreamWriter left in CodeGen/. FileInfoExtensions.EditFile also picks up the fix (previously used new StreamWriter(tempFilename), which — while not a BOM risk on net10.0 — is nice to have consistent); note that method appears unused in the current codebase, but that's pre-existing and out of scope here.

Test coverage

No CodeGen test project exists, so the lack of tests for CodeGenFile matches existing project conventions — not a blocker.

Performance / security

No concerns — Utf8NoBom is a single static readonly instance, and there's no security-relevant surface here beyond the encoding-strictness point above.

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