Skip to content

[R31] Reject async schema upgrade methods with CN0018 - #36

Open
JetF0x wants to merge 1 commit into
Carsillas:rewritefrom
JetF0x:fix/review-r31
Open

JetF0x wants to merge 1 commit into
Carsillas:rewritefrom
JetF0x:fix/review-r31

Conversation

@JetF0x

@JetF0x JetF0x commented Sep 12, 2026

Copy link
Copy Markdown

Addresses review finding R31.

A static async void method marked [UpgradeTo] currently passes signature validation, but generated deserialization invokes it synchronously and completes its writer when the method returns. An incomplete await can therefore omit upgraded fields and make the continuation throw when it writes to the completed payload. The review reproduced an old name-mode payload containing Value=42 returning with Value=0.

This change rejects async upgrade methods with the existing CN0018 error and makes the diagnostic explain that methods must be non-async. It also checks a partial method's implementation: Roslyn exposes the declaration without its implementation's async modifier, so IsAsync alone leaves that path unprotected. The README now states that every upgrade write must finish before the method returns.

Regression coverage includes async void with and without await, an async partial implementation with the attribute on either part, a synchronous partial control, and Task/Task return signatures with and without async. The existing ordinary synchronous void control remains valid.

Validation on Windows using SDK 10.0.400, the existing Roslyn 5.6.0 references, and the .NET 8 runtime:

  • Before the fix, all four new async-void cases failed because the analyzer emitted no diagnostic. A direct IsAsync check alone still failed both partial-method cases.
  • The focused upgrade suite passes all 17 cases. dotnet test Cat.Network.sln --configuration Release --no-restore passes all 237 tests: 155 runtime, 62 analyzer, and 20 generator.
  • A separate net8.0/C# 14 consumer loaded both compiler components. All eight invalid signature variants failed with CN0018 as their only error, with no analyzer or generator crashes. Both synchronous ordinary and partial upgrades built without warnings and actual generated deserialization preserved the legacy value of 42.

This tightens the compile-time contract; existing async void upgrades, including those without await, now fail compilation when the analyzer is enabled. CN0018 remains an error with the same identifier. The generator, runtime, wire format, dependencies, and SDK settings are unchanged, and this patch adds no awaitable upgrade API. Suppressing or omitting the analyzer still bypasses this validation; synchronous methods must also avoid scheduling writes after they return. No other review fix is required for this change.

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