Implement 'Prefer null check over type check' #53947

Youssef1313 · 2021-06-08T16:13:42Z

sharwell · 2021-06-08T16:15:43Z

src/EditorFeatures/CSharp/CSharpEditorResources.resx

@@ -437,4 +437,7 @@
  <data name="Outside_namespace" xml:space="preserve">
    <value>Outside namespace</value>
  </data>
+  <data name="Prefer_is_null_check_over_is_object" xml:space="preserve">
+    <value>Prefer 'is null' check over 'is object'</value>


💭 Should we match semantics?

Suggested change

<value>Prefer 'is null' check over 'is object'</value>

<value>Prefer 'is not null' check over 'is object'</value>

@sharwell I wasn't able to word it accurately in a small sentence. It's both ways (prefer is null over is not object and prefer is not null over is object).

i would have both forms :)

Is something like Prefer 'is null'/'is not null' checks over 'is not object'/'is object' good enough?

I don't love it. But I could live with it. @sharwell ?

My pref really would be a unique string for each, or something like "Prefer 'null' check over type check".

i still like my suggestion here.

I thought I already did that (using unique string for each), but turns out this resource file is for editorconfig UI, and what I did was for the analyzer. Actually, a unique string cannot be done in EditorConfig UI.

I'll use "Prefer 'null' check over type check" for both the analyzer and EditorConfig UI.

src/Analyzers/CSharp/Tests/UseIsNullCheck/UseIsNullCheckOverIsObjectTests.cs

src/Analyzers/CSharp/Analyzers/UseIsNullCheck/CSharpUseIsNullCheckOverIsObject.cs

CyrusNajmabadi · 2021-06-08T17:07:44Z

src/Analyzers/CSharp/Analyzers/UseIsNullCheck/CSharpUseIsNullCheckOverIsObject.cs

+                var severity = option.Notification.Severity;
+                context.ReportDiagnostic(
+                    DiagnosticHelper.Create(
+                        Descriptor, context.Operation.Syntax.GetLocation(), severity, additionalLocations: null, properties: null));


i'd like for us to ensure the syntax is what we expect as well. in case this pattern was somehow emitted in the bound nodes but on syntax we don't expect.

@CyrusNajmabadi I'm checking the syntax in the codefix and throwing ExceptionUtilities.UnexpectedValue on unexpected syntax.

Definitely not the way to do it :-)
Analyzers should not report issues if the fix would crash on them :-)

@CyrusNajmabadi Going to move the check in the analyzer instead. Should I keep throwing or just bail-out on unexpected nodes?

Or perhaps keep the check in the codefix but return silently without crashing instead? (one benefit to this is we can get feedback from users when the codefix doesn't offer anything so we can see what syntax causes that, whether it could be handled, etc)

You sound cleanly bail out in release mode. We can consider a debug assert for debug mode, but my preference is not too have that.

We can effectively drive completeness in these cases through dogfooding and user reports.

src/Analyzers/CSharp/Analyzers/UseIsNullCheck/CSharpUseIsNullCheckOverIsObject.cs

src/Analyzers/CSharp/Tests/UseIsNullCheck/UseIsNullCheckOverIsObjectTests.cs

src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems

MaStr11 · 2021-06-09T15:10:28Z

src/Analyzers/CSharp/CodeFixes/UseIsNullCheck/CSharpUseNullCheckOverTypeCheckCodeFixProvider.cs

+                SyntaxNode? replacement;
+                // Replace 'x is object' with 'x is not null'
+                if (node is BinaryExpressionSyntax binary)
+                {
+                    replacement = SyntaxFactory.IsPatternExpression(
+                        expression: binary.Left,
+                        pattern: SyntaxFactory.UnaryPattern(SyntaxFactory.ConstantPattern(SyntaxFactory.LiteralExpression(SyntaxKind.NullLiteralExpression))));
+                }
+                else if (node is UnaryPatternSyntax)
+                {
+                    replacement = SyntaxFactory.ConstantPattern(SyntaxFactory.LiteralExpression(SyntaxKind.NullLiteralExpression));   
+                }
+                else
+                {
+                    throw ExceptionUtilities.UnexpectedValue(node.Kind());
+                }


Consider using using static Microsoft.CodeAnalysis.CSharp.Syntax.SyntaxFactory; and a switch expression instead (Code not tested):

Suggested change

SyntaxNode? replacement;

// Replace 'x is object' with 'x is not null'

if (node is BinaryExpressionSyntax binary)

{

replacement = SyntaxFactory.IsPatternExpression(

expression: binary.Left,

pattern: SyntaxFactory.UnaryPattern(SyntaxFactory.ConstantPattern(SyntaxFactory.LiteralExpression(SyntaxKind.NullLiteralExpression))));

}

else if (node is UnaryPatternSyntax)

{

replacement = SyntaxFactory.ConstantPattern(SyntaxFactory.LiteralExpression(SyntaxKind.NullLiteralExpression));

}

else

{

throw ExceptionUtilities.UnexpectedValue(node.Kind());

}

var replacement = node switch

{

// Replace 'x is object' with 'x is not null'

BinaryExpressionSyntax binary =>

IsPatternExpression(

expression: binary.Left,

pattern: UnaryPattern(ConstantPattern(LiteralExpression(SyntaxKind.NullLiteralExpression)))),

UnaryPatternSyntax =>

ConstantPattern(LiteralExpression(SyntaxKind.NullLiteralExpression)),

_ => throw ExceptionUtilities.UnexpectedValue(node.Kind()),

};

MaStr11 · 2021-06-09T15:28:14Z

...alyzers/CSharp/Analyzers/UseIsNullCheck/CSharpUseNullCheckOverTypeCheckDiagnosticAnalyzer.cs

+                return negatedPattern.Pattern is ITypePatternOperation typePatternOperation &&
+                    typePatternOperation.InputType.InheritsFromOrEquals(typePatternOperation.MatchedType);


Shouldn't there be a check, that there is no designation present like in is not object o?

@MaStr11 negatedPattern.Pattern won't be ITypePatternOperation for the is not object o case. Going to add a test.

MaStr11 · 2021-06-09T15:31:13Z

src/Analyzers/CSharp/Tests/UseIsNullCheck/UseNullCheckOverTypeCheckDiagnosticAnalyzerTests.cs

+{
+    public bool M(string value)
+    {
+        return value is [|not string|];


Add test for something like:

if (value is not string s) { } else { _ = s == "SomeValue"; }

Youssef1313 · 2021-06-20T18:07:34Z

@CyrusNajmabadi This is ready for another look

Youssef1313 · 2021-06-21T09:27:40Z

D:\workspace\_work\1\s\src\NuGet\Microsoft.Net.Compilers.Toolset\Microsoft.Net.Compilers.Toolset.Package.csproj : error NU3037: Package 'System.Buffers 4.4.0' from source 'https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json': The repository primary signature validity period has expired.
D:\workspace\_work\1\s\src\NuGet\Microsoft.Net.Compilers.Toolset\Microsoft.Net.Compilers.Toolset.Package.csproj : error NU3037: Package 'System.Numerics.Vectors 4.4.0' from source 'https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json': The repository primary signature validity period has expired.
D:\workspace\_work\1\s\.tools\msbuild\16.8.0-preview2.1\tools\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(10,5): error : Unable to delete temporary file 'C:\Users\vsagent\.nuget\packages\system.memory\4.5.3\ov03uxuv.lky'. Error: 'Could not find a part of the path 'C:\Users\vsagent\.nuget\packages\system.memory'.'. [D:\workspace\_work\1\s\src\NuGet\Microsoft.Net.Compilers.Toolset\Microsoft.Net.Compilers.Toolset.Package.csproj]
    0 Warning(s)
    3 Error(s)

Closing and reopening for new build.

…nto use-is-null

Youssef1313 · 2021-06-22T15:17:18Z

@CyrusNajmabadi I addressed your feedback

CyrusNajmabadi · 2021-06-22T18:00:28Z

...alyzers/CSharp/Analyzers/UseIsNullCheck/CSharpUseNullCheckOverTypeCheckDiagnosticAnalyzer.cs

+                }
+
+                context.RegisterOperationAction(c => AnalyzeIsTypeOperation(c), OperationKind.IsType);
+                context.RegisterOperationAction(c => AnalyzeNegatedPatternOperation(c), OperationKind.NegatedPattern);


CyrusNajmabadi · 2021-06-22T18:01:01Z

...alyzers/CSharp/Analyzers/UseIsNullCheck/CSharpUseNullCheckOverTypeCheckDiagnosticAnalyzer.cs

+            }
+
+            severity = option.Notification.Severity;
+            return context.Operation.Syntax is BinaryExpressionSyntax or UnaryPatternSyntax;


consider removing this and moving the individual checks into the respective ANalyzeXXX helpers.

...alyzers/CSharp/Analyzers/UseIsNullCheck/CSharpUseNullCheckOverTypeCheckDiagnosticAnalyzer.cs

CyrusNajmabadi · 2021-06-22T18:05:13Z

src/Analyzers/CSharp/CodeFixes/UseIsNullCheck/CSharpUseNullCheckOverTypeCheckCodeFixProvider.cs

+                            expression: binary.Left,
+                            pattern: UnaryPattern(ConstantPattern(LiteralExpression(SyntaxKind.NullLiteralExpression)))),
+                    UnaryPatternSyntax =>
+                        ConstantPattern(LiteralExpression(SyntaxKind.NullLiteralExpression)),


consider pulling ConstantPattern(LiteralExpression(SyntaxKind.NullLiteralExpression)) out (maybe into a satatic) and just referncing here. But feel free not to :)

CyrusNajmabadi · 2021-06-22T18:10:15Z

src/VisualStudio/CSharp/Impl/CSharpVSResources.resx

@@ -640,4 +640,7 @@
  <data name="Automatically_complete_statement_on_semicolon" xml:space="preserve">
    <value>Automatically complete statement on semicolon</value>
  </data>
+  <data name="Prefer_null_check_over_type_check" xml:space="preserve">
+    <value>Prefer null check over type check</value>


this is inconsistent 'null' should be in quotes. Also... this feels like a duplicated string. Can we unify?

I think the duplication is tracked by #52343. This is not for this analyzer only. :(

CyrusNajmabadi · 2021-06-28T16:34:10Z

thanks!

Implement 'Prefer is null over is object'

245412f

dotnet-issue-labeler bot added the Area-IDE label Jun 8, 2021

sharwell reviewed Jun 8, 2021

View reviewed changes

Delete incorrectly checked-in file

b56d3b5

Youssef1313 commented Jun 8, 2021

View reviewed changes

src/Analyzers/CSharp/Tests/UseIsNullCheck/UseIsNullCheckOverIsObjectTests.cs Outdated Show resolved Hide resolved