Add nullable annotations to Mvc.Core/Routing #28834
Conversation
pranavkm
requested review from
javiercn,
jkotalik and
Tratcher
as code owners
pranavkm
added
the
area-mvc
pranavkm
force-pushed
the
prkrishn/routing-null
branch
from
8d94d50 to
7ad799a
Compare
| @@ -88,7 +90,7 @@ public ActionEndpointFactory(RoutePatternTransformer routePatternTransformer, IE | |||
| endpoints.Add(builder.Build()); | |||
| } | |||
|
|
|||
| if (action.AttributeRouteInfo == null) | |||
| if (action.AttributeRouteInfo?.Template == null) | |||
There was a problem hiding this comment.
Its been a while since I looked at this code. Why add Template to this check?
There was a problem hiding this comment.
The code in the else block assumes presence of AttributeRouteInfo indicates a non-null template. The documentation appears to disagree: https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.Abstractions/src/Routing/AttributeRouteInfo.cs#L13-L16.
I don't think we produce an AttributeRouteInfo without a template configured, but this would appear to be the correct check.
| @@ -63,6 +66,7 @@ public int Order | |||
| int? IRouteTemplateProvider.Order => _order; | |||
|
|
|||
| /// <inheritdoc /> | |||
| public string Name { get; set; } | |||
| [DisallowNull] | |||
| public string? Name { get; set; } | |||
There was a problem hiding this comment.
What does combining string? + DisallowNull do?
There was a problem hiding this comment.
It says while this could return null, you shouldn't assign a null value to it. Essentially the setter is treated as non-null
Splitting up Mvc.Core into multiple PRs to make it more manageable. Once everything is updated, I'll do a cleanup to remove the pragmas from individual files.