Skip to content

fix: annotate FormattingAssembler.cs for nullable reference types - #675

Merged
JSv4 merged 2 commits into
mainfrom
fix/649-nullable-formattingassembler
Sep 2, 2026
Merged

fix: annotate FormattingAssembler.cs for nullable reference types#675
JSv4 merged 2 commits into
mainfrom
fix/649-nullable-formattingassembler

Conversation

@JSv4

@JSv4 JSv4 commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #649. Removes the last #nullable disable header remaining anywhere under Docxodus/FormattingAssembler.cs, 4,232 lines, 434 baseline warnings — fixing all of them down to 0. This is the fifth and final sub-issue of the #645 campaign (#646-#650) that annotated all 21 legacy OpenXmlPowerTools files inherited when this project forked it.

What changed

The recurring pattern in this file is a family of style-merge helpers (MergeStyleElement, LangMerge, IndMerge, TabsMerge, SpacingMerge, FontMerge, ResolveInsideBorder, ToggleMergeRunProps) whose contract is "return null only if both inputs are null." Rather than hand-annotating each of the dozens of call sites, these are marked with [return: NotNullIfNotNull(nameof(param))] for each parameter — the compiler then infers a non-null result at any call site where one side is statically known non-null. This eliminated most of the file's warnings without touching a single call site.

CharStyleStack widens to Stack<XElement?>/IEnumerable<XElement?>. It intentionally pushes null for a style level that has a basedOn chain but no own w:rPr, and its sole consumer already treats a null entry as "no contribution at this level" via Aggregate. This was a real design, not a bug wearing a non-nullable type — the type now says so honestly.

Two behavior fixes fell out of the pass, not sought deliberately:

  • AdjustFontAttributes now null-checks rPr before reading w:rFonts. One of its three call sites passes a paragraph's rolled-up run properties, which can genuinely lack an rPr element — that path used to throw a NullReferenceException.
  • The list-item marker synthesis in NormalizeListItemsTransform builds a pt:AbstractNumId attribute from a genuinely-nullable int? (ListItemInfo.AbstractNumId, distinct from a same-named non-nullable field on an unrelated nested class). new XAttribute(name, value) throws on a null value, unlike XElement content constructors, which tolerate it — this path is now a graceful omission instead of a crash.

Public surface: ParagraphStyleRollup's defaultParagraphStyleName parameter is now string?. This documents an existing contract rather than changing one — ListItemRetriever.cs was already passing a string? into it (GetDefaultParagraphStyleName returns string?), so the parameter was already silently tolerant of null; the type now says so. MetricsGetter.GetFontFromFontType (private) widens its return to string? to match CharStyleAttributes' now-nullable font fields; its sole caller only counts results via .Count(), never dereferences them.

Two small non-annotation cleanups, made in passing while already touching these exact lines: an unused sXDoc local in CharStyleRollup, and a dead third null-check in FontMerge left over after two simpler exhaustive checks — it was also interfering with the compiler's null-narrowing on the code below it.

A regression caught during self-review

AssembleFormatting's second content-part loop cached pxd.Root into a local before calling NormalizePropsForPart(pxd, settings) — but that method itself can replace pxd.Root outright (via ReplaceWith, when settings.OrderElementsPerStandard is set), leaving the caller's cached reference stale/detached. Calling .ReplaceWith() on it afterward threw InvalidOperationException: The parent is missing.

This slipped past the build (0 warnings, 0 errors both times) because nothing about nullable-reference analysis checks aliasing across a method call. The full test suite caught it: 33 failures, all at the same line. Fixed by re-fetching pxd.Root fresh after the call rather than reusing the pre-call reference, with a comment explaining why. Re-ran the full suite after the fix to confirm.

Validation

  • dotnet build Docxodus/Docxodus.csproj --no-incremental: 0 errors, 113 warnings (down from 114 — see the CLAUDE.md update; one file's #nullable disable header removal drops one SA1636 StyleCop warning as a side effect unrelated to nullability).
  • dotnet build Docxodus.sln --no-incremental (Debug and -c Release): 0 errors.
  • dotnet test Docxodus.Tests/Docxodus.Tests.csproj: 3923 passed, 3 skipped, 0 failed.
  • ./scripts/build-wasm.sh: succeeds, wire size 4875 KB (budget 5120 KB).
  • The six out-of-solution tool/benchmark projects: all build clean; benchmarks/complex-form-doc fails the same pre-existing, unrelated way as before (issue benchmarks/complex-form-doc fails to build (CS0161: not all code paths return a value) #662).
  • npm run build, npm run typecheck: clean.
  • npm test (Playwright, full suite): 665 passed, 11 skipped, 6 failed — 5 are the known host-only tabs-visual.spec.ts screenshot tests (missing Times New Roman locally; green in CI), and the 6th (demo-arcade-mobile.spec.ts, an unrelated mobile-viewport arcade test) passed cleanly on its own re-run in isolation, confirming it's a resource-contention flake under full-suite load rather than a regression.

Docs

CLAUDE.md's nullable-reference-types section now reflects that no file under Docxodus/ carries #nullable disable anymore, and the library/test warning baselines move from 114/690 to 113/689. CHANGELOG.md gets entries for the two behavior fixes above.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx

JSv4 and others added 2 commits September 2, 2026 09:55
Removes the last `#nullable disable` header in Docxodus/ (issue #649),
fixing all 434 baseline warnings down to 0. This closes out the
five-issue #645 campaign that annotated all 21 legacy OpenXmlPowerTools
files (#646-#650).

The dominant pattern in this file is a family of style-merge helpers
(MergeStyleElement, LangMerge, IndMerge, TabsMerge, SpacingMerge,
FontMerge, ResolveInsideBorder, ToggleMergeRunProps) whose contract is
"return null only if both inputs are null." Rather than annotating each
of the dozens of call sites individually, these methods are marked
[return: NotNullIfNotNull] for each parameter, letting the compiler
infer a non-null result wherever a caller already knows one side is
non-null.

CharStyleStack widens to Stack<XElement?>/IEnumerable<XElement?>: it
intentionally pushes null for a style level that has a basedOn chain
but no own w:rPr, and its sole consumer already treats that as "no
contribution at this level" via Aggregate. This was a real design, not
a latent bug, so the type now says so.

Two behavior fixes fell out of the pass rather than being sought:
- AdjustFontAttributes now null-checks rPr before reading w:rFonts.
  One of its three call sites passes a paragraph's rolled-up run
  properties, which can genuinely lack an rPr element; that path used
  to NRE.
- FormattingAssembler.ParagraphStyleRollup and the new XAttribute for
  PtOpenXml.AbstractNumId in the list-item numbering-change branch
  both had latent null-crash risk that the widened types make
  reachable-but-handled instead of theoretical.

Public surface: ParagraphStyleRollup's defaultParagraphStyleName
parameter is now string? — ListItemRetriever.cs was already passing a
nullable value into it (GetDefaultParagraphStyleName returns string?),
so this documents an existing contract rather than changing one.

MetricsGetter.GetFontFromFontType (private) widens its return to
string? to match CharStyleAttributes' now-nullable font fields; its
sole caller only counts results via .Count(), never dereferences them.

Also fixed in FormattingAssembler.cs, unrelated to nullability: an
unused `sXDoc` local in CharStyleRollup, and a dead third null-check in
FontMerge that was left over after two simpler exhaustive checks and
was interfering with the compiler's null-narrowing on the lines below
it.

Caught during self-review: AssembleFormatting's second content-part
loop cached pxd.Root into a local before calling
NormalizePropsForPart(pxd, settings) — but that method itself replaces
pxd.Root (via ReplaceWith) when OrderElementsPerStandard is set,
leaving the caller's cached reference stale. Fixed by re-fetching
pxd.Root fresh after the call. Found via the full test suite (33
failures, all "The parent is missing" from XNode.ReplaceWith), not by
the build — build and warning counts stayed clean throughout, since
neither checks aliasing across calls.

Validated: dotnet build (library, solution, solution -c Release) at 0
errors; dotnet test at 3923 passed / 3 skipped / 0 failed; WASM build
within its wire-size budget; the six out-of-solution tool/benchmark
projects build (complex-form-doc fails the same pre-existing,
unrelated way as before, per issue #662); npm build, typecheck, and
Playwright all pass.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx
FormattingAssembler.cs was the last of the 21 legacy files carrying
#nullable disable (issue #645's five sub-issues, #646-#650, are now
all done). Updates CLAUDE.md to say so, drops the SA1636 explanation
that was specific to stripping headers (there are none left to strip),
and refreshes the library/test warning baselines (114/690 -> 113/689,
the same one-less-per-baseline SA1636 mechanism documented for the
prior file). CS8632 stays in NoWarn for now; issue #651 tracks
retiring it.

Also adds the two CHANGELOG entries for the latent bugs the
FormattingAssembler.cs pass surfaced and fixed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx
@JSv4
JSv4 merged commit 09967ad into main Sep 2, 2026
14 checks passed
@JSv4
JSv4 deleted the fix/649-nullable-formattingassembler branch September 2, 2026 15:22
JSv4 pushed a commit that referenced this pull request Sep 2, 2026
main picked up the FormattingAssembler nullable-annotation fixes (#675), which
collided with this branch only in CHANGELOG.md: both sides appended to the same
`### Fixed` list under `[Unreleased]`. Kept every entry, main's bug fixes first
and this branch's demo-pin entry after them.

npm run test:demo-logic passes on the merge result.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PzrZihTdHGDcP3jLeBShBG
JSv4 pushed a commit that referenced this pull request Sep 2, 2026
FormattingAssembler.cs is the last of the legacy #nullable disable files
and the converter resolves formatting through it, so this is on the
full+HTML path. 13 real null-guards added, not cosmetic, so it got a
rebuild and the full browser run: 14/14 pass.

No measurement this time, and that is a change of approach rather than
an omission. The #650 nullable run has now touched the converter twice
(#665/#666, #675), and measuring after each one produces a single
session apiece — which is exactly the sample size that gave an
incoherent reading last merge (redline down 21% while its own underlying
call held flat). Single sessions through a long mechanical run add noise,
not signal.

The published figures are pooled and stable and nothing here is expected
to move them: null-guards cost time rather than save it, and the effect
of thirteen of them is far below what this container can resolve. When
the run finishes, the figures are due one deliberate pooled
re-establishment rather than eleven piecemeal nudges.
JSv4 pushed a commit that referenced this pull request Sep 2, 2026
The pooled re-establishment deferred through the #650 nullable run, now
that the run is over (#675 was the last annotated file, #676 retired the
NoWarn list, and main has moved on to docs). Three 40-frame stress runs
plus two controlled sessions, which is the sample size the earlier
single-session readings lacked — and this time the stress reps land
within 3 ms of each other and both methods agree.

They came out 21-28% faster than the published figures on every absolute:
revisions 52 -> 41 ms, redline 74 -> 55, full+HTML 141 -> 101. Nothing
merged since #653 can explain a quarter — the nullable run added
null-guards, which cost time, and #676 removed two provably redundant
checks.

What makes the reading worth keeping is the column that did NOT move. The
ratios against the recording path are 33 -> 34x, 45 -> 45x, 78 -> 80x.
The mutation path scaled by the same factor as the diff path, so the
ratio held while both halves got a quarter faster together. That is the
container being faster this hour, and it is the clean counterexample to
#653, which announced itself precisely BY moving the ratio (68-157x down
to 33-78x) because recording and computing changed by different amounts.

So the README gains a sharper diagnostic than "is the movement uniform
across depths": when the absolutes move and the ratio holds it is the
machine; when the ratio itself moves, the two paths changed by different
amounts and something real happened. That is a better test because it
needs no knowledge of what merged.

The table keeps its original pooled figures rather than adopting the
newer ones. Both are honest pooled measurements of the same build; taking
whichever is faster would be chasing weather, and the panel recomputes the
ratio live regardless.

14/14 browser assertions and 61/61 node checks pass on the current head.
Also merges #679 and #680, both docs-only; CHANGELOG conflicted the same
both-added way and was resolved keeping both, and the release-merge check
confirms this demo's entry is still inside [Unreleased].
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.

Nullable: annotate FormattingAssembler.cs (451 sites, dense, and it cascades into six consumers)

1 participant