From 746f2e5ab99318c3bef273404d7b21f06e5bbb3a Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 3 Jan 2022 10:37:44 -0800 Subject: [PATCH] Parameter null-checking analyzer and fixer --- .../Analyzers/CSharpAnalyzers.projitems | 1 + .../Analyzers/CSharpAnalyzersResources.resx | 3 + ...ParameterNullCheckingDiagnosticAnalyzer.cs | 242 +++++++++ .../xlf/CSharpAnalyzersResources.cs.xlf | 5 + .../xlf/CSharpAnalyzersResources.de.xlf | 5 + .../xlf/CSharpAnalyzersResources.es.xlf | 5 + .../xlf/CSharpAnalyzersResources.fr.xlf | 5 + .../xlf/CSharpAnalyzersResources.it.xlf | 5 + .../xlf/CSharpAnalyzersResources.ja.xlf | 5 + .../xlf/CSharpAnalyzersResources.ko.xlf | 5 + .../xlf/CSharpAnalyzersResources.pl.xlf | 5 + .../xlf/CSharpAnalyzersResources.pt-BR.xlf | 5 + .../xlf/CSharpAnalyzersResources.ru.xlf | 5 + .../xlf/CSharpAnalyzersResources.tr.xlf | 5 + .../xlf/CSharpAnalyzersResources.zh-Hans.xlf | 5 + .../xlf/CSharpAnalyzersResources.zh-Hant.xlf | 5 + .../CodeFixes/CSharpCodeFixes.projitems | 1 + ...UseParameterNullCheckingCodeFixProvider.cs | 110 ++++ .../Tests/CSharpAnalyzers.UnitTests.projitems | 1 + .../UseParameterNullCheckingTests.cs | 507 ++++++++++++++++++ .../Core/Analyzers/EnforceOnBuildValues.cs | 1 + .../Core/Analyzers/IDEDiagnosticIds.cs | 1 + .../PredefinedCodeFixProviderNames.cs | 1 + src/Compilers/Test/Core/Traits/Traits.cs | 1 + .../AutomationObject.Style.cs | 6 + .../Core/CodeStyle/CodeStyleOptions2.cs | 6 + 26 files changed, 946 insertions(+) create mode 100644 src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs create mode 100644 src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs create mode 100644 src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs diff --git a/src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems b/src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems index 77239030f8743..64c484b1d0853 100644 --- a/src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems +++ b/src/Analyzers/CSharp/Analyzers/CSharpAnalyzers.projitems @@ -103,6 +103,7 @@ + diff --git a/src/Analyzers/CSharp/Analyzers/CSharpAnalyzersResources.resx b/src/Analyzers/CSharp/Analyzers/CSharpAnalyzersResources.resx index 2d3d526546b29..466c18e00e098 100644 --- a/src/Analyzers/CSharp/Analyzers/CSharpAnalyzersResources.resx +++ b/src/Analyzers/CSharp/Analyzers/CSharpAnalyzersResources.resx @@ -340,4 +340,7 @@ Use tuple to swap values + + Use parameter null checking + \ No newline at end of file diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs new file mode 100644 index 0000000000000..d6392f2146f50 --- /dev/null +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -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 + : 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) + { + return; + } + + var compilation = context.Compilation; + var argumentNullException = compilation.GetTypeByMetadataName(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); + if (argumentNullExceptionConstructor is null) + { + return; + } + + var objectType = compilation.GetSpecialType(SpecialType.System_Object); + var referenceEqualsMethod = objectType + .GetMembers(nameof(ReferenceEquals)) + .OfType() + .FirstOrDefault(m => m.DeclaredAccessibility == Accessibility.Public && m.Parameters.Length == 2); + // This seems sufficiently rare that it's not a big deal to just not perform analysis in this case. + 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, + _ => throw ExceptionUtilities.UnexpectedValue(node) + }; + if (block is null) + { + return; + } + + var methodSymbol = semanticModel.GetDeclaredSymbol(node, cancellationToken); + if (methodSymbol is null) + { + return; + } + + foreach (var statement in block.Statements) + { + switch (statement) + { + // if (param == null) { throw new ArgumentNullException(nameof(param)); } + // 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; + 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)) + { + 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)) + { + 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)) + { + continue; + } + + break; + + // this.field = param ?? throw new ArgumentNullException(nameof(param)); + case ExpressionStatementSyntax + { + 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; + } + + context.ReportDiagnostic(DiagnosticHelper.Create(Descriptor, statement.GetLocation(), option.Notification.Severity, additionalLocations: null, properties: null)); + } + + bool areOperandsApplicable(ExpressionSyntax maybeParameter, ExpressionSyntax maybeNullLiteral, [NotNullWhen(true)] out IParameterSymbol? parameter) + { + if (!maybeNullLiteral.IsKind(SyntaxKind.NullLiteralExpression)) + { + parameter = null; + return false; + } + + return isParameter(maybeParameter, out parameter); + } + + bool isParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParameterSymbol? parameter) + { + if (maybeParameter is CastExpressionSyntax { Type: var type, Expression: var operand }) + { + if (semanticModel.GetTypeInfo(type).Type?.SpecialType != SpecialType.System_Object) + { + 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) + { + if (exceptionCreation.ArgumentList is null + || exceptionCreation.ArgumentList.Arguments[0] is not { } argument) + { + return false; + } + var constantValue = semanticModel.GetConstantValue(argument.Expression, cancellationToken); + if (!constantValue.HasValue || constantValue.Value is not string constantString || !string.Equals(constantString, parameterSymbol.Name, StringComparison.Ordinal)) + { + return false; + } + + if (!argumentNullExceptionConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol)) + { + return false; + } + + return true; + } + } + } +} diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.cs.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.cs.xlf index be38ae577fa07..d3bf326ebc1a3 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.cs.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.cs.xlf @@ -262,6 +262,11 @@ Použít new(...) {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Použít porovnávání vzorů diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.de.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.de.xlf index de593ee65610b..3e5a10a5c1b01 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.de.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.de.xlf @@ -262,6 +262,11 @@ "new(...)" verwenden {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Musterabgleich verwenden diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.es.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.es.xlf index ad0f16fb97f56..487cb6690ab0d 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.es.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.es.xlf @@ -262,6 +262,11 @@ Usar "new(...)" {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Usar coincidencia de patrones diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.fr.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.fr.xlf index f9cf8172a54d7..3ea7e6285c43f 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.fr.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.fr.xlf @@ -262,6 +262,11 @@ Utiliser 'new(...)' {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Utiliser les critères spéciaux diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.it.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.it.xlf index 2bced89905496..e5166f9b4e1c5 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.it.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.it.xlf @@ -262,6 +262,11 @@ Usa 'new(...)' {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Usa i criteri di ricerca diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ja.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ja.xlf index f36328909a864..06648a84234eb 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ja.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ja.xlf @@ -262,6 +262,11 @@ 'new(...)' を使用する {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching パターン マッチングを使用します diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ko.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ko.xlf index ce043a110e343..c48fe7cd0888d 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ko.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ko.xlf @@ -262,6 +262,11 @@ 'new(...)' 사용 {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching 패턴 일치 사용 diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.pl.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.pl.xlf index af3991d281da5..92deaef069791 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.pl.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.pl.xlf @@ -262,6 +262,11 @@ Użyj operatora „new(...)” {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Użyj dopasowywania wzorców diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.pt-BR.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.pt-BR.xlf index 67408b2147adf..52318280707e5 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.pt-BR.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.pt-BR.xlf @@ -262,6 +262,11 @@ Usar 'new(...)' {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Usar a correspondência de padrão diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ru.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ru.xlf index 3e87bac5a4c3b..98bde1d848ccc 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ru.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.ru.xlf @@ -262,6 +262,11 @@ Используйте "new(...)". {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Используйте сопоставление шаблонов diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.tr.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.tr.xlf index f3a2d5a24347d..6627d78551333 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.tr.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.tr.xlf @@ -262,6 +262,11 @@ 'new(...)' kullanın {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching Desen eşleştirme kullan diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.zh-Hans.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.zh-Hans.xlf index 7c6167a7e02dc..cf626f8bbc06d 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.zh-Hans.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.zh-Hans.xlf @@ -262,6 +262,11 @@ 使用 "new(...)" {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching 使用模式匹配 diff --git a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.zh-Hant.xlf b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.zh-Hant.xlf index 25266870695f0..c43a1e4f62e47 100644 --- a/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.zh-Hant.xlf +++ b/src/Analyzers/CSharp/Analyzers/xlf/CSharpAnalyzersResources.zh-Hant.xlf @@ -262,6 +262,11 @@ 使用 'new(...)' {Locked="new(...)"} This is a C# construct and should not be localized. + + Use parameter null checking + Use parameter null checking + + Use pattern matching 使用模式比對 diff --git a/src/Analyzers/CSharp/CodeFixes/CSharpCodeFixes.projitems b/src/Analyzers/CSharp/CodeFixes/CSharpCodeFixes.projitems index c58db92c96184..d411389a4b28d 100644 --- a/src/Analyzers/CSharp/CodeFixes/CSharpCodeFixes.projitems +++ b/src/Analyzers/CSharp/CodeFixes/CSharpCodeFixes.projitems @@ -51,6 +51,7 @@ + diff --git a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs new file mode 100644 index 0000000000000..73b26a392a6eb --- /dev/null +++ b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs @@ -0,0 +1,110 @@ +// 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.Collections.Immutable; +using System.Composition; +using System.Diagnostics.CodeAnalysis; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Shared.Extensions; +using Roslyn.Utilities; + +namespace Microsoft.CodeAnalysis.CSharp.UseParameterNullChecking +{ + [ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.UseIsNullCheck), Shared] + internal class CSharpUseParameterNullCheckingCodeFixProvider : SyntaxEditorBasedCodeFixProvider + { + [ImportingConstructor] + [SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")] + public CSharpUseParameterNullCheckingCodeFixProvider() + { + } + + public override ImmutableArray FixableDiagnosticIds + => ImmutableArray.Create(IDEDiagnosticIds.UseParameterNullCheckingId); + + internal sealed override CodeFixCategory CodeFixCategory => CodeFixCategory.CodeStyle; + + public override Task RegisterCodeFixesAsync(CodeFixContext context) + { + var diagnostic = context.Diagnostics[0]; + context.RegisterCodeFix( + new MyCodeAction(CSharpAnalyzersResources.Use_parameter_null_checking, + c => FixAsync(context.Document, diagnostic, c)), + context.Diagnostics); + + return Task.CompletedTask; + } + + protected override async Task FixAllAsync( + Document document, ImmutableArray diagnostics, + SyntaxEditor editor, CancellationToken cancellationToken) + { + var model = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false); + foreach (var diagnostic in diagnostics) + { + var node = diagnostic.Location.FindNode(getInnermostNodeForTie: true, cancellationToken: cancellationToken); + switch (node) + { + case IfStatementSyntax ifStatement: + // if (item == null) throw new ArgumentNullException(nameof(item)); + // if (item is null) throw new ArgumentNullException(nameof(item)); + // if (object.ReferenceEquals(item, null)) throw new ArgumentNullException(nameof(item)); + var (left, right) = ifStatement.Condition switch + { + BinaryExpressionSyntax binary => (binary.Left, binary.Right), + IsPatternExpressionSyntax isPattern => (isPattern.Expression, ((ConstantPatternSyntax)isPattern.Pattern).Expression), + InvocationExpressionSyntax { ArgumentList.Arguments: var arguments } => (arguments[0].Expression, arguments[1].Expression), + _ => throw ExceptionUtilities.UnexpectedValue(ifStatement.Kind()) + }; + + // one of the sides of the binary must be a parameter + var parameterInIf = (IParameterSymbol?)model.GetSymbolInfo(unwrapCast(left), cancellationToken).Symbol + ?? (IParameterSymbol)model.GetSymbolInfo(unwrapCast(right), cancellationToken).Symbol!; + + var parameterSyntax = (ParameterSyntax)parameterInIf.DeclaringSyntaxReferences[0].GetSyntax(cancellationToken); + editor.RemoveNode(ifStatement); + editor.ReplaceNode(parameterSyntax, parameterSyntax.WithExclamationExclamationToken(SyntaxFactory.Token(SyntaxKind.ExclamationExclamationToken))); + break; + case ExpressionStatementSyntax expressionStatement: + // this.item = item ?? throw new ArgumentNullException(nameof(item)); + var assignment = (AssignmentExpressionSyntax)expressionStatement.Expression; + var nullCoalescing = (BinaryExpressionSyntax)assignment.Right; + var parameterReferenceSyntax = nullCoalescing.Left; + + var parameterSymbol = (IParameterSymbol)model.GetSymbolInfo(unwrapCast(parameterReferenceSyntax), cancellationToken).Symbol!; + var parameterDeclarationSyntax = (ParameterSyntax)parameterSymbol.DeclaringSyntaxReferences[0].GetSyntax(cancellationToken); + + editor.ReplaceNode(nullCoalescing, parameterReferenceSyntax.WithAppendedTrailingTrivia(SyntaxFactory.ElasticMarker)); + editor.ReplaceNode(parameterDeclarationSyntax, parameterDeclarationSyntax.WithExclamationExclamationToken(SyntaxFactory.Token(SyntaxKind.ExclamationExclamationToken))); + break; + } + } + + static ExpressionSyntax unwrapCast(ExpressionSyntax expression) + { + if (expression is CastExpressionSyntax { Expression: var operand }) + { + return operand; + } + + return expression; + } + } + + private class MyCodeAction : CustomCodeActions.DocumentChangeAction + { + public MyCodeAction(string title, Func> createChangedDocument) + : base(title, createChangedDocument, equivalenceKey: nameof(CSharpAnalyzersResources.Use_parameter_null_checking)) + { + } + } + } +} diff --git a/src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems b/src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems index f084e0292c272..94a76dcbafdea 100644 --- a/src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems +++ b/src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems @@ -92,6 +92,7 @@ + diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs new file mode 100644 index 0000000000000..40d4737ab0099 --- /dev/null +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -0,0 +1,507 @@ +// 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.Threading.Tasks; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Shared.Extensions; +using Microsoft.CodeAnalysis.CSharp.UseParameterNullChecking; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics; +using Microsoft.CodeAnalysis.Test.Utilities; +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UseParameterNullChecking +{ + public partial class UseParameterNullCheckingTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest + { + private static readonly CSharpParseOptions s_parseOptions = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersionExtensions.CSharpNext); + + public UseParameterNullCheckingTests(ITestOutputHelper logger) + : base(logger) + { + } + + internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace) + => (new CSharpUseParameterNullCheckingDiagnosticAnalyzer(), new CSharpUseParameterNullCheckingCodeFixProvider()); + + [Theory] + [Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + [InlineData("==")] + [InlineData("is")] + public async Task TestNoBraces(string @operator) + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||]s " + @operator + @" null) + throw new ArgumentNullException(nameof(s)); + } +}", +@"using System; + +class C +{ + void M(string s!!) + { + } +}", parseOptions: s_parseOptions); + } + + [Theory] + [Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + [InlineData("==")] + [InlineData("is")] + public async Task TestWithBraces(string @operator) + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||]s " + @operator + @" null) + { + throw new ArgumentNullException(nameof(s)); + } + } +}", +@"using System; + +class C +{ + void M(string s!!) + { + } +}", parseOptions: s_parseOptions); + } + + [Theory] + [Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + [InlineData("==")] + [InlineData("is")] + public async Task TestLocalFunction(string @operator) + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M() + { + local(""""); + void local(string s) + { + if ([||]s " + @operator + @" null) + { + throw new ArgumentNullException(nameof(s)); + } + } + } +}", +@"using System; + +class C +{ + void M() + { + local(""""); + void local(string s!!) + { + } + } +}", parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestEqualitySwapped() + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||]null == (object)s) + throw new ArgumentNullException(nameof(s)); + } +}", +@"using System; + +class C +{ + void M(string s!!) + { + } +}", parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNotEquality() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||](object)s != null) + throw new ArgumentNullException(nameof(s)); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNullCoalescingThrow() + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + private readonly string s; + void M(string s) + { + this.s = [||]s ?? throw new ArgumentNullException(nameof(s)); + } +}", +@"using System; + +class C +{ + private readonly string s; + void M(string s!!) + { + this.s = s; + } +}", parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestReferenceEqualsCheck() + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + private readonly string s; + void M(string s) + { + if ([||]object.ReferenceEquals(s, null)) + { + throw new ArgumentNullException(nameof(s)); + } + } +}", +@"using System; + +class C +{ + private readonly string s; + void M(string s!!) + { + } +}", parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNotCustomReferenceEqualsCheck() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + private readonly string s; + void M(string s) + { + if ([||]ReferenceEquals(s, null)) + { + throw new ArgumentNullException(nameof(s)); + } + } + + bool ReferenceEquals(object o1, object o2) => false; +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNotEqualitySwapped() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||]null != (object)s) + throw new ArgumentNullException(nameof(s)); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestMissingPreCSharp11() + { + await TestMissingAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||](object)s == null) + throw new ArgumentNullException(nameof(s)); + } +}", parameters: new TestParameters(parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp10))); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestOnlyForObjectCast() + { + await TestMissingAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||](string)s == null) + throw new ArgumentNullException(nameof(s)); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestFixAll1() + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s1, string s2) + { + if ({|FixAllInDocument:|}(object)s1 == null) + throw new ArgumentNullException(nameof(s1)); + + if (null == (object)s2) + throw new ArgumentNullException(nameof(s2)); + } +}", +@"using System; + +class C +{ + void M(string s1!!, string s2!!) + { + } +}", parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestFixAllNested1() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s2) + { + if ({|FixAllInDocument:|}(object)((object)s2 == null) == null)) + throw new ArgumentNullException(nameof(s2)); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestConstrainedTypeParameter() + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(T s) where T : class + { + if ([||](object)s == null) + throw new ArgumentNullException(nameof(s)); + } +}", +@"using System; + +class C +{ + void M(T s!!) where T : class + { + } +}", parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestUnconstrainedTypeParameter() + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(T s) + { + if ([||](object)s == null) + throw new ArgumentNullException(nameof(s)); + } +}", +@"using System; + +class C +{ + void M(T s!!) + { + } +}", parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestComplexExpr() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string[] s) + { + if ([||](object)s[0] == null) + throw new ArgumentNullException(nameof(s)); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestComplexExpr2() + { + await TestMissingInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s1, string s2) + { + if ([||]s1 == null || s2 == null) + throw new ArgumentNullException(nameof(s)); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNotOnDefault() + { + await TestMissingAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||](object)s == default) + throw new ArgumentNullException(nameof(s)); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestWithStringExceptionArgument() + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||]s == null) + throw new ArgumentNullException(""s""); + } +}", +@"using System; + +class C +{ + void M(string s!!) + { + } +}", + parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestWithStringExceptionArgument2() + { + await TestInRegularAndScriptAsync( +@"using System; + +class C +{ + void M(string HelloWorld) + { + if ([||]HelloWorld == null) + throw new ArgumentNullException(""Hello"" + ""World""); + } +}", +@"using System; + +class C +{ + void M(string HelloWorld!!) + { + } +}", +parseOptions: s_parseOptions); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNotWithUnexpectedExceptionArgument() + { + await TestMissingAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||]s == null) + throw new ArgumentNullException(""banana""); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNotWithExceptionNoArguments() + { + await TestMissingAsync( +@"using System; + +class C +{ + void M(string s) + { + if ([||]s == null) + throw new ArgumentNullException(); + } +}", parameters: new TestParameters(parseOptions: s_parseOptions)); + } + } +} diff --git a/src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs b/src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs index e1026b4c559a1..03b65a64deaef 100644 --- a/src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs +++ b/src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs @@ -87,6 +87,7 @@ internal static class EnforceOnBuildValues public const EnforceOnBuild PopulateSwitchStatement = /*IDE0010*/ EnforceOnBuild.WhenExplicitlyEnabled; public const EnforceOnBuild UseInferredMemberName = /*IDE0037*/ EnforceOnBuild.WhenExplicitlyEnabled; public const EnforceOnBuild UseIsNullCheck = /*IDE0041*/ EnforceOnBuild.WhenExplicitlyEnabled; + public const EnforceOnBuild UseParameterNullChecking = /*IDE0181*/ EnforceOnBuild.WhenExplicitlyEnabled; public const EnforceOnBuild AddRequiredParentheses = /*IDE0048*/ EnforceOnBuild.WhenExplicitlyEnabled; public const EnforceOnBuild ExpressionValueIsUnused = /*IDE0058*/ EnforceOnBuild.WhenExplicitlyEnabled; public const EnforceOnBuild MakeStructFieldsWritable = /*IDE0064*/ EnforceOnBuild.WhenExplicitlyEnabled; diff --git a/src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs b/src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs index 0b02a866c04ed..1b576c4c8be03 100644 --- a/src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs +++ b/src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs @@ -166,6 +166,7 @@ internal static class IDEDiagnosticIds public const string SimplifyPropertyPatternDiagnosticId = "IDE0170"; public const string UseTupleSwapDiagnosticId = "IDE0180"; + public const string UseParameterNullCheckingId = "IDE0181"; // Analyzer error Ids public const string AnalyzerChangedId = "IDE1001"; diff --git a/src/Analyzers/Core/CodeFixes/PredefinedCodeFixProviderNames.cs b/src/Analyzers/Core/CodeFixes/PredefinedCodeFixProviderNames.cs index 68ee4523ee54a..e84bd13bf549f 100644 --- a/src/Analyzers/Core/CodeFixes/PredefinedCodeFixProviderNames.cs +++ b/src/Analyzers/Core/CodeFixes/PredefinedCodeFixProviderNames.cs @@ -133,6 +133,7 @@ internal static class PredefinedCodeFixProviderNames public const string UseInterpolatedVerbatimString = nameof(UseInterpolatedVerbatimString); public const string UseIsNotExpression = nameof(UseIsNotExpression); public const string UseIsNullCheck = nameof(UseIsNullCheck); + public const string UseParameterNullChecking = nameof(UseParameterNullChecking); public const string UseIsNullCheckForCastAndEqualityOperator = nameof(UseIsNullCheckForCastAndEqualityOperator); public const string UseIsNullCheckForReferenceEquals = nameof(UseIsNullCheckForReferenceEquals); public const string UseLocalFunction = nameof(UseLocalFunction); diff --git a/src/Compilers/Test/Core/Traits/Traits.cs b/src/Compilers/Test/Core/Traits/Traits.cs index 66a26418fe87d..cb0992ea28c49 100644 --- a/src/Compilers/Test/Core/Traits/Traits.cs +++ b/src/Compilers/Test/Core/Traits/Traits.cs @@ -194,6 +194,7 @@ public static class Features public const string CodeActionsUseInterpolatedVerbatimString = "CodeActions.UseInterpolatedVerbatimString"; public const string CodeActionsUseIsNotExpression = "CodeActions.UseIsNotExpression"; public const string CodeActionsUseIsNullCheck = "CodeActions.UseIsNullCheck"; + public const string CodeActionsUseParameterNullChecking = "CodeActions.UseParameterNullChecking"; public const string CodeActionsUseLocalFunction = "CodeActions.UseLocalFunction"; public const string CodeActionsUseNamedArguments = "CodeActions.UseNamedArguments"; public const string CodeActionsUseNotPattern = "CodeActions.UseNotPattern"; diff --git a/src/VisualStudio/CSharp/Impl/Options/AutomationObject/AutomationObject.Style.cs b/src/VisualStudio/CSharp/Impl/Options/AutomationObject/AutomationObject.Style.cs index e8f977663d36b..041f6a95da0d7 100644 --- a/src/VisualStudio/CSharp/Impl/Options/AutomationObject/AutomationObject.Style.cs +++ b/src/VisualStudio/CSharp/Impl/Options/AutomationObject/AutomationObject.Style.cs @@ -237,6 +237,12 @@ public string Style_PreferIsNullCheckOverReferenceEqualityMethod set { SetXmlOption(CodeStyleOptions2.PreferIsNullCheckOverReferenceEqualityMethod, value); } } + public string Style_PreferParameterNullChecking + { + get { return GetXmlOption(CodeStyleOptions2.PreferParameterNullChecking); } + set { SetXmlOption(CodeStyleOptions2.PreferParameterNullChecking, value); } + } + public string Style_PreferNullCheckOverTypeCheck { get { return GetXmlOption(CSharpCodeStyleOptions.PreferNullCheckOverTypeCheck); } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs index 5970fc48fd2c3..ab188eacddebf 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs @@ -215,6 +215,12 @@ private static PerLanguageOption2> CreateQualifyAccessOpt "dotnet_style_prefer_is_null_check_over_reference_equality_method", $"TextEditor.%LANGUAGE%.Specific.{nameof(PreferIsNullCheckOverReferenceEqualityMethod)}"); + internal static readonly PerLanguageOption2> PreferParameterNullChecking = CreateOption( + CodeStyleOptionGroups.ExpressionLevelPreferences, nameof(PreferParameterNullChecking), + defaultValue: TrueWithSuggestionEnforcement, + "dotnet_style_prefer_parameter_null_checking", + $"TextEditor.%LANGUAGE%.Specific.{nameof(PreferParameterNullChecking)}"); + internal static readonly PerLanguageOption2> PreferConditionalExpressionOverAssignment = CreateOption( CodeStyleOptionGroups.ExpressionLevelPreferences, nameof(PreferConditionalExpressionOverAssignment), defaultValue: TrueWithSilentEnforcement,