fix: annotate WmlToHtmlConverter.Charts.cs for nullable reference types - #665
Merged
Conversation
First of two PRs for #648 (per the issue's own staged plan: Charts.cs is self-contained and proves the approach before the much larger WmlToHtmlConverter.cs). Removes the #nullable disable header from WmlToHtmlConverter.Charts.cs and resolves the resulting 56 warnings to zero. ProcessChart's return type widens to XElement? and ReadCachedChartSeries's to CachedChartSeries? -- both already returned null on unsupported/malformed charts; ProcessChart's sole caller (ProcessImage in WmlToHtmlConverter.cs) already null-checks the result. The series LINQ pipeline switches from a `Where(x => x != null && ...)` null filter to `OfType<CachedChartSeries>()`, which narrows the element type as well as filtering, so no `!` is needed on any downstream use of the series list. The small internal helpers (ReadChartTitle, ReadCachedText, ReadChartColor, ReadIntAttribute, ReadDoubleAttribute, ReadChartFontSize, ReadCachedPoints) all already null-check their XElement parameter internally via `?.`; their signatures now say so (XElement?) instead of claiming non-null and relying on undocumented caller discipline. Two `!` assertions preserve real invariants that aren't statically provable from within this file alone (each with an inline comment): ReadChartColor's final `rgb` is proven non-null by the IsHexColor branches above it, and ReadCachedPoints' double.Parse call only ever sees Text values that already passed the same TryParse check one LINQ stage earlier. Neither converts a would-be crash into a silent skip. No new tests: pure nullable-annotation pass, no behavior change. Validated by dotnet build (0 warnings from this file, 116 total unchanged), the chart-specific HCO087-098 suite (18/18) and the HC001[HC043-Chart.docx] fixture, the full Docxodus.Tests suite (3923 passed, 3 skipped, 0 failed), npm run build + npx tsc --noEmit, and scripts/build-wasm.sh. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx
Reflects Docxodus/WmlToHtmlConverter.Charts.cs no longer carrying #nullable disable: 2 legacy files remain (down from 3), and stripping both now measures at 1,077 warnings (963 distinct CS86xx sites), down from 1,131 (1,016). Also updates the library/test warning baselines to what a fresh --no-incremental build measures on this branch (115/691). That's one lower than the 116/692 the prior PR recorded on main; the cause wasn't tracked down, but the measurement here was taken twice and is reproducible. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx
JSv4
force-pushed
the
fix/648-nullable-htmlconverter-charts
branch
from
September 2, 2026 11:48
5a811a3 to
61ef831
Compare
JSv4
added a commit
that referenced
this pull request
Sep 2, 2026
Removes the #nullable disable header from WmlToHtmlConverter.cs (10,482 lines) and fixes every warning that surfaces once the file is nullable-checked, following the same pass already completed for its .Charts.cs half in #665. 523 baseline warnings -> 0. Public surface changes: - ProcessImage's imageHandler parameter is now Func<ImageInfo, XElement?>?, and settings is WmlToHtmlConverterSettings? settings = null. - WmlToHtmlConverterSettings.ImageHandler / HtmlConverterSettings .ImageHandler (legacy settings type) widen the delegate's own return type to Func<ImageInfo, XElement?>?, not just the field -- required because tools/docx2html/Program.cs's ImageHandler lambda already returns null on unsupported formats and decode failures, which only became visible once this file left #nullable disable. - CommentInfo.Author / .Date / .Initials are now string?. Two narrow, deliberate behavior changes, both confirmed against the 3,923-test suite with no change in outcome: - isRtl = rPr?.Element(W.rtl) != null (was rPr.Element(...)) -- a run with no w:rPr now resolves to non-RTL instead of throwing. - The three SectionAnnotation.SectionElement reads and the two pre-existing ones now share one invariant instead of two different ones: every SectionAnnotation is constructed with a resolved, non-null SectionElement (see InitializeSectionAnnotation), so all five reads use sectAnnotation?.SectionElement! rather than mixing ! at two sites and ?. at three. Everything else preserves today's exact behavior, including several Dictionary.ContainsKey/indexer calls on a nullable key left un-guarded (via !) because that's already how the code depends on the BCL's null-key ArgumentNullException. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx
JSv4
added a commit
that referenced
this pull request
Sep 2, 2026
* fix: annotate WmlToHtmlConverter.cs for nullable reference types Removes the #nullable disable header from WmlToHtmlConverter.cs (10,482 lines) and fixes every warning that surfaces once the file is nullable-checked, following the same pass already completed for its .Charts.cs half in #665. 523 baseline warnings -> 0. Public surface changes: - ProcessImage's imageHandler parameter is now Func<ImageInfo, XElement?>?, and settings is WmlToHtmlConverterSettings? settings = null. - WmlToHtmlConverterSettings.ImageHandler / HtmlConverterSettings .ImageHandler (legacy settings type) widen the delegate's own return type to Func<ImageInfo, XElement?>?, not just the field -- required because tools/docx2html/Program.cs's ImageHandler lambda already returns null on unsupported formats and decode failures, which only became visible once this file left #nullable disable. - CommentInfo.Author / .Date / .Initials are now string?. Two narrow, deliberate behavior changes, both confirmed against the 3,923-test suite with no change in outcome: - isRtl = rPr?.Element(W.rtl) != null (was rPr.Element(...)) -- a run with no w:rPr now resolves to non-RTL instead of throwing. - The three SectionAnnotation.SectionElement reads and the two pre-existing ones now share one invariant instead of two different ones: every SectionAnnotation is constructed with a resolved, non-null SectionElement (see InitializeSectionAnnotation), so all five reads use sectAnnotation?.SectionElement! rather than mixing ! at two sites and ?. at three. Everything else preserves today's exact behavior, including several Dictionary.ContainsKey/indexer calls on a nullable key left un-guarded (via !) because that's already how the code depends on the BCL's null-key ArgumentNullException. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx * docs: correct warning baselines and legacy-file count after #648 WmlToHtmlConverter.cs is annotated now, leaving FormattingAssembler.cs as the sole remaining #nullable disable header (issue #649). Also records the StyleCop SA1636/SA1633 interaction discovered while re-measuring: removing one of these legacy headers promotes the inherited Microsoft copyright comment to first-in-file, which matches stylecop.json's copyrightText exactly and drops a warning from each baseline independent of the CS86xx cleanup itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
JSv4
pushed a commit
that referenced
this pull request
Sep 2, 2026
#663-#666 continue the #650 nullable run. Two of them land on this demo's measured path: #665 and #666 annotate WmlToHtmlConverter.cs and .Charts.cs, which are the conversion stage of the full+HTML depth. They are not cosmetic either — removing the #nullable disable header added 27 null-handling lines to the converter, which is real runtime code, so this got a rebuild and the full browser run rather than a wave-through. 14/14 pass. Measured, and the numbers are NOT republished, deliberately. A single controlled session read 12-25% faster across most rows (revisions 50->38, redline 67->53, full 127->106, conversion stage in isolation 60->53) — but oneCall, which IS the redline depth's call, came back flat at 51->50. Redline cannot drop 21% while docxDiffCompareProducts holds; the pattern contradicts itself. Added null-guards should also cost time rather than save it, so the direction is wrong for the change too. That is variance, on one session, and the published figures came from a pooled two-session controlled measurement. The README asks for pooling before believing a movement and for labelling one that fits nothing as the machine; publishing this would break both rules on my own page. Also merges #663 (HtmlToWmlConverter/Core) and #664 (DocumentBuilder), neither of which this demo calls.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First of two PRs for #648 (WmlToHtmlConverter.cs + .Charts.cs, 604 sites total). The issue itself asks for this split: "
WmlToHtmlConverter.Charts.csfirst as a separate PR — 55 sites, self-contained, and it proves the approach on the smaller half." This PR does exactly that; the much largerWmlToHtmlConverter.cs(549 sites) is the follow-up. #648 stays open until that lands.Removes the
#nullable disableheader fromWmlToHtmlConverter.Charts.cs(the DOCX chart → inline SVG renderer) and resolves the resulting 56 warnings to zero. No public API is touched — every changed member isprivate static.Notable decisions
ProcessChart's return type widens toXElement?andReadCachedChartSeries's toCachedChartSeries?— both already returnednullon unsupported or malformed chart data.ProcessChart's only caller (ProcessImageinWmlToHtmlConverter.cs) already null-checks the result, so this is a pure signature-matches-reality change..Where(x => x != null && ...)to.OfType<CachedChartSeries>().Where(...)—OfTypeboth filters nulls and narrows the element type, so nothing downstream needs a!.ReadChartTitle,ReadCachedText,ReadChartColor,ReadIntAttribute,ReadDoubleAttribute,ReadChartFontSize,ReadCachedPoints) already null-checked theirXElementparameter internally via?.— their signatures now say so (XElement?) instead of claiming non-null and relying on undocumented caller discipline.!assertions preserve real invariants that aren't statically provable from within this file alone (each has an inline comment):ReadChartColor's finalrgbis proven non-null by theIsHexColorchecks immediately above it, andReadCachedPoints'double.Parsecall only ever seesTextvalues that already passed the sameTryParsecheck one LINQ stage earlier (IsHexColoritself lives inWmlToHtmlConverter.cs, out of scope for this PR, so it can't gain a[NotNullWhen]attribute here).Also updates
CLAUDE.md's "legacy files" bookkeeping (2 remain, down from 3, now that this file no longer carries the header) and the projected warning count if both remaining files were stripped. While re-measuring I also found a fresh--no-incrementalbuild on this branch reports 115/691 library/test warnings, one lower on each than the 116/692 the prior PR (#664) recorded on main — the discrepancy is reproducible (measured twice) but I didn't track down its cause, soCLAUDE.mdnow states the measured values without claiming why they moved.Validation
dotnet build Docxodus/Docxodus.csproj --no-incremental— 0 warnings from this file, 115 total (matches the updated baseline).dotnet test --filter "FullyQualifiedName~HCO08 or FullyQualifiedName~HCO09"— 18/18 passed (the chart-specific test suite: clustered/stacked/percent-stacked/doughnut/pie/line charts, generated and fixture-based).dotnet test --filter "FullyQualifiedName~HcTests"— 139/139 passed (includes theHC043-Chart.docxfixture through the general HTML-conversion suite).Docxodus.Testssuite — 3923 passed, 3 skipped, 0 failed.git status --short TestFiles/clean — no fixture pollution../scripts/build-wasm.sh— succeeded, within size budget.npm run build+npx tsc --noEmit— clean.npm test(full Playwright suite, 671 tests across chromium) — 666 passed, 5 pre-existing failures (thetabs-visual.spec.tsscreenshot tests, which fail on this host because it lacks Times New Roman and are green in CI — unrelated to this change), 11 skipped.No new tests: this is a pure nullable-annotation pass with no behavior change, so the existing suite is the correct gate.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VSnwkK1Nx6zZb2RnnoxKdx