Enable nullable on Diagnostics and Rewrite middleware #28621
Conversation
Tratcher
left a comment
Tratcher
left a comment
There was a problem hiding this comment.
Diagnostics look fine. I'll let Justin review Rewrite 😁.
| ~Microsoft.AspNetCore.Rewrite.RewriteContext.Logger.set -> void | ||
| ~Microsoft.AspNetCore.Rewrite.RewriteContext.StaticFileProvider.get -> Microsoft.Extensions.FileProviders.IFileProvider | ||
| ~Microsoft.AspNetCore.Rewrite.RewriteContext.StaticFileProvider.set -> void | ||
| Microsoft.AspNetCore.Rewrite.RewriteContext.HttpContext.get -> Microsoft.AspNetCore.Http.HttpContext! |
There was a problem hiding this comment.
Is ~ supposed to be removed here?
There was a problem hiding this comment.
Yes. No ~ means it is annotated.
There was a problem hiding this comment.
IIRC, ~ means that the signature contains at least one reference type which is not known to be either nullable or non-nullable
| @@ -1,48 +1 @@ | |||
| #nullable enable | |||
| Microsoft.AspNetCore.Builder.DeveloperExceptionPageOptions.FileProvider.get -> Microsoft.Extensions.FileProviders.IFileProvider? | |||
There was a problem hiding this comment.
I think this duplication in PublicAPI files was caused by dotnet/roslyn-analyzers#4584
| @@ -1,18 +1,25 @@ | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
There was a problem hiding this comment.
📝 The encoding changes are concerning
There was a problem hiding this comment.
My changes are removing the BOMs which I assume is correct 🤷
There was a problem hiding this comment.
C# source files should have a byte order mark. It looks like the repository has an incorrectly-configured .editorconfig file which is causing these changes to appear.
| } | ||
|
|
||
| return new MatchResults { BackReferences = prevBackReferences, Success = condResult.Success }; | ||
| return new MatchResults(condResult!.Success, prevBackReferences); |
There was a problem hiding this comment.
💡 Consider proper argument validation instead of suppression. If conditions is empty when this method is called, a NullReferenceException will be thrown here.
There was a problem hiding this comment.
I looked at the source and this method will only be called if there are conditions, which means there will be a condResult. I've added a Debug.Assert with message to make that explicit.
| if (condResult.Success && trackAllCaptures && prevBackReferences != null) | ||
| { | ||
| prevBackReferences.Add(currentBackReferences); | ||
| prevBackReferences.Add(currentBackReferences!); |
There was a problem hiding this comment.
💡 If applicable, consider applying MemberNotNullWhen to condResult.Success, and changing this line to:
| prevBackReferences.Add(currentBackReferences!); | |
| prevBackReferences.Add(condResult.BackReferences); |
amcasey
added
the
area-middleware
Addresses #5680