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 1 commit
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,242 @@
// 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.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis.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 class CSharpUseParameterNullCheckingDiagnosticAnalyzer
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
: AbstractBuiltInCodeStyleDiagnosticAnalyzer
{
private const string ArgumentNullExceptionName = $"{nameof(System)}.{nameof(ArgumentNullException)}";

public CSharpUseParameterNullCheckingDiagnosticAnalyzer()
: base(IDEDiagnosticIds.UseParameterNullCheckingId,
EnforceOnBuildValues.UseParameterNullChecking,
CodeStyleOptions2.PreferParameterNullChecking,
CSharpAnalyzersResources.Use_parameter_null_checking,
new LocalizableResourceString(nameof(AnalyzersResources.Null_check_can_be_simplified), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)))
{
}

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

protected override void InitializeWorker(AnalysisContext context)
=> context.RegisterCompilationStartAction(context =>
{
if (((CSharpCompilation)context.Compilation).LanguageVersion < LanguageVersionExtensions.CSharpNext)
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
{
return;
}

var compilation = context.Compilation;
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
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;
}

var argumentNullExceptionConstructor = argumentNullException.InstanceConstructors.FirstOrDefault(m =>
m.DeclaredAccessibility == Accessibility.Public
&& m.Parameters.Length == 1
&& m.Parameters[0].Type.SpecialType == SpecialType.System_String);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
if (argumentNullExceptionConstructor is null)
{
return;
}

var objectType = compilation.GetSpecialType(SpecialType.System_Object);
var referenceEqualsMethod = objectType
.GetMembers(nameof(ReferenceEquals))
.OfType<IMethodSymbol>()
.FirstOrDefault(m => m.DeclaredAccessibility == Accessibility.Public && m.Parameters.Length == 2);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
// This seems sufficiently rare that it's not a big deal to just not perform analysis in this case.
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
if (referenceEqualsMethod is null)
{
return;
}

context.RegisterSyntaxNodeAction(context => AnalyzeSyntax(context, argumentNullExceptionConstructor, referenceEqualsMethod), SyntaxKind.MethodDeclaration, SyntaxKind.LocalFunctionStatement);
});

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

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

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

var node = context.Node;
var block = node switch
{
MethodDeclarationSyntax methodDecl => methodDecl.Body,
LocalFunctionStatementSyntax localFunctionStatement => localFunctionStatement.Body,
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
_ => throw ExceptionUtilities.UnexpectedValue(node)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
};
if (block is null)
{
return;
}

var methodSymbol = semanticModel.GetDeclaredSymbol(node, cancellationToken);
if (methodSymbol is null)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
return;
}

foreach (var statement in block.Statements)
{
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 }:
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 } }:
if (!referenceEqualsMethod.Equals(semanticModel.GetSymbolInfo(receiver, cancellationToken).Symbol))
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}

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

default:
continue;
}

IParameterSymbol? parameter;
if (!areOperandsApplicable(left, right, out parameter)
&& !areOperandsApplicable(right, left, out parameter))
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}

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

if (throwStatement is null
|| throwStatement.Expression is not ObjectCreationExpressionSyntax thrownInIf
|| !isConstructorApplicable(thrownInIf, parameter))
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}

break;

// 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 }
}
}
}:
if (!isParameter(maybeParameter, out var parameterInNullCoalescing) || !isConstructorApplicable(thrownInNullCoalescing, parameterInNullCoalescing))
{
continue;
}

break;

default:
continue;
}
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.


context.ReportDiagnostic(DiagnosticHelper.Create(Descriptor, statement.GetLocation(), option.Notification.Severity, additionalLocations: null, properties: null));
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
}

bool areOperandsApplicable(ExpressionSyntax maybeParameter, ExpressionSyntax maybeNullLiteral, [NotNullWhen(true)] out IParameterSymbol? parameter)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
if (!maybeNullLiteral.IsKind(SyntaxKind.NullLiteralExpression))
{
parameter = null;
return false;
}

return isParameter(maybeParameter, out parameter);
}

bool isParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParameterSymbol? parameter)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
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
{
parameter = null;
return false;
}

maybeParameter = operand;
}

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

parameter = parameterSymbol;
return true;
}

bool isConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, IParameterSymbol parameterSymbol)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
if (exceptionCreation.ArgumentList is null
|| exceptionCreation.ArgumentList.Arguments[0] is not { } argument)
{
return false;
}
var constantValue = semanticModel.GetConstantValue(argument.Expression, cancellationToken);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
if (!constantValue.HasValue || constantValue.Value is not string constantString || !string.Equals(constantString, parameterSymbol.Name, StringComparison.Ordinal))
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
return false;
}

if (!argumentNullExceptionConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol))
{
return false;
}

return true;
}
}
}
}

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.

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