Guard Any.Reproducibly's async surface against silent test failures - #318
Merged
Conversation
The asynchronous Any.Reproducibly overloads returned a Task under the same name as the synchronous ones. An async lambda is a better conversion to Func<Task> than to Action, so Any.Reproducibly(async () => ...) bound to the async overload and returned a Task; in a synchronous void test method that Task was discarded, the body's assertions ran on a continuation after the method had returned, and a failing test passed green. The compiler's CS4014 does not fire in a synchronous method, so nothing warned. Rename the two async overloads to Any.ReproduciblyAsync (TAP-conventional, returns an awaited Task); the synchronous Any.Reproducibly(Action) is unchanged. This is one half of the fix for the silent-green footgun; the analyzers that make the remaining mistakes un-compilable land next. Call sites, the public API baselines (both target frameworks), and the user guide (EN + FR) are updated. The xUnit [Reproducible] adapter is unaffected — it drives Any.UseSeed directly. BREAKING CHANGE: Any.Reproducibly(Func<Task>) and Any.Reproducibly(int, Func<Task>) are renamed to Any.ReproduciblyAsync. Replace Any.Reproducibly(async () => ...) with await Any.ReproduciblyAsync(async () => ...). Refs: #317
JustDummies now ships its own first-party Roslyn analyzers, in a new JustDummies.Analyzers project packaged inside the JustDummies NuGet package (analyzers/dotnet/cs), error-agnostic and independent of FirstClassErrors, under a JustDummies-owned diagnostic-id scheme (JDxxx). Two error-severity rules close the silent-green footgun that the type system cannot express (C# has no non-droppable Task, and no way to forbid an async-lambda-to-Action conversion): * JD001 — an async lambda passed to the synchronous Any.Reproducibly. Bound to an Action it becomes async void, whose failures escape the reproducible scope; the fix is Any.ReproduciblyAsync, awaited. * JD002 — a discarded Any.ReproduciblyAsync task (a bare statement or "_ ="), which silently drops the body's failures. Together with the rename in the previous commit, the common footgun is now a compile error rather than a green build over a failing test. The rejected alternatives — an [Obsolete] poison overload (a deprecated member in a fresh 1.0) and a blocking sync-over-async overload (deadlock risk) — and this decision are recorded in ADR-0043 (Proposed). Per-rule help pages (EN + FR) and the shared analyzer index are added; the analyzer packs once across the two target frameworks, and the JustDummies standalone-boundary test still passes (the packaging reference is build-only). Refs: #317
…producibly-async # Conflicts: # doc/handwritten/for-maintainers/adr/README.md
The JustDummies analyzers ship inside the JustDummies package, so they fall under the "mutate the analyzers too" convention that main introduced while this branch was open (ADR-0043). Add build/stryker/justdummies-analyzers.json and a matrix leg in justdummies-mutation.yml (gate and weekly sweep), so the new rules are mutated on the diff like every other shipped component. Strengthen the analyzer tests to kill the meaningful survivors: a same-named Reproducibly / ReproduciblyAsync on another type must not trip the rule (this pins the JustDummies.Any type check and JD002's short-circuit), and each rule asserts its message names ReproduciblyAsync. The residual survivors are the analyzer-infrastructure and descriptor-string class the FirstClassErrors analyzers carry too; the gate uses break: 0, so it reports rather than blocks. Refs: #317
Main merged its own ADR-0043 (gate pull requests on mutation score) while this branch was open, so this branch's ADR is renumbered to 0044. The files were renamed in the merge; this aligns the title and the language-banner links inside them. Refs: #317
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
2 participants
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
Any.Reproduciblyoverloaded a synchronousActionand an asynchronousFunc<Task>on one name. Anasynclambda binds to theFunc<Task>overload, which returned aTask; in a synchronousvoidtest method that task was discarded and a failing async test passed green (its assertions ran on a continuation after the method returned;CS4014does not fire in a sync method). The async overload is renamed to the TAP-conventionalReproduciblyAsync, and a new first-party analyzer makes the two mistakes the type system cannot express — passing an async body toReproducibly, and discarding aReproduciblyAsynctask — compile errors.Type of change
Changes
Any.Reproducibly(Func<Task>)andAny.Reproducibly(int, Func<Task>)→Any.ReproduciblyAsync. The synchronousAny.Reproducibly(Action)is unchanged. Call sites, both public-API baselines, and the user guide (EN + FR) are updated. The xUnit[Reproducible]adapter is unaffected — it drivesAny.UseSeeddirectly.JustDummies.Analyzersproject, packaged inside the JustDummies NuGet package atanalyzers/dotnet/cs(mirroring how FirstClassErrors packs its analyzer), error-agnostic and independent of FirstClassErrors, under a JustDummies-owned diagnostic-id scheme (JDxxx). Two error-severity rules:asynclambda passed to the synchronousAny.Reproducibly. Bound to anActionit becomesasync void, whose failures escape the reproducible scope. Fix:Any.ReproduciblyAsync, awaited.Any.ReproduciblyAsynctask (a bare statement, or_ =), which silently drops the body's failures.$(RoslynFloorVersion); rules are release-tracked. The packaging reference is build-only (ReferenceOutputAssembly=false), so the standalone-boundary architecture test still passes, and the analyzer packs once across the two target frameworks. Both new projects are wired into the solution'sNestedProjects.main(which advanced while this branch was open): the analyzer is added to the new JustDummies mutation gate — ajustdummies-analyzersstryker config + matrix leg (gate and weekly sweep), consistent with main's "mutate the analyzers too" policy — and the analyzer tests are strengthened to kill the meaningful mutants.Why not the alternatives (recorded in ADR-0044): an
[Obsolete(error)]"poison" overload was rejected — a member deprecated in a fresh 1.0 is a contradiction that clutters the shipped surface; a blockingvoid Reproducibly(Func<Task>)was rejected — sync-over-async risks deadlock under a capturedSynchronizationContext. The analyzer keeps the surface clean (two honest methods) and the failure mode loud (a compile error), following ADR-0035's grain: types where they can carry the rule, a check for what they cannot.Testing
dotnet build FirstClassErrors.sln— 0 warnings, 0 errors.dotnet test FirstClassErrors.sln— 1809 tests across 15 projects, 0 failures.JustDummies.Analyzers.UnitTests(10 tests) and the existingFirstClassErrors.Analyzers.UnitTests.Additionally:
Taskthe call site never observes), then removed.ReproduciblyAsync.dotnet stryker --config-file build/stryker/justdummies-analyzers.jsonscores 63%, above thelow: 60watermark; the config usesbreak: 0, so the gate reports survivors rather than blocking. The residual survivors are the analyzer-infrastructure calls,isEnabledByDefaultflags, and descriptor cosmetic strings that the FirstClassErrors analyzers carry too.JustDummiesandJustDummies.Analyzersboth build at 0 warnings (public-API baseline consistent; RS2008 release tracking satisfied).dotnet build JustDummies -f netstandard2.0 -p:EnableNet472Floor=true: 0 errors.dotnet pack JustDummiesproduces exactly oneanalyzers/dotnet/cs/JustDummies.Analyzers.dll.The net472 support floor's test run was not exercised locally (the
framework-floorjob needs Windows); the floor build was.Documentation
doc/updatedPer-rule help pages
JD001/JD002(EN + FR) are added, and the shared analyzer index is made product-neutral with a JustDummies section (the JustDummies rules ship in the JustDummies package).JustDummies/CHANGELOG.mdis untouched (its Unreleased section is drafted from merged PRs).Architecture decisions
Proposed: ADR-0044ADR-0044 (Proposed) records that JustDummies ships first-party analyzers (a new cross-cutting capability and a
JDxxxdiagnostic-id policy), and the reproducible async-safety contract (Reproducibly/ReproduciblyAsync+ JD001/JD002) chosen over the poison-overload and blocking alternatives. It follows ADR-0031/0035 and does not conflict with ADR-0038/0039. (Numbered 0044 rather than 0043:mainmerged its own ADR-0043 — gate PRs on mutation score — while this branch was open; the ADR index conflict was resolved to carry both.)Related issues
Closes #317
🤖 Generated with Claude Code