[release/7.0] Disable nullability warnings in JSON source generator #74861
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsBackport of #74801 to release/7.0 /cc @eiriktsarpalis @krwq Customer ImpactTestingRiskIMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
@carlossanlop I'm guessing this does not apply to RC fixes right? |
|
Approved, we would potentially service for it. |
| #nullable enable | ||
|
|
||
| #nullable enable annotations | ||
| #nullable disable warnings |
There was a problem hiding this comment.
Are we ever generating public API? With annotations enabled we will still be outputting nullability metadata, and if we have reason to believe that the annotations are incorrect, that'll mean incorrect nullability information surfaced to consumers of the APIs.
If this is all internal to the assembly, it's probably fine. If it's not, we might instead consider doing:
#nullable disable
#pragma warning disable CS8632instead, or something along those lines.
There was a problem hiding this comment.
@eiriktsarpalis do you want to resolve this question before I hit merge?
There was a problem hiding this comment.
Yes let's please resolve this q first.
There was a problem hiding this comment.
we do produce some public APIs - AFAIK they're correct though
Correct, not needed for RCs. |
| @@ -112,6 +112,10 @@ | |||
| <Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\DynamicallyAccessedMemberTypes.cs" /> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\src\Microsoft.Extensions.Primitives.csproj" /> | |||
There was a problem hiding this comment.
This doesn't feel right to me. I know these are tests, but having something in System depend on things in Microsoft.Extensions feels backwards.
Is there something about StringValues that we can isolate into a unit test type? Or do we actually have to use the StringValues type in order to test the scenario?
There was a problem hiding this comment.
StringValues is what customer reported so it was added here as a test to make sure that specific scenario works. I think that should be fine for tests, we already depend on other things like JSON.NET
ghost
locked as resolved and limited conversation to collaborators
Backport of #74801 to release/7.0
/cc @eiriktsarpalis @krwq
Customer Impact
Customers have reported that certain nullability annotations cause JSON source-gen to generate code which produce code with warnings. Some libraries (i.e. Microsoft.Extensions.Primitives) added nullability annotations to their libraries and customers upgrading now start seeing nullability warnings. This is super annoying for users as they cannot suppress them in the source code and in certain situations where treating warnings as errors is enabled may cause build errors.
While this is not a regression in JSON itself, combination of this bug with new nullability annotations is a regression from product perspective.
Related issues:
StringValueswithJsonSerializerContextproduces warnings with7.0.0-preview*#74652Testing
Customer reported scenarios are added as tests, existing scenarios are already tested.
Risk
Minimal - product change is limited to disabling nullability warnings in the generated code.
IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.