Enable nullability in RDG and fix warnings #47144
Conversation
captainsafia
commented
Mar 10, 2023
captainsafia
commented
- Enable nullability in RDG project and fix warnings
- Regenerate baselines directory
captainsafia
requested review from
BrennanConroy,
Tratcher and
halter73
as code owners
ghost
added
the
src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.Generators.csproj
Show resolved
Hide resolved
| internal static void EmitJsonPreparation(this EndpointResponse endpointResponse, CodeWriter codeWriter) | ||
| { | ||
| if (endpoint.Response.IsSerializable) | ||
| if (endpointResponse is { IsSerializable: true, ResponseType: {} responseType }) |
There was a problem hiding this comment.
Is there a reason we are checking ResponseType for null here, but not below in EmitJsonResponse?
There was a problem hiding this comment.
We call EmitJsonResponse from the the main request handler where we do null checking on the type earlier on (ref).
We use null coalescing when we call EmitJsonPreparation here so the null check isn't really required there.
Side note: I'm doing a lot of shuffling in this codepath in another branch as I'm working through a bug I found in writing JSON response for certain async handlers.
Edit: You're talking about ResponseType not Response 🤦🏽♀️
| } | ||
|
|
||
| private static string EmitResponseWritingCall(this Endpoint endpoint) | ||
| private static string? EmitResponseWritingCall(this EndpointResponse endpointResponse, bool isAwaitable) |
There was a problem hiding this comment.
When does this method ever return null?
There was a problem hiding this comment.
Not needed. Will update.
| <DefineConstants>$(DefineConstants),INTERNAL_NULLABLE_ATTRIBUTES</DefineConstants> | ||
| <NoWarn>$(NoWarn);nullable</NoWarn> | ||
| <!-- Repo-specific property to enable nullability warnings for ns2.0 --> | ||
| <NoWarn Condition=" '$(WarnOnNullable)' != 'true' ">$(NoWarn);nullable</NoWarn> |
There was a problem hiding this comment.
How many projects break if we omitted this check? Do many other netstandard2.0 projects even have nullability enabled? I wonder if we should invert this to opt out rather than in, so new netstandard2.0 projects get warnings as soon as we enable nullability.
There was a problem hiding this comment.
I didn't see how many projects were impacted but there were about ~150 warnings throughout the repo when I removed this. I filed #47179 to look at this.
I wonder if we should invert this to opt out rather than in, so new netstandard2.0 projects get warnings as soon as we enable nullability.
Yeah, that might be a prudent idea. I'll link to this comment in the issue above since it would be a good first step towards addressing the problem in the repo.
amcasey
added
area-networking
