Enable nullable everywhere #954
Conversation
I'd like to add more csproj's soon, and to have nullable enabled in those places, so for simplicity I'm enabling it globally first.
| } | ||
| } | ||
|
|
||
| #endif |
There was a problem hiding this comment.
The other project brings in a package for this but I'm pretty meh on that.
In the future I'm going to make a folder for common source for analyzers and some Directory.Build.targets magic to <Compile Include="..." LinkBase="Common" />-in some shared analyzer code and this can go in there.
src/D2L.CodeStyle.TestAnalyzers/Extensions/Microsoft.CodeAnalysis.cs
Outdated
Show resolved
Hide resolved
| var syntaxReference = attribute.ApplicationSyntaxReference; | ||
|
|
||
| if( syntaxReference == null ) { | ||
| // ?? |
There was a problem hiding this comment.
I don't know if this is possible but also don't care much. I changed the behaviour to just silently continue if it happens
| SourceText? allowedListText = bannedListFile.GetText(); | ||
|
|
||
| if( allowedListText == null ) { | ||
| throw new Exception( "Couldn't read config" ); |
There was a problem hiding this comment.
I'm not sure how this would happen so I didn't try hard on this exception; I figure its better than the NullReferenceException we'd otherwise have. The IDE/CLI will tell you which analyzer threw an exception so I figure this is enough hint for now.
src/D2L.CodeStyle.TestAnalyzers/Extensions/Microsoft.CodeAnalysis.cs
Outdated
Show resolved
Hide resolved
| @@ -53,7 +54,7 @@ internal sealed class CategoryAnalyzer : DiagnosticAnalyzer { | |||
| context: ctx, | |||
| bannedCategories: bannedCategories, | |||
| types: types, | |||
| syntax: ctx.Node as MethodDeclarationSyntax | |||
| syntax: (ctx.Node as MethodDeclarationSyntax)! | |||
There was a problem hiding this comment.
nit: SyntaxNodeAnalysisContext.Node is already non-nullable. If this were a hard-cast would we be able to avoid needing to force non-null?
There was a problem hiding this comment.
Ah yeah sure. Technically a hard cast throws a different exception/earlier but the reason I used ! is that in the context the cast will always succeed, so yeah I'll switch to hard cast
| compilation.GetTypeByMetadataName( "NUnit.Framework.TestCaseAttribute" ), | ||
| compilation.GetTypeByMetadataName( "NUnit.Framework.TestCaseSourceAttribute" ), | ||
| compilation.GetTypeByMetadataName( "NUnit.Framework.TheoryAttribute" ) | ||
| }.Where( x => x != null )! |
There was a problem hiding this comment.
Is the null-forgiving op necessary on the result of Where?
There was a problem hiding this comment.
Yep, well known problem.
They've talked about extending nullable analysis more to handle LINQ stuff but nothing very concrete. Also related to covariance headaches (e.g. Task<T> vs Task<T?>)
The Where( x => x is not null )! thing is pretty common from what I've seen. Sometimes people make their own extensions. I've also seen .OfType<T>() but that feels even worse to me (not obvious what kind of "casting" is happening there)
src/D2L.CodeStyle.TestAnalyzers/NUnit/ConfigTestSetupStringsAnalyzer.cs
Outdated
Show resolved
Hide resolved
| // TODO: can this actually be null? | ||
| var syntaxReference = attribute.ApplicationSyntaxReference!; |
There was a problem hiding this comment.
comment is just there as a hint that this ! is a guess.
Clearly it hasn't caused troubles yet 🤷
| @@ -189,11 +193,16 @@ ISymbol symbol | |||
|
|
|||
| TypedConstant arg = args[0]; | |||
|
|
|||
| if( arg.Type == null ) { | |||
| return; | |||
| } | |||
There was a problem hiding this comment.
Behaviour change that I think is reasonable (ditto below)?
| if( arg.Type.SpecialType != SpecialType.System_String ) { | ||
| return; | ||
| } | ||
|
|
||
| string category = arg.Value as string; | ||
| string category = (string?)arg.Value!; |
There was a problem hiding this comment.
We know that arg.Type is System.String. I'm not sure if null can actually be returned here, I somewhat doubt it -- but this is equivalent behaviour fwiw
There was a problem hiding this comment.
Does [TestCategory( null )] bail out on an earlier check? I guess it's treated as object because the constant is evaluated independently of the context it's used in?
| if( testFixtureAttribute == null ) { | ||
| types = null; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
(reasonable imo) behaviour change
There was a problem hiding this comment.
(clearly its not crashing today, so maybe in practice not, but I suppose NUnit could rename / move these types in the future.)
| if( arg.Type.SpecialType != SpecialType.System_String ) { | ||
| return; | ||
| } | ||
|
|
||
| string category = arg.Value as string; | ||
| string category = (string?)arg.Value!; |
There was a problem hiding this comment.
Does [TestCategory( null )] bail out on an earlier check? I guess it's treated as object because the constant is evaluated independently of the context it's used in?
I'd like to add more csproj's soon, and to have nullable enabled in those places, so for simplicity I'm enabling it globally first.