diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0018Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0018Tests.cs index d754a6605a0..7a593bd42dc 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0018Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0018Tests.cs @@ -326,7 +326,7 @@ await Verifier.CreateAnalyzer(code) } [Fact] - public async Task AZC0018ProducedForMethodsWithNoRequestContentButProtocolAndConvenience() + public async Task AZC0018NotProducedForMethodsWithRequestContentAndOptionalRequestContext() { const string code = @" using Azure; @@ -339,110 +339,12 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(string a, CancellationToken cancellationToken = default) - { - return null; - } - - public virtual Response Get(string a, CancellationToken cancellationToken = default) - { - return null; - } - - public virtual Task {|AZC0018:GetAsync|}(string a, Azure.RequestContext context = null) - { - return null; - } - - public virtual Response {|AZC0018:Get|}(string a, Azure.RequestContext context = null) - { - return null; - } - } -}"; - await Verifier.CreateAnalyzer(code) - .RunAsync(); - } - - [Fact] - public async Task AZC0018ProducedForMethodsWithRequiredRequestContentAndRequiredRequestContext() - { - const string code = @" -using Azure; -using Azure.Core; -using System.Threading; -using System.Threading.Tasks; -using System.Collections.Generic; - -namespace Azure.Core -{ - internal static partial class Argument - { - public static void AssertNotNull(T value, string name) - { - if (value is null) - { - throw new System.ArgumentNullException(name); - } - } - } -} - -namespace RandomNamespace -{ - public class SomeClient - { - public virtual Task {|AZC0018:GetAsync|}(RequestContent content, RequestContext context) - { - Argument.AssertNotNull(content, nameof(content)); - return null; - } - - public virtual Response {|AZC0018:Get|}(RequestContent content, RequestContext context) - { - Argument.AssertNotNull(content, nameof(content)); - return null; - } - } -}"; - await Verifier.CreateAnalyzer(code) - .RunAsync(); - } - - [Fact] - public async Task AZC0018ProducedForMethodsWithOptionalRequestContentAndOptionalRequestContext() - { - const string code = @" -using Azure; -using Azure.Core; -using System.Threading; -using System.Threading.Tasks; -using System.Collections.Generic; - -namespace Azure.Core -{ - internal static partial class Argument - { - public static void AssertNotNull(T value, string name) - { - if (value is null) - { - throw new System.ArgumentNullException(name); - } - } - } -} - -namespace RandomNamespace -{ - public class SomeClient - { - public virtual Task {|AZC0018:GetAsync|}(RequestContent content, RequestContext context = null) + public virtual Task GetAsync(RequestContent content, RequestContext context = null) { return null; } - public virtual Response {|AZC0018:Get|}(RequestContent content, RequestContext context = null) + public virtual Response Get(RequestContent content, RequestContext context = null) { return null; } diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0019Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0019Tests.cs new file mode 100644 index 00000000000..c6626adf639 --- /dev/null +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0019Tests.cs @@ -0,0 +1,51 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Threading.Tasks; +using Xunit; +using Verifier = Azure.ClientSdk.Analyzers.Tests.AzureAnalyzerVerifier; + +namespace Azure.ClientSdk.Analyzers.Tests +{ + public class AZC0019Tests + { + [Fact] + public async Task AZC0019ProducedForMethodsWithNoRequestContentButProtocolAndConvenience() + { + const string code = @" +using Azure; +using Azure.Core; +using System.Threading; +using System.Threading.Tasks; +using System.Collections.Generic; + +namespace RandomNamespace +{ + public class SomeClient + { + public virtual Task GetAsync(string a, CancellationToken cancellationToken = default) + { + return null; + } + + public virtual Response Get(string a, CancellationToken cancellationToken = default) + { + return null; + } + + public virtual Task {|AZC0019:GetAsync|}(string a, Azure.RequestContext context = null) + { + return null; + } + + public virtual Response {|AZC0019:Get|}(string a, Azure.RequestContext context = null) + { + return null; + } + } +}"; + await Verifier.CreateAnalyzer(code) + .RunAsync(); + } + } +} diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs index bbc822fa204..b84c873b587 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs @@ -5,7 +5,6 @@ using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; namespace Azure.ClientSdk.Analyzers @@ -30,85 +29,10 @@ public class ClientMethodsAnalyzer : ClientAnalyzerBase Descriptors.AZC0004, Descriptors.AZC0015, Descriptors.AZC0017, - Descriptors.AZC0018 + Descriptors.AZC0018, + Descriptors.AZC0019, }); - public override void Initialize(AnalysisContext context) - { - context.EnableConcurrentExecution(); - context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); - base.Initialize(context); - context.RegisterCodeBlockAction(c => AnalyzeCodeBlock(c)); - } - - private void AnalyzeCodeBlock(CodeBlockAnalysisContext codeBlock) - { - var symbol = codeBlock.OwningSymbol; - if (symbol is IMethodSymbol methodSymbol && !IsCheckExempt(methodSymbol)) - { - var lastParameter = methodSymbol.Parameters.LastOrDefault(); - if (lastParameter != null && IsRequestContext(lastParameter)) - { - var requestContent = methodSymbol.Parameters.FirstOrDefault(p => IsRequestContent(p)); - if (requestContent != null) - { - bool isRequired = ContainsAssertNotNull(codeBlock, requestContent.Name); - if (isRequired && !lastParameter.IsOptional) - { - codeBlock.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, symbol.Locations.FirstOrDefault())); - } - if (!isRequired && lastParameter.IsOptional) - { - codeBlock.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, symbol.Locations.FirstOrDefault())); - } - } - } - } - } - - private static bool ContainsAssertNotNull(CodeBlockAnalysisContext codeBlock, string variableName) - { - // Check Argument.AssertNotNull(variableName, nameof(variableName)); - foreach (var invocation in codeBlock.CodeBlock.DescendantNodes().OfType()) - { - if (invocation.Expression is MemberAccessExpressionSyntax assertNotNull && assertNotNull.Name.Identifier.Text == "AssertNotNull") - { - if (assertNotNull.Expression is IdentifierNameSyntax identifierName && identifierName.Identifier.Text == "Argument" || - assertNotNull.Expression is MemberAccessExpressionSyntax memberAccessExpression && memberAccessExpression.Name.Identifier.Text == "Argument") - { - var argumentsList = invocation.ArgumentList.Arguments; - if (argumentsList.Count != 2) - { - continue; - } - if (argumentsList.First().Expression is IdentifierNameSyntax first) - { - if (first.Identifier.Text != variableName) - { - continue; - } - if (argumentsList.Last().Expression is InvocationExpressionSyntax second) - { - if (second.Expression is IdentifierNameSyntax nameof && nameof.Identifier.Text == "nameof") - { - if (second.ArgumentList.Arguments.Count != 1) - { - continue; - } - if (second.ArgumentList.Arguments.First().Expression is IdentifierNameSyntax contentName && contentName.Identifier.Text == variableName) - { - return true; - } - } - } - } - } - } - } - - return false; - } - private static bool IsRequestContent(IParameterSymbol parameterSymbol) { return parameterSymbol.Type.Name == "RequestContent"; @@ -175,10 +99,8 @@ private static string GetFullNamespaceName(IParameterSymbol parameter) } // A protocol method should not have model as parameter. If it has ambiguity with convenience method, it should have required RequestContext. - // Ambiguity: - // 1) has optional RequestContent. - // 2) doesn't have a RequestContent, but there is a method ending with CancellationToken has same type of parameters - // No ambiguity: has required RequestContent. + // Ambiguity: doesn't have a RequestContent, but there is a method ending with CancellationToken has same type of parameters + // No ambiguity: has RequestContent. private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context, IMethodSymbol method) { var containsModel = method.Parameters.Any(p => @@ -202,10 +124,9 @@ private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context IMethodSymbol convenienceMethod = FindMethod(methodList, method.TypeParameters, parametersWithoutLast, symbol => IsCancellationToken(symbol), ParameterEquivalenceComparerOptionalIgnore.Default); if (convenienceMethod != null) { - context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); + context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0019, method.Locations.FirstOrDefault()), method); } } - // Optional RequestContent or required RequestContent is checked in AnalyzeCodeBlock. } // A protocol method should not have model as type. Accepted return type: Response, Task, Pageable, AsyncPageable, Operation, Task>, Operation, Task, Operation>, Task>> diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs index 6da37731416..5ae43a1000f 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs @@ -114,14 +114,20 @@ internal class Descriptors public static DiagnosticDescriptor AZC0017 = new DiagnosticDescriptor( nameof(AZC0017), - "Do ensure convenience method not take RequestContent as parameter type.", + "Invalid convenience method signature.", "Convenience method shouldn't have prameters with type RequestContent.", "Usage", DiagnosticSeverity.Warning, isEnabledByDefault: true, description: null); public static DiagnosticDescriptor AZC0018 = new DiagnosticDescriptor( nameof(AZC0018), - "Invalid protocl method signature.", + "Invalid protocol method signature.", "Protocol method should take a RequestContext parameter called `context` and not use a model as parameter or return types. Protocol methods should not have optional parameters if ambiguity exists between the protocol method and convenience methods.", + "Usage", DiagnosticSeverity.Warning, isEnabledByDefault: true, description: null); + + public static DiagnosticDescriptor AZC0019 = new DiagnosticDescriptor( + nameof(AZC0019), + "Potential ambiguous call exists.", + "There will be ambiguous call when user calls with required parameters.", "Usage", DiagnosticSeverity.Warning, isEnabledByDefault: true, description: null); #endregion diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/SymbolAnalyzerBase.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/SymbolAnalyzerBase.cs index 3e5f7880953..65e1040138e 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/SymbolAnalyzerBase.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/SymbolAnalyzerBase.cs @@ -16,7 +16,7 @@ public abstract class SymbolAnalyzerBase : DiagnosticAnalyzer protected INamedTypeSymbol ClientOptionsType { get; private set; } - public override void Initialize(AnalysisContext context) + public sealed override void Initialize(AnalysisContext context) { context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); context.EnableConcurrentExecution(); @@ -81,4 +81,4 @@ public void ReportDiagnostic(Diagnostic diagnostic, ISymbol symbol) } } } -} \ No newline at end of file +}