Skip to content

test: repair the net48 leg against the merged DeferredEchoQueue - #84

Closed
dborgards wants to merge 1 commit into
mainfrom
fix/net48-taskcompletionsource
Closed

dborgards wants to merge 1 commit into
mainfrom
fix/net48-taskcompletionsource

Conversation

@dborgards

Copy link
Copy Markdown
Owner

main is currently broken on the Windows leg. This restores it.

What happened

#83 (the net48 test leg) and #81 (DeferredEchoQueue, and the J1939 rebind probe) were each green, and merged textually clean. Neither was ever built against the other: strict_required_status_checks_policy is false on the main ruleset, so GitHub did not re-run #83's checks after #81 landed, and #83's green checkmarks were stale by the time it merged. Since these three checks are now required, a broken Windows leg on main blocks every subsequent pull request.

Reproduced on main @ 8c94d51:

dotnet build tests/CanKit.Pro.Tests -f net48 -c Release -p:CanKitProTestNetFrameworkLeg=true

DeferredEchoQueue.cs(46,43): error CS0305: Using the generic type 'TaskCompletionSource<TResult>' requires 1 type arguments
J1939NodeTests.cs(857,36): error CS7036: There is no argument given that corresponds to the required parameter 'startIndex' of 'BitConverter.ToInt32(byte[], int)'
J1939NodeTests.cs(867,26): error CS0117: 'BitConverter' does not contain a definition for 'TryWriteBytes'

Three .NET 5+/Core-only APIs on a leg that compiles against netstandard2.0. Only the first was reported initially — the compiler stopped there.

The two fixes, and why they differ

TaskCompletionSource (non-generic) → polyfilled, in Infrastructure/TaskCompletionSourcePolyfill.cs, next to the existing Task.WaitAsync shim, under #if !NET and in the System.Threading.Tasks namespace. Arity keeps it unambiguous: TaskCompletionSource<T> still resolves to the framework's generic type everywhere, and only the arity-0 spelling — which .NET Framework simply does not have — resolves to the shim.

Shimming rather than rewriting the caller, because DeferredEchoQueue is deliberate test infrastructure that #24 is expected to build on, and a shim spares it (and the next file like it) from having to know the net48 leg exists. That is also why the surface is reproduced in full rather than trimmed to today's single TrySetResult() caller: a shim missing SetResult would only move the surprise.

It is a thin forwarder over TaskCompletionSource<bool>. One difference is not reproducible, and is documented in the file: .Task is statically a Task, as on .NET, but at run time it is a Task<bool>. Awaiting, cancelling, faulting and combining all behave identically; only reflection over its type could tell, and nothing does. .NET 8's SetFromTask/TrySetFromTask are left out on purpose — unlike the rest, they have real semantics to get wrong rather than one call to forward.

BitConverter → not polyfillable, so the call sites changed. BitConverter is a static BCL class: the span overloads .NET Framework lacks cannot be added from outside it, by extension method or otherwise. So the two sites in J1939NodeTests use the array overloads:

BitConverter.ToInt32(m.Payload.Span.Slice(0, 4))   ->  BitConverter.ToInt32(m.Payload.Span.Slice(0, 4).ToArray(), 0)
BitConverter.TryWriteBytes(payload.AsSpan(0, 4), seq)  ->  BitConverter.GetBytes(seq).CopyTo(payload, 0)

The same four bytes in the same machine endianness, compiling unchanged on both legs with no #if, and the test's intent is untouched. A comment says why, so they are not modernised back into a broken Windows leg.

Verification on the merged tree

  • dotnet build tests/CanKit.Pro.Tests -f net48 -c Release -p:CanKitProTestNetFrameworkLeg=true on macOS: clean, 0 warnings (was 3 errors).
  • dotnet build CanKit.Pro.sln -c Release: clean, 0 warnings.
  • dotnet test CanKit.Pro.sln -c Release: 409/409 passed on net10.0.
  • The "netstandard2.0 surface is approved for free" finding from test: execute the netstandard2.0 build on a net48 leg #83 still holds: all nine netstandard2.0 assemblies re-rendered and diffed against the current approvals — byte-identical.

Not verifiable locally (macOS, no Mono): actually executing the net48 leg. That is the Windows CI job's call.

Note on process

This is a separate branch because #83 had already merged by the time the collision was found, so there was no branch left to fix. The underlying gap is that main's ruleset does not require branches to be up to date before merging, which is what let two individually-green PRs combine into a red main. Worth considering strict_required_status_checks_policy: true now that a Windows-only compile path exists — that class of break cannot happen on the Linux/macOS legs, because they only ever build one TFM.

🤖 Generated with Claude Code

#83 (the net48 leg) and #81 (DeferredEchoQueue and the J1939 rebind probe) were
green side by side and merged clean, but neither was ever built against the
other: main's Windows leg does not compile.

  DeferredEchoQueue.cs(46): CS0305 'TaskCompletionSource<TResult>' requires 1
                            type argument
  J1939NodeTests.cs(857):   CS7036 BitConverter.ToInt32(byte[], int)
  J1939NodeTests.cs(867):   CS0117 BitConverter has no TryWriteBytes

The non-generic TaskCompletionSource is polyfilled, next to the Task.WaitAsync
shim and for the same reason: DeferredEchoQueue is deliberate test
infrastructure that issue #24 will build on, and a shim spares it — and the
next file like it — from knowing the net48 leg exists. The surface is
reproduced in full rather than trimmed to today's single TrySetResult caller,
since a shim missing SetResult would only move the surprise.

BitConverter cannot be treated the same way: it is a static BCL class, so the
span overloads .NET Framework lacks cannot be added from outside it. The two
call sites use the array overloads instead — the same four bytes in the same
machine endianness, compiling unchanged on both legs with no #if — and carry a
comment so they are not modernised back.

Verified on the merged tree: net48 compiles clean (0 warnings), net10.0 runs
409/409 locally, and the netstandard2.0 public surface is still byte-identical
to all nine approval files.

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-project-only compile fixes with no production code or API surface changes; behavioral intent of the J1939 test is unchanged (same endianness and bytes).

Overview
Fixes a broken Windows (net48) test leg on main after two merged PRs introduced .NET 5+ APIs that do not exist on .NET Framework.

Adds TaskCompletionSourcePolyfill.cs under #if !NET, mirroring the existing Task.WaitAsync shim: a non-generic TaskCompletionSource in System.Threading.Tasks that forwards to TaskCompletionSource<bool>, so DeferredEchoQueue and future test code can use the arity-0 type without #if or rewrites.

In J1939NodeTests (rebind BAM duplicate probe), BitConverter span APIs are replaced with array overloads (ToInt32(bytes, 0) and GetBytes + CopyTo) because static BCL members cannot be polyfilled. Comments document why so the sites are not “modernized” back into a failing net48 build.

Production / netstandard2.0 surface is unchanged — test-only infrastructure and one test file.

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

@dborgards

Copy link
Copy Markdown
Owner Author

Superseded by #85, which is already merged. Closing.

Both halves of this pull request turned out to be unnecessary once the suite moved to the Polyfill package:

The TaskCompletionSource shim — Polyfill ships the non-generic type for net48 at contentFiles/cs/net48/TaskCompletionSource.cs, in namespace System.Threading.Tasks, with the same forwarding-over-TaskCompletionSource<bool> design and the TaskCreationOptions constructor this file reproduced. The hand-rolled TaskWaitAsyncPolyfill.cs went the same way in #85.

The BitConverter rewrite — the premise here was:

BitConverter is a static BCL class, so the members .NET Framework lacks cannot be polyfilled the way Task.WaitAsync and TaskCompletionSource are

That was true of classic extension methods, but not of C# 14 extension members, which can add static members to an existing type. Polyfill 11.3.0 uses exactly that, and its net48 Polyfill_BitConverter.cs provides ToInt32(ReadOnlySpan<byte>) (line 115) and TryWriteBytes(Span<byte>, int) (line 207). The net48 build in #85 compiled J1939NodeTests.cs unchanged, span overloads and all, with 0 errors and 0 warnings.

So the .ToArray() and GetBytes().CopyTo() rewrites are not needed, and keeping them would add a copy per delivered frame in a test that measures timing.

Nothing here is lost — the same net48 leg is green on main now, without the two shims. Thanks for the diagnosis; the analysis of why net48 broke was right, only the remedy has been overtaken.

@dborgards dborgards closed this Sep 10, 2026
@dborgards
dborgards deleted the fix/net48-taskcompletionsource branch September 16, 2026 04:14
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