Skip to content

fix(930): UiThread dispatcher-exit null guard, ILGlobals dead public statics, stale doc-comment line counts - #935

Merged
drmoisan merged 16 commits into
mainfrom
bug/csharp-latent-hazards-uithread-ilglobals-comments-930
Sep 29, 2026
Merged

drmoisan merged 16 commits into
mainfrom
bug/csharp-latent-hazards-uithread-ilglobals-comments-930

Conversation

@drmoisan

Copy link
Copy Markdown
Owner

Suggested title

fix(930): UiThread dispatcher-exit null guard, ILGlobals dead public statics, stale doc-comment line counts

Summary

  • Adds the missing _dispatcher is not null guard to the dispatcher exit of UiThread.SynchronizationContextAwaiter.IsCompleted, so a null captured dispatcher can no longer match a thread that owns no dispatcher (null equals null). Regression test recorded failing before the change and passing after it.
  • Breaking public API change: removes the public mutable static fields ILGlobals.Cache and ILGlobals.modules from UtilitiesCS (SDIL Reader). Neither had a consumer in the repository; see Backward Compatibility.
  • Replaces the Cache_IsInitialized test with two structural tests that pin the rule that ILGlobals exposes no public mutable static.
  • Removes the stale numeric line counts from the doc comments of two QuickFiler partial-class parts, so the comments cannot drift again.
  • Full C# toolchain passed in one clean pass; first-party coverage 85.31% to 85.32% lines and 79.71% to 79.73% branches; all 7 acceptance criteria met.

Why

Issue #930 consolidates three independent low-risk latent defects in production C#, each verified present on main on 2026-09-28:

  1. Bug: uithread-dispatcher-exit-null-dispatcher-referenceequals #889: the dispatcher exit of IsCompleted compared Dispatcher.FromThread(Thread.CurrentThread) with the captured _dispatcher without a null test. The captured-context exit directly above it already carries this guard (added in PR fix(threading): harden the captured-UI-context exit of IsCompleted and settle AC5 #890).
  2. Bug: ilglobals-latent-unsafe-public-static-members #863: ILGlobals still declared two public writable static fields after the opcode-table publication was hardened (Bug: ilglobals-loadopcodes-unsynchronised-static-race #824). Cache was referenced only by one test assertion; modules had no reference.
  3. Bug: breadcrumb-partial-class-line-count-comments-stale #862: two doc comments stated line counts (487 and 481) for their primary partial-class files that no longer matched the files.

What Changed

Production code

  • UtilitiesCS/Threading/UiThread.cs: one operand && _dispatcher is not null added to the dispatcher-exit return, plus one comment line (2 lines added, 0 removed).
  • UtilitiesCS/NewtonsoftHelpers/SDIL Reader/ILGlobals.cs: public static Dictionary<int, object> Cache and public static Module[]? modules deleted (3 lines removed).
  • QuickFiler/Viewers/BreadcrumbBridgeCoordinator.Search.cs and QuickFiler/Viewers/BreadcrumbItemViewerLifecycleCoordinator.Search.cs: the parenthesised line-count token removed from one comment line each; the explanation of the 500-line ceiling is retained.

Tests

  • UtilitiesCS.Test/Threading/UiThreadApartmentMeasurement_Tests.cs: new test IsCompleted_WhenTheAwaiterContextIsAForeignDispatcherContextAndNoUiDispatcherWasCaptured_ReturnsFalse in the existing UiThreadPredicateHardening_Tests class (which already carries [DoNotParallelize]; no attribute added).
  • UtilitiesCS.Test/NewtonsoftHelpers/SDILReader/ILGlobals_Tests.cs: Cache_IsInitialized replaced by PublicStaticFields_AreAllInitOnly and PublicStaticFields_AreExactlyTheTwoOpCodeTables.

Docs and evidence

  • Feature folder docs/features/active/2026-09-28-csharp-latent-hazards-uithread-ilglobals-comments-930/: plan, baseline, regression and QA evidence, reduced-audit artifacts, and one documentation-only remediation cycle (see Verification).

Architecture / How It Fits Together

IsCompleted has independent proof exits that decide whether an awaiter may continue synchronously on the UI thread. The change makes the dispatcher exit apply the same null rule as the captured-context exit, so both exits require a captured dispatcher before comparing it with the executing thread's dispatcher. ILGlobals now publishes only its two readonly opcode tables. The QuickFiler changes are comment-only.

Verification

Completed (from the evidence under the feature folder):

  • Regression, Bug: uithread-dispatcher-exit-null-dispatcher-referenceequals #889: the new test failed against unmodified source (Total 3, executed 3, passed 2, failed 1) and passed after the guard; all twelve pre-existing IsCompleted tests still pass.
  • Regression, Bug: ilglobals-latent-unsafe-public-static-members #863: the two structural tests failed against unmodified source (passed 13, failed 2) and passed after the deletion (passed 15, failed 0). The solution rebuilt with 0 Error(s) after the deletion, and a repository-wide name, string and reflection search found no remaining reference in any .cs file.
  • Final toolchain, one iteration, LOOP: CLEAN PASS:
    • dotnet tool run csharpier format . rewrote nothing in the owned files; dotnet tool run csharpier check . exit 0.
    • Analyzer Rebuild (/t:Rebuild ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true): exit 0, 0 Error(s), 0 Warning(s).
    • Nullable Rebuild (/t:Rebuild ... /p:TreatWarningsAsErrors=true): exit 0, 0 Error(s), no CS86xx errors.
    • MSTest with coverage (repository runner): 7322 total, 7322 passed, 0 failed; First-party coverage: lines 56084/65736 (85.32%), branches 13597/17054 (79.73%) against a baseline of 85.31% and 79.71%.
    • The changed executable line is covered and the return statement reports 4/4 condition coverage; per-file uncovered-line counts did not rise (UiThread.cs 3 to 3, ILGlobals.cs 2 to 2).
  • Concurrency regime unchanged: no new DoNotParallelize, no worker-count change, no retry, no sleep, runsettings hash unchanged.
  • Reduced audit: first pass Blocking 0, Non-blocking 1 (an absolute Visual Studio install path transcribed into two evidence files, which reopened AC7). A documentation-only remediation cycle replaced it with a placeholder and added a general drive-rooted path scan (7 hits before, 0 after, with positive controls). Exit reaudit: Blocking 0, Non-blocking 0, Informational 9. Acceptance criteria 7 of 7.

Local run conditions:

  • Four shell-icon test classes (ShellUtilities_Tests, ShellUtilitiesStatic_Tests, SysImageListHelperTests, OSBrowser_Tests) hang on the development workstation and were excluded identically from the baseline and final local coverage runs. The mstest-coverage CI workflow runs them unfiltered.
  • The known flaky test TryAddValuesAsync_UpdatesExistingValue did not fail; the one permitted re-measurement was not used.

Recommended:

  • Confirm the required CI checks are green on the PR head, including the mstest-coverage workflow for the four shell-icon classes.

Backward Compatibility / Migration Notes

  • Breaking change: SDILReader.ILGlobals.Cache (public static Dictionary<int, object>) and SDILReader.ILGlobals.modules (public static Module[]?) are removed from the public surface of UtilitiesCS. No in-repository consumer exists: the full solution rebuilt with zero errors and the reference search found no use by name, string literal or reflection. An external consumer, if any, would fail to compile and should not have been writing these process-wide fields; there is no replacement member.
  • No other public signature changes. No project, package or configuration file changes.

Risks and Mitigations

  • Behavioral change in IsCompleted: when no UI dispatcher was captured, the dispatcher exit now returns false instead of true on a thread without a dispatcher. This only affects the case where the previous result was the defect; the continuation is then posted to the context rather than run inline. Covered by the new regression test and the unchanged passing IsCompleted suite. Rollback: revert the one-operand change.
  • Removal of ILGlobals fields: mitigated by the solution-wide rebuild and reference search above.

Review Guide

  1. UtilitiesCS/Threading/UiThread.cs (2 lines).
  2. UtilitiesCS/NewtonsoftHelpers/SDIL Reader/ILGlobals.cs (3 deletions) and ILGlobals_Tests.cs.
  3. UiThreadApartmentMeasurement_Tests.cs (one new test).
  4. The two QuickFiler comment edits.
  5. Evidence and audits under the feature folder; the most useful are evidence/qa-gates/toolchain-final-pass.md, evidence/qa-gates/coverage-comparison.md, evidence/regression-testing/889-fail-before.md, evidence/regression-testing/863-reference-search.md, and the *.2026-09-29T10-16.md reaudit artifacts.

The branch also contains a merge of main (no conflicts; no overlap with the six code paths).

Follow-ups

  • The analyzer references in UtilitiesCS.csproj and VBFunctions.csproj name Meziantou.Analyzer 3.0.235 and SVGControl.Test.csproj names MSTest.Analyzers 4.4.0, while package restore installs 3.0.290 and 4.4.1; a fresh worktree's analyzer Rebuild fails with CS0006 until the named versions are installed. Pre-existing on main; not changed here.
  • using System.Collections.Generic; in ILGlobals.cs is now unused (no analyzer diagnostic).
  • The coverage runner exposes no test-filter extension point and no hang timeout, which required the local override described above.

GitHub Auto-close

🤖 Generated with Claude Code

https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm

…iteria for issue 930

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KoweznWqwJTkNCf6756FoF
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com
…an, round 4

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
…, stale doc-comment line counts Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
…7 reopened

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
…INSTALL-ROOT placeholder and check off AC7 Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
…omments-930

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

Claude-Session: https://claude.ai/code/session_01KNZiXntshsLY8vqqCHUvHm
@drmoisan
drmoisan merged commit dcce3c8 into main Sep 29, 2026
6 checks passed
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