Skip to content

build: fix the red net48 leg with Polyfill instead of a hand-rolled shim - #85

Merged
dborgards merged 1 commit into
mainfrom
fix/polyfill-net48
Sep 10, 2026
Merged

dborgards merged 1 commit into
mainfrom
fix/polyfill-net48

Conversation

@dborgards

Copy link
Copy Markdown
Owner

main is red. windows-latest has failed since #83 merged:

DeferredEchoQueue.cs(46,43): error CS0305: Using the generic type
'TaskCompletionSource<TResult>' requires 1 type arguments

How it got there

#81 and #83 were both green and merged four minutes apart. #81 added DeferredEchoQueue, which uses the non-generic TaskCompletionSource. #83 added the net48 leg that now has to compile it. Neither was ever built against the other: strict_required_status_checks_policy is false on the main ruleset, so GitHub does not re-run a pull request's checks when main moves underneath it. Both green checkmarks were honest about a tree that no longer existed.

The non-generic TaskCompletionSource is .NET 5+. netstandard2.0 and net48 have only TaskCompletionSource<TResult>.

The fix

Rather than adding a second shim to the hand-rolled TaskWaitAsyncPolyfill, this switches to Polyfill 11.3.0, which supplies both the non-generic TaskCompletionSource and Task.WaitAsync. TaskWaitAsyncPolyfill.cs (99 lines) is deleted.

Why the package is safe for a library repository:

  • Source only — <developmentDependency>true</developmentDependency>, no lib/ folder. The code compiles in; the package never appears as a dependency of anything shipped. The reference is scoped to the test project with PrivateAssets=all regardless.
  • No using directives needed — the package ships global using global::Polyfills; in its own Polyfill.cs, so the extension-method polyfills resolve at every call site.
  • LangVersion 12 → 14, because Polyfill's sources use C# 14 extension members (extension(Byte) { … }). The language version is independent of the target frameworks, so the netstandard2.0 assets are unaffected.

Verified locally

check result
dotnet build -f net48 succeeded, 0 warnings, 0 errors
dotnet build CanKit.Pro.sln -c Release succeeded, 0 warnings
dotnet test -c Release 409 passed, 0 failed
*.approved.txt diff empty — the public API surface does not move
dotnet pack no PolyfillTargetsForNuget warning

Follow-ups, deliberately not here

  1. src/ still carries six byte-identical copies of IsExternalInit.cs, one per package, which Polyfill would also supply. That means a PackageReference in the shipping projects, so it belongs in its own pull request rather than in a fix for a red main.
  2. This incident is an argument for flipping strict_required_status_checks_policy to true on the main ruleset. It costs a branch update before merging; it would have caught this automatically.

🤖 Generated with Claude Code

`main` has been failing on windows-latest since #83 merged:

    DeferredEchoQueue.cs(46,43): error CS0305: Using the generic type
    'TaskCompletionSource<TResult>' requires 1 type arguments

#81 and #83 were both green and merged four minutes apart. #81 added
`DeferredEchoQueue`, which uses the non-generic `TaskCompletionSource`; #83
added the net48 leg that has to compile it. Neither pull request was ever
built against the other, because `strict_required_status_checks_policy` is
`false` on the `main` ruleset, so GitHub does not re-run a pull request's
checks when `main` moves underneath it.

The non-generic `TaskCompletionSource` is .NET 5+; netstandard2.0 and net48
have only `TaskCompletionSource<TResult>`.

Rather than growing the hand-rolled `TaskWaitAsyncPolyfill` a second shim,
this switches to the Polyfill package, which supplies both that type and
`Task.WaitAsync`, is maintained across 22 target frameworks, and is source
only (developmentDependency, no lib/) so it never becomes a dependency of
anything this repository ships. It also adds `global using global::Polyfills;`
itself, so no call site needs a using directive.

Polyfill's sources use C# 14 extension members, hence LangVersion 12 -> 14.
The language version is independent of the target frameworks, so the
netstandard2.0 assets are unchanged; the API approvals confirm it -- they do
not move.

Verified locally: net48 builds clean (0 warnings), the full solution builds
clean, 409/409 tests pass on net10.0, and `dotnet pack` produces no
PolyfillTargetsForNuget warning since the reference is scoped to the test
project.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Test-only build wiring and a deleted local shim; shipped packages are unchanged aside from a higher C# language ceiling that does not alter emitted library APIs.

Overview
Restores the net48 test leg after it started failing on non-generic TaskCompletionSource (used by DeferredEchoQueue) and on widespread Task.WaitAsync calls that .NET Framework lacks.

The hand-rolled TaskWaitAsyncPolyfill.cs shim is removed. Tests instead reference the source-only Polyfill package (11.3.0) via central versioning and tests/Directory.Build.props, scoped with PrivateAssets=all so nothing shipped picks it up.

Repo-wide LangVersion is raised 12 → 14 so Polyfill’s sources parse (C# 14 extension members); comments note this does not change how existing library code compiles on netstandard2.0.

Reviewed by Cursor Bugbot for commit 201ff63. Bugbot is set up for automated code reviews on this repo. Configure here.

@dborgards
dborgards merged commit 0f75b21 into main Sep 10, 2026
11 checks passed
@dborgards
dborgards deleted the fix/polyfill-net48 branch September 10, 2026 21:48
dborgards pushed a commit that referenced this pull request Sep 13, 2026
…te it

Codex is right that ci.yml carries a merge_group trigger and that a merge queue
is exactly the mechanism for testing a queued branch against current main
without a base merge. My rationale did not account for it.

It is not right that the workflow "already handles this case". Checked rather
than assumed: filtering CI runs by event merge_group returns zero. The trigger
has never fired, because the queue is configured in the workflow but not enabled
on the branch. Every merge to main, #100 and #101 today included, went in as a
plain merge commit, and the base merges this wave paid for were real.

So the rationale now says both things: the base-merge cost is real as things
stand, and it has a known expiry the day the queue is enabled -- at which point
that half of the argument goes away and the supervision half, which is the
reason the rule exists, does not. A rule whose stated cost can quietly stop
being true invites being dismissed later on exactly that ground.

Filed as #106, because an inert guard reads as protection that is not there, and
because the failure it was built for already happened once (#85, from two green
pull requests merged four minutes apart).

Markdown only; no code, project or workflow file touched, so no build, test or
format result is claimed.

Refs #85, #106.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
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