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 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/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 new file mode 100644 index 0000000000..3eda8d5bf0 --- /dev/null +++ b/src/Analyzers/CSharp/Analysis/InvalidArgumentNullCheckAnalyzer.cs @@ -0,0 +1,138 @@ +// 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; + +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) + { + ArgumentNullCheckAnalysis nullCheck = ArgumentNullCheckAnalysis.Create(statement, context.SemanticModel, context.CancellationToken); + + if (nullCheck.Success) + { + ParameterSyntax parameter = FindParameter(nullCheck.Name); + + if (parameter is not null) + { + if (parameter.Default?.Value.IsKind( + SyntaxKind.NullLiteralExpression, + SyntaxKind.DefaultLiteralExpression, + SyntaxKind.DefaultExpression) == true + || IsNullableReferenceType(context, parameter)) + { + if (statement is IfStatementSyntax ifStatement) + { + context.ReportDiagnostic(DiagnosticRules.InvalidArgumentNullCheck, ifStatement.IfKeyword); + } + else + { + context.ReportDiagnostic(DiagnosticRules.InvalidArgumentNullCheck, statement); + } + } + } + } + } + + 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(string name) + { + for (int i = 0; i <= lastIndex; i++) + { + if (parameters[i].Identifier.ValueText == name) + return parameters[i]; + } + + return null; + } + } +} diff --git a/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs b/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs index 8712feca6a..f851844545 100644 --- a/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs +++ b/src/Analyzers/CSharp/DiagnosticIdentifiers.Generated.cs @@ -212,5 +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 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 bd08134461..b8ec15c83f 100644 --- a/src/Analyzers/CSharp/DiagnosticRules.Generated.cs +++ b/src/Analyzers/CSharp/DiagnosticRules.Generated.cs @@ -2509,5 +2509,17 @@ public static partial class DiagnosticRules helpLinkUri: DiagnosticIdentifiers.SimplifyArgumentNullCheck, customTags: Array.Empty()); + /// RCS1256 + 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.InvalidArgumentNullCheck, + customTags: Array.Empty()); + } } \ No newline at end of file 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 @@ public static ArgumentNullCheckAnalysis Create( 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 @@ public static ArgumentNullCheckAnalysis Create( { 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 static ArgumentNullCheckAnalysis CreateFromArgumentNullExceptionThrowIfN CancellationToken cancellationToken) { var style = ArgumentNullCheckStyle.None; + string identifier = null; var success = false; if (statement is ExpressionStatementSyntax expressionStatement) @@ -97,16 +104,20 @@ private static ArgumentNullCheckAnalysis CreateFromArgumentNullExceptionThrowIfN { 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 new file mode 100644 index 0000000000..ae6731f3bb --- /dev/null +++ b/src/Tests/Analyzers.Tests/RCS1256InvalidArgumentNullCheckTests.cs @@ -0,0 +1,212 @@ +// 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 RCS1256InvalidArgumentNullCheckTests : AbstractCSharpDiagnosticVerifier +{ + public override DiagnosticDescriptor Descriptor { get; } = DiagnosticRules.InvalidArgumentNullCheck; + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidArgumentNullCheck)] + 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.InvalidArgumentNullCheck)] + 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.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); + } +} +", @" +using System; + +#nullable enable + +class C +{ + void M(string? p) + { + + M(p); + } +} +"); + } + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.InvalidArgumentNullCheck)] + 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.InvalidArgumentNullCheck)] + 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 = """"; + } +} +"); + } +}