Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable nullable everywhere #954

Merged
merged 3 commits into from
Aug 7, 2024
Merged

Enable nullable everywhere #954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9270442
Enable nullable everywhere
j3parker Aug 7, 2024
7640157
Use more [NotNullWhen(..)]
j3parker Aug 7, 2024
91b2853
Misc. tweaks
j3parker Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
<LangVersion>10.0</LangVersion>
<ImplicitUsings>true</ImplicitUsings>
<InvariantGlobalization>true</InvariantGlobalization>
<Nullable>enable</Nullable>
</PropertyGroup>
</Project>
1 change: 0 additions & 1 deletion src/D2L.CodeStyle.Analyzers/D2L.CodeStyle.Analyzers.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
<GeneratePackageOnBuild>True</GeneratePackageOnBuild>
<IncludeBuildOutput>False</IncludeBuildOutput>
<IncludeSymbols>True</IncludeSymbols>
<Nullable>enable</Nullable>
<WarningsAsErrors>CA2016,Nullable</WarningsAsErrors>
<TreatWarningsAsErrors>false</TreatWarningsAsErrors>
<EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
Expand Down
14 changes: 14 additions & 0 deletions src/D2L.CodeStyle.TestAnalyzers/Common/NotNullWhenAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#if !NETSTANDARD2_1

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage( AttributeTargets.Parameter )]
internal sealed class NotNullWhenAttribute : Attribute {
public bool ReturnValue { get; }

public NotNullWhenAttribute( bool returnValue ) {
ReturnValue = returnValue;
}
}

#endif
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

9 changes: 7 additions & 2 deletions src/D2L.CodeStyle.TestAnalyzers/Extensions/Microsoft.CodeAnalysis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Text;
using Microsoft.CodeAnalysis;

Expand All @@ -9,7 +10,9 @@ public static class RoslynExtensions {

// Copied from the non-test assembly because we do not reference it.

public static bool IsNullOrErrorType( this ITypeSymbol symbol ) {
public static bool IsNullOrErrorType(
[NotNullWhen( false )] this ITypeSymbol? symbol
) {

if( symbol == null ) {
return true;
Expand All @@ -26,7 +29,9 @@ public static bool IsNullOrErrorType( this ITypeSymbol symbol ) {
return false;
}

public static bool IsNullOrErrorType( this ISymbol symbol ) {
public static bool IsNullOrErrorType(
[NotNullWhen( false )] this ISymbol? symbol
) {

if( symbol == null ) {
return true;
Expand Down
67 changes: 45 additions & 22 deletions src/D2L.CodeStyle.TestAnalyzers/NUnit/CategoryAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Linq;
using D2L.CodeStyle.TestAnalyzers.Common;
Expand Down Expand Up @@ -42,7 +43,7 @@ public override void Initialize( AnalysisContext context ) {
}

private static void OnCompilationStart( CompilationStartAnalysisContext context ) {
if( !TryLoadNUnitTypes( context.Compilation, out NUnitTypes types ) ) {
if( !TryLoadNUnitTypes( context.Compilation, out NUnitTypes? types ) ) {
return;
}

Expand All @@ -53,7 +54,7 @@ private static void OnCompilationStart( CompilationStartAnalysisContext context
context: ctx,
bannedCategories: bannedCategories,
types: types,
syntax: ctx.Node as MethodDeclarationSyntax
syntax: (MethodDeclarationSyntax)ctx.Node
),
SyntaxKind.MethodDeclaration
);
Expand All @@ -69,9 +70,12 @@ private static void OnCompilationEnd( CompilationAnalysisContext context, NUnitT
return;
}

// TODO: can this actually be null?
var syntaxReference = attribute.ApplicationSyntaxReference!;
Comment on lines +73 to +74
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment is just there as a hint that this ! is a guess.
Clearly it hasn't caused troubles yet 🤷


context.ReportDiagnostic( Diagnostic.Create(
Diagnostics.NUnitCategory,
attribute.ApplicationSyntaxReference.GetSyntax( context.CancellationToken ).GetLocation(),
syntaxReference.GetSyntax( context.CancellationToken ).GetLocation(),
$"Assemblies cannot be categorized as any of [{string.Join( ", ", ProhibitedAssemblyCategories )}], but saw '{category}'."
) );
} );
Expand All @@ -85,7 +89,7 @@ MethodDeclarationSyntax syntax
) {
SemanticModel model = context.SemanticModel;

IMethodSymbol method = model.GetDeclaredSymbol( syntax, context.CancellationToken );
IMethodSymbol? method = model.GetDeclaredSymbol( syntax, context.CancellationToken );
if( method == null ) {
return;
}
Expand Down Expand Up @@ -117,8 +121,8 @@ private static bool IsTestMethod(
IMethodSymbol method
) {
foreach( AttributeData attribute in method.GetAttributes() ) {
INamedTypeSymbol attributeType = attribute.AttributeClass;
if( types.TestAttributes.Contains( attributeType ) ) {
INamedTypeSymbol? attributeType = attribute.AttributeClass;
if( attributeType != null && types.TestAttributes.Contains( attributeType ) ) {
return true;
}
}
Expand Down Expand Up @@ -159,7 +163,7 @@ private static void VisitCategories(
Action<string, AttributeData> visitor
) {
foreach( AttributeData attribute in symbol.GetAttributes() ) {
INamedTypeSymbol attributeType = attribute.AttributeClass;
INamedTypeSymbol? attributeType = attribute.AttributeClass;
if( types.CategoryAttribute.Equals( attributeType, SymbolEqualityComparer.Default ) ) {
VisitCategoryAttribute( attribute, visitor );
continue;
Expand Down Expand Up @@ -189,11 +193,16 @@ Action<string, AttributeData> visitor

TypedConstant arg = args[0];

if( arg.Type == null ) {
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure


visitor( category, attribute );
}

Expand All @@ -208,11 +217,16 @@ Action<string, AttributeData> visitor

TypedConstant arg = namedArg.Value;

if( arg.Type == null ) {
continue;
}

if( arg.Type.SpecialType != SpecialType.System_String ) {
continue;
}

string categoryCsv = arg.Value as string;
string categoryCsv = (string?)arg.Value!;

foreach( string category in categoryCsv.Split( ',' ) ) {
visitor( category.Trim(), attribute );
}
Expand All @@ -221,24 +235,29 @@ Action<string, AttributeData> visitor

private static bool TryLoadNUnitTypes(
Compilation compilation,
out NUnitTypes types
[NotNullWhen( true )]
out NUnitTypes? types
) {
INamedTypeSymbol categoryAttribute = compilation.GetTypeByMetadataName( "NUnit.Framework.CategoryAttribute" );
INamedTypeSymbol? categoryAttribute = compilation.GetTypeByMetadataName( "NUnit.Framework.CategoryAttribute" );
if( categoryAttribute == null || categoryAttribute.TypeKind == TypeKind.Error ) {
types = null;
return false;
}

ImmutableHashSet<INamedTypeSymbol> testAttributes = ImmutableHashSet
.Create<INamedTypeSymbol>(
SymbolEqualityComparer.Default,
compilation.GetTypeByMetadataName( "NUnit.Framework.TestAttribute" ),
compilation.GetTypeByMetadataName( "NUnit.Framework.TestCaseAttribute" ),
compilation.GetTypeByMetadataName( "NUnit.Framework.TestCaseSourceAttribute" ),
compilation.GetTypeByMetadataName( "NUnit.Framework.TheoryAttribute" )
);
ImmutableHashSet<INamedTypeSymbol> testAttributes = new[] {
compilation.GetTypeByMetadataName( "NUnit.Framework.TestAttribute" ),
compilation.GetTypeByMetadataName( "NUnit.Framework.TestCaseAttribute" ),
compilation.GetTypeByMetadataName( "NUnit.Framework.TestCaseSourceAttribute" ),
compilation.GetTypeByMetadataName( "NUnit.Framework.TheoryAttribute" )
}.Where( x => x != null )!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the null-forgiving op necessary on the result of Where?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

.ToImmutableHashSet<INamedTypeSymbol>( SymbolEqualityComparer.Default );

INamedTypeSymbol testFixtureAttribute = compilation.GetTypeByMetadataName( "NUnit.Framework.TestFixtureAttribute" );
INamedTypeSymbol? testFixtureAttribute = compilation.GetTypeByMetadataName( "NUnit.Framework.TestFixtureAttribute" );

if( testFixtureAttribute == null ) {
types = null;
return false;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(reasonable imo) behaviour change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(clearly its not crashing today, so maybe in practice not, but I suppose NUnit could rename / move these types in the future.)


types = new NUnitTypes( categoryAttribute, testAttributes, testFixtureAttribute );
return true;
Expand Down Expand Up @@ -268,15 +287,19 @@ AnalyzerOptions options
StringComparer.Ordinal
);

AdditionalText bannedListFile = options.AdditionalFiles.FirstOrDefault(
AdditionalText? bannedListFile = options.AdditionalFiles.FirstOrDefault(
file => Path.GetFileName( file.Path ) == "BannedTestCategoriesList.txt"
);

if( bannedListFile == null ) {
return bannedList.ToImmutableHashSet();
}

SourceText allowedListText = bannedListFile.GetText();
SourceText? allowedListText = bannedListFile.GetText();

if( allowedListText == null ) {
throw new Exception( "Couldn't read config" );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

foreach( TextLine line in allowedListText.Lines ) {
bannedList.Add( line.ToString().Trim() );
Expand Down
8 changes: 4 additions & 4 deletions src/D2L.CodeStyle.TestAnalyzers/NUnit/ConfigTestSetupStringsAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public override void Initialize( AnalysisContext context ) {

private static void Register( CompilationStartAnalysisContext context ) {

INamedTypeSymbol attributeType =
INamedTypeSymbol? attributeType =
context.Compilation.GetTypeByMetadataName( AttributeTypeName );

if( attributeType.IsNullOrErrorType() ) {
Expand Down Expand Up @@ -73,7 +73,7 @@ INamedTypeSymbol attributeType

foreach( AttributeSyntax attribute in attributes ) {

ISymbol symbol = context
ISymbol? symbol = context
.SemanticModel
.GetSymbolInfo( attribute, context.CancellationToken )
.Symbol;
Expand All @@ -88,9 +88,9 @@ INamedTypeSymbol attributeType
}

SeparatedSyntaxList<AttributeArgumentSyntax> arguments =
attribute.ArgumentList.Arguments;
attribute.ArgumentList?.Arguments ?? default;

if( arguments.Count != 1 ) {
if( arguments == default || arguments.Count != 1 ) {
continue;
}

Expand Down
2 changes: 1 addition & 1 deletion src/D2L.CodeStyle.TestAnalyzers/ServiceLocator/CustomTestServiceLocatorAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public override void Initialize( AnalysisContext context ) {
public static void RegisterServiceLocatorAnalyzer(
CompilationStartAnalysisContext context
) {
INamedTypeSymbol factoryType = context.Compilation
INamedTypeSymbol? factoryType = context.Compilation
.GetTypeByMetadataName( TestServiceLocatorFactoryType );

if( factoryType.IsNullOrErrorType() ) {
Expand Down
Loading