From 96f5a43cf07aa88166a2253703e72aa3ab6b99f1 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 4 Dec 2021 12:27:59 +0100 Subject: [PATCH 01/14] Improve analyzer ValidateArgumentsCorrectly (RCS1227) --- ...lidateArgumentsCorrectlyCodeFixProvider.cs | 33 ++++-- src/Analyzers/Analyzers.xml | 14 ++- .../ValidateArgumentsCorrectlyAnalyzer.cs | 101 ++++++++++++++++-- .../RCS1227ValidateArgumentsCorrectlyTests.cs | 64 +++++++++++ 4 files changed, 192 insertions(+), 20 deletions(-) diff --git a/src/Analyzers.CodeFixes/CSharp/CodeFixes/ValidateArgumentsCorrectlyCodeFixProvider.cs b/src/Analyzers.CodeFixes/CSharp/CodeFixes/ValidateArgumentsCorrectlyCodeFixProvider.cs index 1146c922ad..c8fc126b55 100644 --- a/src/Analyzers.CodeFixes/CSharp/CodeFixes/ValidateArgumentsCorrectlyCodeFixProvider.cs +++ b/src/Analyzers.CodeFixes/CSharp/CodeFixes/ValidateArgumentsCorrectlyCodeFixProvider.cs @@ -40,16 +40,29 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) return; Diagnostic diagnostic = context.Diagnostics[0]; + Document document = context.Document; - CodeAction codeAction = CodeAction.Create( - "Validate arguments correctly", - ct => RefactorAsync(context.Document, statement, ct), - GetEquivalenceKey(diagnostic)); + if (context.Span == statement.Span) + { + CodeAction codeAction = CodeAction.Create( + "Remove null check", + ct => RemoveUnnecessaryNullCheckAsync(document, (IfStatementSyntax)statement, ct), + GetEquivalenceKey(diagnostic)); + + context.RegisterCodeFix(codeAction, diagnostic); + } + else + { + CodeAction codeAction = CodeAction.Create( + "Validate arguments correctly", + ct => AddLocalFunctionWithIteratorAsync(document, statement, ct), + GetEquivalenceKey(diagnostic)); - context.RegisterCodeFix(codeAction, diagnostic); + context.RegisterCodeFix(codeAction, diagnostic); + } } - private static async Task RefactorAsync( + private static async Task AddLocalFunctionWithIteratorAsync( Document document, StatementSyntax statement, CancellationToken cancellationToken) @@ -104,5 +117,13 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) return await document.ReplaceStatementsAsync(statementsInfo, newStatements, cancellationToken).ConfigureAwait(false); } } + + private static Task RemoveUnnecessaryNullCheckAsync( + Document document, + IfStatementSyntax ifStatement, + CancellationToken cancellationToken) + { + return document.RemoveStatementAsync(ifStatement, cancellationToken); + } } } diff --git a/src/Analyzers/Analyzers.xml b/src/Analyzers/Analyzers.xml index e01a3b87b0..3ac048719c 100644 --- a/src/Analyzers/Analyzers.xml +++ b/src/Analyzers/Analyzers.xml @@ -4831,12 +4831,16 @@ class Foo Design Info true - - An iterator method (a method that contains `yield`) will not validate arguments until the caller begins to enumerate the result items. + **Case #1** - To ensure that arguments are validated immediately (when the method is called), move - the iterator to a separate method (local function). - +An iterator method (a method that contains `yield`) will not validate arguments until the caller begins to enumerate the result items. + +To ensure that arguments are validated immediately (when the method is called), move +the iterator to a separate method (local function). + +**Case #2** + +Remove condition where a parameter that has default value equal to `null` is checked for null and then `ArgumentNullException` is thrown. Foo(IEnumerable items) diff --git a/src/Analyzers/CSharp/Analysis/ValidateArgumentsCorrectlyAnalyzer.cs b/src/Analyzers/CSharp/Analysis/ValidateArgumentsCorrectlyAnalyzer.cs index 70262ff635..95c56efd82 100644 --- a/src/Analyzers/CSharp/Analysis/ValidateArgumentsCorrectlyAnalyzer.cs +++ b/src/Analyzers/CSharp/Analysis/ValidateArgumentsCorrectlyAnalyzer.cs @@ -1,12 +1,12 @@ // Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; using System.Collections.Immutable; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Text; +using Roslynator.CSharp.Syntax; using Roslynator.CSharp.SyntaxWalkers; namespace Roslynator.CSharp.Analysis @@ -32,33 +32,49 @@ public override void Initialize(AnalysisContext context) base.Initialize(context); context.RegisterSyntaxNodeAction(f => AnalyzeMethodDeclaration(f), SyntaxKind.MethodDeclaration); + context.RegisterSyntaxNodeAction(f => AnalyzeConstructorDeclaration(f), SyntaxKind.ConstructorDeclaration); } private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context) { var methodDeclaration = (MethodDeclarationSyntax)context.Node; - BlockSyntax body = methodDeclaration.Body; + AnalyzeParameterList(context, methodDeclaration.ParameterList, methodDeclaration.Body); + } + + private static void AnalyzeConstructorDeclaration(SyntaxNodeAnalysisContext context) + { + var constructorDeclaration = (ConstructorDeclarationSyntax)context.Node; + + AnalyzeParameterList(context, constructorDeclaration.ParameterList, constructorDeclaration.Body); + } + private static void AnalyzeParameterList(SyntaxNodeAnalysisContext context, ParameterListSyntax parameterList, BlockSyntax body) + { if (body == null) return; - ParameterListSyntax parameterList = methodDeclaration.ParameterList; - if (parameterList == null) return; - if (!parameterList.Parameters.Any()) + SeparatedSyntaxList parameters = parameterList.Parameters; + + if (!parameters.Any()) return; SyntaxList statements = body.Statements; int statementCount = statements.Count; + if (statementCount == 0) + return; + + AnalyzeUnnecessaryNullCheck(context, parameters, statements); + int index = -1; for (int i = 0; i < statementCount; i++) { - if (IsNullCheck(statements[i])) + if (IsConditionWithThrow(statements[i])) { index++; } @@ -101,10 +117,77 @@ private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context) Location.Create(body.SyntaxTree, new TextSpan(statements[index + 1].SpanStart, 0))); } - private static bool IsNullCheck(StatementSyntax statement) + private static void AnalyzeUnnecessaryNullCheck( + SyntaxNodeAnalysisContext context, + SeparatedSyntaxList parameters, + SyntaxList statements) + { + int lastIndex = -1; + + foreach (ParameterSyntax parameter in parameters) + { + if (parameter.IsParams()) + break; + + lastIndex++; + } + + if (lastIndex == -1) + return; + + foreach (StatementSyntax statement in statements) + { + if (statement is IfStatementSyntax ifStatement + && ifStatement.IsSimpleIf() + && ifStatement.SingleNonBlockStatementOrDefault() is ThrowStatementSyntax throwStatement) + { + NullCheckExpressionInfo nullCheck = SyntaxInfo.NullCheckExpressionInfo(ifStatement.Condition, context.SemanticModel, NullCheckStyles.CheckingNull); + + if (nullCheck.Success) + { + ParameterSyntax parameter = GetParameter(nullCheck.Expression); + + if (parameter?.Default?.Value.IsKind( + SyntaxKind.NullLiteralExpression, + SyntaxKind.DefaultLiteralExpression, + SyntaxKind.DefaultExpression) == true) + { + ITypeSymbol exceptionSymbol = context.SemanticModel.GetTypeSymbol(throwStatement.Expression, context.CancellationToken); + + if (exceptionSymbol.HasMetadataName(MetadataNames.System_ArgumentNullException)) + { + context.ReportDiagnostic(DiagnosticRules.ValidateArgumentsCorrectly, ifStatement); + } + } + } + } + else + { + break; + } + } + + ParameterSyntax GetParameter(ExpressionSyntax expression) + { + if (expression is IdentifierNameSyntax identifierName) + { + string identifierText = identifierName.Identifier.ValueText; + for (int i = 0; i <= lastIndex; i++) + { + if (parameters[i].Identifier.ValueText == identifierText) + return parameters[i]; + } + } + + return null; + } + } + + private static bool IsConditionWithThrow(StatementSyntax statement) { - return statement.IsKind(SyntaxKind.IfStatement) - && ((IfStatementSyntax)statement).SingleNonBlockStatementOrDefault().IsKind(SyntaxKind.ThrowStatement); + return statement is IfStatementSyntax ifStatement + && ifStatement.IsSimpleIf() + && ifStatement.SingleNonBlockStatementOrDefault().IsKind(SyntaxKind.ThrowStatement); } } } diff --git a/src/Tests/Analyzers.Tests/RCS1227ValidateArgumentsCorrectlyTests.cs b/src/Tests/Analyzers.Tests/RCS1227ValidateArgumentsCorrectlyTests.cs index 5fd63b2413..510b05e19c 100644 --- a/src/Tests/Analyzers.Tests/RCS1227ValidateArgumentsCorrectlyTests.cs +++ b/src/Tests/Analyzers.Tests/RCS1227ValidateArgumentsCorrectlyTests.cs @@ -153,6 +153,70 @@ async IAsyncEnumerable M2() "); } + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.ValidateArgumentsCorrectly)] + public async Task Test_Method_UnnecessaryNullCheckOfOptionalParameter() + { + await VerifyDiagnosticAndFixAsync(@" +using System; + +class C +{ + void M(string p = null) + { + [|if (p == null) + { + throw new ArgumentNullException(nameof(p)); + }|] + + M(p); + } +} +", @" +using System; + +class C +{ + void M(string p = null) + { + + M(p); + } +} +"); + } + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.ValidateArgumentsCorrectly)] + public async Task Test_Constructor_UnnecessaryNullCheckOfOptionalParameter() + { + await VerifyDiagnosticAndFixAsync(@" +using System; + +class C +{ + C(string p = null) + { + [|if (p == null) + { + throw new ArgumentNullException(nameof(p)); + }|] + + string x = null; + } +} +", @" +using System; + +class C +{ + C(string p = null) + { + + string x = null; + } +} +"); + } + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.ValidateArgumentsCorrectly)] public async Task TestNoDiagnostic_NoStatement() { From 81ddc6fbdeef60221826fbbe273f156f9cf8f5e2 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 18 Dec 2021 22:46:11 +0100 Subject: [PATCH 02/14] x --- .../CodeFixes/IfStatementCodeFixProvider.cs | 13 +- ...lidateArgumentsCorrectlyCodeFixProvider.cs | 30 +--- src/Analyzers/Analyzers.InvalidNullCheck.xml | 11 ++ .../Analysis/InvalidNullCheckAnalyzer.cs | 150 +++++++++++++++++ .../ValidateArgumentsCorrectlyAnalyzer.cs | 68 -------- .../CSharp/DiagnosticIdentifiers.Generated.cs | 1 + .../CSharp/DiagnosticRules.Generated.cs | 12 ++ .../RCS1227ValidateArgumentsCorrectlyTests.cs | 64 -------- .../RCS1251InvalidNullCheckTests.cs | 151 ++++++++++++++++++ 9 files changed, 342 insertions(+), 158 deletions(-) create mode 100644 src/Analyzers/Analyzers.InvalidNullCheck.xml create mode 100644 src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs create mode 100644 src/Tests/Analyzers.Tests/RCS1251InvalidNullCheckTests.cs diff --git a/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs b/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs index e47b3d3c0d..cc5c6c0581 100644 --- a/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs +++ b/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs @@ -33,7 +33,8 @@ public override ImmutableArray FixableDiagnosticIds DiagnosticIdentifiers.ConvertIfToReturnStatement, DiagnosticIdentifiers.ConvertIfToAssignment, DiagnosticIdentifiers.ReduceIfNesting, - DiagnosticIdentifiers.UseExceptionFilter); + DiagnosticIdentifiers.UseExceptionFilter, + DiagnosticIdentifiers.InvalidNullCheck); } } @@ -110,6 +111,16 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) }, GetEquivalenceKey(diagnostic)); + context.RegisterCodeFix(codeAction, diagnostic); + break; + } + case DiagnosticIdentifiers.InvalidNullCheck: + { + CodeAction codeAction = CodeAction.Create( + "Remove null check", + ct => context.Document.RemoveStatementAsync(ifStatement, ct), + GetEquivalenceKey(diagnostic)); + context.RegisterCodeFix(codeAction, diagnostic); break; } diff --git a/src/Analyzers.CodeFixes/CSharp/CodeFixes/ValidateArgumentsCorrectlyCodeFixProvider.cs b/src/Analyzers.CodeFixes/CSharp/CodeFixes/ValidateArgumentsCorrectlyCodeFixProvider.cs index c8fc126b55..7af3a79820 100644 --- a/src/Analyzers.CodeFixes/CSharp/CodeFixes/ValidateArgumentsCorrectlyCodeFixProvider.cs +++ b/src/Analyzers.CodeFixes/CSharp/CodeFixes/ValidateArgumentsCorrectlyCodeFixProvider.cs @@ -42,24 +42,12 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) Diagnostic diagnostic = context.Diagnostics[0]; Document document = context.Document; - if (context.Span == statement.Span) - { - CodeAction codeAction = CodeAction.Create( - "Remove null check", - ct => RemoveUnnecessaryNullCheckAsync(document, (IfStatementSyntax)statement, ct), - GetEquivalenceKey(diagnostic)); - - context.RegisterCodeFix(codeAction, diagnostic); - } - else - { - CodeAction codeAction = CodeAction.Create( - "Validate arguments correctly", - ct => AddLocalFunctionWithIteratorAsync(document, statement, ct), - GetEquivalenceKey(diagnostic)); + CodeAction codeAction = CodeAction.Create( + "Validate arguments correctly", + ct => AddLocalFunctionWithIteratorAsync(document, statement, ct), + GetEquivalenceKey(diagnostic)); - context.RegisterCodeFix(codeAction, diagnostic); - } + context.RegisterCodeFix(codeAction, diagnostic); } private static async Task AddLocalFunctionWithIteratorAsync( @@ -117,13 +105,5 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) return await document.ReplaceStatementsAsync(statementsInfo, newStatements, cancellationToken).ConfigureAwait(false); } } - - private static Task RemoveUnnecessaryNullCheckAsync( - Document document, - IfStatementSyntax ifStatement, - CancellationToken cancellationToken) - { - return document.RemoveStatementAsync(ifStatement, cancellationToken); - } } } diff --git a/src/Analyzers/Analyzers.InvalidNullCheck.xml b/src/Analyzers/Analyzers.InvalidNullCheck.xml new file mode 100644 index 0000000000..9a99677585 --- /dev/null +++ b/src/Analyzers/Analyzers.InvalidNullCheck.xml @@ -0,0 +1,11 @@ + + + + RCS1251 + Invalid null check. + Roslynator + Info + true + + + \ No newline at end of file diff --git a/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs b/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs new file mode 100644 index 0000000000..2e7f57cb5a --- /dev/null +++ b/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs @@ -0,0 +1,150 @@ +// Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Roslynator.CSharp.Syntax; + +namespace Roslynator.CSharp.Analysis +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class InvalidNullCheckAnalyzer : BaseDiagnosticAnalyzer + { + private static ImmutableArray _supportedDiagnostics; + + public override ImmutableArray SupportedDiagnostics + { + get + { + if (_supportedDiagnostics.IsDefault) + Immutable.InterlockedInitialize(ref _supportedDiagnostics, DiagnosticRules.InvalidNullCheck); + + return _supportedDiagnostics; + } + } + + public override void Initialize(AnalysisContext context) + { + base.Initialize(context); + + context.RegisterSyntaxNodeAction(f => AnalyzeMethodDeclaration(f), SyntaxKind.MethodDeclaration); + context.RegisterSyntaxNodeAction(f => AnalyzeConstructorDeclaration(f), SyntaxKind.ConstructorDeclaration); + } + + private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context) + { + var methodDeclaration = (MethodDeclarationSyntax)context.Node; + + AnalyzeParameterList(context, methodDeclaration.ParameterList, methodDeclaration.Body); + } + + private static void AnalyzeConstructorDeclaration(SyntaxNodeAnalysisContext context) + { + var constructorDeclaration = (ConstructorDeclarationSyntax)context.Node; + + AnalyzeParameterList(context, constructorDeclaration.ParameterList, constructorDeclaration.Body); + } + + private static void AnalyzeParameterList(SyntaxNodeAnalysisContext context, ParameterListSyntax parameterList, BlockSyntax body) + { + if (body == null) + return; + + if (parameterList == null) + return; + + SeparatedSyntaxList parameters = parameterList.Parameters; + + if (!parameters.Any()) + return; + + SyntaxList statements = body.Statements; + + int statementCount = statements.Count; + + if (statementCount == 0) + return; + + int lastIndex = -1; + + foreach (ParameterSyntax parameter in parameters) + { + if (parameter.IsParams()) + break; + + lastIndex++; + } + + if (lastIndex == -1) + return; + + foreach (StatementSyntax statement in statements) + { + if (statement is not IfStatementSyntax ifStatement + || !ifStatement.IsSimpleIf() + || ifStatement.SingleNonBlockStatementOrDefault() is not ThrowStatementSyntax throwStatement) + { + break; + } + + if (throwStatement.Expression.IsKind(SyntaxKind.ObjectCreationExpression) + && !ifStatement.SpanContainsDirectives()) + { + NullCheckExpressionInfo nullCheck = SyntaxInfo.NullCheckExpressionInfo(ifStatement.Condition, context.SemanticModel, NullCheckStyles.CheckingNull); + + if (nullCheck.Success) + { + ParameterSyntax parameter = GetParameter(nullCheck.Expression); + + if (parameter?.Default?.Value.IsKind( + SyntaxKind.NullLiteralExpression, + SyntaxKind.DefaultLiteralExpression, + SyntaxKind.DefaultExpression) == true + || IsNullableReferenceType(context, parameter)) + { + ITypeSymbol exceptionSymbol = context.SemanticModel.GetTypeSymbol(throwStatement.Expression, context.CancellationToken); + + if (exceptionSymbol.HasMetadataName(MetadataNames.System_ArgumentNullException)) + context.ReportDiagnostic(DiagnosticRules.InvalidNullCheck, ifStatement); + } + } + } + } + + bool IsNullableReferenceType(SyntaxNodeAnalysisContext context, ParameterSyntax parameter) + { + TypeSyntax type = parameter.Type; + + if (type.IsKind(SyntaxKind.NullableType)) + { + ITypeSymbol typeSymbol = context.SemanticModel.GetTypeSymbol(type, context.CancellationToken); + + if (!typeSymbol.IsKind(SymbolKind.ErrorType) + && typeSymbol.IsReferenceType) + { + return true; + } + } + + return false; + } + + ParameterSyntax GetParameter(ExpressionSyntax expression) + { + if (expression is IdentifierNameSyntax identifierName) + { + string identifierText = identifierName.Identifier.ValueText; + for (int i = 0; i <= lastIndex; i++) + { + if (parameters[i].Identifier.ValueText == identifierText) + return parameters[i]; + } + } + + return null; + } + } + } +} diff --git a/src/Analyzers/CSharp/Analysis/ValidateArgumentsCorrectlyAnalyzer.cs b/src/Analyzers/CSharp/Analysis/ValidateArgumentsCorrectlyAnalyzer.cs index 95c56efd82..9c0df2c072 100644 --- a/src/Analyzers/CSharp/Analysis/ValidateArgumentsCorrectlyAnalyzer.cs +++ b/src/Analyzers/CSharp/Analysis/ValidateArgumentsCorrectlyAnalyzer.cs @@ -69,8 +69,6 @@ private static void AnalyzeParameterList(SyntaxNodeAnalysisContext context, Para if (statementCount == 0) return; - AnalyzeUnnecessaryNullCheck(context, parameters, statements); - int index = -1; for (int i = 0; i < statementCount; i++) { @@ -117,72 +115,6 @@ private static void AnalyzeParameterList(SyntaxNodeAnalysisContext context, Para Location.Create(body.SyntaxTree, new TextSpan(statements[index + 1].SpanStart, 0))); } - private static void AnalyzeUnnecessaryNullCheck( - SyntaxNodeAnalysisContext context, - SeparatedSyntaxList parameters, - SyntaxList statements) - { - int lastIndex = -1; - - foreach (ParameterSyntax parameter in parameters) - { - if (parameter.IsParams()) - break; - - lastIndex++; - } - - if (lastIndex == -1) - return; - - foreach (StatementSyntax statement in statements) - { - if (statement is IfStatementSyntax ifStatement - && ifStatement.IsSimpleIf() - && ifStatement.SingleNonBlockStatementOrDefault() is ThrowStatementSyntax throwStatement) - { - NullCheckExpressionInfo nullCheck = SyntaxInfo.NullCheckExpressionInfo(ifStatement.Condition, context.SemanticModel, NullCheckStyles.CheckingNull); - - if (nullCheck.Success) - { - ParameterSyntax parameter = GetParameter(nullCheck.Expression); - - if (parameter?.Default?.Value.IsKind( - SyntaxKind.NullLiteralExpression, - SyntaxKind.DefaultLiteralExpression, - SyntaxKind.DefaultExpression) == true) - { - ITypeSymbol exceptionSymbol = context.SemanticModel.GetTypeSymbol(throwStatement.Expression, context.CancellationToken); - - if (exceptionSymbol.HasMetadataName(MetadataNames.System_ArgumentNullException)) - { - context.ReportDiagnostic(DiagnosticRules.ValidateArgumentsCorrectly, ifStatement); - } - } - } - } - else - { - break; - } - } - - ParameterSyntax GetParameter(ExpressionSyntax expression) - { - if (expression is IdentifierNameSyntax identifierName) - { - string identifierText = identifierName.Identifier.ValueText; - for (int i = 0; i <= lastIndex; i++) - { - if (parameters[i].Identifier.ValueText == identifierText) - return parameters[i]; - } - } - - return null; - } - } - private static bool IsConditionWithThrow(StatementSyntax statement) { return statement is IfStatementSyntax ifStatement diff --git a/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs b/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs index 759cc3bcf3..93c707cb57 100644 --- a/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs +++ b/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs @@ -206,5 +206,6 @@ public static partial class DiagnosticIdentifiers public const string UseElementAccess = "RCS1246"; public const string FixDocumentationCommentTag = "RCS1247"; public const string UsePatternMatchingToCheckForNullOrViceVersa = "RCS1248"; + public const string InvalidNullCheck = "RCS1251"; } } \ No newline at end of file diff --git a/src/Analyzers/CSharp/DiagnosticRules.Generated.cs b/src/Analyzers/CSharp/DiagnosticRules.Generated.cs index a09176e4dc..9045595392 100644 --- a/src/Analyzers/CSharp/DiagnosticRules.Generated.cs +++ b/src/Analyzers/CSharp/DiagnosticRules.Generated.cs @@ -2441,6 +2441,18 @@ public static partial class DiagnosticRules helpLinkUri: DiagnosticIdentifiers.UsePatternMatchingToCheckForNullOrViceVersa, customTags: Array.Empty()); + /// RCS1251 + public static readonly DiagnosticDescriptor InvalidNullCheck = DiagnosticDescriptorFactory.Create( + id: DiagnosticIdentifiers.InvalidNullCheck, + title: "Invalid null check.", + messageFormat: "Invalid null check.", + category: DiagnosticCategories.Roslynator, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: null, + helpLinkUri: DiagnosticIdentifiers.InvalidNullCheck, + customTags: Array.Empty()); + public static partial class ReportOnly { /// RCS1014a diff --git a/src/Tests/Analyzers.Tests/RCS1227ValidateArgumentsCorrectlyTests.cs b/src/Tests/Analyzers.Tests/RCS1227ValidateArgumentsCorrectlyTests.cs index 510b05e19c..5fd63b2413 100644 --- a/src/Tests/Analyzers.Tests/RCS1227ValidateArgumentsCorrectlyTests.cs +++ b/src/Tests/Analyzers.Tests/RCS1227ValidateArgumentsCorrectlyTests.cs @@ -153,70 +153,6 @@ async IAsyncEnumerable M2() "); } - [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.ValidateArgumentsCorrectly)] - public async Task Test_Method_UnnecessaryNullCheckOfOptionalParameter() - { - await VerifyDiagnosticAndFixAsync(@" -using System; - -class C -{ - void M(string p = null) - { - [|if (p == null) - { - throw new ArgumentNullException(nameof(p)); - }|] - - M(p); - } -} -", @" -using System; - -class C -{ - void M(string p = null) - { - - M(p); - } -} -"); - } - - [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.ValidateArgumentsCorrectly)] - public async Task Test_Constructor_UnnecessaryNullCheckOfOptionalParameter() - { - await VerifyDiagnosticAndFixAsync(@" -using System; - -class C -{ - C(string p = null) - { - [|if (p == null) - { - throw new ArgumentNullException(nameof(p)); - }|] - - string x = null; - } -} -", @" -using System; - -class C -{ - C(string p = null) - { - - string x = null; - } -} -"); - } - [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.ValidateArgumentsCorrectly)] public async Task TestNoDiagnostic_NoStatement() { diff --git a/src/Tests/Analyzers.Tests/RCS1251InvalidNullCheckTests.cs b/src/Tests/Analyzers.Tests/RCS1251InvalidNullCheckTests.cs new file mode 100644 index 0000000000..cc7460cc9e --- /dev/null +++ b/src/Tests/Analyzers.Tests/RCS1251InvalidNullCheckTests.cs @@ -0,0 +1,151 @@ +// Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Roslynator.CSharp.CodeFixes; +using Roslynator.Testing.CSharp; +using Xunit; + +namespace Roslynator.CSharp.Analysis.Tests +{ + public class RCS1251InvalidNullCheckTests : AbstractCSharpDiagnosticVerifier + { + public override DiagnosticDescriptor Descriptor { get; } = DiagnosticRules.InvalidNullCheck; + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidNullCheck)] + public async Task Test_Method_OptionalParameter() + { + await VerifyDiagnosticAndFixAsync(@" +using System; + +class C +{ + void M(string p = null) + { + [|if (p == null) + { + throw new ArgumentNullException(nameof(p)); + }|] + + M(p); + } +} +", @" +using System; + +class C +{ + void M(string p = null) + { + + M(p); + } +} +"); + } + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidNullCheck)] + public async Task Test_Method_NullableReferenceTypeParameter() + { + await VerifyDiagnosticAndFixAsync(@" +using System; + +#nullable enable + +class C +{ + void M(string? p) + { + [|if (p == null) + { + throw new ArgumentNullException(nameof(p)); + }|] + + M(p); + } +} +", @" +using System; + +#nullable enable + +class C +{ + void M(string? p) + { + + M(p); + } +} +"); + } + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidNullCheck)] + public async Task Test_Constructor_OptionalParameter() + { + await VerifyDiagnosticAndFixAsync(@" +using System; + +class C +{ + C(string p = null) + { + [|if (p == null) + { + throw new ArgumentNullException(nameof(p)); + }|] + + string x = null; + } +} +", @" +using System; + +class C +{ + C(string p = null) + { + + string x = null; + } +} +"); + } + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidNullCheck)] + public async Task Test_Constructor_NullableReferenceTypeParameter() + { + await VerifyDiagnosticAndFixAsync(@" +using System; + +#nullable enable + +class C +{ + C(string? p) + { + [|if (p == null) + { + throw new ArgumentNullException(nameof(p)); + }|] + + string x = """"; + } +} +", @" +using System; + +#nullable enable + +class C +{ + C(string? p) + { + + string x = """"; + } +} +"); + } + } +} From df09ba83c481fdbeb42488c8e7033f55f9abc930 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sun, 19 Dec 2021 13:16:48 +0100 Subject: [PATCH 03/14] x --- src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs b/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs index 2e7f57cb5a..d39150a3a5 100644 --- a/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs +++ b/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs @@ -121,7 +121,7 @@ bool IsNullableReferenceType(SyntaxNodeAnalysisContext context, ParameterSyntax { ITypeSymbol typeSymbol = context.SemanticModel.GetTypeSymbol(type, context.CancellationToken); - if (!typeSymbol.IsKind(SymbolKind.ErrorType) + if (typeSymbol?.IsKind(SymbolKind.ErrorType) == false && typeSymbol.IsReferenceType) { return true; From 118cc95a2cee996bbdba717d4076f27f416f24a8 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Mon, 27 Dec 2021 13:06:11 +0100 Subject: [PATCH 04/14] x --- .../Analysis/InvalidNullCheckAnalyzer.cs | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs b/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs index d39150a3a5..ec97048e15 100644 --- a/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs +++ b/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs @@ -96,18 +96,21 @@ private static void AnalyzeParameterList(SyntaxNodeAnalysisContext context, Para if (nullCheck.Success) { - ParameterSyntax parameter = GetParameter(nullCheck.Expression); + ParameterSyntax parameter = FindParameter(nullCheck.Expression); - if (parameter?.Default?.Value.IsKind( - SyntaxKind.NullLiteralExpression, - SyntaxKind.DefaultLiteralExpression, - SyntaxKind.DefaultExpression) == true - || IsNullableReferenceType(context, parameter)) + if (parameter != null) { - ITypeSymbol exceptionSymbol = context.SemanticModel.GetTypeSymbol(throwStatement.Expression, context.CancellationToken); - - if (exceptionSymbol.HasMetadataName(MetadataNames.System_ArgumentNullException)) - context.ReportDiagnostic(DiagnosticRules.InvalidNullCheck, ifStatement); + if (parameter.Default?.Value.IsKind( + SyntaxKind.NullLiteralExpression, + SyntaxKind.DefaultLiteralExpression, + SyntaxKind.DefaultExpression) == true + || IsNullableReferenceType(context, parameter)) + { + ITypeSymbol exceptionSymbol = context.SemanticModel.GetTypeSymbol(throwStatement.Expression, context.CancellationToken); + + if (exceptionSymbol.HasMetadataName(MetadataNames.System_ArgumentNullException)) + context.ReportDiagnostic(DiagnosticRules.InvalidNullCheck, ifStatement); + } } } } @@ -131,7 +134,7 @@ bool IsNullableReferenceType(SyntaxNodeAnalysisContext context, ParameterSyntax return false; } - ParameterSyntax GetParameter(ExpressionSyntax expression) + ParameterSyntax FindParameter(ExpressionSyntax expression) { if (expression is IdentifierNameSyntax identifierName) { From fa108ca98dd8679b8ce5789f29c6d0702c2a6136 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sun, 13 Mar 2022 13:58:57 +0100 Subject: [PATCH 05/14] x --- docs/Configuration.md | 6 ++--- src/Analyzers/Analyzers.InvalidNullCheck.xml | 2 +- .../CSharp/DiagnosticIdentifiers.Generated.cs | 2 +- .../CSharp/DiagnosticRules.Generated.cs | 24 +++++++++---------- ...sts.cs => RCS1256InvalidNullCheckTests.cs} | 2 +- .../src/configurationFiles.generated.ts | 6 ++--- 6 files changed, 21 insertions(+), 21 deletions(-) rename src/Tests/Analyzers.Tests/{RCS1251InvalidNullCheckTests.cs => RCS1256InvalidNullCheckTests.cs} (97%) diff --git a/docs/Configuration.md b/docs/Configuration.md index e9584d4287..bfd4c3faf6 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -918,9 +918,6 @@ dotnet_diagnostic.rcs1249.severity = suggestion dotnet_diagnostic.rcs1250.severity = none # Options: roslynator_object_creation_type_style, roslynator_use_var_instead_of_implicit_object_creation -# Invalid null check -dotnet_diagnostic.rcs1251.severity = suggestion - # Remove unnecessary braces from record declaration dotnet_diagnostic.rcs1251.severity = suggestion @@ -936,6 +933,9 @@ dotnet_diagnostic.rcs1253.severity = none dotnet_diagnostic.rcs1254.severity = suggestion # Options: roslynator_enum_flag_value_style +# Invalid null check +dotnet_diagnostic.rcs1256.severity = suggestion + # Use pattern matching dotnet_diagnostic.rcs9001.severity = silent diff --git a/src/Analyzers/Analyzers.InvalidNullCheck.xml b/src/Analyzers/Analyzers.InvalidNullCheck.xml index 9a99677585..84198adc5b 100644 --- a/src/Analyzers/Analyzers.InvalidNullCheck.xml +++ b/src/Analyzers/Analyzers.InvalidNullCheck.xml @@ -1,7 +1,7 @@ - RCS1251 + RCS1256 Invalid null check. Roslynator Info diff --git a/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs b/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs index 980ea55d1e..dd832a0392 100644 --- a/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs +++ b/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs @@ -207,10 +207,10 @@ public static partial class DiagnosticIdentifiers public const string NormalizeNullCheck = "RCS1248"; public const string UnnecessaryNullForgivingOperator = "RCS1249"; public const string UseImplicitOrExplicitObjectCreation = "RCS1250"; - public const string InvalidNullCheck = "RCS1251"; public const string RemoveUnnecessaryBraces = "RCS1251"; public const string NormalizeUsageOfInfiniteLoop = "RCS1252"; public const string FormatDocumentationCommentSummary = "RCS1253"; public const string NormalizeFormatOfEnumFlagValue = "RCS1254"; + public const string InvalidNullCheck = "RCS1256"; } } \ No newline at end of file diff --git a/src/Analyzers/CSharp/DiagnosticRules.Generated.cs b/src/Analyzers/CSharp/DiagnosticRules.Generated.cs index 069e73a212..8a770e06a7 100644 --- a/src/Analyzers/CSharp/DiagnosticRules.Generated.cs +++ b/src/Analyzers/CSharp/DiagnosticRules.Generated.cs @@ -2449,18 +2449,6 @@ public static partial class DiagnosticRules helpLinkUri: DiagnosticIdentifiers.UseImplicitOrExplicitObjectCreation, customTags: Array.Empty()); - /// RCS1251 - public static readonly DiagnosticDescriptor InvalidNullCheck = DiagnosticDescriptorFactory.Create( - id: DiagnosticIdentifiers.InvalidNullCheck, - title: "Invalid null check.", - messageFormat: "Invalid null check.", - category: DiagnosticCategories.Roslynator, - defaultSeverity: DiagnosticSeverity.Info, - isEnabledByDefault: true, - description: null, - helpLinkUri: DiagnosticIdentifiers.InvalidNullCheck, - customTags: Array.Empty()); - /// RCS1251 public static readonly DiagnosticDescriptor RemoveUnnecessaryBraces = DiagnosticDescriptorFactory.Create( id: DiagnosticIdentifiers.RemoveUnnecessaryBraces, @@ -2509,5 +2497,17 @@ public static partial class DiagnosticRules helpLinkUri: DiagnosticIdentifiers.NormalizeFormatOfEnumFlagValue, customTags: Array.Empty()); + /// RCS1256 + public static readonly DiagnosticDescriptor InvalidNullCheck = DiagnosticDescriptorFactory.Create( + id: DiagnosticIdentifiers.InvalidNullCheck, + title: "Invalid null check.", + messageFormat: "Invalid null check.", + category: DiagnosticCategories.Roslynator, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: null, + helpLinkUri: DiagnosticIdentifiers.InvalidNullCheck, + customTags: Array.Empty()); + } } \ No newline at end of file diff --git a/src/Tests/Analyzers.Tests/RCS1251InvalidNullCheckTests.cs b/src/Tests/Analyzers.Tests/RCS1256InvalidNullCheckTests.cs similarity index 97% rename from src/Tests/Analyzers.Tests/RCS1251InvalidNullCheckTests.cs rename to src/Tests/Analyzers.Tests/RCS1256InvalidNullCheckTests.cs index cc7460cc9e..6242667b51 100644 --- a/src/Tests/Analyzers.Tests/RCS1251InvalidNullCheckTests.cs +++ b/src/Tests/Analyzers.Tests/RCS1256InvalidNullCheckTests.cs @@ -8,7 +8,7 @@ namespace Roslynator.CSharp.Analysis.Tests { - public class RCS1251InvalidNullCheckTests : AbstractCSharpDiagnosticVerifier + public class RCS1256InvalidNullCheckTests : AbstractCSharpDiagnosticVerifier { public override DiagnosticDescriptor Descriptor { get; } = DiagnosticRules.InvalidNullCheck; diff --git a/src/VisualStudioCode/package/src/configurationFiles.generated.ts b/src/VisualStudioCode/package/src/configurationFiles.generated.ts index 5e80257777..a5b564c080 100644 --- a/src/VisualStudioCode/package/src/configurationFiles.generated.ts +++ b/src/VisualStudioCode/package/src/configurationFiles.generated.ts @@ -890,9 +890,6 @@ roslynator_analyzers.enabled_by_default = true|false #dotnet_diagnostic.rcs1250.severity = none # Options: roslynator_object_creation_type_style, roslynator_use_var_instead_of_implicit_object_creation -# Invalid null check -#dotnet_diagnostic.rcs1251.severity = suggestion - # Remove unnecessary braces from record declaration #dotnet_diagnostic.rcs1251.severity = suggestion @@ -908,6 +905,9 @@ roslynator_analyzers.enabled_by_default = true|false #dotnet_diagnostic.rcs1254.severity = suggestion # Options: roslynator_enum_flag_value_style +# Invalid null check +#dotnet_diagnostic.rcs1256.severity = suggestion + # Use pattern matching #dotnet_diagnostic.rcs9001.severity = silent From 85e398da463fed85a82e2f9710350e3f7658fca2 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 18:32:24 +0100 Subject: [PATCH 06/14] update --- .../CodeFixes/IfStatementCodeFixProvider.cs | 8 +- src/Analyzers/Analyzers.InvalidNullCheck.xml | 8 +- .../InvalidArgumentNullCheckAnalyzer.cs | 152 +++++++++++++++++ .../Analysis/InvalidNullCheckAnalyzer.cs | 153 ------------------ .../CSharp/DiagnosticIdentifiers.Generated.cs | 2 +- .../CSharp/DiagnosticRules.Generated.cs | 10 +- ...> RCS1256InvalidArgumentNullCheckTests.cs} | 47 +++--- 7 files changed, 190 insertions(+), 190 deletions(-) create mode 100644 src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs delete mode 100644 src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs rename src/Tests/Analyzers.Tests/{RCS1256InvalidNullCheckTests.cs => RCS1256InvalidArgumentNullCheckTests.cs} (56%) diff --git a/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs b/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs index 21071f2794..6918408e57 100644 --- a/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs +++ b/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs @@ -38,7 +38,7 @@ public override ImmutableArray FixableDiagnosticIds DiagnosticIdentifiers.ReduceIfNesting, DiagnosticIdentifiers.UseExceptionFilter, DiagnosticIdentifiers.SimplifyArgumentNullCheck, - DiagnosticIdentifiers.InvalidNullCheck); + DiagnosticIdentifiers.InvalidArgumentNullCheck); } } @@ -113,17 +113,17 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) ifStatement, cancellationToken: ct); }, - GetEquivalenceKey(diagnostic)); + GetEquivalenceKey(diagnostic)); context.RegisterCodeFix(codeAction, diagnostic); break; } - case DiagnosticIdentifiers.InvalidNullCheck: + case DiagnosticIdentifiers.InvalidArgumentNullCheck: { CodeAction codeAction = CodeAction.Create( "Remove null check", ct => context.Document.RemoveStatementAsync(ifStatement, ct), - GetEquivalenceKey(diagnostic)); + GetEquivalenceKey(diagnostic)); context.RegisterCodeFix(codeAction, diagnostic); break; diff --git a/src/Analyzers/Analyzers.InvalidNullCheck.xml b/src/Analyzers/Analyzers.InvalidNullCheck.xml index 84198adc5b..33033e180f 100644 --- a/src/Analyzers/Analyzers.InvalidNullCheck.xml +++ b/src/Analyzers/Analyzers.InvalidNullCheck.xml @@ -1,11 +1,13 @@ - + RCS1256 - Invalid null check. + Invalid argument null check. Roslynator Info true - + This analyzer reports null checks of arguments that are: +- are annotated as nullable reference type. +- are optional and its default value is `null`. \ No newline at end of file diff --git a/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs b/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs new file mode 100644 index 0000000000..3d8a6324e7 --- /dev/null +++ b/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs @@ -0,0 +1,152 @@ +// Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Roslynator.CSharp.Syntax; + +namespace Roslynator.CSharp.Analysis; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class InvalidArgumentNullCheckAnalyzer : BaseDiagnosticAnalyzer +{ + private static ImmutableArray _supportedDiagnostics; + + public override ImmutableArray SupportedDiagnostics + { + get + { + if (_supportedDiagnostics.IsDefault) + Immutable.InterlockedInitialize(ref _supportedDiagnostics, DiagnosticRules.InvalidArgumentNullCheck); + + return _supportedDiagnostics; + } + } + + public override void Initialize(AnalysisContext context) + { + base.Initialize(context); + + context.RegisterSyntaxNodeAction(f => AnalyzeMethodDeclaration(f), SyntaxKind.MethodDeclaration); + context.RegisterSyntaxNodeAction(f => AnalyzeConstructorDeclaration(f), SyntaxKind.ConstructorDeclaration); + } + + private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context) + { + var methodDeclaration = (MethodDeclarationSyntax)context.Node; + + AnalyzeParameterList(context, methodDeclaration.ParameterList, methodDeclaration.Body); + } + + private static void AnalyzeConstructorDeclaration(SyntaxNodeAnalysisContext context) + { + var constructorDeclaration = (ConstructorDeclarationSyntax)context.Node; + + AnalyzeParameterList(context, constructorDeclaration.ParameterList, constructorDeclaration.Body); + } + + private static void AnalyzeParameterList(SyntaxNodeAnalysisContext context, ParameterListSyntax parameterList, BlockSyntax body) + { + if (body is null) + return; + + if (parameterList is null) + return; + + SeparatedSyntaxList parameters = parameterList.Parameters; + + if (!parameters.Any()) + return; + + SyntaxList statements = body.Statements; + + int statementCount = statements.Count; + + if (statementCount == 0) + return; + + int lastIndex = -1; + + foreach (ParameterSyntax parameter in parameters) + { + if (parameter.IsParams()) + break; + + lastIndex++; + } + + if (lastIndex == -1) + return; + + foreach (StatementSyntax statement in statements) + { + if (statement is not IfStatementSyntax ifStatement + || !ifStatement.IsSimpleIf() + || ifStatement.SingleNonBlockStatementOrDefault() is not ThrowStatementSyntax throwStatement) + { + break; + } + + if (throwStatement.Expression.IsKind(SyntaxKind.ObjectCreationExpression) + && !ifStatement.SpanContainsDirectives()) + { + NullCheckExpressionInfo nullCheck = SyntaxInfo.NullCheckExpressionInfo(ifStatement.Condition, context.SemanticModel, NullCheckStyles.CheckingNull); + + if (nullCheck.Success) + { + ParameterSyntax parameter = FindParameter(nullCheck.Expression); + + if (parameter is not null) + { + if (parameter.Default?.Value.IsKind( + SyntaxKind.NullLiteralExpression, + SyntaxKind.DefaultLiteralExpression, + SyntaxKind.DefaultExpression) == true + || IsNullableReferenceType(context, parameter)) + { + ITypeSymbol exceptionSymbol = context.SemanticModel.GetTypeSymbol(throwStatement.Expression, context.CancellationToken); + + if (exceptionSymbol.HasMetadataName(MetadataNames.System_ArgumentNullException)) + context.ReportDiagnostic(DiagnosticRules.InvalidArgumentNullCheck, ifStatement); + } + } + } + } + } + + bool IsNullableReferenceType(SyntaxNodeAnalysisContext context, ParameterSyntax parameter) + { + TypeSyntax type = parameter.Type; + + if (type.IsKind(SyntaxKind.NullableType)) + { + ITypeSymbol typeSymbol = context.SemanticModel.GetTypeSymbol(type, context.CancellationToken); + + if (typeSymbol?.IsKind(SymbolKind.ErrorType) == false + && typeSymbol.IsReferenceType) + { + return true; + } + } + + return false; + } + + ParameterSyntax FindParameter(ExpressionSyntax expression) + { + if (expression is IdentifierNameSyntax identifierName) + { + string identifierText = identifierName.Identifier.ValueText; + for (int i = 0; i <= lastIndex; i++) + { + if (parameters[i].Identifier.ValueText == identifierText) + return parameters[i]; + } + } + + return null; + } + } +} diff --git a/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs b/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs deleted file mode 100644 index ec97048e15..0000000000 --- a/src/Analyzers/CSharp/Analysis/InvalidNullCheckAnalyzer.cs +++ /dev/null @@ -1,153 +0,0 @@ -// Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System.Collections.Immutable; -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; -using Microsoft.CodeAnalysis.Diagnostics; -using Roslynator.CSharp.Syntax; - -namespace Roslynator.CSharp.Analysis -{ - [DiagnosticAnalyzer(LanguageNames.CSharp)] - public sealed class InvalidNullCheckAnalyzer : BaseDiagnosticAnalyzer - { - private static ImmutableArray _supportedDiagnostics; - - public override ImmutableArray SupportedDiagnostics - { - get - { - if (_supportedDiagnostics.IsDefault) - Immutable.InterlockedInitialize(ref _supportedDiagnostics, DiagnosticRules.InvalidNullCheck); - - return _supportedDiagnostics; - } - } - - public override void Initialize(AnalysisContext context) - { - base.Initialize(context); - - context.RegisterSyntaxNodeAction(f => AnalyzeMethodDeclaration(f), SyntaxKind.MethodDeclaration); - context.RegisterSyntaxNodeAction(f => AnalyzeConstructorDeclaration(f), SyntaxKind.ConstructorDeclaration); - } - - private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context) - { - var methodDeclaration = (MethodDeclarationSyntax)context.Node; - - AnalyzeParameterList(context, methodDeclaration.ParameterList, methodDeclaration.Body); - } - - private static void AnalyzeConstructorDeclaration(SyntaxNodeAnalysisContext context) - { - var constructorDeclaration = (ConstructorDeclarationSyntax)context.Node; - - AnalyzeParameterList(context, constructorDeclaration.ParameterList, constructorDeclaration.Body); - } - - private static void AnalyzeParameterList(SyntaxNodeAnalysisContext context, ParameterListSyntax parameterList, BlockSyntax body) - { - if (body == null) - return; - - if (parameterList == null) - return; - - SeparatedSyntaxList parameters = parameterList.Parameters; - - if (!parameters.Any()) - return; - - SyntaxList statements = body.Statements; - - int statementCount = statements.Count; - - if (statementCount == 0) - return; - - int lastIndex = -1; - - foreach (ParameterSyntax parameter in parameters) - { - if (parameter.IsParams()) - break; - - lastIndex++; - } - - if (lastIndex == -1) - return; - - foreach (StatementSyntax statement in statements) - { - if (statement is not IfStatementSyntax ifStatement - || !ifStatement.IsSimpleIf() - || ifStatement.SingleNonBlockStatementOrDefault() is not ThrowStatementSyntax throwStatement) - { - break; - } - - if (throwStatement.Expression.IsKind(SyntaxKind.ObjectCreationExpression) - && !ifStatement.SpanContainsDirectives()) - { - NullCheckExpressionInfo nullCheck = SyntaxInfo.NullCheckExpressionInfo(ifStatement.Condition, context.SemanticModel, NullCheckStyles.CheckingNull); - - if (nullCheck.Success) - { - ParameterSyntax parameter = FindParameter(nullCheck.Expression); - - if (parameter != null) - { - if (parameter.Default?.Value.IsKind( - SyntaxKind.NullLiteralExpression, - SyntaxKind.DefaultLiteralExpression, - SyntaxKind.DefaultExpression) == true - || IsNullableReferenceType(context, parameter)) - { - ITypeSymbol exceptionSymbol = context.SemanticModel.GetTypeSymbol(throwStatement.Expression, context.CancellationToken); - - if (exceptionSymbol.HasMetadataName(MetadataNames.System_ArgumentNullException)) - context.ReportDiagnostic(DiagnosticRules.InvalidNullCheck, ifStatement); - } - } - } - } - } - - bool IsNullableReferenceType(SyntaxNodeAnalysisContext context, ParameterSyntax parameter) - { - TypeSyntax type = parameter.Type; - - if (type.IsKind(SyntaxKind.NullableType)) - { - ITypeSymbol typeSymbol = context.SemanticModel.GetTypeSymbol(type, context.CancellationToken); - - if (typeSymbol?.IsKind(SymbolKind.ErrorType) == false - && typeSymbol.IsReferenceType) - { - return true; - } - } - - return false; - } - - ParameterSyntax FindParameter(ExpressionSyntax expression) - { - if (expression is IdentifierNameSyntax identifierName) - { - string identifierText = identifierName.Identifier.ValueText; - for (int i = 0; i <= lastIndex; i++) - { - if (parameters[i].Identifier.ValueText == identifierText) - return parameters[i]; - } - } - - return null; - } - } - } -} diff --git a/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs b/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs index 66c09c1bb7..f851844545 100644 --- a/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs +++ b/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs @@ -212,6 +212,6 @@ public static partial class DiagnosticIdentifiers public const string FormatDocumentationCommentSummary = "RCS1253"; public const string NormalizeFormatOfEnumFlagValue = "RCS1254"; public const string SimplifyArgumentNullCheck = "RCS1255"; - public const string InvalidNullCheck = "RCS1256"; + public const string InvalidArgumentNullCheck = "RCS1256"; } } \ No newline at end of file diff --git a/src/Analyzers/CSharp/DiagnosticRules.Generated.cs b/src/Analyzers/CSharp/DiagnosticRules.Generated.cs index 984a76ce4c..b8ec15c83f 100644 --- a/src/Analyzers/CSharp/DiagnosticRules.Generated.cs +++ b/src/Analyzers/CSharp/DiagnosticRules.Generated.cs @@ -2510,15 +2510,15 @@ public static partial class DiagnosticRules customTags: Array.Empty()); /// RCS1256 - public static readonly DiagnosticDescriptor InvalidNullCheck = DiagnosticDescriptorFactory.Create( - id: DiagnosticIdentifiers.InvalidNullCheck, - title: "Invalid null check.", - messageFormat: "Invalid null check.", + public static readonly DiagnosticDescriptor InvalidArgumentNullCheck = DiagnosticDescriptorFactory.Create( + id: DiagnosticIdentifiers.InvalidArgumentNullCheck, + title: "Invalid argument null check.", + messageFormat: "Invalid argument null check.", category: DiagnosticCategories.Roslynator, defaultSeverity: DiagnosticSeverity.Info, isEnabledByDefault: true, description: null, - helpLinkUri: DiagnosticIdentifiers.InvalidNullCheck, + helpLinkUri: DiagnosticIdentifiers.InvalidArgumentNullCheck, customTags: Array.Empty()); } diff --git a/src/Tests/Analyzers.Tests/RCS1256InvalidNullCheckTests.cs b/src/Tests/Analyzers.Tests/RCS1256InvalidArgumentNullCheckTests.cs similarity index 56% rename from src/Tests/Analyzers.Tests/RCS1256InvalidNullCheckTests.cs rename to src/Tests/Analyzers.Tests/RCS1256InvalidArgumentNullCheckTests.cs index 6242667b51..64e61e4d9a 100644 --- a/src/Tests/Analyzers.Tests/RCS1256InvalidNullCheckTests.cs +++ b/src/Tests/Analyzers.Tests/RCS1256InvalidArgumentNullCheckTests.cs @@ -6,16 +6,16 @@ using Roslynator.Testing.CSharp; using Xunit; -namespace Roslynator.CSharp.Analysis.Tests +namespace Roslynator.CSharp.Analysis.Tests; + +public class RCS1256InvalidArgumentNullCheckTests : AbstractCSharpDiagnosticVerifier { - public class RCS1256InvalidNullCheckTests : AbstractCSharpDiagnosticVerifier - { - public override DiagnosticDescriptor Descriptor { get; } = DiagnosticRules.InvalidNullCheck; + public override DiagnosticDescriptor Descriptor { get; } = DiagnosticRules.InvalidArgumentNullCheck; - [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidNullCheck)] - public async Task Test_Method_OptionalParameter() - { - await VerifyDiagnosticAndFixAsync(@" + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidArgumentNullCheck)] + public async Task Test_Method_OptionalParameter() + { + await VerifyDiagnosticAndFixAsync(@" using System; class C @@ -42,12 +42,12 @@ void M(string p = null) } } "); - } + } - [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidNullCheck)] - public async Task Test_Method_NullableReferenceTypeParameter() - { - await VerifyDiagnosticAndFixAsync(@" + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidArgumentNullCheck)] + public async Task Test_Method_NullableReferenceTypeParameter() + { + await VerifyDiagnosticAndFixAsync(@" using System; #nullable enable @@ -78,12 +78,12 @@ void M(string? p) } } "); - } + } - [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidNullCheck)] - public async Task Test_Constructor_OptionalParameter() - { - await VerifyDiagnosticAndFixAsync(@" + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidArgumentNullCheck)] + public async Task Test_Constructor_OptionalParameter() + { + await VerifyDiagnosticAndFixAsync(@" using System; class C @@ -110,12 +110,12 @@ class C } } "); - } + } - [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidNullCheck)] - public async Task Test_Constructor_NullableReferenceTypeParameter() - { - await VerifyDiagnosticAndFixAsync(@" + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidArgumentNullCheck)] + public async Task Test_Constructor_NullableReferenceTypeParameter() + { + await VerifyDiagnosticAndFixAsync(@" using System; #nullable enable @@ -146,6 +146,5 @@ class C } } "); - } } } From d2d089406b6274c1b2f50ae9c336a591c5c7b778 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 18:37:05 +0100 Subject: [PATCH 07/14] update --- docs/Configuration.md | 3 --- src/Analyzers/Analyzers.InvalidNullCheck.xml | 4 ++-- src/Analyzers/Analyzers.xml | 15 +++++---------- .../package/src/configurationFiles.generated.ts | 3 --- 4 files changed, 7 insertions(+), 18 deletions(-) diff --git a/docs/Configuration.md b/docs/Configuration.md index dd4177ea4f..09929292d4 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -933,9 +933,6 @@ dotnet_diagnostic.rcs1253.severity = none dotnet_diagnostic.rcs1254.severity = suggestion # Options: roslynator_enum_flag_value_style -# Invalid null check -dotnet_diagnostic.rcs1256.severity = suggestion - # Use pattern matching dotnet_diagnostic.rcs9001.severity = silent diff --git a/src/Analyzers/Analyzers.InvalidNullCheck.xml b/src/Analyzers/Analyzers.InvalidNullCheck.xml index 33033e180f..87901471ae 100644 --- a/src/Analyzers/Analyzers.InvalidNullCheck.xml +++ b/src/Analyzers/Analyzers.InvalidNullCheck.xml @@ -7,7 +7,7 @@ Info true This analyzer reports null checks of arguments that are: -- are annotated as nullable reference type. -- are optional and its default value is `null`. +- annotated as nullable reference type. +- optional and its default value is `null`. \ No newline at end of file diff --git a/src/Analyzers/Analyzers.xml b/src/Analyzers/Analyzers.xml index 0dfeaf63e7..059a8910cd 100644 --- a/src/Analyzers/Analyzers.xml +++ b/src/Analyzers/Analyzers.xml @@ -4885,16 +4885,11 @@ class Foo Design Info true - **Case #1** - -An iterator method (a method that contains `yield`) will not validate arguments until the caller begins to enumerate the result items. - -To ensure that arguments are validated immediately (when the method is called), move -the iterator to a separate method (local function). - -**Case #2** - -Remove condition where a parameter that has default value equal to `null` is checked for null and then `ArgumentNullException` is thrown. + + An iterator method (a method that contains `yield`) will not validate arguments until the caller begins to enumerate the result items. + To ensure that arguments are validated immediately (when the method is called), move + the iterator to a separate method (local function). + Foo(IEnumerable items) diff --git a/src/VisualStudioCode/package/src/configurationFiles.generated.ts b/src/VisualStudioCode/package/src/configurationFiles.generated.ts index b0eb34d7f1..249d8ae86d 100644 --- a/src/VisualStudioCode/package/src/configurationFiles.generated.ts +++ b/src/VisualStudioCode/package/src/configurationFiles.generated.ts @@ -905,9 +905,6 @@ roslynator_analyzers.enabled_by_default = true|false #dotnet_diagnostic.rcs1254.severity = suggestion # Options: roslynator_enum_flag_value_style -# Invalid null check -#dotnet_diagnostic.rcs1256.severity = suggestion - # Use pattern matching #dotnet_diagnostic.rcs9001.severity = silent From 9139d7d04efbd6fa87876150034603a167cb6093 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 18:37:35 +0100 Subject: [PATCH 08/14] update --- src/Analyzers/Analyzers.xml | 1 + src/CommandLine/Properties/launchSettings.json | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Analyzers/Analyzers.xml b/src/Analyzers/Analyzers.xml index 059a8910cd..0211989aff 100644 --- a/src/Analyzers/Analyzers.xml +++ b/src/Analyzers/Analyzers.xml @@ -4887,6 +4887,7 @@ class Foo true An iterator method (a method that contains `yield`) will not validate arguments until the caller begins to enumerate the result items. + To ensure that arguments are validated immediately (when the method is called), move the iterator to a separate method (local function). diff --git a/src/CommandLine/Properties/launchSettings.json b/src/CommandLine/Properties/launchSettings.json index 3f374ad18d..8d5d7e0f29 100644 --- a/src/CommandLine/Properties/launchSettings.json +++ b/src/CommandLine/Properties/launchSettings.json @@ -2,7 +2,7 @@ "profiles": { "CommandLine": { "commandName": "Project", - "commandLineArgs": "migrate \"E:\\Projects\\Roslynator\\src\\Migration.Test\" --target-version 3.0 --identifier roslynator.analyzers -d" + "commandLineArgs": "analyze \"C:\\code\\datamole\\ddp-kernel-device-management\\src\\Datamole.DataPlatform.DeviceManagement.sln\" --supported-diagnostics RCS1256 --analyzer-assemblies \"C:\\code\\public\\roslynator\\src\\Analyzers\\bin\\Debug\\netstandard2.0\\Roslynator.CSharp.Analyzers.dll\"" } } } \ No newline at end of file From 18226c335cbe3793a8344b80a03ee89324cdee04 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 18:38:04 +0100 Subject: [PATCH 09/14] update --- src/Analyzers/Analyzers.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analyzers/Analyzers.xml b/src/Analyzers/Analyzers.xml index 0211989aff..7124e69790 100644 --- a/src/Analyzers/Analyzers.xml +++ b/src/Analyzers/Analyzers.xml @@ -4887,7 +4887,7 @@ class Foo true An iterator method (a method that contains `yield`) will not validate arguments until the caller begins to enumerate the result items. - + To ensure that arguments are validated immediately (when the method is called), move the iterator to a separate method (local function). From 1f3f28adcc2457400a7374ff359703a0f26b311c Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 18:39:02 +0100 Subject: [PATCH 10/14] Revert "update" This reverts commit 9139d7d04efbd6fa87876150034603a167cb6093. # Conflicts: # src/Analyzers/Analyzers.xml --- src/CommandLine/Properties/launchSettings.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CommandLine/Properties/launchSettings.json b/src/CommandLine/Properties/launchSettings.json index 8d5d7e0f29..3f374ad18d 100644 --- a/src/CommandLine/Properties/launchSettings.json +++ b/src/CommandLine/Properties/launchSettings.json @@ -2,7 +2,7 @@ "profiles": { "CommandLine": { "commandName": "Project", - "commandLineArgs": "analyze \"C:\\code\\datamole\\ddp-kernel-device-management\\src\\Datamole.DataPlatform.DeviceManagement.sln\" --supported-diagnostics RCS1256 --analyzer-assemblies \"C:\\code\\public\\roslynator\\src\\Analyzers\\bin\\Debug\\netstandard2.0\\Roslynator.CSharp.Analyzers.dll\"" + "commandLineArgs": "migrate \"E:\\Projects\\Roslynator\\src\\Migration.Test\" --target-version 3.0 --identifier roslynator.analyzers -d" } } } \ No newline at end of file From dc4ebf350d7389ce69ae522fab4fc28df1d81e59 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 18:40:41 +0100 Subject: [PATCH 11/14] update --- src/Analyzers/Analyzers.InvalidNullCheck.xml | 13 ------------- src/Analyzers/Analyzers.xml | 13 ++++++++++++- .../Analysis/InvalidArgumentNullCheckAnalyzer.cs | 2 -- 3 files changed, 12 insertions(+), 16 deletions(-) delete mode 100644 src/Analyzers/Analyzers.InvalidNullCheck.xml diff --git a/src/Analyzers/Analyzers.InvalidNullCheck.xml b/src/Analyzers/Analyzers.InvalidNullCheck.xml deleted file mode 100644 index 87901471ae..0000000000 --- a/src/Analyzers/Analyzers.InvalidNullCheck.xml +++ /dev/null @@ -1,13 +0,0 @@ - - - - RCS1256 - Invalid argument null check. - Roslynator - Info - true - This analyzer reports null checks of arguments that are: -- annotated as nullable reference type. -- optional and its default value is `null`. - - \ No newline at end of file diff --git a/src/Analyzers/Analyzers.xml b/src/Analyzers/Analyzers.xml index 7124e69790..a47b20e773 100644 --- a/src/Analyzers/Analyzers.xml +++ b/src/Analyzers/Analyzers.xml @@ -5744,7 +5744,7 @@ void M() RCS1255 Simplify argument null check. - General + Roslynator Info false Use `ArgumentNullException.ThrowIfNull` instead of `if` null check. @@ -5758,4 +5758,15 @@ void M() + + RCS1256 + Invalid argument null check. + Roslynator + Info + true + This analyzer reports null checks of arguments that are: +- annotated as nullable reference type. +- optional and its default value is `null`. + + \ No newline at end of file diff --git a/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs b/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs index 3d8a6324e7..a6cd406247 100644 --- a/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs +++ b/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs @@ -36,14 +36,12 @@ public override void Initialize(AnalysisContext context) private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context) { var methodDeclaration = (MethodDeclarationSyntax)context.Node; - AnalyzeParameterList(context, methodDeclaration.ParameterList, methodDeclaration.Body); } private static void AnalyzeConstructorDeclaration(SyntaxNodeAnalysisContext context) { var constructorDeclaration = (ConstructorDeclarationSyntax)context.Node; - AnalyzeParameterList(context, constructorDeclaration.ParameterList, constructorDeclaration.Body); } From 3f9ac71cdc7e9cc441b3f92d7d570ca6aff83c05 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 18:44:31 +0100 Subject: [PATCH 12/14] changelog --- ChangeLog.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog.md b/ChangeLog.md index ecb7a1ab5f..1910fbe631 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add analyzer "Simplify argument null check" ([RCS1255](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1255.md)) ([#994](https://github.com/JosefPihrt/Roslynator/pull/994)). - Use `ArgumentNullException.ThrowIfNull` instead of `if` null check. - Not enabled by default +- Add analyzer "Invalid argument null check" ([RCS1256](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1256.md)) ([#888](https://github.com/JosefPihrt/Roslynator/pull/888)). + - This analyzer reports null checks of arguments that are: + - annotated as nullable reference type. + - optional and its default value is `null`. ### Changed From 18e44018491526d5be106e4d721daa7d208ce974 Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 19:10:14 +0100 Subject: [PATCH 13/14] update --- .../CodeFixes/IfStatementCodeFixProvider.cs | 13 +-- ...InvalidArgumentNullCheckCodeFixProvider.cs | 40 ++++++++++ .../InvalidArgumentNullCheckAnalyzer.cs | 50 +++++------- src/Common/ArgumentNullCheckAnalysis.cs | 43 ++++++---- .../RCS1256InvalidArgumentNullCheckTests.cs | 80 ++++++++++++++++--- 5 files changed, 158 insertions(+), 68 deletions(-) create mode 100644 src/Analyzers.CodeFixes/CSharp/CodeFixes/InvalidArgumentNullCheckCodeFixProvider.cs diff --git a/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs b/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs index 6918408e57..d9e614ad25 100644 --- a/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs +++ b/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs @@ -37,8 +37,7 @@ public override ImmutableArray FixableDiagnosticIds DiagnosticIdentifiers.ConvertIfToAssignment, DiagnosticIdentifiers.ReduceIfNesting, DiagnosticIdentifiers.UseExceptionFilter, - DiagnosticIdentifiers.SimplifyArgumentNullCheck, - DiagnosticIdentifiers.InvalidArgumentNullCheck); + DiagnosticIdentifiers.SimplifyArgumentNullCheck); } } @@ -117,16 +116,6 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) context.RegisterCodeFix(codeAction, diagnostic); break; - } - case DiagnosticIdentifiers.InvalidArgumentNullCheck: - { - CodeAction codeAction = CodeAction.Create( - "Remove null check", - ct => context.Document.RemoveStatementAsync(ifStatement, ct), - GetEquivalenceKey(diagnostic)); - - context.RegisterCodeFix(codeAction, diagnostic); - break; } case DiagnosticIdentifiers.SimplifyArgumentNullCheck: { diff --git a/src/Analyzers.CodeFixes/CSharp/CodeFixes/InvalidArgumentNullCheckCodeFixProvider.cs b/src/Analyzers.CodeFixes/CSharp/CodeFixes/InvalidArgumentNullCheckCodeFixProvider.cs new file mode 100644 index 0000000000..e433955c7c --- /dev/null +++ b/src/Analyzers.CodeFixes/CSharp/CodeFixes/InvalidArgumentNullCheckCodeFixProvider.cs @@ -0,0 +1,40 @@ +// Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Immutable; +using System.Composition; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Roslynator.CodeFixes; + +namespace Roslynator.CSharp.CodeFixes; + +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(InvalidArgumentNullCheckCodeFixProvider))] +[Shared] +public sealed class InvalidArgumentNullCheckCodeFixProvider : BaseCodeFixProvider +{ + public override ImmutableArray FixableDiagnosticIds + { + get { return ImmutableArray.Create(DiagnosticIdentifiers.InvalidArgumentNullCheck); } + } + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + SyntaxNode root = await context.GetSyntaxRootAsync().ConfigureAwait(false); + + if (!TryFindFirstAncestorOrSelf(root, context.Span, out StatementSyntax statement)) + return; + + Document document = context.Document; + Diagnostic diagnostic = context.Diagnostics[0]; + + CodeAction codeAction = CodeAction.Create( + "Remove null check", + ct => context.Document.RemoveStatementAsync(statement, ct), + GetEquivalenceKey(diagnostic)); + + context.RegisterCodeFix(codeAction, diagnostic); + } +} diff --git a/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs b/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs index a6cd406247..3eda8d5bf0 100644 --- a/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs +++ b/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs @@ -5,7 +5,6 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; -using Roslynator.CSharp.Syntax; namespace Roslynator.CSharp.Analysis; @@ -80,34 +79,27 @@ private static void AnalyzeParameterList(SyntaxNodeAnalysisContext context, Para foreach (StatementSyntax statement in statements) { - if (statement is not IfStatementSyntax ifStatement - || !ifStatement.IsSimpleIf() - || ifStatement.SingleNonBlockStatementOrDefault() is not ThrowStatementSyntax throwStatement) - { - break; - } + ArgumentNullCheckAnalysis nullCheck = ArgumentNullCheckAnalysis.Create(statement, context.SemanticModel, context.CancellationToken); - if (throwStatement.Expression.IsKind(SyntaxKind.ObjectCreationExpression) - && !ifStatement.SpanContainsDirectives()) + if (nullCheck.Success) { - NullCheckExpressionInfo nullCheck = SyntaxInfo.NullCheckExpressionInfo(ifStatement.Condition, context.SemanticModel, NullCheckStyles.CheckingNull); + ParameterSyntax parameter = FindParameter(nullCheck.Name); - if (nullCheck.Success) + if (parameter is not null) { - ParameterSyntax parameter = FindParameter(nullCheck.Expression); - - if (parameter is not null) + if (parameter.Default?.Value.IsKind( + SyntaxKind.NullLiteralExpression, + SyntaxKind.DefaultLiteralExpression, + SyntaxKind.DefaultExpression) == true + || IsNullableReferenceType(context, parameter)) { - if (parameter.Default?.Value.IsKind( - SyntaxKind.NullLiteralExpression, - SyntaxKind.DefaultLiteralExpression, - SyntaxKind.DefaultExpression) == true - || IsNullableReferenceType(context, parameter)) + if (statement is IfStatementSyntax ifStatement) { - ITypeSymbol exceptionSymbol = context.SemanticModel.GetTypeSymbol(throwStatement.Expression, context.CancellationToken); - - if (exceptionSymbol.HasMetadataName(MetadataNames.System_ArgumentNullException)) - context.ReportDiagnostic(DiagnosticRules.InvalidArgumentNullCheck, ifStatement); + context.ReportDiagnostic(DiagnosticRules.InvalidArgumentNullCheck, ifStatement.IfKeyword); + } + else + { + context.ReportDiagnostic(DiagnosticRules.InvalidArgumentNullCheck, statement); } } } @@ -132,16 +124,12 @@ bool IsNullableReferenceType(SyntaxNodeAnalysisContext context, ParameterSyntax return false; } - ParameterSyntax FindParameter(ExpressionSyntax expression) + ParameterSyntax FindParameter(string name) { - if (expression is IdentifierNameSyntax identifierName) + for (int i = 0; i <= lastIndex; i++) { - string identifierText = identifierName.Identifier.ValueText; - for (int i = 0; i <= lastIndex; i++) - { - if (parameters[i].Identifier.ValueText == identifierText) - return parameters[i]; - } + if (parameters[i].Identifier.ValueText == name) + return parameters[i]; } return null; diff --git a/src/Common/ArgumentNullCheckAnalysis.cs b/src/Common/ArgumentNullCheckAnalysis.cs index 265edee18d..cc69afef83 100644 --- a/src/Common/ArgumentNullCheckAnalysis.cs +++ b/src/Common/ArgumentNullCheckAnalysis.cs @@ -10,14 +10,15 @@ namespace Roslynator.CSharp; internal readonly struct ArgumentNullCheckAnalysis { - private ArgumentNullCheckAnalysis(ArgumentNullCheckStyle style, bool success) + private ArgumentNullCheckAnalysis(ArgumentNullCheckStyle style, string name, bool success) { Style = style; + Name = name; Success = success; } public ArgumentNullCheckStyle Style { get; } - + public string Name { get; } public bool Success { get; } public static ArgumentNullCheckAnalysis Create( @@ -37,6 +38,7 @@ private ArgumentNullCheckAnalysis(ArgumentNullCheckStyle style, bool success) if (statement is IfStatementSyntax ifStatement) { var style = ArgumentNullCheckStyle.None; + string identifier = null; var success = false; if (ifStatement.SingleNonBlockStatementOrDefault() is ThrowStatementSyntax throwStatement @@ -52,22 +54,26 @@ private ArgumentNullCheckAnalysis(ArgumentNullCheckStyle style, bool success) { style = ArgumentNullCheckStyle.IfStatement; - if (name is null - || (nullCheck.Expression is IdentifierNameSyntax identifierName - && string.Equals(name, identifierName.Identifier.ValueText, StringComparison.Ordinal))) + if (nullCheck.Expression is IdentifierNameSyntax identifierName) { - if (semanticModel - .GetSymbol(objectCreation, cancellationToken)? - .ContainingType? - .HasMetadataName(MetadataNames.System_ArgumentNullException) == true) + identifier = identifierName.Identifier.ValueText; + + if (name is null + || string.Equals(name, identifierName.Identifier.ValueText, StringComparison.Ordinal)) { - success = true; + if (semanticModel + .GetSymbol(objectCreation, cancellationToken)? + .ContainingType? + .HasMetadataName(MetadataNames.System_ArgumentNullException) == true) + { + success = true; + } } } } } - return new ArgumentNullCheckAnalysis(style, success); + return new ArgumentNullCheckAnalysis(style, identifier, success); } else { @@ -82,6 +88,7 @@ private ArgumentNullCheckAnalysis(ArgumentNullCheckStyle style, bool success) CancellationToken cancellationToken) { var style = ArgumentNullCheckStyle.None; + string identifier = null; var success = false; if (statement is ExpressionStatementSyntax expressionStatement) @@ -97,16 +104,20 @@ private ArgumentNullCheckAnalysis(ArgumentNullCheckStyle style, bool success) { style = ArgumentNullCheckStyle.ThrowIfNullMethod; - if (name is null - || (invocationInfo.Arguments.SingleOrDefault(shouldThrow: false)?.Expression is IdentifierNameSyntax identifierName - && string.Equals(name, identifierName.Identifier.ValueText, StringComparison.Ordinal))) + if (invocationInfo.Arguments.SingleOrDefault(shouldThrow: false)?.Expression is IdentifierNameSyntax identifierName) { - success = true; + identifier = identifierName.Identifier.ValueText; + + if (name is null + || string.Equals(name, identifierName.Identifier.ValueText, StringComparison.Ordinal)) + { + success = true; + } } } } - return new ArgumentNullCheckAnalysis(style, success); + return new ArgumentNullCheckAnalysis(style, identifier, success); } public static bool IsArgumentNullExceptionThrowIfNullCheck( diff --git a/src/Tests/Analyzers.Tests/RCS1256InvalidArgumentNullCheckTests.cs b/src/Tests/Analyzers.Tests/RCS1256InvalidArgumentNullCheckTests.cs index 64e61e4d9a..ae6731f3bb 100644 --- a/src/Tests/Analyzers.Tests/RCS1256InvalidArgumentNullCheckTests.cs +++ b/src/Tests/Analyzers.Tests/RCS1256InvalidArgumentNullCheckTests.cs @@ -8,7 +8,7 @@ namespace Roslynator.CSharp.Analysis.Tests; -public class RCS1256InvalidArgumentNullCheckTests : AbstractCSharpDiagnosticVerifier +public class RCS1256InvalidArgumentNullCheckTests : AbstractCSharpDiagnosticVerifier { public override DiagnosticDescriptor Descriptor { get; } = DiagnosticRules.InvalidArgumentNullCheck; @@ -22,10 +22,10 @@ class C { void M(string p = null) { - [|if (p == null) + [|if|] (p == null) { throw new ArgumentNullException(nameof(p)); - }|] + } M(p); } @@ -56,10 +56,72 @@ class C { void M(string? p) { - [|if (p == null) + [|if|] (p == null) { throw new ArgumentNullException(nameof(p)); - }|] + } + + M(p); + } +} +", @" +using System; + +#nullable enable + +class C +{ + void M(string? p) + { + + M(p); + } +} +"); + } + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidArgumentNullCheck)] + public async Task Test_Method_OptionalParameter_ThrowIfNull() + { + await VerifyDiagnosticAndFixAsync(@" +using System; + +class C +{ + void M(string p = null) + { + [|ArgumentNullException.ThrowIfNull(p);|] + + M(p); + } +} +", @" +using System; + +class C +{ + void M(string p = null) + { + + M(p); + } +} +"); + } + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidArgumentNullCheck)] + public async Task Test_Method_NullableReferenceTypeParameter_ThrowIfNull() + { + await VerifyDiagnosticAndFixAsync(@" +using System; + +#nullable enable + +class C +{ + void M(string? p) + { + [|ArgumentNullException.ThrowIfNull(p);|] M(p); } @@ -90,10 +152,10 @@ class C { C(string p = null) { - [|if (p == null) + [|if|] (p == null) { throw new ArgumentNullException(nameof(p)); - }|] + } string x = null; } @@ -124,10 +186,10 @@ class C { C(string? p) { - [|if (p == null) + [|if|] (p == null) { throw new ArgumentNullException(nameof(p)); - }|] + } string x = """"; } From f420618a38519f537f33b9a50846c959be9e57ae Mon Sep 17 00:00:00 2001 From: Josef Pihrt Date: Sat, 19 Nov 2022 19:13:17 +0100 Subject: [PATCH 14/14] update --- .../CSharp/CodeFixes/IfStatementCodeFixProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs b/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs index d9e614ad25..70320b2b05 100644 --- a/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs +++ b/src/Analyzers.CodeFixes/CSharp/CodeFixes/IfStatementCodeFixProvider.cs @@ -114,8 +114,8 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) }, GetEquivalenceKey(diagnostic)); - context.RegisterCodeFix(codeAction, diagnostic); - break; + context.RegisterCodeFix(codeAction, diagnostic); + break; } case DiagnosticIdentifiers.SimplifyArgumentNullCheck: {