From 746f2e5ab99318c3bef273404d7b21f06e5bbb3a Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 3 Jan 2022 10:37:44 -0800 Subject: [PATCH 01/15] 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, From 718fa55000d409ca804d0e52b5df406037b1d4bf Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 3 Jan 2022 16:23:06 -0800 Subject: [PATCH 02/15] Move to new test framework. Various fixes. --- ...ParameterNullCheckingDiagnosticAnalyzer.cs | 5 +- ...UseParameterNullCheckingCodeFixProvider.cs | 4 +- .../Tests/CSharpAnalyzers.UnitTests.projitems | 2 +- .../UseParameterNullCheckingTests.cs | 351 ++++++++++++------ 4 files changed, 235 insertions(+), 127 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index d6392f2146f50..fe631c7b6d302 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -66,7 +66,7 @@ protected override void InitializeWorker(AnalysisContext context) return; } - context.RegisterSyntaxNodeAction(context => AnalyzeSyntax(context, argumentNullExceptionConstructor, referenceEqualsMethod), SyntaxKind.MethodDeclaration, SyntaxKind.LocalFunctionStatement); + context.RegisterSyntaxNodeAction(context => AnalyzeSyntax(context, argumentNullExceptionConstructor, referenceEqualsMethod), SyntaxKind.ConstructorDeclaration, SyntaxKind.MethodDeclaration, SyntaxKind.LocalFunctionStatement); }); private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argumentNullExceptionConstructor, IMethodSymbol referenceEqualsMethod) @@ -86,6 +86,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu var block = node switch { MethodDeclarationSyntax methodDecl => methodDecl.Body, + ConstructorDeclarationSyntax constructorDecl => constructorDecl.Body, LocalFunctionStatementSyntax localFunctionStatement => localFunctionStatement.Body, _ => throw ExceptionUtilities.UnexpectedValue(node) }; @@ -220,7 +221,7 @@ bool isParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParam bool isConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, IParameterSymbol parameterSymbol) { if (exceptionCreation.ArgumentList is null - || exceptionCreation.ArgumentList.Arguments[0] is not { } argument) + || exceptionCreation.ArgumentList.Arguments.FirstOrDefault() is not { } argument) { return false; } diff --git a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs index 73b26a392a6eb..90522c1ceda07 100644 --- a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Immutable; using System.Composition; -using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.CodeActions; @@ -13,6 +12,7 @@ using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Host.Mef; using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; @@ -22,7 +22,7 @@ namespace Microsoft.CodeAnalysis.CSharp.UseParameterNullChecking 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")] + [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] public CSharpUseParameterNullCheckingCodeFixProvider() { } diff --git a/src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems b/src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems index 94a76dcbafdea..2aa0e218be626 100644 --- a/src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems +++ b/src/Analyzers/CSharp/Tests/CSharpAnalyzers.UnitTests.projitems @@ -92,7 +92,7 @@ - + diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index 40d4737ab0099..d41f1772f1365 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -9,49 +9,49 @@ using Microsoft.CodeAnalysis.CSharp.UseParameterNullChecking; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics; +using Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions; using Microsoft.CodeAnalysis.Test.Utilities; using Xunit; using Xunit.Abstractions; namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UseParameterNullChecking { - public partial class UseParameterNullCheckingTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest + using VerifyCS = CSharpCodeFixVerifier; + + public partial class UseParameterNullCheckingTests { 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; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { void M(string s) { - if ([||]s " + @operator + @" null) - throw new ArgumentNullException(nameof(s)); + [|if (s " + @operator + @" null) + throw new ArgumentNullException(nameof(s));|] } }", -@"using System; + FixedCode = @" +using System; class C { void M(string s!!) { } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Theory] @@ -60,27 +60,32 @@ void M(string s!!) [InlineData("is")] public async Task TestWithBraces(string @operator) { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { void M(string s) { - if ([||]s " + @operator + @" null) + [|if (s " + @operator + @" null) { throw new ArgumentNullException(nameof(s)); - } + }|] } }", -@"using System; + FixedCode = @" +using System; class C { void M(string s!!) { } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Theory] @@ -89,8 +94,10 @@ void M(string s!!) [InlineData("is")] public async Task TestLocalFunction(string @operator) { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { @@ -99,14 +106,15 @@ void M() local(""""); void local(string s) { - if ([||]s " + @operator + @" null) + [|if (s " + @operator + @" null) { throw new ArgumentNullException(nameof(s)); - } + }|] } } }", -@"using System; + FixedCode = @" +using System; class C { @@ -117,93 +125,114 @@ void local(string s!!) { } } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestEqualitySwapped() { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { void M(string s) { - if ([||]null == (object)s) - throw new ArgumentNullException(nameof(s)); + [|if (null == (object)s) + throw new ArgumentNullException(nameof(s));|] } }", -@"using System; + FixedCode = @" +using System; class C { void M(string s!!) { } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNotEquality() { - await TestMissingInRegularAndScriptAsync( -@"using System; + var testCode = @" +using System; class C { void M(string s) { - if ([||](object)s != null) + if ((object)s != null) throw new ArgumentNullException(nameof(s)); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNullCoalescingThrow() { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { private readonly string s; - void M(string s) + public C(string s) { - this.s = [||]s ?? throw new ArgumentNullException(nameof(s)); + [|this.s = s ?? throw new ArgumentNullException(nameof(s));|] } }", -@"using System; + FixedCode = @" +using System; class C { private readonly string s; - void M(string s!!) + public C(string s!!) { this.s = s; } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestReferenceEqualsCheck() { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { private readonly string s; void M(string s) { - if ([||]object.ReferenceEquals(s, null)) + [|if (object.ReferenceEquals(s, null)) { throw new ArgumentNullException(nameof(s)); - } + }|] } }", -@"using System; + FixedCode = @" +using System; class C { @@ -211,159 +240,201 @@ class C void M(string s!!) { } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNotCustomReferenceEqualsCheck() { - await TestMissingInRegularAndScriptAsync( -@"using System; + var testCode = @" +using System; class C { private readonly string s; void M(string s) { - if ([||]ReferenceEquals(s, null)) + if (ReferenceEquals(s, null)) { throw new ArgumentNullException(nameof(s)); } } bool ReferenceEquals(object o1, object o2) => false; -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNotEqualitySwapped() { - await TestMissingInRegularAndScriptAsync( -@"using System; + var testCode = @"using System; class C { void M(string s) { - if ([||]null != (object)s) + if (null != (object)s) throw new ArgumentNullException(nameof(s)); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestMissingPreCSharp11() { - await TestMissingAsync( -@"using System; + var testCode = @" +using System; class C { void M(string s) { - if ([||](object)s == null) + if ((object)s == null) throw new ArgumentNullException(nameof(s)); } -}", parameters: new TestParameters(parseOptions: CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp10))); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersion.CSharp10 + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestOnlyForObjectCast() { - await TestMissingAsync( -@"using System; + var testCode = @"using System; class C { void M(string s) { - if ([||](string)s == null) + if ((string)s == null) throw new ArgumentNullException(nameof(s)); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestFixAll1() { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { void M(string s1, string s2) { - if ({|FixAllInDocument:|}(object)s1 == null) - throw new ArgumentNullException(nameof(s1)); + [|if ((object)s1 == null) + throw new ArgumentNullException(nameof(s1));|] - if (null == (object)s2) - throw new ArgumentNullException(nameof(s2)); + [|if (null == (object)s2) + throw new ArgumentNullException(nameof(s2));|] } }", -@"using System; + FixedCode = @" +using System; class C { void M(string s1!!, string s2!!) { } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] - public async Task TestFixAllNested1() + public async Task TestNested1() { - await TestMissingInRegularAndScriptAsync( -@"using System; + var testCode = @"using System; class C { void M(string s2) { - if ({|FixAllInDocument:|}(object)((object)s2 == null) == null)) + if ((object)((object)s2 == null) == null) throw new ArgumentNullException(nameof(s2)); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestConstrainedTypeParameter() { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { void M(T s) where T : class { - if ([||](object)s == null) - throw new ArgumentNullException(nameof(s)); + [|if ((object)s == null) + throw new ArgumentNullException(nameof(s));|] } }", -@"using System; + FixedCode = @" +using System; class C { void M(T s!!) where T : class { } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestUnconstrainedTypeParameter() { - await TestInRegularAndScriptAsync( + await new VerifyCS.Test() + { + TestCode = @"using System; class C { void M(T s) { - if ([||](object)s == null) - throw new ArgumentNullException(nameof(s)); + [|if ((object)s == null) + throw new ArgumentNullException(nameof(s));|] } }", + FixedCode = @"using System; class C @@ -371,72 +442,93 @@ class C void M(T s!!) { } -}", parseOptions: s_parseOptions); +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestComplexExpr() { - await TestMissingInRegularAndScriptAsync( -@"using System; + var testCode = @"using System; class C { void M(string[] s) { - if ([||](object)s[0] == null) + if ((object)s[0] == null) throw new ArgumentNullException(nameof(s)); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestComplexExpr2() { - await TestMissingInRegularAndScriptAsync( -@"using System; + var testCode = @"using System; class C { void M(string s1, string s2) { - if ([||]s1 == null || s2 == null) - throw new ArgumentNullException(nameof(s)); + if (s1 == null || s2 == null) + throw new ArgumentNullException(nameof(s1)); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNotOnDefault() { - await TestMissingAsync( -@"using System; + var testCode = @" +using System; class C { void M(string s) { - if ([||](object)s == default) + if ((object)s == default) throw new ArgumentNullException(nameof(s)); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestWithStringExceptionArgument() { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { void M(string s) { - if ([||]s == null) - throw new ArgumentNullException(""s""); + [|if (s == null) + throw new ArgumentNullException(""s"");|] } }", -@"using System; + FixedCode = @" +using System; class C { @@ -444,24 +536,28 @@ void M(string s!!) { } }", - parseOptions: s_parseOptions); + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestWithStringExceptionArgument2() { - await TestInRegularAndScriptAsync( -@"using System; + await new VerifyCS.Test() + { + TestCode = @" +using System; class C { void M(string HelloWorld) { - if ([||]HelloWorld == null) - throw new ArgumentNullException(""Hello"" + ""World""); + [|if (HelloWorld == null) + throw new ArgumentNullException(""Hello"" + ""World"");|] } }", -@"using System; + FixedCode = @" +using System; class C { @@ -469,39 +565,50 @@ void M(string HelloWorld!!) { } }", -parseOptions: s_parseOptions); + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNotWithUnexpectedExceptionArgument() { - await TestMissingAsync( -@"using System; + var testCode = @"using System; class C { void M(string s) { - if ([||]s == null) + if (s == null) throw new ArgumentNullException(""banana""); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNotWithExceptionNoArguments() { - await TestMissingAsync( -@"using System; + var testCode = @"using System; class C { void M(string s) { - if ([||]s == null) + if (s == null) throw new ArgumentNullException(); } -}", parameters: new TestParameters(parseOptions: s_parseOptions)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); } } } From 2dd31c59cb098d75b8f722b2e4114b6e70c54772 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 3 Jan 2022 17:06:14 -0800 Subject: [PATCH 03/15] support lambdas. other minor feedback items. --- ...ParameterNullCheckingDiagnosticAnalyzer.cs | 47 +++---- .../UseParameterNullCheckingTests.cs | 118 +++++++++++++++++- .../Core/Analyzers/EnforceOnBuildValues.cs | 2 +- .../Core/Analyzers/IDEDiagnosticIds.cs | 2 +- 4 files changed, 144 insertions(+), 25 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index fe631c7b6d302..84adeb074c8eb 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -56,20 +56,21 @@ protected override void InitializeWorker(AnalysisContext context) } var objectType = compilation.GetSpecialType(SpecialType.System_Object); - var referenceEqualsMethod = objectType + var referenceEqualsMethod = (IMethodSymbol?)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.ConstructorDeclaration, SyntaxKind.MethodDeclaration, SyntaxKind.LocalFunctionStatement); + .FirstOrDefault(m => m is IMethodSymbol { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 2 }); + + context.RegisterSyntaxNodeAction( + context => AnalyzeSyntax(context, argumentNullExceptionConstructor, referenceEqualsMethod), + SyntaxKind.ConstructorDeclaration, + SyntaxKind.MethodDeclaration, + SyntaxKind.LocalFunctionStatement, + SyntaxKind.SimpleLambdaExpression, + SyntaxKind.ParenthesizedLambdaExpression, + SyntaxKind.AnonymousMethodExpression); }); - private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argumentNullExceptionConstructor, IMethodSymbol referenceEqualsMethod) + private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argumentNullExceptionConstructor, IMethodSymbol? referenceEqualsMethod) { var cancellationToken = context.CancellationToken; @@ -88,6 +89,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu MethodDeclarationSyntax methodDecl => methodDecl.Body, ConstructorDeclarationSyntax constructorDecl => constructorDecl.Body, LocalFunctionStatementSyntax localFunctionStatement => localFunctionStatement.Body, + AnonymousFunctionExpressionSyntax anonymousFunction => anonymousFunction.Block, _ => throw ExceptionUtilities.UnexpectedValue(node) }; if (block is null) @@ -95,7 +97,9 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu return; } - var methodSymbol = semanticModel.GetDeclaredSymbol(node, cancellationToken); + var methodSymbol = node is AnonymousFunctionExpressionSyntax + ? semanticModel.GetSymbolInfo(node, cancellationToken).Symbol + : semanticModel.GetDeclaredSymbol(node, cancellationToken); if (methodSymbol is null) { return; @@ -121,7 +125,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu right = patternExpression; break; case { Condition: InvocationExpressionSyntax { Expression: var receiver, ArgumentList.Arguments: { Count: 2 } arguments } }: - if (!referenceEqualsMethod.Equals(semanticModel.GetSymbolInfo(receiver, cancellationToken).Symbol)) + if (referenceEqualsMethod == null || !referenceEqualsMethod.Equals(semanticModel.GetSymbolInfo(receiver, cancellationToken).Symbol)) { continue; } @@ -135,8 +139,8 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu } IParameterSymbol? parameter; - if (!areOperandsApplicable(left, right, out parameter) - && !areOperandsApplicable(right, left, out parameter)) + if (!AreOperandsApplicable(left, right, out parameter) + && !AreOperandsApplicable(right, left, out parameter)) { continue; } @@ -150,7 +154,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu if (throwStatement is null || throwStatement.Expression is not ObjectCreationExpressionSyntax thrownInIf - || !isConstructorApplicable(thrownInIf, parameter)) + || !IsConstructorApplicable(thrownInIf, parameter)) { continue; } @@ -170,7 +174,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu } } }: - if (!isParameter(maybeParameter, out var parameterInNullCoalescing) || !isConstructorApplicable(thrownInNullCoalescing, parameterInNullCoalescing)) + if (!IsParameter(maybeParameter, out var parameterInNullCoalescing) || !IsConstructorApplicable(thrownInNullCoalescing, parameterInNullCoalescing)) { continue; } @@ -184,7 +188,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu 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) + bool AreOperandsApplicable(ExpressionSyntax maybeParameter, ExpressionSyntax maybeNullLiteral, [NotNullWhen(true)] out IParameterSymbol? parameter) { if (!maybeNullLiteral.IsKind(SyntaxKind.NullLiteralExpression)) { @@ -192,10 +196,10 @@ bool areOperandsApplicable(ExpressionSyntax maybeParameter, ExpressionSyntax may return false; } - return isParameter(maybeParameter, out parameter); + return IsParameter(maybeParameter, out parameter); } - bool isParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParameterSymbol? parameter) + bool IsParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParameterSymbol? parameter) { if (maybeParameter is CastExpressionSyntax { Type: var type, Expression: var operand }) { @@ -218,13 +222,14 @@ bool isParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParam return true; } - bool isConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, IParameterSymbol parameterSymbol) + bool IsConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, IParameterSymbol parameterSymbol) { if (exceptionCreation.ArgumentList is null || exceptionCreation.ArgumentList.Arguments.FirstOrDefault() 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)) { diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index d41f1772f1365..ed3db776e8674 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -20,8 +20,6 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UseParameterNullCheckin public partial class UseParameterNullCheckingTests { - private static readonly CSharpParseOptions s_parseOptions = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersionExtensions.CSharpNext); - [Theory] [Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] [InlineData("==")] @@ -610,5 +608,121 @@ void M(string s) LanguageVersion = LanguageVersionExtensions.CSharpNext }.RunAsync(); } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestSimpleLambda() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + Action lambda = x => + { + [|if (x is null) + throw new ArgumentNullException(nameof(x));|] + }; +} +", + FixedCode = @"using System; + +class C +{ + Action lambda = x!! => + { + }; +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestParenthesizedLambdaNoType() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + Action lambda = (x) => + { + [|if (x is null) + throw new ArgumentNullException(nameof(x));|] + }; +} +", + FixedCode = @"using System; + +class C +{ + Action lambda = (x!!) => + { + }; +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestParenthesizedLambdaWithType() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + Action lambda = (string x) => + { + [|if (x is null) + throw new ArgumentNullException(nameof(x));|] + }; +} +", + FixedCode = @"using System; + +class C +{ + Action lambda = (string x!!) => + { + }; +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestAnonymousMethod() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + Action lambda = delegate (string x) + { + [|if (x is null) + throw new ArgumentNullException(nameof(x));|] + }; +} +", + FixedCode = @"using System; + +class C +{ + Action lambda = delegate (string x!!) + { + }; +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } } } diff --git a/src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs b/src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs index 03b65a64deaef..4f5e6d27eedfe 100644 --- a/src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs +++ b/src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs @@ -87,7 +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 UseParameterNullChecking = /*IDE0190*/ 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 1b576c4c8be03..6d7d5b43b7cfd 100644 --- a/src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs +++ b/src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs @@ -166,7 +166,7 @@ internal static class IDEDiagnosticIds public const string SimplifyPropertyPatternDiagnosticId = "IDE0170"; public const string UseTupleSwapDiagnosticId = "IDE0180"; - public const string UseParameterNullCheckingId = "IDE0181"; + public const string UseParameterNullCheckingId = "IDE0190"; // Analyzer error Ids public const string AnalyzerChangedId = "IDE1001"; From b3ce666318947f761acf827f7686236b997ea558 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 3 Jan 2022 17:19:15 -0800 Subject: [PATCH 04/15] Use AdditionalLocations for parameter location --- ...ParameterNullCheckingDiagnosticAnalyzer.cs | 11 +++-- ...UseParameterNullCheckingCodeFixProvider.cs | 40 ++++--------------- 2 files changed, 15 insertions(+), 36 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index 84adeb074c8eb..6e479a6440b6b 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -107,6 +107,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu foreach (var statement in block.Statements) { + IParameterSymbol? parameter; switch (statement) { // if (param == null) { throw new ArgumentNullException(nameof(param)); } @@ -138,7 +139,6 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu continue; } - IParameterSymbol? parameter; if (!AreOperandsApplicable(left, right, out parameter) && !AreOperandsApplicable(right, left, out parameter)) { @@ -174,7 +174,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu } } }: - if (!IsParameter(maybeParameter, out var parameterInNullCoalescing) || !IsConstructorApplicable(thrownInNullCoalescing, parameterInNullCoalescing)) + if (!IsParameter(maybeParameter, out parameter) || !IsConstructorApplicable(thrownInNullCoalescing, parameter)) { continue; } @@ -185,7 +185,12 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu continue; } - context.ReportDiagnostic(DiagnosticHelper.Create(Descriptor, statement.GetLocation(), option.Notification.Severity, additionalLocations: null, properties: null)); + context.ReportDiagnostic(DiagnosticHelper.Create( + Descriptor, + statement.GetLocation(), + option.Notification.Severity, + additionalLocations: new[] { parameter.Locations[0] }, + properties: null)); } bool AreOperandsApplicable(ExpressionSyntax maybeParameter, ExpressionSyntax maybeNullLiteral, [NotNullWhen(true)] out IParameterSymbol? parameter) diff --git a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs index 90522c1ceda07..d6d952a47107b 100644 --- a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs @@ -43,60 +43,34 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context) return Task.CompletedTask; } - protected override async Task FixAllAsync( + protected override 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; + default: + throw ExceptionUtilities.UnexpectedValue(node); } - } - static ExpressionSyntax unwrapCast(ExpressionSyntax expression) - { - if (expression is CastExpressionSyntax { Expression: var operand }) - { - return operand; - } - - return expression; + var parameterSyntax = (ParameterSyntax)diagnostic.AdditionalLocations[0].FindNode(cancellationToken); + editor.ReplaceNode(parameterSyntax, parameterSyntax.WithExclamationExclamationToken(SyntaxFactory.Token(SyntaxKind.ExclamationExclamationToken))); } + + return Task.CompletedTask; } private class MyCodeAction : CustomCodeActions.DocumentChangeAction From bdda177645ef4df04ab8044a53738708ac771cd4 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 4 Jan 2022 17:04:41 -0800 Subject: [PATCH 05/15] more --- ...ParameterNullCheckingDiagnosticAnalyzer.cs | 53 ++-- ...UseParameterNullCheckingCodeFixProvider.cs | 4 +- .../UseParameterNullCheckingTests.cs | 263 +++++++++++++++++- .../Core/EditorFeaturesResources.resx | 3 + .../Core/xlf/EditorFeaturesResources.cs.xlf | 5 + .../Core/xlf/EditorFeaturesResources.de.xlf | 5 + .../Core/xlf/EditorFeaturesResources.es.xlf | 5 + .../Core/xlf/EditorFeaturesResources.fr.xlf | 5 + .../Core/xlf/EditorFeaturesResources.it.xlf | 5 + .../Core/xlf/EditorFeaturesResources.ja.xlf | 5 + .../Core/xlf/EditorFeaturesResources.ko.xlf | 5 + .../Core/xlf/EditorFeaturesResources.pl.xlf | 5 + .../xlf/EditorFeaturesResources.pt-BR.xlf | 5 + .../Core/xlf/EditorFeaturesResources.ru.xlf | 5 + .../Core/xlf/EditorFeaturesResources.tr.xlf | 5 + .../xlf/EditorFeaturesResources.zh-Hans.xlf | 5 + .../xlf/EditorFeaturesResources.zh-Hant.xlf | 5 + .../IDEDiagnosticIDConfigurationTests.cs | 3 + .../CSharp/Impl/CSharpVSResources.resx | 3 + .../CSharpCodeStyleSettingsProvider.cs | 4 + .../Impl/Options/Formatting/StyleViewModel.cs | 23 ++ .../CSharp/Impl/xlf/CSharpVSResources.cs.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.de.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.es.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.fr.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.it.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.ja.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.ko.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.pl.xlf | 5 + .../Impl/xlf/CSharpVSResources.pt-BR.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.ru.xlf | 5 + .../CSharp/Impl/xlf/CSharpVSResources.tr.xlf | 5 + .../Impl/xlf/CSharpVSResources.zh-Hans.xlf | 5 + .../Impl/xlf/CSharpVSResources.zh-Hant.xlf | 5 + .../CommonCodeStyleSettingsProvider.cs | 4 + .../BasicEditorConfigGeneratorTests.vb | 2 + .../CSharpEditorConfigGeneratorTests.vb | 2 + .../CodeStyle/CSharpCodeStyleOptions.cs | 6 + 38 files changed, 470 insertions(+), 30 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index 6e479a6440b6b..d80fcf67bcff3 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -14,7 +14,7 @@ namespace Microsoft.CodeAnalysis.CSharp.UseParameterNullChecking { [DiagnosticAnalyzer(LanguageNames.CSharp)] - internal class CSharpUseParameterNullCheckingDiagnosticAnalyzer + internal sealed class CSharpUseParameterNullCheckingDiagnosticAnalyzer : AbstractBuiltInCodeStyleDiagnosticAnalyzer { private const string ArgumentNullExceptionName = $"{nameof(System)}.{nameof(ArgumentNullException)}"; @@ -34,12 +34,12 @@ public override DiagnosticAnalyzerCategory GetAnalyzerCategory() protected override void InitializeWorker(AnalysisContext context) => context.RegisterCompilationStartAction(context => { - if (((CSharpCompilation)context.Compilation).LanguageVersion < LanguageVersionExtensions.CSharpNext) + var compilation = (CSharpCompilation)context.Compilation; + if (compilation.LanguageVersion < LanguageVersionExtensions.CSharpNext) { return; } - var compilation = context.Compilation; var argumentNullException = compilation.GetTypeByMetadataName(ArgumentNullExceptionName); if (argumentNullException is null) { @@ -98,9 +98,9 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu } var methodSymbol = node is AnonymousFunctionExpressionSyntax - ? semanticModel.GetSymbolInfo(node, cancellationToken).Symbol - : semanticModel.GetDeclaredSymbol(node, cancellationToken); - if (methodSymbol is null) + ? (IMethodSymbol?)semanticModel.GetSymbolInfo(node, cancellationToken).Symbol + : (IMethodSymbol?)semanticModel.GetDeclaredSymbol(node, cancellationToken); + if (methodSymbol is null || methodSymbol.Parameters.IsEmpty) { return; } @@ -117,7 +117,10 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu ExpressionSyntax left, right; switch (ifStatement) { - case { Condition: BinaryExpressionSyntax { OperatorToken.RawKind: (int)SyntaxKind.EqualsEqualsToken } binary }: + case { Condition: BinaryExpressionSyntax { OperatorToken.RawKind: (int)SyntaxKind.EqualsEqualsToken } binary } + // Only suggest the fix on built-in `==` operators where we know we won't change behavior + when semanticModel.GetSymbolInfo(binary).Symbol is IMethodSymbol { MethodKind: MethodKind.BuiltinOperator }: + left = binary.Left; right = binary.Right; break; @@ -125,11 +128,8 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu left = patternInput; right = patternExpression; break; - case { Condition: InvocationExpressionSyntax { Expression: var receiver, ArgumentList.Arguments: { Count: 2 } arguments } }: - if (referenceEqualsMethod == null || !referenceEqualsMethod.Equals(semanticModel.GetSymbolInfo(receiver, cancellationToken).Symbol)) - { - continue; - } + case { Condition: InvocationExpressionSyntax { Expression: var receiver, ArgumentList.Arguments: { Count: 2 } arguments } } + when referenceEqualsMethod != null && referenceEqualsMethod.Equals(semanticModel.GetSymbolInfo(receiver, cancellationToken).Symbol): left = arguments[0].Expression; right = arguments[1].Expression; @@ -152,8 +152,7 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu _ => null }; - if (throwStatement is null - || throwStatement.Expression is not ObjectCreationExpressionSyntax thrownInIf + if (throwStatement?.Expression is not ObjectCreationExpressionSyntax thrownInIf || !IsConstructorApplicable(thrownInIf, parameter)) { continue; @@ -185,14 +184,21 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu continue; } - context.ReportDiagnostic(DiagnosticHelper.Create( - Descriptor, - statement.GetLocation(), - option.Notification.Severity, - additionalLocations: new[] { parameter.Locations[0] }, - properties: null)); + if (parameter.DeclaringSyntaxReferences.FirstOrDefault() is SyntaxReference reference + && reference.SyntaxTree.Equals(statement.SyntaxTree) + && reference.GetSyntax() is ParameterSyntax parameterSyntax) + { + context.ReportDiagnostic(DiagnosticHelper.Create( + Descriptor, + statement.GetLocation(), + option.Notification.Severity, + additionalLocations: new[] { parameterSyntax.GetLocation() }, + properties: null)); + } } + return; + bool AreOperandsApplicable(ExpressionSyntax maybeParameter, ExpressionSyntax maybeNullLiteral, [NotNullWhen(true)] out IParameterSymbol? parameter) { if (!maybeNullLiteral.IsKind(SyntaxKind.NullLiteralExpression)) @@ -206,6 +212,8 @@ bool AreOperandsApplicable(ExpressionSyntax maybeParameter, ExpressionSyntax may bool IsParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParameterSymbol? parameter) { + // `(object)x == null` is often used to ensure reference equality is used. + // therefore, we specially unwrap casts when the cast is to `object`. if (maybeParameter is CastExpressionSyntax { Type: var type, Expression: var operand }) { if (semanticModel.GetTypeInfo(type).Type?.SpecialType != SpecialType.System_Object) @@ -229,14 +237,13 @@ bool IsParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParam bool IsConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, IParameterSymbol parameterSymbol) { - if (exceptionCreation.ArgumentList is null - || exceptionCreation.ArgumentList.Arguments.FirstOrDefault() is not { } argument) + if (exceptionCreation.ArgumentList?.Arguments.FirstOrDefault() 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)) + if (constantValue.Value is not string constantString || !string.Equals(constantString, parameterSymbol.Name, StringComparison.Ordinal)) { return false; } diff --git a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs index d6d952a47107b..ff65e1b1ab43e 100644 --- a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs @@ -18,8 +18,8 @@ namespace Microsoft.CodeAnalysis.CSharp.UseParameterNullChecking { - [ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.UseIsNullCheck), Shared] - internal class CSharpUseParameterNullCheckingCodeFixProvider : SyntaxEditorBasedCodeFixProvider + [ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.UseParameterNullChecking), Shared] + internal sealed class CSharpUseParameterNullCheckingCodeFixProvider : SyntaxEditorBasedCodeFixProvider { [ImportingConstructor] [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index ed3db776e8674..e251443fb4e3c 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -18,7 +18,7 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UseParameterNullCheckin { using VerifyCS = CSharpCodeFixVerifier; - public partial class UseParameterNullCheckingTests + public class UseParameterNullCheckingTests { [Theory] [Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] @@ -211,8 +211,26 @@ public C(string s!!) } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] - public async Task TestReferenceEqualsCheck() + public async Task TestNullCoalescingThrowExpressionBody() { + var testCode = @" +using System; + +class C +{ + private readonly string s; + public C(string s) + => this.s = s ?? throw new ArgumentNullException(nameof(s)); +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + + // TODO: the fixer should work in this scenario +#if false await new VerifyCS.Test() { TestCode = @" @@ -221,9 +239,47 @@ public async Task TestReferenceEqualsCheck() class C { private readonly string s; - void M(string s) + public C(string s) + => [|this.s = s ?? throw new ArgumentNullException(nameof(s));|] +}", + FixedCode = @" +using System; + +class C +{ + private readonly string s; + public C(string s!!) + => this.s = s; +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); +#endif + } + + [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + [InlineData("object")] + [InlineData("C")] + public async Task TestReferenceEqualsCheck(string className) + { + await new VerifyCS.Test() + { + TestCode = @" +using System; + +class C +{ + private readonly string s; + void M1(string s) + { + [|if (" + className + @".ReferenceEquals(s, null)) + { + throw new ArgumentNullException(nameof(s)); + }|] + } + + void M2(string s) { - [|if (object.ReferenceEquals(s, null)) + [|if (" + className + @".ReferenceEquals(null, s)) { throw new ArgumentNullException(nameof(s)); }|] @@ -235,7 +291,11 @@ void M(string s) class C { private readonly string s; - void M(string s!!) + void M1(string s!!) + { + } + + void M2(string s!!) { } }", @@ -724,5 +784,198 @@ class C LanguageVersion = LanguageVersionExtensions.CSharpNext }.RunAsync(); } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestLocalFunctionWithinAccessor() + { + var testCode = @"using System; +class C +{ + private int _p; + + public int P + { + get => _p; + set + { + local(); + + void local() + { + if (value == null) throw new ArgumentNullException(nameof(value)); + _p = value; + } + } + } +} +"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestLocalFunctionCapturingParameter() + { + var testCode = @"using System; +class C +{ + void M(string param) + { + local(); + + void local() + { + if (param == null) + throw new ArgumentNullException(nameof(param)); + } + } +} +"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNotWithUserDefinedOperator() + { + var testCode = @"using System; +class C +{ + public static bool operator ==(C c1, C c2) => true; + public static bool operator !=(C c1, C c2) => false; + + static void M(C c) + { + if (c == null) + throw new ArgumentNullException(nameof(c)); + } +} +"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestWithUnusedUserDefinedOperator() + { + await new VerifyCS.Test() + { + TestCode = @"using System; +class C +{ + public static bool operator ==(C c1, C c2) => true; + public static bool operator !=(C c1, C c2) => false; + + static void M(C c) + { + [|if ((object)c == null) + throw new ArgumentNullException(nameof(c));|] + } +} +", + FixedCode = @"using System; +class C +{ + public static bool operator ==(C c1, C c2) => true; + public static bool operator !=(C c1, C c2) => false; + + static void M(C c!!) + { + } +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestWithPreprocessorDirective() + { + await new VerifyCS.Test() + { + TestCode = @"using System; +class C +{ + static void M(C c) + { +#nullable enable + [|if ((object)c == null) + throw new ArgumentNullException(nameof(c));|] +#nullable disable + } +} +", + FixedCode = @"using System; +class C +{ + static void M(C c!!) + { +#nullable disable + } +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestPartialMethod() + { + var partialDeclaration = @" +partial class C +{ + static partial void M(C c); +} +"; + + await new VerifyCS.Test() + { + TestState = + { + Sources = + { + partialDeclaration, + @"using System; +partial class C +{ + static partial void M(C c) + { + [|if ((object)c == null) + throw new ArgumentNullException(nameof(c));|] + } +} +" + } + }, + FixedState = + { + Sources = + { + partialDeclaration, + @"using System; +partial class C +{ + static partial void M(C c!!) + { + } +} +" + } + }, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } } } diff --git a/src/EditorFeatures/Core/EditorFeaturesResources.resx b/src/EditorFeatures/Core/EditorFeaturesResources.resx index ee32fef772903..0c792ec87a5cb 100644 --- a/src/EditorFeatures/Core/EditorFeaturesResources.resx +++ b/src/EditorFeatures/Core/EditorFeaturesResources.resx @@ -975,6 +975,9 @@ Do you want to proceed? Prefer 'is null' for reference equality checks + + Prefer parameter null checking + Prefer 'this.' or 'Me.' diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf index b55de38bbb9d5..2f1472081b9b4 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf @@ -192,6 +192,11 @@ U kontrol rovnosti odkazů dávat přednost možnosti is null + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' Preferovat this. nebo Me. diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf index e926155e837aa..14e518b225048 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf @@ -192,6 +192,11 @@ "is null" für Verweisübereinstimmungsprüfungen vorziehen + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' "this." oder "Me." bevorzugen diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf index 859fbe8d25002..d83b7d1e8fec4 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf @@ -192,6 +192,11 @@ Preferir “is null” para comprobaciones de igualdad de referencias + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' Preferir "this." o "Me." diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf index d043127c5d95d..0c9b5753b01c2 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf @@ -192,6 +192,11 @@ Préférer 'is nul' pour les vérifications d'égalité de référence + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' Préférer 'this.' ou 'Me.' diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf index 1e6ea2c35ebc7..7a6143de9dba5 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf @@ -192,6 +192,11 @@ Preferisci 'is null' per i controlli di uguaglianza dei riferimenti + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' Preferisci 'this.' o 'Me.' diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf index 4de85135e04b2..df6ab84f4b1b9 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf @@ -192,6 +192,11 @@ 参照の等値性のチェックには 'is null' を優先する + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' 'this.' または 'Me' を優先する diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf index 8d3ef95735b22..bb861aeed429a 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf @@ -192,6 +192,11 @@ 참조 같음 검사에 대해 'is null' 선호 + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' 'this.' 또는 'Me.'를 기본으로 사용하세요. diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf index c0b02d84f796e..0597003591579 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf @@ -192,6 +192,11 @@ Preferuj wyrażenie „is null” w przypadku sprawdzeń odwołań pod kątem równości + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' Preferuj zapis „this.” lub „me.” diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pt-BR.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pt-BR.xlf index ca25dc5627759..6af66a8645b95 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pt-BR.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pt-BR.xlf @@ -192,6 +192,11 @@ Preferir 'is null' para as verificações de igualdade de referência + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' Preferir 'this.' ou 'Me.' diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf index 8c401b366167b..9018492b5424d 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf @@ -192,6 +192,11 @@ Использовать "is null" вместо проверки ссылок на равенство. + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' Предпочитать "this." или "Me". diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf index e898ae599474d..858463a06915d 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf @@ -192,6 +192,11 @@ Başvuru eşitliği denetimleri için 'is null'ı tercih et + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' 'this.' veya 'Me.' tercih et diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hans.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hans.xlf index 3733a9f97d464..c043df5a33dae 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hans.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hans.xlf @@ -192,6 +192,11 @@ 引用相等检查偏好 “is null” + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' 首选 "this." 或 "Me." diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hant.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hant.xlf index 74974444712cc..a18c97069d778 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hant.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hant.xlf @@ -192,6 +192,11 @@ 參考相等檢查最好使用 'is null' + + Prefer parameter null checking + Prefer parameter null checking + + Prefer 'this.' or 'Me.' 建議使用 'this.' 或 'Me.' diff --git a/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs b/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs index 02c55dcec4e8c..2a3a094f0b872 100644 --- a/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs +++ b/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs @@ -429,6 +429,9 @@ public void CSharp_VerifyIDEDiagnosticSeveritiesAreConfigurable() # IDE0180 dotnet_diagnostic.IDE0180.severity = %value% +# IDE0190 +dotnet_diagnostic.IDE0190.severity = %value% + # IDE2000 dotnet_diagnostic.IDE2000.severity = %value% diff --git a/src/VisualStudio/CSharp/Impl/CSharpVSResources.resx b/src/VisualStudio/CSharp/Impl/CSharpVSResources.resx index 258958fa8a8b1..40a3b3131603e 100644 --- a/src/VisualStudio/CSharp/Impl/CSharpVSResources.resx +++ b/src/VisualStudio/CSharp/Impl/CSharpVSResources.resx @@ -524,6 +524,9 @@ Prefer 'is null' for reference equality checks 'is null' is a C# string and should not be localized. + + Prefer parameter null checking + Report invalid placeholders in 'string.Format' calls diff --git a/src/VisualStudio/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs b/src/VisualStudio/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs index 1e726b9de6f24..ba1cc5efde3d1 100644 --- a/src/VisualStudio/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs +++ b/src/VisualStudio/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs @@ -103,6 +103,10 @@ private IEnumerable GetNullCheckingCodeStyleOptions(AnalyzerCo description: CSharpVSResources.Prefer_null_check_over_type_check, editorConfigOptions: editorConfigOptions, visualStudioOptions: visualStudioOptions, updater: updaterService, fileName: FileName); + yield return CodeStyleSetting.Create(option: CodeStyleOptions2.PreferParameterNullChecking, + description: CSharpVSResources.Prefer_parameter_null_checking, + editorConfigOptions: editorConfigOptions, + visualStudioOptions: visualStudioOptions, updater: updaterService, fileName: FileName); } private IEnumerable GetModifierCodeStyleOptions(AnalyzerConfigOptions editorConfigOptions, OptionSet visualStudioOptions, OptionUpdater updaterService) diff --git a/src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs b/src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs index d2fbb60139c4b..a72f0dfc8989a 100644 --- a/src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs +++ b/src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs @@ -1122,6 +1122,28 @@ void M2(string value1, string value2) //] }} }} +"; + + private static readonly string s_preferParameterNullChecking = $@" +using System; + +class Customer +{{ +//[ + // {ServicesVSResources.Prefer_colon} + void M1(string value!!) + {{ + }} +//] +//[ + // {ServicesVSResources.Over_colon} + void M2(string value) + {{ + if (value is null) + throw new ArgumentNullException(nameof(value)); + }} +//] +}} "; private static readonly string s_preferNullcheckOverTypeCheck = $@" @@ -2108,6 +2130,7 @@ internal StyleViewModel(OptionStore optionStore, IServiceProvider serviceProvide CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions2.PreferCoalesceExpression, ServicesVSResources.Prefer_coalesce_expression, s_preferCoalesceExpression, s_preferCoalesceExpression, this, optionStore, nullCheckingGroupTitle)); CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions2.PreferNullPropagation, ServicesVSResources.Prefer_null_propagation, s_preferNullPropagation, s_preferNullPropagation, this, optionStore, nullCheckingGroupTitle)); CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions2.PreferIsNullCheckOverReferenceEqualityMethod, CSharpVSResources.Prefer_is_null_for_reference_equality_checks, s_preferIsNullOverReferenceEquals, s_preferIsNullOverReferenceEquals, this, optionStore, nullCheckingGroupTitle)); + CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions2.PreferParameterNullChecking, CSharpVSResources.Prefer_parameter_null_checking, s_preferParameterNullChecking, s_preferParameterNullChecking, this, optionStore, nullCheckingGroupTitle)); CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CSharpCodeStyleOptions.PreferNullCheckOverTypeCheck, CSharpVSResources.Prefer_null_check_over_type_check, s_preferNullcheckOverTypeCheck, s_preferNullcheckOverTypeCheck, this, optionStore, nullCheckingGroupTitle)); // Using directive preferences. diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.cs.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.cs.xlf index bcb0b9dda1e3a..c06cecf28b22f 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.cs.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.cs.xlf @@ -142,6 +142,11 @@ Upřednostňovat kontrolu hodnoty null před kontrolou typu + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Upřednostňovat porovnávání vzorů diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.de.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.de.xlf index c8727c855de95..5939f02eec3c0 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.de.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.de.xlf @@ -142,6 +142,11 @@ "NULL"-Überprüfung vor Typüberprüfung bevorzugen + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Musterabgleich bevorzugen diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.es.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.es.xlf index 9f520f7aa0694..9881a2f526e40 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.es.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.es.xlf @@ -142,6 +142,11 @@ Preferir comprobación "null' sobre comprobación de tipo + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Preferir coincidencia de patrones diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.fr.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.fr.xlf index 4b3d4bbe92c02..bd23ab7804c77 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.fr.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.fr.xlf @@ -142,6 +142,11 @@ Préférer la vérification « null » à la vérification de type + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Préférer les critères spéciaux diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.it.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.it.xlf index 436467d250a55..129b9798d0e8e 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.it.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.it.xlf @@ -142,6 +142,11 @@ Preferisci il controllo 'null' al controllo del tipo + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Preferisci i criteri di ricerca diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ja.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ja.xlf index 74ded6521f820..23702d25d215c 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ja.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ja.xlf @@ -142,6 +142,11 @@ 型のチェックよりも 'null 値' チェックを優先する + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching パターン マッチングを優先する diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ko.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ko.xlf index ae5e528b54aef..7c44d4a628d73 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ko.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ko.xlf @@ -142,6 +142,11 @@ 형식 검사보다 'null' 검사 선호 + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching 패턴 일치 선호 diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pl.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pl.xlf index 230dbc961c0fd..c6fc1fd110317 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pl.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pl.xlf @@ -142,6 +142,11 @@ Ustaw preferencje na sprawdzanie wartości „null” zamiast sprawdzania typu + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Preferuj dopasowywanie do wzorca diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pt-BR.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pt-BR.xlf index c5ae128b775b0..d6544ac27686b 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pt-BR.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.pt-BR.xlf @@ -142,6 +142,11 @@ Preferir a verificação 'nula' à verificação de tipo + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Preferir a correspondência de padrões diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ru.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ru.xlf index c020550f114e7..40f05e0f8675c 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ru.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.ru.xlf @@ -142,6 +142,11 @@ Предпочитать проверку "null" проверке типа + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Предпочитать соответствие шаблону diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.tr.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.tr.xlf index d95ae7aa6044c..c0ec9176998da 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.tr.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.tr.xlf @@ -142,6 +142,11 @@ Tür denetimi yerine 'null' denetimini tercih edin + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching Desen eşleştirmeyi tercih et diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hans.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hans.xlf index 36757db206d37..c6461e38bfd9e 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hans.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hans.xlf @@ -142,6 +142,11 @@ 与类型检查相比，首选 Null 检查 + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching 首选模式匹配 diff --git a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hant.xlf b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hant.xlf index d020b3844fde6..e922ea7b6dfd6 100644 --- a/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hant.xlf +++ b/src/VisualStudio/CSharp/Impl/xlf/CSharpVSResources.zh-Hant.xlf @@ -142,6 +142,11 @@ 建議使用 'null' 檢查而非鍵入檢查 + + Prefer parameter null checking + Prefer parameter null checking + + Prefer pattern matching 建議使用模式比對 diff --git a/src/VisualStudio/Core/Def/EditorConfigSettings/DataProvider/CodeStyle/CommonCodeStyleSettingsProvider.cs b/src/VisualStudio/Core/Def/EditorConfigSettings/DataProvider/CodeStyle/CommonCodeStyleSettingsProvider.cs index f3022ef26f3ab..abcb72609ba77 100644 --- a/src/VisualStudio/Core/Def/EditorConfigSettings/DataProvider/CodeStyle/CommonCodeStyleSettingsProvider.cs +++ b/src/VisualStudio/Core/Def/EditorConfigSettings/DataProvider/CodeStyle/CommonCodeStyleSettingsProvider.cs @@ -109,6 +109,10 @@ private IEnumerable GetNullCheckingCodeStyleOptions(AnalyzerCo description: EditorFeaturesResources.Prefer_is_null_for_reference_equality_checks, editorConfigOptions: options, visualStudioOptions: visualStudioOptions, updater: updater, fileName: FileName); + yield return CodeStyleSetting.Create(option: CodeStyleOptions2.PreferParameterNullChecking, + description: EditorFeaturesResources.Prefer_parameter_null_checking, + editorConfigOptions: options, + visualStudioOptions: visualStudioOptions, updater: updater, fileName: FileName); } private IEnumerable GetModifierCodeStyleOptions(AnalyzerConfigOptions options, OptionSet visualStudioOptions, OptionUpdater updater) diff --git a/src/VisualStudio/Core/Test/Options/BasicEditorConfigGeneratorTests.vb b/src/VisualStudio/Core/Test/Options/BasicEditorConfigGeneratorTests.vb index b2bc3eb08c6d8..9fe4f719a9d76 100644 --- a/src/VisualStudio/Core/Test/Options/BasicEditorConfigGeneratorTests.vb +++ b/src/VisualStudio/Core/Test/Options/BasicEditorConfigGeneratorTests.vb @@ -76,6 +76,7 @@ dotnet_style_prefer_conditional_expression_over_return = true dotnet_style_prefer_inferred_anonymous_type_member_names = true dotnet_style_prefer_inferred_tuple_names = true dotnet_style_prefer_is_null_check_over_reference_equality_method = true +dotnet_style_prefer_parameter_null_checking = true dotnet_style_prefer_simplified_boolean_expressions = true dotnet_style_prefer_simplified_interpolation = true @@ -214,6 +215,7 @@ dotnet_style_prefer_conditional_expression_over_return = true dotnet_style_prefer_inferred_anonymous_type_member_names = true dotnet_style_prefer_inferred_tuple_names = true dotnet_style_prefer_is_null_check_over_reference_equality_method = true +dotnet_style_prefer_parameter_null_checking = true dotnet_style_prefer_simplified_boolean_expressions = true dotnet_style_prefer_simplified_interpolation = true diff --git a/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb b/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb index 9ba945865f45c..4ca624c73218c 100644 --- a/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb +++ b/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb @@ -75,6 +75,7 @@ dotnet_style_prefer_conditional_expression_over_return = true dotnet_style_prefer_inferred_anonymous_type_member_names = true dotnet_style_prefer_inferred_tuple_names = true dotnet_style_prefer_is_null_check_over_reference_equality_method = true +dotnet_style_prefer_parameter_null_checking = true dotnet_style_prefer_simplified_boolean_expressions = true dotnet_style_prefer_simplified_interpolation = true @@ -308,6 +309,7 @@ dotnet_style_prefer_conditional_expression_over_return = true dotnet_style_prefer_inferred_anonymous_type_member_names = true dotnet_style_prefer_inferred_tuple_names = true dotnet_style_prefer_is_null_check_over_reference_equality_method = true +dotnet_style_prefer_parameter_null_checking = true dotnet_style_prefer_simplified_boolean_expressions = true dotnet_style_prefer_simplified_interpolation = true diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs index be5c6225f880a..a9430fceb2571 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions.cs @@ -300,6 +300,12 @@ private static Option2> CreateUsingDirectiv "csharp_style_prefer_null_check_over_type_check", $"TextEditor.CSharp.Specific.{nameof(PreferNullCheckOverTypeCheck)}"); + internal static readonly Option2> PreferParameterNullChecking = CreateOption( + CSharpCodeStyleOptionGroups.NullCheckingPreferences, nameof(PreferParameterNullChecking), + defaultValue: s_trueWithSuggestionEnforcement, + "csharp_style_prefer_parameter_null_checking", + $"TextEditor.CSharp.Specific.{nameof(PreferParameterNullChecking)}"); + public static Option2> AllowEmbeddedStatementsOnSameLine { get; } = CreateOption( CSharpCodeStyleOptionGroups.NewLinePreferences, nameof(AllowEmbeddedStatementsOnSameLine), defaultValue: CodeStyleOptions2.TrueWithSilentEnforcement, From 484d3d484041b25b71b17bf421325608504700fb Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Wed, 5 Jan 2022 08:07:53 -0800 Subject: [PATCH 06/15] Fix more tests --- .../Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs | 3 +++ .../Core/Test/Options/CSharpEditorConfigGeneratorTests.vb | 1 + 2 files changed, 4 insertions(+) diff --git a/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs b/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs index 2a3a094f0b872..b801ef8ce4b43 100644 --- a/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs +++ b/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs @@ -1013,6 +1013,9 @@ No editorconfig based code style option # IDE0180, PreferTupleSwap csharp_style_prefer_tuple_swap = true +# IDE0190, PreferParameterNullChecking +dotnet_style_prefer_parameter_null_checking = true + # IDE1005, PreferConditionalDelegateCall csharp_style_conditional_delegate_call = true diff --git a/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb b/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb index 4ca624c73218c..6ee018f6c3068 100644 --- a/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb +++ b/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb @@ -353,6 +353,7 @@ csharp_style_prefer_switch_expression = true # Null-checking preferences csharp_style_conditional_delegate_call = true +csharp_style_prefer_parameter_null_checking = true # Modifier preferences csharp_prefer_static_local_function = true From 96bcd3e76d2f691b55cf1fbdeabbb459d66ce89d Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Wed, 5 Jan 2022 09:25:42 -0800 Subject: [PATCH 07/15] fix --- .../Core/Test/Options/CSharpEditorConfigGeneratorTests.vb | 1 + 1 file changed, 1 insertion(+) diff --git a/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb b/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb index 6ee018f6c3068..ab5104ddccd82 100644 --- a/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb +++ b/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb @@ -119,6 +119,7 @@ csharp_style_prefer_switch_expression = true # Null-checking preferences csharp_style_conditional_delegate_call = true +csharp_style_prefer_parameter_null_checking = true # Modifier preferences csharp_prefer_static_local_function = true From 553d946efa1319371dbfd56cfaccbb9075e6ee18 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 7 Jan 2022 09:40:56 -0800 Subject: [PATCH 08/15] Handle operators and 0-param exception ctor. More tests. --- ...ParameterNullCheckingDiagnosticAnalyzer.cs | 148 +++--- .../UseParameterNullCheckingTests.cs | 471 +++++++++++++++++- 2 files changed, 543 insertions(+), 76 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index d80fcf67bcff3..d51c2b11ec766 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -3,7 +3,6 @@ // 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; @@ -46,11 +45,29 @@ protected override void InitializeWorker(AnalysisContext context) 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) + IMethodSymbol? argumentNullExceptionConstructor = null; + IMethodSymbol? argumentNullExceptionStringConstructor = null; + foreach (var constructor in argumentNullException.InstanceConstructors) + { + if (argumentNullExceptionConstructor is not null && argumentNullExceptionStringConstructor is not null) + { + break; + } + + switch (constructor) + { + case { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 0 }: + argumentNullExceptionConstructor = constructor; + break; + case { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 1 } + when constructor.Parameters[0].Type.SpecialType == SpecialType.System_String: + + argumentNullExceptionStringConstructor = constructor; + break; + } + } + + if (argumentNullExceptionConstructor is null || argumentNullExceptionStringConstructor is null) { return; } @@ -60,17 +77,25 @@ protected override void InitializeWorker(AnalysisContext context) .GetMembers(nameof(ReferenceEquals)) .FirstOrDefault(m => m is IMethodSymbol { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 2 }); + // We are potentially interested in any declaration that has parameters. + // However, we avoid indexers specifically because of the complexity of locating and deleting equivalent null checks across multiple accessors. context.RegisterSyntaxNodeAction( - context => AnalyzeSyntax(context, argumentNullExceptionConstructor, referenceEqualsMethod), + context => AnalyzeSyntax(context, argumentNullExceptionConstructor, argumentNullExceptionStringConstructor, referenceEqualsMethod), SyntaxKind.ConstructorDeclaration, SyntaxKind.MethodDeclaration, SyntaxKind.LocalFunctionStatement, SyntaxKind.SimpleLambdaExpression, SyntaxKind.ParenthesizedLambdaExpression, - SyntaxKind.AnonymousMethodExpression); + SyntaxKind.AnonymousMethodExpression, + SyntaxKind.OperatorDeclaration, + SyntaxKind.ConversionOperatorDeclaration); }); - private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argumentNullExceptionConstructor, IMethodSymbol? referenceEqualsMethod) + private void AnalyzeSyntax( + SyntaxNodeAnalysisContext context, + IMethodSymbol argumentNullExceptionConstructor, + IMethodSymbol argumentNullExceptionStringConstructor, + IMethodSymbol? referenceEqualsMethod) { var cancellationToken = context.CancellationToken; @@ -90,8 +115,12 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu ConstructorDeclarationSyntax constructorDecl => constructorDecl.Body, LocalFunctionStatementSyntax localFunctionStatement => localFunctionStatement.Body, AnonymousFunctionExpressionSyntax anonymousFunction => anonymousFunction.Block, + OperatorDeclarationSyntax operatorDecl => operatorDecl.Body, + ConversionOperatorDeclarationSyntax conversionDecl => conversionDecl.Body, _ => throw ExceptionUtilities.UnexpectedValue(node) }; + + // More scenarios should be supported eventually: https://github.com/dotnet/roslyn/issues/58699 if (block is null) { return; @@ -107,7 +136,28 @@ private void AnalyzeSyntax(SyntaxNodeAnalysisContext context, IMethodSymbol argu foreach (var statement in block.Statements) { - IParameterSymbol? parameter; + var parameter = TryGetParameterNullCheckedByStatement(statement); + if (parameter is not null + && (!parameter.Type.IsValueType + || parameter.Type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T + || parameter.Type.TypeKind is TypeKind.Pointer or TypeKind.FunctionPointer) + && parameter.DeclaringSyntaxReferences.FirstOrDefault() is SyntaxReference reference + && reference.SyntaxTree.Equals(statement.SyntaxTree) + && reference.GetSyntax() is ParameterSyntax parameterSyntax) + { + context.ReportDiagnostic(DiagnosticHelper.Create( + Descriptor, + statement.GetLocation(), + option.Notification.Severity, + additionalLocations: new[] { parameterSyntax.GetLocation() }, + properties: null)); + } + } + + return; + + IParameterSymbol? TryGetParameterNullCheckedByStatement(StatementSyntax statement) + { switch (statement) { // if (param == null) { throw new ArgumentNullException(nameof(param)); } @@ -136,13 +186,15 @@ when semanticModel.GetSymbolInfo(binary).Symbol is IMethodSymbol { MethodKind: M break; default: - continue; + return null; } - if (!AreOperandsApplicable(left, right, out parameter) - && !AreOperandsApplicable(right, left, out parameter)) + var parameterInBinary = left.IsKind(SyntaxKind.NullLiteralExpression) ? TryGetParameter(right) + : right.IsKind(SyntaxKind.NullLiteralExpression) ? TryGetParameter(left) + : null; + if (parameterInBinary is null) { - continue; + return null; } var throwStatement = ifStatement.Statement switch @@ -153,12 +205,12 @@ when semanticModel.GetSymbolInfo(binary).Symbol is IMethodSymbol { MethodKind: M }; if (throwStatement?.Expression is not ObjectCreationExpressionSyntax thrownInIf - || !IsConstructorApplicable(thrownInIf, parameter)) + || !IsConstructorApplicable(thrownInIf, parameterInBinary)) { - continue; + return null; } - break; + return parameterInBinary; // this.field = param ?? throw new ArgumentNullException(nameof(param)); case ExpressionStatementSyntax @@ -173,44 +225,20 @@ when semanticModel.GetSymbolInfo(binary).Symbol is IMethodSymbol { MethodKind: M } } }: - if (!IsParameter(maybeParameter, out parameter) || !IsConstructorApplicable(thrownInNullCoalescing, parameter)) + var coalescedParameter = TryGetParameter(maybeParameter); + if (coalescedParameter is null || !IsConstructorApplicable(thrownInNullCoalescing, coalescedParameter)) { - continue; + return null; } - break; + return coalescedParameter; default: - continue; + return null; } - - if (parameter.DeclaringSyntaxReferences.FirstOrDefault() is SyntaxReference reference - && reference.SyntaxTree.Equals(statement.SyntaxTree) - && reference.GetSyntax() is ParameterSyntax parameterSyntax) - { - context.ReportDiagnostic(DiagnosticHelper.Create( - Descriptor, - statement.GetLocation(), - option.Notification.Severity, - additionalLocations: new[] { parameterSyntax.GetLocation() }, - properties: null)); - } - } - - return; - - 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) + IParameterSymbol? TryGetParameter(ExpressionSyntax maybeParameter) { // `(object)x == null` is often used to ensure reference equality is used. // therefore, we specially unwrap casts when the cast is to `object`. @@ -218,8 +246,7 @@ bool IsParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParam { if (semanticModel.GetTypeInfo(type).Type?.SpecialType != SpecialType.System_Object) { - parameter = null; - return false; + return null; } maybeParameter = operand; @@ -227,33 +254,38 @@ bool IsParameter(ExpressionSyntax maybeParameter, [NotNullWhen(true)] out IParam if (semanticModel.GetSymbolInfo(maybeParameter).Symbol is not IParameterSymbol { ContainingSymbol: { } containingSymbol } parameterSymbol || !containingSymbol.Equals(methodSymbol)) { - parameter = null; - return false; + return null; } - parameter = parameterSymbol; - return true; + return parameterSymbol; } bool IsConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, IParameterSymbol parameterSymbol) { - if (exceptionCreation.ArgumentList?.Arguments.FirstOrDefault() is not { } argument) + if (exceptionCreation.ArgumentList?.Arguments is not { } arguments) { return false; } - var constantValue = semanticModel.GetConstantValue(argument.Expression, cancellationToken); - if (constantValue.Value is not string constantString || !string.Equals(constantString, parameterSymbol.Name, StringComparison.Ordinal)) + if (arguments.Count == 0) + { + // 'new ArgumentNullException()' + return argumentNullExceptionConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol); + } + + if (arguments.Count != 1) { return false; } - if (!argumentNullExceptionConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol)) + // 'new ArgumentNullException(nameof(param))' (or equivalent) + var constantValue = semanticModel.GetConstantValue(arguments[0].Expression, cancellationToken); + if (constantValue.Value is not string constantString || !string.Equals(constantString, parameterSymbol.Name, StringComparison.Ordinal)) { return false; } - return true; + return argumentNullExceptionStringConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol); } } } diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index e251443fb4e3c..363352b8ba2df 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -210,9 +210,74 @@ public C(string s!!) }.RunAsync(); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestOperator() + { + await new VerifyCS.Test() + { + TestCode = @" +using System; + +class C +{ + public static C operator +(C c, string s) + { + [|if (s is null) + throw new ArgumentNullException(nameof(s));|] + + return new C(); + } +}", + FixedCode = @" +using System; + +class C +{ + public static C operator +(C c, string s!!) + { + return new C(); + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestConversionOperator() + { + await new VerifyCS.Test() + { + TestCode = @" +using System; + +class C +{ + public static implicit operator C(string s) + { + [|if (s is null) + throw new ArgumentNullException(nameof(s));|] + + return new C(); + } +}", + FixedCode = @" +using System; + +class C +{ + public static implicit operator C(string s!!) + { + return new C(); + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNullCoalescingThrowExpressionBody() { + // We'd like to support this eventually. https://github.com/dotnet/roslyn/issues/58699 var testCode = @" using System; @@ -228,9 +293,166 @@ public C(string s) FixedCode = testCode, LanguageVersion = LanguageVersionExtensions.CSharpNext }.RunAsync(); + } - // TODO: the fixer should work in this scenario -#if false + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNullCoalescingThrowBaseClause() + { + // We'd like to support this eventually. https://github.com/dotnet/roslyn/issues/58699 + var testCode = @" +using System; + +class Base { public Base(string s) { } } +class C : Base +{ + public C(string s) : base(s ?? throw new ArgumentNullException(nameof(s))) { } +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestAlreadyNullChecked() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + public C(string s!!) + { + [|if (s is null) + throw new ArgumentNullException(nameof(s));|] + } +}", + FixedCode = @"using System; + +class C +{ + public C(string s!!) + { + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestSingleLineNoBraces() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + public C(string s) + { + [|if (s is null) throw new ArgumentNullException(nameof(s));|] + } +}", + FixedCode = @"using System; + +class C +{ + public C(string s!!) + { + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestSingleLineWithBraces() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + public C(string s) + { + [|if (s is null) { throw new ArgumentNullException(nameof(s)); }|] + } +}", + FixedCode = @"using System; + +class C +{ + public C(string s!!) + { + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestLeadingAndTrailingTrivia() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + public C(string s) + { + // comment1 + [|if (s is null) { throw new ArgumentNullException(nameof(s)); }|] // comment2 + } +}", + FixedCode = @"using System; + +class C +{ + public C(string s!!) + { + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestAssignThenTest() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + public C(string s) + { + s = ""a""; + [|if (s is null) { throw new ArgumentNullException(nameof(s)); }|] + } +}", + FixedCode = @"using System; + +class C +{ + public C(string s!!) + { + s = ""a""; + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestRedundantNullChecks() + { + // TODO: what test framework settings do we use here? await new VerifyCS.Test() { TestCode = @" @@ -238,22 +460,40 @@ public C(string s) class C { - private readonly string s; public C(string s) - => [|this.s = s ?? throw new ArgumentNullException(nameof(s));|] + { + [|if (s is null) + throw new ArgumentNullException(nameof(s));|] + + [|if (s is null) + throw new ArgumentNullException(nameof(s));|] + } }", - FixedCode = @" + FixedState = + { + Sources = + { + @" using System; class C { - private readonly string s; public C(string s!!) - => this.s = s; + { + if (s is null) + throw new ArgumentNullException(nameof(s)); + } }", - LanguageVersion = LanguageVersionExtensions.CSharpNext + }, + ExpectedDiagnostics = + { + // /0/Test0.cs(8,9): info IDE0190: Null check can be simplified + VerifyCS.Diagnostic("IDE0190").WithSeverity(DiagnosticSeverity.Info).WithSpan(8, 9, 9, 56).WithSpan(6, 14, 6, 24) + } + }, + LanguageVersion = LanguageVersionExtensions.CSharpNext, + NumberOfFixAllIterations = 1 }.RunAsync(); -#endif } [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] @@ -476,6 +716,47 @@ void M(T s!!) where T : class }.RunAsync(); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestOnStructConstrainedTypeParameter() + { + await new VerifyCS.Test() + { + TestCode = @" +using System; + +class C +{ + void M1(T s) where T : struct + { + if ((object)s == null) + throw new ArgumentNullException(nameof(s)); + } + + void M2(T? s) where T : struct + { + [|if ((object)s == null) + throw new ArgumentNullException(nameof(s));|] + } +}", + FixedCode = @" +using System; + +class C +{ + void M1(T s) where T : struct + { + if ((object)s == null) + throw new ArgumentNullException(nameof(s)); + } + + void M2(T? s!!) where T : struct + { + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestUnconstrainedTypeParameter() { @@ -649,22 +930,28 @@ void M(string s) } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] - public async Task TestNotWithExceptionNoArguments() + public async Task TestWithExceptionNoArguments() { - var testCode = @"using System; + await new VerifyCS.Test() + { + TestCode = @"using System; class C { void M(string s) { - if (s == null) - throw new ArgumentNullException(); + [|if (s == null) + throw new ArgumentNullException();|] } -}"; - await new VerifyCS.Test() - { - TestCode = testCode, - FixedCode = testCode, +}", + FixedCode = @"using System; + +class C +{ + void M(string s!!) + { + } +}", LanguageVersion = LanguageVersionExtensions.CSharpNext }.RunAsync(); } @@ -900,6 +1187,39 @@ static void M(C c!!) }.RunAsync(); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestIsWithUnusedUserDefinedOperator() + { + await new VerifyCS.Test() + { + TestCode = @"using System; +class C +{ + public static bool operator ==(C c1, C c2) => true; + public static bool operator !=(C c1, C c2) => false; + + static void M(C c) + { + [|if (c is null) + throw new ArgumentNullException(nameof(c));|] + } +} +", + FixedCode = @"using System; +class C +{ + public static bool operator ==(C c1, C c2) => true; + public static bool operator !=(C c1, C c2) => false; + + static void M(C c!!) + { + } +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestWithPreprocessorDirective() { @@ -930,6 +1250,121 @@ static void M(C c!!) }.RunAsync(); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestWithIfPreprocessorDirective() + { + await new VerifyCS.Test() + { + TestCode = @"#define DEBUG +using System; +class C +{ + static void M(C c) + { +#if DEBUG + [|if ((object)c == null) + throw new ArgumentNullException(nameof(c));|] +#endif + } +} +", + FixedCode = @"#define DEBUG +using System; +class C +{ + static void M(C c!!) + { + +#if DEBUG +#endif + } +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestWithAlias() + { + await new VerifyCS.Test() + { + TestCode = @"using Ex = System.ArgumentNullException; +class C +{ + static void M(C c) + { + [|if ((object)c == null) + throw new Ex(nameof(c));|] + } +} +", + FixedCode = @"using Ex = System.ArgumentNullException; +class C +{ + static void M(C c!!) + { + } +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestPointer() + { + await new VerifyCS.Test() + { + TestCode = @"using System; +class C +{ + static unsafe void M(int* ptr) + { + [|if (ptr == null) + throw new ArgumentNullException(nameof(ptr));|] + } +} +", + FixedCode = @"using System; +class C +{ + static unsafe void M(int* ptr!!) + { + } +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestFunctionPointer() + { + await new VerifyCS.Test() + { + TestCode = @"using System; +class C +{ + static unsafe void M(delegate* ptr) + { + [|if (ptr == null) + throw new ArgumentNullException(nameof(ptr));|] + } +} +", + FixedCode = @"using System; +class C +{ + static unsafe void M(delegate* ptr!!) + { + } +} +", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestPartialMethod() { From 31acd9c7639362ec24dc59e0364da554bb70a5b4 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 7 Jan 2022 10:46:21 -0800 Subject: [PATCH 09/15] Adjust feature config (WIP) --- .../CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs | 5 +++-- src/EditorFeatures/Core/EditorFeaturesResources.resx | 3 --- src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf | 5 ----- .../Core/xlf/EditorFeaturesResources.pt-BR.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf | 5 ----- src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf | 5 ----- .../Core/xlf/EditorFeaturesResources.zh-Hans.xlf | 5 ----- .../Core/xlf/EditorFeaturesResources.zh-Hant.xlf | 5 ----- .../Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs | 2 +- .../CodeStyle/CSharpCodeStyleSettingsProvider.cs | 2 +- .../Impl/Options/AutomationObject/AutomationObject.Style.cs | 4 ++-- .../CSharp/Impl/Options/Formatting/StyleViewModel.cs | 2 +- .../CodeStyle/CommonCodeStyleSettingsProvider.cs | 4 ---- .../Core/Test/Options/BasicEditorConfigGeneratorTests.vb | 2 -- .../Core/Test/Options/CSharpEditorConfigGeneratorTests.vb | 2 -- .../Compiler/Core/CodeStyle/CodeStyleOptions2.cs | 6 ------ 23 files changed, 8 insertions(+), 89 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index d51c2b11ec766..97ca96ce9a573 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -5,6 +5,7 @@ using System; using System.Linq; using Microsoft.CodeAnalysis.CodeStyle; +using Microsoft.CodeAnalysis.CSharp.CodeStyle; using Microsoft.CodeAnalysis.CSharp.Shared.Extensions; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; @@ -21,7 +22,7 @@ internal sealed class CSharpUseParameterNullCheckingDiagnosticAnalyzer public CSharpUseParameterNullCheckingDiagnosticAnalyzer() : base(IDEDiagnosticIds.UseParameterNullCheckingId, EnforceOnBuildValues.UseParameterNullChecking, - CodeStyleOptions2.PreferParameterNullChecking, + CSharpCodeStyleOptions.PreferParameterNullChecking, CSharpAnalyzersResources.Use_parameter_null_checking, new LocalizableResourceString(nameof(AnalyzersResources.Null_check_can_be_simplified), AnalyzersResources.ResourceManager, typeof(AnalyzersResources))) { @@ -102,7 +103,7 @@ private void AnalyzeSyntax( var semanticModel = context.SemanticModel; var syntaxTree = semanticModel.SyntaxTree; - var option = context.Options.GetOption(CodeStyleOptions2.PreferParameterNullChecking, semanticModel.Language, syntaxTree, cancellationToken); + var option = context.Options.GetOption(CSharpCodeStyleOptions.PreferParameterNullChecking, syntaxTree, cancellationToken); if (!option.Value) { return; diff --git a/src/EditorFeatures/Core/EditorFeaturesResources.resx b/src/EditorFeatures/Core/EditorFeaturesResources.resx index 0c792ec87a5cb..ee32fef772903 100644 --- a/src/EditorFeatures/Core/EditorFeaturesResources.resx +++ b/src/EditorFeatures/Core/EditorFeaturesResources.resx @@ -975,9 +975,6 @@ Do you want to proceed? Prefer 'is null' for reference equality checks - - Prefer parameter null checking - Prefer 'this.' or 'Me.' diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf index 2f1472081b9b4..b55de38bbb9d5 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.cs.xlf @@ -192,11 +192,6 @@ U kontrol rovnosti odkazů dávat přednost možnosti is null - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' Preferovat this. nebo Me. diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf index 14e518b225048..e926155e837aa 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.de.xlf @@ -192,11 +192,6 @@ "is null" für Verweisübereinstimmungsprüfungen vorziehen - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' "this." oder "Me." bevorzugen diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf index d83b7d1e8fec4..859fbe8d25002 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.es.xlf @@ -192,11 +192,6 @@ Preferir “is null” para comprobaciones de igualdad de referencias - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' Preferir "this." o "Me." diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf index 0c9b5753b01c2..d043127c5d95d 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.fr.xlf @@ -192,11 +192,6 @@ Préférer 'is nul' pour les vérifications d'égalité de référence - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' Préférer 'this.' ou 'Me.' diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf index 7a6143de9dba5..1e6ea2c35ebc7 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.it.xlf @@ -192,11 +192,6 @@ Preferisci 'is null' per i controlli di uguaglianza dei riferimenti - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' Preferisci 'this.' o 'Me.' diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf index df6ab84f4b1b9..4de85135e04b2 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ja.xlf @@ -192,11 +192,6 @@ 参照の等値性のチェックには 'is null' を優先する - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' 'this.' または 'Me' を優先する diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf index bb861aeed429a..8d3ef95735b22 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ko.xlf @@ -192,11 +192,6 @@ 참조 같음 검사에 대해 'is null' 선호 - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' 'this.' 또는 'Me.'를 기본으로 사용하세요. diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf index 0597003591579..c0b02d84f796e 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pl.xlf @@ -192,11 +192,6 @@ Preferuj wyrażenie „is null” w przypadku sprawdzeń odwołań pod kątem równości - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' Preferuj zapis „this.” lub „me.” diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pt-BR.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pt-BR.xlf index 6af66a8645b95..ca25dc5627759 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pt-BR.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.pt-BR.xlf @@ -192,11 +192,6 @@ Preferir 'is null' para as verificações de igualdade de referência - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' Preferir 'this.' ou 'Me.' diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf index 9018492b5424d..8c401b366167b 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.ru.xlf @@ -192,11 +192,6 @@ Использовать "is null" вместо проверки ссылок на равенство. - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' Предпочитать "this." или "Me". diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf index 858463a06915d..e898ae599474d 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.tr.xlf @@ -192,11 +192,6 @@ Başvuru eşitliği denetimleri için 'is null'ı tercih et - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' 'this.' veya 'Me.' tercih et diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hans.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hans.xlf index c043df5a33dae..3733a9f97d464 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hans.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hans.xlf @@ -192,11 +192,6 @@ 引用相等检查偏好 “is null” - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' 首选 "this." 或 "Me." diff --git a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hant.xlf b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hant.xlf index a18c97069d778..74974444712cc 100644 --- a/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hant.xlf +++ b/src/EditorFeatures/Core/xlf/EditorFeaturesResources.zh-Hant.xlf @@ -192,11 +192,6 @@ 參考相等檢查最好使用 'is null' - - Prefer parameter null checking - Prefer parameter null checking - - Prefer 'this.' or 'Me.' 建議使用 'this.' 或 'Me.' diff --git a/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs b/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs index b801ef8ce4b43..5d8b95a977999 100644 --- a/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs +++ b/src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs @@ -1014,7 +1014,7 @@ No editorconfig based code style option csharp_style_prefer_tuple_swap = true # IDE0190, PreferParameterNullChecking -dotnet_style_prefer_parameter_null_checking = true +csharp_style_prefer_parameter_null_checking = true # IDE1005, PreferConditionalDelegateCall csharp_style_conditional_delegate_call = true diff --git a/src/VisualStudio/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs b/src/VisualStudio/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs index ba1cc5efde3d1..5a391c3d78d37 100644 --- a/src/VisualStudio/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs +++ b/src/VisualStudio/CSharp/Impl/EditorConfigSettings/DataProvider/CodeStyle/CSharpCodeStyleSettingsProvider.cs @@ -103,7 +103,7 @@ private IEnumerable GetNullCheckingCodeStyleOptions(AnalyzerCo description: CSharpVSResources.Prefer_null_check_over_type_check, editorConfigOptions: editorConfigOptions, visualStudioOptions: visualStudioOptions, updater: updaterService, fileName: FileName); - yield return CodeStyleSetting.Create(option: CodeStyleOptions2.PreferParameterNullChecking, + yield return CodeStyleSetting.Create(option: CSharpCodeStyleOptions.PreferParameterNullChecking, description: CSharpVSResources.Prefer_parameter_null_checking, editorConfigOptions: editorConfigOptions, visualStudioOptions: visualStudioOptions, updater: updaterService, fileName: FileName); diff --git a/src/VisualStudio/CSharp/Impl/Options/AutomationObject/AutomationObject.Style.cs b/src/VisualStudio/CSharp/Impl/Options/AutomationObject/AutomationObject.Style.cs index 041f6a95da0d7..f4b23b785a1b0 100644 --- a/src/VisualStudio/CSharp/Impl/Options/AutomationObject/AutomationObject.Style.cs +++ b/src/VisualStudio/CSharp/Impl/Options/AutomationObject/AutomationObject.Style.cs @@ -239,8 +239,8 @@ public string Style_PreferIsNullCheckOverReferenceEqualityMethod public string Style_PreferParameterNullChecking { - get { return GetXmlOption(CodeStyleOptions2.PreferParameterNullChecking); } - set { SetXmlOption(CodeStyleOptions2.PreferParameterNullChecking, value); } + get { return GetXmlOption(CSharpCodeStyleOptions.PreferParameterNullChecking); } + set { SetXmlOption(CSharpCodeStyleOptions.PreferParameterNullChecking, value); } } public string Style_PreferNullCheckOverTypeCheck diff --git a/src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs b/src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs index a72f0dfc8989a..4a4d360b1675b 100644 --- a/src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs +++ b/src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs @@ -2130,7 +2130,7 @@ internal StyleViewModel(OptionStore optionStore, IServiceProvider serviceProvide CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions2.PreferCoalesceExpression, ServicesVSResources.Prefer_coalesce_expression, s_preferCoalesceExpression, s_preferCoalesceExpression, this, optionStore, nullCheckingGroupTitle)); CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions2.PreferNullPropagation, ServicesVSResources.Prefer_null_propagation, s_preferNullPropagation, s_preferNullPropagation, this, optionStore, nullCheckingGroupTitle)); CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions2.PreferIsNullCheckOverReferenceEqualityMethod, CSharpVSResources.Prefer_is_null_for_reference_equality_checks, s_preferIsNullOverReferenceEquals, s_preferIsNullOverReferenceEquals, this, optionStore, nullCheckingGroupTitle)); - CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions2.PreferParameterNullChecking, CSharpVSResources.Prefer_parameter_null_checking, s_preferParameterNullChecking, s_preferParameterNullChecking, this, optionStore, nullCheckingGroupTitle)); + CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CSharpCodeStyleOptions.PreferParameterNullChecking, CSharpVSResources.Prefer_parameter_null_checking, s_preferParameterNullChecking, s_preferParameterNullChecking, this, optionStore, nullCheckingGroupTitle)); CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CSharpCodeStyleOptions.PreferNullCheckOverTypeCheck, CSharpVSResources.Prefer_null_check_over_type_check, s_preferNullcheckOverTypeCheck, s_preferNullcheckOverTypeCheck, this, optionStore, nullCheckingGroupTitle)); // Using directive preferences. diff --git a/src/VisualStudio/Core/Def/EditorConfigSettings/DataProvider/CodeStyle/CommonCodeStyleSettingsProvider.cs b/src/VisualStudio/Core/Def/EditorConfigSettings/DataProvider/CodeStyle/CommonCodeStyleSettingsProvider.cs index abcb72609ba77..f3022ef26f3ab 100644 --- a/src/VisualStudio/Core/Def/EditorConfigSettings/DataProvider/CodeStyle/CommonCodeStyleSettingsProvider.cs +++ b/src/VisualStudio/Core/Def/EditorConfigSettings/DataProvider/CodeStyle/CommonCodeStyleSettingsProvider.cs @@ -109,10 +109,6 @@ private IEnumerable GetNullCheckingCodeStyleOptions(AnalyzerCo description: EditorFeaturesResources.Prefer_is_null_for_reference_equality_checks, editorConfigOptions: options, visualStudioOptions: visualStudioOptions, updater: updater, fileName: FileName); - yield return CodeStyleSetting.Create(option: CodeStyleOptions2.PreferParameterNullChecking, - description: EditorFeaturesResources.Prefer_parameter_null_checking, - editorConfigOptions: options, - visualStudioOptions: visualStudioOptions, updater: updater, fileName: FileName); } private IEnumerable GetModifierCodeStyleOptions(AnalyzerConfigOptions options, OptionSet visualStudioOptions, OptionUpdater updater) diff --git a/src/VisualStudio/Core/Test/Options/BasicEditorConfigGeneratorTests.vb b/src/VisualStudio/Core/Test/Options/BasicEditorConfigGeneratorTests.vb index 9fe4f719a9d76..b2bc3eb08c6d8 100644 --- a/src/VisualStudio/Core/Test/Options/BasicEditorConfigGeneratorTests.vb +++ b/src/VisualStudio/Core/Test/Options/BasicEditorConfigGeneratorTests.vb @@ -76,7 +76,6 @@ dotnet_style_prefer_conditional_expression_over_return = true dotnet_style_prefer_inferred_anonymous_type_member_names = true dotnet_style_prefer_inferred_tuple_names = true dotnet_style_prefer_is_null_check_over_reference_equality_method = true -dotnet_style_prefer_parameter_null_checking = true dotnet_style_prefer_simplified_boolean_expressions = true dotnet_style_prefer_simplified_interpolation = true @@ -215,7 +214,6 @@ dotnet_style_prefer_conditional_expression_over_return = true dotnet_style_prefer_inferred_anonymous_type_member_names = true dotnet_style_prefer_inferred_tuple_names = true dotnet_style_prefer_is_null_check_over_reference_equality_method = true -dotnet_style_prefer_parameter_null_checking = true dotnet_style_prefer_simplified_boolean_expressions = true dotnet_style_prefer_simplified_interpolation = true diff --git a/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb b/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb index ab5104ddccd82..0fd050a669a7b 100644 --- a/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb +++ b/src/VisualStudio/Core/Test/Options/CSharpEditorConfigGeneratorTests.vb @@ -75,7 +75,6 @@ dotnet_style_prefer_conditional_expression_over_return = true dotnet_style_prefer_inferred_anonymous_type_member_names = true dotnet_style_prefer_inferred_tuple_names = true dotnet_style_prefer_is_null_check_over_reference_equality_method = true -dotnet_style_prefer_parameter_null_checking = true dotnet_style_prefer_simplified_boolean_expressions = true dotnet_style_prefer_simplified_interpolation = true @@ -310,7 +309,6 @@ dotnet_style_prefer_conditional_expression_over_return = true dotnet_style_prefer_inferred_anonymous_type_member_names = true dotnet_style_prefer_inferred_tuple_names = true dotnet_style_prefer_is_null_check_over_reference_equality_method = true -dotnet_style_prefer_parameter_null_checking = true dotnet_style_prefer_simplified_boolean_expressions = true dotnet_style_prefer_simplified_interpolation = true diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs index ab188eacddebf..5970fc48fd2c3 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs @@ -215,12 +215,6 @@ 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, From c4e1150b545767805ff8244134b26258c99edd9f Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 7 Jan 2022 11:36:58 -0800 Subject: [PATCH 10/15] Update test (WIP) --- .../UseParameterNullCheckingTests.cs | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index 363352b8ba2df..ed5f2a598b095 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -452,7 +452,14 @@ public C(string s!!) [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestRedundantNullChecks() { - // TODO: what test framework settings do we use here? + // Produces the following error: + // Message:  + // System.InvalidOperationException : GetCurrentNode returned null with the following node: string s - line 321 + // Stack Trace:  + // Contract.Fail(String message, Int32 lineNumber) line 115 + // ReplaceChange.Apply(SyntaxNode root, SyntaxGenerator generator) line 321 + // SyntaxEditor.GetChangedRoot() line 92 + // d__5.MoveNext() line 71 await new VerifyCS.Test() { TestCode = @" @@ -469,30 +476,17 @@ public C(string s) throw new ArgumentNullException(nameof(s));|] } }", - FixedState = - { - Sources = - { - @" + FixedCode = +@" using System; class C { public C(string s!!) { - if (s is null) - throw new ArgumentNullException(nameof(s)); } }", - }, - ExpectedDiagnostics = - { - // /0/Test0.cs(8,9): info IDE0190: Null check can be simplified - VerifyCS.Diagnostic("IDE0190").WithSeverity(DiagnosticSeverity.Info).WithSpan(8, 9, 9, 56).WithSpan(6, 14, 6, 24) - } - }, - LanguageVersion = LanguageVersionExtensions.CSharpNext, - NumberOfFixAllIterations = 1 + LanguageVersion = LanguageVersionExtensions.CSharpNext }.RunAsync(); } From f961f3354295992efe4d1444c0c6d4650fca8ef5 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 7 Jan 2022 12:00:56 -0800 Subject: [PATCH 11/15] Fix test --- .../CSharpUseParameterNullCheckingCodeFixProvider.cs | 12 ++++++++++-- .../UseParameterNullCheckingTests.cs | 8 -------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs index ff65e1b1ab43e..c103653fa3751 100644 --- a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs @@ -13,6 +13,7 @@ using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Editing; using Microsoft.CodeAnalysis.Host.Mef; +using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; @@ -47,6 +48,9 @@ protected override Task FixAllAsync( Document document, ImmutableArray diagnostics, SyntaxEditor editor, CancellationToken cancellationToken) { + // Tracking parameters which have already been fixed by a fix-all operation. + // This avoids crashing the fixer when the same parameter is null-tested multiple times. + using var _ = PooledHashSet.GetInstance(out var fixedParameterLocations); foreach (var diagnostic in diagnostics) { var node = diagnostic.Location.FindNode(getInnermostNodeForTie: true, cancellationToken: cancellationToken); @@ -66,8 +70,12 @@ protected override Task FixAllAsync( throw ExceptionUtilities.UnexpectedValue(node); } - var parameterSyntax = (ParameterSyntax)diagnostic.AdditionalLocations[0].FindNode(cancellationToken); - editor.ReplaceNode(parameterSyntax, parameterSyntax.WithExclamationExclamationToken(SyntaxFactory.Token(SyntaxKind.ExclamationExclamationToken))); + var parameterLocation = diagnostic.AdditionalLocations[0]; + if (fixedParameterLocations.Add(parameterLocation)) + { + var parameterSyntax = (ParameterSyntax)parameterLocation.FindNode(cancellationToken); + editor.ReplaceNode(parameterSyntax, parameterSyntax.WithExclamationExclamationToken(SyntaxFactory.Token(SyntaxKind.ExclamationExclamationToken))); + } } return Task.CompletedTask; diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index ed5f2a598b095..95ba263c31d17 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -452,14 +452,6 @@ public C(string s!!) [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestRedundantNullChecks() { - // Produces the following error: - // Message:  - // System.InvalidOperationException : GetCurrentNode returned null with the following node: string s - line 321 - // Stack Trace:  - // Contract.Fail(String message, Int32 lineNumber) line 115 - // ReplaceChange.Apply(SyntaxNode root, SyntaxGenerator generator) line 321 - // SyntaxEditor.GetChangedRoot() line 92 - // d__5.MoveNext() line 71 await new VerifyCS.Test() { TestCode = @" From 5a759d738ffbb7e0b35a4371ae11ac8580ee5a68 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 7 Jan 2022 12:52:46 -0800 Subject: [PATCH 12/15] Handle by-ref parameters --- ...ParameterNullCheckingDiagnosticAnalyzer.cs | 1 + .../UseParameterNullCheckingTests.cs | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index 97ca96ce9a573..a05d68464880d 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -142,6 +142,7 @@ private void AnalyzeSyntax( && (!parameter.Type.IsValueType || parameter.Type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T || parameter.Type.TypeKind is TypeKind.Pointer or TypeKind.FunctionPointer) + && parameter.RefKind == RefKind.None && parameter.DeclaringSyntaxReferences.FirstOrDefault() is SyntaxReference reference && reference.SyntaxTree.Equals(statement.SyntaxTree) && reference.GetSyntax() is ParameterSyntax parameterSyntax) diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index 95ba263c31d17..5d376e7c6087a 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -482,6 +482,34 @@ public C(string s!!) }.RunAsync(); } + [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + [InlineData("ref")] + [InlineData("in")] + [InlineData("out")] + public async Task TestRefParameter(string refKind) + { + // https://github.com/dotnet/roslyn/issues/58699 + // When the implementation changes to permit ref/in parameters, we should also change the fixer. + var testCode = @" +using System; + +class C +{ + public C(" + refKind + @" string s) + { + if (s is null) + throw new ArgumentNullException(nameof(s)); + } +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + CompilerDiagnostics = Testing.CompilerDiagnostics.None, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] [InlineData("object")] [InlineData("C")] From d4a1407eb61a3bed8a9b944f6913e9ec63ddef91 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 7 Jan 2022 14:43:38 -0800 Subject: [PATCH 13/15] Fix base call --- .../CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index a05d68464880d..12f25cb7cfb97 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -23,7 +23,7 @@ public CSharpUseParameterNullCheckingDiagnosticAnalyzer() : base(IDEDiagnosticIds.UseParameterNullCheckingId, EnforceOnBuildValues.UseParameterNullChecking, CSharpCodeStyleOptions.PreferParameterNullChecking, - CSharpAnalyzersResources.Use_parameter_null_checking, + LanguageNames.CSharp, new LocalizableResourceString(nameof(AnalyzersResources.Null_check_can_be_simplified), AnalyzersResources.ResourceManager, typeof(AnalyzersResources))) { } From 5b1dcb333dcae3f188b8e2473730cfe0fb9930c9 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 7 Jan 2022 17:58:00 -0800 Subject: [PATCH 14/15] Imitate UseNotPattern and add test --- ...ParameterNullCheckingDiagnosticAnalyzer.cs | 4 +++- ...UseParameterNullCheckingCodeFixProvider.cs | 8 +++---- .../UseParameterNullCheckingTests.cs | 23 +++++++++++++++++++ 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index 12f25cb7cfb97..5766053ec9e39 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -18,13 +18,15 @@ internal sealed class CSharpUseParameterNullCheckingDiagnosticAnalyzer : AbstractBuiltInCodeStyleDiagnosticAnalyzer { private const string ArgumentNullExceptionName = $"{nameof(System)}.{nameof(ArgumentNullException)}"; + private static readonly LocalizableResourceString s_resourceTitle = new(nameof(AnalyzersResources.Null_check_can_be_simplified), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)); public CSharpUseParameterNullCheckingDiagnosticAnalyzer() : base(IDEDiagnosticIds.UseParameterNullCheckingId, EnforceOnBuildValues.UseParameterNullChecking, CSharpCodeStyleOptions.PreferParameterNullChecking, LanguageNames.CSharp, - new LocalizableResourceString(nameof(AnalyzersResources.Null_check_can_be_simplified), AnalyzersResources.ResourceManager, typeof(AnalyzersResources))) + s_resourceTitle, + s_resourceTitle) { } diff --git a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs index c103653fa3751..9d5177b21c74e 100644 --- a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs @@ -37,10 +37,8 @@ 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)), + new MyCodeAction(c => FixAsync(context.Document, diagnostic, c)), context.Diagnostics); - return Task.CompletedTask; } @@ -83,8 +81,8 @@ protected override Task FixAllAsync( private class MyCodeAction : CustomCodeActions.DocumentChangeAction { - public MyCodeAction(string title, Func> createChangedDocument) - : base(title, createChangedDocument, equivalenceKey: nameof(CSharpAnalyzersResources.Use_parameter_null_checking)) + public MyCodeAction(Func> createChangedDocument) + : base(CSharpAnalyzersResources.Use_parameter_null_checking, createChangedDocument, nameof(CSharpUseParameterNullCheckingCodeFixProvider)) { } } diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index 5d376e7c6087a..5afd309739c99 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -701,6 +701,29 @@ void M(string s2) }.RunAsync(); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestNestedStatements() + { + var testCode = @"using System; + +class C +{ + void M(string s2) + { + { + if (s2 == null) + throw new ArgumentNullException(nameof(s2)); + } + } +}"; + await new VerifyCS.Test() + { + TestCode = testCode, + FixedCode = testCode, + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestConstrainedTypeParameter() { From 46382fafafc25c2da9855243a2849213587fe0da Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 11 Jan 2022 13:14:10 -0800 Subject: [PATCH 15/15] Address feedback --- ...ParameterNullCheckingDiagnosticAnalyzer.cs | 42 +++++--- ...UseParameterNullCheckingCodeFixProvider.cs | 14 ++- .../UseParameterNullCheckingTests.cs | 98 ++++++++++++++++++- .../Core/Analyzers/IDEDiagnosticIds.cs | 1 + .../AddParameterCheckTests.cs | 23 ++++- 5 files changed, 164 insertions(+), 14 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs index 5766053ec9e39..db85ff5cdea6a 100644 --- a/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UseParameterNullChecking/CSharpUseParameterNullCheckingDiagnosticAnalyzer.cs @@ -3,12 +3,14 @@ // 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.CodeStyle; using Microsoft.CodeAnalysis.CSharp.Shared.Extensions; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.CSharp.UseParameterNullChecking @@ -42,7 +44,7 @@ protected override void InitializeWorker(AnalysisContext context) return; } - var argumentNullException = compilation.GetTypeByMetadataName(ArgumentNullExceptionName); + var argumentNullException = compilation.GetBestTypeByMetadataName(ArgumentNullExceptionName); if (argumentNullException is null) { return; @@ -140,11 +142,7 @@ private void AnalyzeSyntax( foreach (var statement in block.Statements) { var parameter = TryGetParameterNullCheckedByStatement(statement); - if (parameter is not null - && (!parameter.Type.IsValueType - || parameter.Type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T - || parameter.Type.TypeKind is TypeKind.Pointer or TypeKind.FunctionPointer) - && parameter.RefKind == RefKind.None + if (ParameterCanUseNullChecking(parameter) && parameter.DeclaringSyntaxReferences.FirstOrDefault() is SyntaxReference reference && reference.SyntaxTree.Equals(statement.SyntaxTree) && reference.GetSyntax() is ParameterSyntax parameterSyntax) @@ -160,6 +158,23 @@ private void AnalyzeSyntax( return; + bool ParameterCanUseNullChecking([NotNullWhen(true)] IParameterSymbol? parameter) + { + if (parameter is null) + return false; + + if (parameter.RefKind != RefKind.None) + return false; + + if (parameter.Type.IsValueType) + { + return parameter.Type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T + || parameter.Type.TypeKind is TypeKind.Pointer or TypeKind.FunctionPointer; + } + + return true; + } + IParameterSymbol? TryGetParameterNullCheckedByStatement(StatementSyntax statement) { switch (statement) @@ -271,10 +286,16 @@ bool IsConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, I return false; } - if (arguments.Count == 0) + // 'new ArgumentNullException()' + if (argumentNullExceptionConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol)) + { + return arguments.Count == 0; + } + + // 'new ArgumentNullException(nameof(param))' (or equivalent) + if (!argumentNullExceptionStringConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol)) { - // 'new ArgumentNullException()' - return argumentNullExceptionConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol); + return false; } if (arguments.Count != 1) @@ -282,14 +303,13 @@ bool IsConstructorApplicable(ObjectCreationExpressionSyntax exceptionCreation, I return false; } - // 'new ArgumentNullException(nameof(param))' (or equivalent) var constantValue = semanticModel.GetConstantValue(arguments[0].Expression, cancellationToken); if (constantValue.Value is not string constantString || !string.Equals(constantString, parameterSymbol.Name, StringComparison.Ordinal)) { return false; } - return argumentNullExceptionStringConstructor.Equals(semanticModel.GetSymbolInfo(exceptionCreation, cancellationToken).Symbol); + return true; } } } diff --git a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs index 9d5177b21c74e..5bc8f3a915a18 100644 --- a/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/UseParameterNullChecking/CSharpUseParameterNullCheckingCodeFixProvider.cs @@ -72,7 +72,19 @@ protected override Task FixAllAsync( if (fixedParameterLocations.Add(parameterLocation)) { var parameterSyntax = (ParameterSyntax)parameterLocation.FindNode(cancellationToken); - editor.ReplaceNode(parameterSyntax, parameterSyntax.WithExclamationExclamationToken(SyntaxFactory.Token(SyntaxKind.ExclamationExclamationToken))); + if (parameterSyntax.ExclamationExclamationToken.IsKind(SyntaxKind.None)) + { + var identifier = parameterSyntax.Identifier; + var newIdentifier = identifier.WithoutTrailingTrivia(); + var newExclamationExclamationToken = SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.ExclamationExclamationToken, identifier.TrailingTrivia); + editor.ReplaceNode(parameterSyntax, parameterSyntax.Update( + parameterSyntax.AttributeLists, + parameterSyntax.Modifiers, + parameterSyntax.Type, + newIdentifier, + newExclamationExclamationToken, + parameterSyntax.Default)); + } } } diff --git a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs index 5afd309739c99..bc69220f51600 100644 --- a/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs +++ b/src/Analyzers/CSharp/Tests/UseParameterNullChecking/UseParameterNullCheckingTests.cs @@ -395,7 +395,7 @@ public C(string s!!) } [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] - public async Task TestLeadingAndTrailingTrivia() + public async Task TestLeadingAndTrailingTrivia1() { await new VerifyCS.Test() { @@ -421,6 +421,72 @@ public C(string s!!) }.RunAsync(); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestLeadingAndTrailingTrivia2() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + public C( + string x // comment + , string y // comment, + , string z // comment + ) + { + [|if (x is null) throw new ArgumentNullException(nameof(x));|] + [|if (y is null) throw new ArgumentNullException(nameof(y));|] + [|if (z is null) throw new ArgumentNullException(nameof(z));|] + } +}", + FixedCode = @"using System; + +class C +{ + public C( + string x!! // comment + , string y!! // comment, + , string z!! // comment + ) + { + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestLeadingAndTrailingTrivia3() + { + await new VerifyCS.Test() + { + TestCode = @"using System; + +class C +{ + public C( + string x /* comment */ !! + ) + { + [|if (x is null) throw new ArgumentNullException(nameof(x));|] + } +}", + FixedCode = @"using System; + +class C +{ + public C( + string x /* comment */ !! + ) + { + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestAssignThenTest() { @@ -587,6 +653,7 @@ void M(string s) [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestNotEqualitySwapped() { + // https://github.com/dotnet/roslyn/issues/58699 var testCode = @"using System; class C @@ -887,6 +954,35 @@ void M(string s) }.RunAsync(); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] + public async Task TestOptionalParameter() + { + await new VerifyCS.Test() + { + TestCode = @" +using System; + +class C +{ + void M(string s = ""a"") + { + [|if ((object)s == null) + throw new ArgumentNullException(nameof(s));|] + } +}", + FixedCode = @" +using System; + +class C +{ + void M(string s!! = ""a"") + { + } +}", + LanguageVersion = LanguageVersionExtensions.CSharpNext + }.RunAsync(); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseIsNullCheck)] public async Task TestWithStringExceptionArgument() { diff --git a/src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs b/src/Analyzers/Core/Analyzers/IDEDiagnosticIds.cs index 6d7d5b43b7cfd..592390e6f38ab 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 = "IDE0190"; // Analyzer error Ids diff --git a/src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs b/src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs index 7470bfef5aabb..5a7a43b86ad35 100644 --- a/src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs +++ b/src/EditorFeatures/CSharpTest/InitializeParameter/AddParameterCheckTests.cs @@ -1,4 +1,4 @@ -// Licensed to the .NET Foundation under one or more agreements. +// 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. @@ -76,6 +76,27 @@ public C([||]string s!!) }.RunAsync(); } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)] + public async Task TestRecordPrimaryConstructor() + { + // https://github.com/dotnet/roslyn/issues/58779 + // Note: we declare a field within the record to work around missing IsExternalInit errors + await new VerifyCS.Test + { + LanguageVersion = LanguageVersionExtensions.CSharpNext, + TestCode = @" +using System; + +record Rec([||]string s) { public string s = s; } +", + FixedCode = @" +using System; + +record Rec(string s) { public string s = s; } +" + }.RunAsync(); + } + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)] public async Task TestSimpleReferenceType_AlreadyNullChecked2() {