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.

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

Parameter null-checking analyzer and fixer #58182

Merged
merged 16 commits into from
Jan 12, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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 src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
<Compile Include="$(MSBuildThisFileDirectory)UseIsNullCheck\CSharpUseNullCheckOverTypeCheckDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseIsNullCheck\CSharpUseIsNullCheckForCastAndEqualityOperatorDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseIsNullCheck\CSharpUseIsNullCheckForReferenceEqualsDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseParameterNullChecking\CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseTupleSwap\CSharpUseTupleSwapDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseLocalFunction\CSharpUseLocalFunctionDiagnosticAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)UseNullPropagation\CSharpUseNullPropagationDiagnosticAnalyzer.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -340,4 +340,7 @@
<data name="Use_tuple_to_swap_values" xml:space="preserve">
<value>Use tuple to swap values</value>
</data>
<data name="Use_parameter_null_checking" xml:space="preserve">
<value>Use parameter null checking</value>
</data>
</root>
Original file line number Diff line number Diff line change
@@ -0,0 +1,294 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Linq;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Shared.Extensions;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.UseParameterNullChecking
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal sealed class CSharpUseParameterNullCheckingDiagnosticAnalyzer
: AbstractBuiltInCodeStyleDiagnosticAnalyzer
{
private const string ArgumentNullExceptionName = $"{nameof(System)}.{nameof(ArgumentNullException)}";

public CSharpUseParameterNullCheckingDiagnosticAnalyzer()
: base(IDEDiagnosticIds.UseParameterNullCheckingId,
EnforceOnBuildValues.UseParameterNullChecking,
CSharpCodeStyleOptions.PreferParameterNullChecking,
CSharpAnalyzersResources.Use_parameter_null_checking,
new LocalizableResourceString(nameof(AnalyzersResources.Null_check_can_be_simplified), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)))
{
}
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved

public override DiagnosticAnalyzerCategory GetAnalyzerCategory()
=> DiagnosticAnalyzerCategory.SemanticSpanAnalysis;

protected override void InitializeWorker(AnalysisContext context)
=> context.RegisterCompilationStartAction(context =>
{
var compilation = (CSharpCompilation)context.Compilation;
if (compilation.LanguageVersion < LanguageVersionExtensions.CSharpNext)
{
return;
}

var argumentNullException = compilation.GetTypeByMetadataName(ArgumentNullExceptionName);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var argumentNullException = compilation.GetTypeByMetadataName(ArgumentNullExceptionName);
var argumentNullException = compilation.GetBestTypeByMetadataName(ArgumentNullExceptionName);

if (argumentNullException is null)
{
return;
}

IMethodSymbol? argumentNullExceptionConstructor = null;
IMethodSymbol? argumentNullExceptionStringConstructor = null;
foreach (var constructor in argumentNullException.InstanceConstructors)
{
if (argumentNullExceptionConstructor is not null && argumentNullExceptionStringConstructor is not null)
{
break;
}

switch (constructor)
{
case { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 0 }:
argumentNullExceptionConstructor = constructor;
break;
case { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 1 }
when constructor.Parameters[0].Type.SpecialType == SpecialType.System_String:

argumentNullExceptionStringConstructor = constructor;
break;
}
}

if (argumentNullExceptionConstructor is null || argumentNullExceptionStringConstructor is null)
{
return;
}

var objectType = compilation.GetSpecialType(SpecialType.System_Object);
var referenceEqualsMethod = (IMethodSymbol?)objectType
.GetMembers(nameof(ReferenceEquals))
.FirstOrDefault(m => m is IMethodSymbol { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 2 });

// We are potentially interested in any declaration that has parameters.
// However, we avoid indexers specifically because of the complexity of locating and deleting equivalent null checks across multiple accessors.
context.RegisterSyntaxNodeAction(
context => AnalyzeSyntax(context, argumentNullExceptionConstructor, argumentNullExceptionStringConstructor, referenceEqualsMethod),
SyntaxKind.ConstructorDeclaration,
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
SyntaxKind.MethodDeclaration,
SyntaxKind.LocalFunctionStatement,
SyntaxKind.SimpleLambdaExpression,
SyntaxKind.ParenthesizedLambdaExpression,
SyntaxKind.AnonymousMethodExpression,
SyntaxKind.OperatorDeclaration,
SyntaxKind.ConversionOperatorDeclaration);
});

private void AnalyzeSyntax(
SyntaxNodeAnalysisContext context,
IMethodSymbol argumentNullExceptionConstructor,
IMethodSymbol argumentNullExceptionStringConstructor,
IMethodSymbol? referenceEqualsMethod)
{
var cancellationToken = context.CancellationToken;

var semanticModel = context.SemanticModel;
var syntaxTree = semanticModel.SyntaxTree;

var option = context.Options.GetOption(CSharpCodeStyleOptions.PreferParameterNullChecking, syntaxTree, cancellationToken);
if (!option.Value)
{
return;
}

var node = context.Node;
var block = node switch
{
MethodDeclarationSyntax methodDecl => methodDecl.Body,
ConstructorDeclarationSyntax constructorDecl => constructorDecl.Body,
LocalFunctionStatementSyntax localFunctionStatement => localFunctionStatement.Body,
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
AnonymousFunctionExpressionSyntax anonymousFunction => anonymousFunction.Block,
OperatorDeclarationSyntax operatorDecl => operatorDecl.Body,
ConversionOperatorDeclarationSyntax conversionDecl => conversionDecl.Body,
_ => throw ExceptionUtilities.UnexpectedValue(node)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
};

// More scenarios should be supported eventually: https://github.com/dotnet/roslyn/issues/58699
if (block is null)
{
return;
}

var methodSymbol = node is AnonymousFunctionExpressionSyntax
? (IMethodSymbol?)semanticModel.GetSymbolInfo(node, cancellationToken).Symbol
: (IMethodSymbol?)semanticModel.GetDeclaredSymbol(node, cancellationToken);
if (methodSymbol is null || methodSymbol.Parameters.IsEmpty)
{
return;
}

foreach (var statement in block.Statements)
{
var parameter = TryGetParameterNullCheckedByStatement(statement);
if (parameter is not null
&& (!parameter.Type.IsValueType
|| parameter.Type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T
|| parameter.Type.TypeKind is TypeKind.Pointer or TypeKind.FunctionPointer)
Copy link
Member

Choose a reason for hiding this comment

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

extract into helper plz (can be local function or method). mixing the && || and ! is too much for my brain :)

&& parameter.RefKind == RefKind.None
&& parameter.DeclaringSyntaxReferences.FirstOrDefault() is SyntaxReference reference
&& reference.SyntaxTree.Equals(statement.SyntaxTree)
&& reference.GetSyntax() is ParameterSyntax parameterSyntax)
{
context.ReportDiagnostic(DiagnosticHelper.Create(
Descriptor,
statement.GetLocation(),
option.Notification.Severity,
additionalLocations: new[] { parameterSyntax.GetLocation() },
properties: null));
}
}

return;

IParameterSymbol? TryGetParameterNullCheckedByStatement(StatementSyntax statement)
{
switch (statement)
{
// if (param == null) { throw new ArgumentNullException(nameof(param)); }
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
// if (param is null) { throw new ArgumentNullException(nameof(param)); }
// if (object.ReferenceEquals(param, null)) { throw new ArgumentNullException(nameof(param)); }
case IfStatementSyntax ifStatement:
ExpressionSyntax left, right;
Copy link
Member

Choose a reason for hiding this comment

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

i would extract the entire if-handling code to it's own helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I won't make this change due to lack of uniformity with the ExpressionStatement case. See #58182 (comment)

switch (ifStatement)
{
case { Condition: BinaryExpressionSyntax { OperatorToken.RawKind: (int)SyntaxKind.EqualsEqualsToken } binary }
// Only suggest the fix on built-in `==` operators where we know we won't change behavior
when semanticModel.GetSymbolInfo(binary).Symbol is IMethodSymbol { MethodKind: MethodKind.BuiltinOperator }:

left = binary.Left;
right = binary.Right;
break;
case { Condition: IsPatternExpressionSyntax { Expression: var patternInput, Pattern: ConstantPatternSyntax { Expression: var patternExpression } } }:
left = patternInput;
right = patternExpression;
break;
case { Condition: InvocationExpressionSyntax { Expression: var receiver, ArgumentList.Arguments: { Count: 2 } arguments } }
when referenceEqualsMethod != null && referenceEqualsMethod.Equals(semanticModel.GetSymbolInfo(receiver, cancellationToken).Symbol):

left = arguments[0].Expression;
right = arguments[1].Expression;
break;

default:
return null;
}

var parameterInBinary = left.IsKind(SyntaxKind.NullLiteralExpression) ? TryGetParameter(right)
: right.IsKind(SyntaxKind.NullLiteralExpression) ? TryGetParameter(left)
: null;
if (parameterInBinary is null)
{
return null;
}

var throwStatement = ifStatement.Statement switch
{
ThrowStatementSyntax @throw => @throw,
BlockSyntax { Statements: { Count: 1 } statements } => statements[0] as ThrowStatementSyntax,
_ => null
};

if (throwStatement?.Expression is not ObjectCreationExpressionSyntax thrownInIf
|| !IsConstructorApplicable(thrownInIf, parameterInBinary))
{
return null;
}

return parameterInBinary;

// this.field = param ?? throw new ArgumentNullException(nameof(param));
case ExpressionStatementSyntax
Copy link
Member

Choose a reason for hiding this comment

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

similarly, can you extract this case out to a helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that since much of the complexity was in the pattern itself, the extracted logic didn't feel sufficiently complex to justify a single-use helper.

{
Expression: AssignmentExpressionSyntax
{
Right: BinaryExpressionSyntax
{
OperatorToken.RawKind: (int)SyntaxKind.QuestionQuestionToken,
Left: ExpressionSyntax maybeParameter,
Right: ThrowExpressionSyntax { Expression: ObjectCreationExpressionSyntax thrownInNullCoalescing }
}
}
}:
var coalescedParameter = TryGetParameter(maybeParameter);
if (coalescedParameter is null || !IsConstructorApplicable(thrownInNullCoalescing, coalescedParameter))
{
return null;
}

return coalescedParameter;

default:
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

i recommend extracting this out to a helper that just returns the parameter. the nested switch/break/continue is pretty hard to erady (for me at least). tn you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this leans pretty hard on definite assignment to ensure correctness. I'm open to making the change you've described.

Copy link
Member

Choose a reason for hiding this comment

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

i think you could still depend on it. you'd just have two separate methods for the If vs Assign cases.

}

IParameterSymbol? TryGetParameter(ExpressionSyntax maybeParameter)
{
// `(object)x == null` is often used to ensure reference equality is used.
// therefore, we specially unwrap casts when the cast is to `object`.
if (maybeParameter is CastExpressionSyntax { Type: var type, Expression: var operand })
{
if (semanticModel.GetTypeInfo(type).Type?.SpecialType != SpecialType.System_Object)
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
{
return null;
}

maybeParameter = operand;
}

if (semanticModel.GetSymbolInfo(maybeParameter).Symbol is not IParameterSymbol { ContainingSymbol: { } containingSymbol } parameterSymbol || !containingSymbol.Equals(methodSymbol))
{
return null;
}

return parameterSymbol;
}

bool IsConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, IParameterSymbol parameterSymbol)
{
if (exceptionCreation.ArgumentList?.Arguments is not { } arguments)
{
return false;
}

if (arguments.Count == 0)
{
// 'new ArgumentNullException()'
return argumentNullExceptionConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol);
}

if (arguments.Count != 1)
{
return false;
}

// 'new ArgumentNullException(nameof(param))' (or equivalent)
var constantValue = semanticModel.GetConstantValue(arguments[0].Expression, cancellationToken);
if (constantValue.Value is not string constantString || !string.Equals(constantString, parameterSymbol.Name, StringComparison.Ordinal))
{
return false;
}

return argumentNullExceptionStringConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol);
Copy link
Member

Choose a reason for hiding this comment

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

consider reordering. it's a personal nit, but it seems more sensible to first see if you're doing new ArgumntNullException then check the argument.

}
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading