From 42a37ae909d32534162b00e2c99cc9757028a06d Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Mon, 10 Jul 2023 13:57:38 +0800 Subject: [PATCH 01/14] Fix client constructure issue. --- .../AZC0004Tests.cs | 27 ++++++++++++++++++- .../ClientMethodsAnalyzer.cs | 4 +-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs index 3d35c522ea7..da09da167c3 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs @@ -1,6 +1,6 @@ // 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; @@ -496,5 +496,30 @@ await Verifier.CreateAnalyzer(code) .WithDisabledDiagnostics("AZC0015") .RunAsync(); } + + [Fact] + public async Task AZC0004NotProducedForConstructorOrProperty() + { + const string code = @" +namespace RandomNamespace +{ + public class SomeClient + { + private string _id; + public virtual string Id => _id; + + protected SomeClient() + { + } + + public SomeClient(string id) + { + _id = id; + } + } +}"; + 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 db3ce3d066f..7e74d7a62bc 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs @@ -297,7 +297,7 @@ public override void AnalyzeCore(ISymbolAnalysisContext context) List visitedSyncMember = new List(); foreach (var member in type.GetMembers()) { - if (member is IMethodSymbol asyncMethodSymbol && asyncMethodSymbol.Name.EndsWith(AsyncSuffix) && member.DeclaredAccessibility == Accessibility.Public) + if (member is IMethodSymbol asyncMethodSymbol && asyncMethodSymbol.MethodKind == MethodKind.Ordinary && asyncMethodSymbol.Name.EndsWith(AsyncSuffix) && member.DeclaredAccessibility == Accessibility.Public) { CheckClientMethod(context, asyncMethodSymbol); @@ -315,7 +315,7 @@ public override void AnalyzeCore(ISymbolAnalysisContext context) CheckClientMethod(context, syncMember); } } - else if (member is IMethodSymbol syncMethodSymbol && !member.IsImplicitlyDeclared && !syncMethodSymbol.Name.EndsWith(AsyncSuffix) && member.DeclaredAccessibility == Accessibility.Public && !visitedSyncMember.Contains(syncMethodSymbol)) + else if (member is IMethodSymbol syncMethodSymbol && syncMethodSymbol.MethodKind == MethodKind.Ordinary && !member.IsImplicitlyDeclared && !syncMethodSymbol.Name.EndsWith(AsyncSuffix) && member.DeclaredAccessibility == Accessibility.Public && !visitedSyncMember.Contains(syncMethodSymbol)) { var asyncMemberName = member.Name + AsyncSuffix; var asyncMember = FindMethod(type.GetMembers(asyncMemberName).OfType(), syncMethodSymbol.TypeParameters, syncMethodSymbol.Parameters); From 73d1830ee43cb05b6f1f3fb2cad6589ab67f1ac9 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Wed, 12 Jul 2023 14:40:09 +0800 Subject: [PATCH 02/14] Update --- .../AZC0004Tests.cs | 26 +++- .../AZC0018Tests.cs | 112 +++++++++++++++++- .../ClientAnalyzerBase.cs | 21 +++- .../ClientMethodsAnalyzer.cs | 85 +++++++++---- .../Azure.ClientSdk.Analyzers/Descriptors.cs | 4 +- 5 files changed, 213 insertions(+), 35 deletions(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs index da09da167c3..70bc7444df0 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs @@ -498,7 +498,7 @@ await Verifier.CreateAnalyzer(code) } [Fact] - public async Task AZC0004NotProducedForConstructorOrProperty() + public async Task AZC0004NotProducedForConstructorOrPropertyOrOverride() { const string code = @" namespace RandomNamespace @@ -508,6 +508,8 @@ public class SomeClient private string _id; public virtual string Id => _id; + public override bool Equals(object obj) => base.Equals(obj); + protected SomeClient() { } @@ -517,6 +519,28 @@ public SomeClient(string id) _id = id; } } +}"; + await Verifier.CreateAnalyzer(code) + .RunAsync(); + } + + [Fact] + public async Task AZC0004NotProducedForGetSubClientMethod() + { + const string code = @" +namespace RandomNamespace +{ + public class SomeClient + { + public SubClient GetSubClient() + { + return null; + } + } + + public class SubClient + { + } }"; await Verifier.CreateAnalyzer(code) .RunAsync(); 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 cc55370a9fc..d754a6605a0 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 @@ -62,6 +62,16 @@ public virtual Operation GetOperationOfT(string s, RequestContext co { return null; } + + public virtual Task>> GetOperationOfPageableAsync(string s, RequestContext context) + { + return null; + } + + public virtual Operation> GetOperationOfPageable(string s, RequestContext context) + { + return null; + } } }"; await Verifier.CreateAnalyzer(code) @@ -192,6 +202,38 @@ await Verifier.CreateAnalyzer(code) .RunAsync(); } + [Fact] + public async Task AZC0018ProducedForMethodsWithOperationOfPageableModel() + { + const string code = @" +using Azure; +using Azure.Core; +using System.Threading; +using System.Threading.Tasks; + +namespace RandomNamespace +{ + public class Model + { + string a; + } + public class SomeClient + { + public virtual Task>> {|AZC0018:GetAsync|}(string s, RequestContext context) + { + return null; + } + + public virtual Operation> {|AZC0018:Get|}(string s, RequestContext context) + { + return null; + } + } +}"; + await Verifier.CreateAnalyzer(code) + .RunAsync(); + } + [Fact] public async Task AZC0018ProducedForMethodsWithParameterModel() { @@ -226,7 +268,7 @@ await Verifier.CreateAnalyzer(code) } [Fact] - public async Task AZC0018ProducedForMethodsWithNoRequestContentAndOptionalRequestContext() + public async Task AZC0018NotProducedForMethodsWithNoRequestContentAndRequiredContext() { const string code = @" using Azure; @@ -239,6 +281,74 @@ namespace RandomNamespace { public class SomeClient { + public virtual Task GetAsync(string a, Azure.RequestContext context) + { + return null; + } + + public virtual Response Get(string a, Azure.RequestContext context) + { + return null; + } + } +}"; + await Verifier.CreateAnalyzer(code) + .RunAsync(); + } + + [Fact] + public async Task AZC0018NotProducedForMethodsWithNoRequestContentButOnlyProtocol() + { + 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, Azure.RequestContext context = null) + { + return null; + } + + public virtual Response Get(string a, Azure.RequestContext context = null) + { + return null; + } + } +}"; + await Verifier.CreateAnalyzer(code) + .RunAsync(); + } + + [Fact] + public async Task AZC0018ProducedForMethodsWithNoRequestContentButProtocolAndConvenience() + { + 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 {|AZC0018:GetAsync|}(string a, Azure.RequestContext context = null) { return null; diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs index 2e3480b9910..64a24b19211 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs @@ -26,16 +26,26 @@ public override void Analyze(ISymbolAnalysisContext context) AnalyzeCore(context); } + protected class ParameterEquivalenceComparerOptionalIgnore : ParameterEquivalenceComparer + { + public static new ParameterEquivalenceComparerOptionalIgnore Default { get; } = new ParameterEquivalenceComparerOptionalIgnore(); + + public override bool Equals(IParameterSymbol x, IParameterSymbol y) + { + return x.Name.Equals(y.Name) && TypeSymbolEquals(x.Type, y.Type); + } + } + protected class ParameterEquivalenceComparer : IEqualityComparer, IEqualityComparer { public static ParameterEquivalenceComparer Default { get; } = new ParameterEquivalenceComparer(); - public bool Equals(IParameterSymbol x, IParameterSymbol y) + public virtual bool Equals(IParameterSymbol x, IParameterSymbol y) { return x.Name.Equals(y.Name) && TypeSymbolEquals(x.Type, y.Type) && x.IsOptional == y.IsOptional; } - private bool TypeSymbolEquals(ITypeSymbol x, ITypeSymbol y) + protected bool TypeSymbolEquals(ITypeSymbol x, ITypeSymbol y) { switch (x) { @@ -86,20 +96,19 @@ protected static IMethodSymbol FindMethod(IEnumerable methodSymbo parameters.SequenceEqual(symbol.Parameters, ParameterEquivalenceComparer.Default)); } - protected static IMethodSymbol FindMethod(IEnumerable methodSymbols, ImmutableArray genericParameters, ImmutableArray parameters, Func lastParameter) + protected static IMethodSymbol FindMethod(IEnumerable methodSymbols, ImmutableArray genericParameters, ImmutableArray parameters, Func lastParameter, ParameterEquivalenceComparer comparer = null) { return methodSymbols.SingleOrDefault(symbol => { - - if (!symbol.Parameters.Any() || !genericParameters.SequenceEqual(symbol.TypeParameters, ParameterEquivalenceComparer.Default)) + if (!symbol.Parameters.Any() || !genericParameters.SequenceEqual(symbol.TypeParameters, comparer ?? ParameterEquivalenceComparer.Default)) { return false; } var allButLast = symbol.Parameters.RemoveAt(symbol.Parameters.Length - 1); - return allButLast.SequenceEqual(parameters, ParameterEquivalenceComparer.Default) && lastParameter(symbol.Parameters.Last()); + return allButLast.SequenceEqual(parameters, comparer ?? ParameterEquivalenceComparer.Default) && lastParameter(symbol.Parameters.Last()); }); } 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 7e74d7a62bc..4dc8ea120d9 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs @@ -36,7 +36,7 @@ public override void Initialize(AnalysisContext context) private void AnalyzeCodeBlock(CodeBlockAnalysisContext codeBlock) { var symbol = codeBlock.OwningSymbol; - if (symbol is IMethodSymbol methodSymbol) + if (symbol is IMethodSymbol methodSymbol && !IsCheckExempt(methodSymbol)) { var lastParameter = methodSymbol.Parameters.LastOrDefault(); if (lastParameter != null && IsRequestContext(lastParameter)) @@ -111,6 +111,11 @@ private static bool IsRequestContext(IParameterSymbol parameterSymbol) return parameterSymbol.Name == "context" && parameterSymbol.Type.Name == "RequestContext"; } + private static bool IsCancellationToken(IParameterSymbol parameterSymbol) + { + return parameterSymbol.Name == "cancellationToken" && parameterSymbol.Type.Name == "CancellationToken" && parameterSymbol.IsOptional; + } + private static void CheckClientMethod(ISymbolAnalysisContext context, IMethodSymbol member) { static bool IsCancellationOrRequestContext(IParameterSymbol parameterSymbol) @@ -118,11 +123,6 @@ static bool IsCancellationOrRequestContext(IParameterSymbol parameterSymbol) return IsCancellationToken(parameterSymbol) || IsRequestContext(parameterSymbol); } - static bool IsCancellationToken(IParameterSymbol parameterSymbol) - { - return parameterSymbol.Name == "cancellationToken" && parameterSymbol.Type.Name == "CancellationToken" && parameterSymbol.IsOptional; - } - CheckClientMethodReturnType(context, member); if (!member.IsVirtual && !member.IsOverride) @@ -168,7 +168,9 @@ 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: no RequestContent or has optional RequestContent. + // 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. private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context, IMethodSymbol method) { @@ -185,9 +187,12 @@ private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context } var requestContent = method.Parameters.FirstOrDefault(p => p.Type.Name == "RequestContent"); - if (requestContent == null) + if (requestContent == null && method.Parameters.Last().IsOptional) { - if (method.Parameters.LastOrDefault().IsOptional) + INamedTypeSymbol type = (INamedTypeSymbol)context.Symbol; + var methodList = type.GetMembers(method.Name).OfType().Where(member => !SymbolEqualityComparer.Default.Equals(member, method)); + var convenienceMethod = FindMethod(methodList, method.TypeParameters, method.Parameters.RemoveAt(method.Parameters.Length - 1), symbol => IsCancellationToken(symbol), ParameterEquivalenceComparerOptionalIgnore.Default); + if (convenienceMethod != null) { context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); } @@ -195,9 +200,31 @@ private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context // 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 + // A protocol method should not have model as type. Accepted return type: Response, Task, Pageable, AsyncPageable, Operation, Task>, Operation, Task, Operation>, Task>> private static void CheckProtocolMethodReturnType(ISymbolAnalysisContext context, IMethodSymbol method) { + bool IsValidPageable(ITypeSymbol typeSymbol) + { + if (typeSymbol.Name != "Pageable" && typeSymbol.Name != "AsyncPageable") + { + return false; + } + + var pageableTypeSymbol = typeSymbol as INamedTypeSymbol; + if (!pageableTypeSymbol.IsGenericType) + { + return false; + } + + var pageableReturn = pageableTypeSymbol.TypeArguments.Single(); + if (pageableReturn.Name != "BinaryData") + { + return false; + } + + return true; + } + ITypeSymbol originalType = method.ReturnType; ITypeSymbol unwrappedType = method.ReturnType; @@ -221,6 +248,15 @@ private static void CheckProtocolMethodReturnType(ISymbolAnalysisContext context if (unwrappedType is INamedTypeSymbol operationTypeSymbol && operationTypeSymbol.IsGenericType) { var operationReturn = operationTypeSymbol.TypeArguments.Single(); + if (operationReturn.Name == "Pageable" || operationReturn.Name == "AsyncPageable") + { + if (!IsValidPageable(operationReturn)) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); + } + return; + } + if (operationReturn.Name != "BinaryData") { context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); @@ -229,21 +265,11 @@ private static void CheckProtocolMethodReturnType(ISymbolAnalysisContext context return; } else if (originalType.Name == "Pageable" || originalType.Name == "AsyncPageable") - { - if (originalType is INamedTypeSymbol pageableTypeSymbol) + { + if (!IsValidPageable(originalType)) { - if (!pageableTypeSymbol.IsGenericType) - { - context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); - } - - var pageableReturn = pageableTypeSymbol.TypeArguments.Single(); - if (pageableReturn.Name != "BinaryData") - { - context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); - } + context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); } - return; } @@ -291,13 +317,22 @@ bool IsOrImplements(ITypeSymbol typeSymbol, string typeName) } + private bool IsCheckExempt(IMethodSymbol method) + { + return method.MethodKind != MethodKind.Ordinary || + method.DeclaredAccessibility != Accessibility.Public || + method.OverriddenMethod != null || + method.IsImplicitlyDeclared || + (method.Name.StartsWith("Get") && method.Name.EndsWith("Client")); + } + public override void AnalyzeCore(ISymbolAnalysisContext context) { INamedTypeSymbol type = (INamedTypeSymbol)context.Symbol; List visitedSyncMember = new List(); foreach (var member in type.GetMembers()) { - if (member is IMethodSymbol asyncMethodSymbol && asyncMethodSymbol.MethodKind == MethodKind.Ordinary && asyncMethodSymbol.Name.EndsWith(AsyncSuffix) && member.DeclaredAccessibility == Accessibility.Public) + if (member is IMethodSymbol asyncMethodSymbol && !IsCheckExempt(asyncMethodSymbol) && asyncMethodSymbol.Name.EndsWith(AsyncSuffix)) { CheckClientMethod(context, asyncMethodSymbol); @@ -315,7 +350,7 @@ public override void AnalyzeCore(ISymbolAnalysisContext context) CheckClientMethod(context, syncMember); } } - else if (member is IMethodSymbol syncMethodSymbol && syncMethodSymbol.MethodKind == MethodKind.Ordinary && !member.IsImplicitlyDeclared && !syncMethodSymbol.Name.EndsWith(AsyncSuffix) && member.DeclaredAccessibility == Accessibility.Public && !visitedSyncMember.Contains(syncMethodSymbol)) + else if (member is IMethodSymbol syncMethodSymbol && !IsCheckExempt(syncMethodSymbol) && !syncMethodSymbol.Name.EndsWith(AsyncSuffix) && !visitedSyncMember.Contains(syncMethodSymbol)) { var asyncMemberName = member.Name + AsyncSuffix; var asyncMember = FindMethod(type.GetMembers(asyncMemberName).OfType(), syncMethodSymbol.TypeParameters, syncMethodSymbol.Parameters); 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 42161acffca..51331f93a7d 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs @@ -120,8 +120,8 @@ internal class Descriptors public static DiagnosticDescriptor AZC0018 = new DiagnosticDescriptor( nameof(AZC0018), - "Do ensure protocol method take a RequestContext parameter called context and not take models as parameter type or return type.", - "Protocol method should have requestContext as the last parameter and don't have model as parameter type or return type.", + "Do ensure protocol method take a RequestContext parameter called context and not take models as parameter type or return type. Do ensure protocol method will not cause ambiguous call with convenience method.", + "Protocol method should have requestContext as the last parameter and don't have model as parameter type or return type. Protocol method should not have optional parameters if ambiguity exists between protocol method and convenience method.", "Usage", DiagnosticSeverity.Warning, isEnabledByDefault: true, description: null); #endregion From 9f9b90ed63d02376f3e466a0f65bd8f815b0e0c6 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Mon, 17 Jul 2023 16:54:19 +0800 Subject: [PATCH 03/14] Update --- .../ClientMethodsAnalyzer.cs | 68 ++++++++++--------- .../Azure.ClientSdk.Analyzers/Descriptors.cs | 4 +- 2 files changed, 39 insertions(+), 33 deletions(-) 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 4dc8ea120d9..d500588f70e 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs @@ -15,6 +15,14 @@ public class ClientMethodsAnalyzer : ClientAnalyzerBase { private const string AsyncSuffix = "Async"; + private const string PageableTypeName = "Pageable"; + private const string AsyncPageableTypeName = "AsyncPageable"; + private const string BinaryDataTypeName = "BinaryData"; + private const string ResponseTypeName = "Response"; + private const string NullableResponseTypeName = "NullableResponse"; + private const string OperationTypeName = "Operation"; + private const string TaskTypeName = "Task"; + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(new[] { Descriptors.AZC0002, @@ -140,8 +148,7 @@ static bool IsCancellationOrRequestContext(IParameterSymbol parameterSymbol) else if (IsCancellationToken(lastArgument)) { // A convenience method should not have RequestContent as parameter - var requestContent = member.Parameters.FirstOrDefault(parameter => parameter.Type.Name == "RequestContent"); - if (requestContent != null) + if (member.Parameters.FirstOrDefault(IsRequestContent) != null) { context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0017, member.Locations.FirstOrDefault()), member); } @@ -186,7 +193,7 @@ private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context return; } - var requestContent = method.Parameters.FirstOrDefault(p => p.Type.Name == "RequestContent"); + var requestContent = method.Parameters.FirstOrDefault(IsRequestContent); if (requestContent == null && method.Parameters.Last().IsOptional) { INamedTypeSymbol type = (INamedTypeSymbol)context.Symbol; @@ -205,7 +212,7 @@ private static void CheckProtocolMethodReturnType(ISymbolAnalysisContext context { bool IsValidPageable(ITypeSymbol typeSymbol) { - if (typeSymbol.Name != "Pageable" && typeSymbol.Name != "AsyncPageable") + if ((!IsOrImplements(typeSymbol, PageableTypeName)) && (!IsOrImplements(typeSymbol, AsyncPageableTypeName))) { return false; } @@ -217,7 +224,7 @@ bool IsValidPageable(ITypeSymbol typeSymbol) } var pageableReturn = pageableTypeSymbol.TypeArguments.Single(); - if (pageableReturn.Name != "BinaryData") + if (!IsOrImplements(pageableReturn, BinaryDataTypeName)) { return false; } @@ -230,12 +237,12 @@ bool IsValidPageable(ITypeSymbol typeSymbol) if (method.ReturnType is INamedTypeSymbol namedTypeSymbol && namedTypeSymbol.IsGenericType && - namedTypeSymbol.Name == "Task") + namedTypeSymbol.Name == TaskTypeName) { unwrappedType = namedTypeSymbol.TypeArguments.Single(); } - if (unwrappedType.Name == "Response") + if (IsOrImplements(unwrappedType, ResponseTypeName)) { if (unwrappedType is INamedTypeSymbol responseTypeSymbol && responseTypeSymbol.IsGenericType) { @@ -243,12 +250,12 @@ bool IsValidPageable(ITypeSymbol typeSymbol) } return; } - else if (unwrappedType.Name == "Operation") + else if (IsOrImplements(unwrappedType, OperationTypeName)) { if (unwrappedType is INamedTypeSymbol operationTypeSymbol && operationTypeSymbol.IsGenericType) { var operationReturn = operationTypeSymbol.TypeArguments.Single(); - if (operationReturn.Name == "Pageable" || operationReturn.Name == "AsyncPageable") + if (IsOrImplements(operationReturn, PageableTypeName) || IsOrImplements(operationReturn, AsyncPageableTypeName)) { if (!IsValidPageable(operationReturn)) { @@ -257,14 +264,14 @@ bool IsValidPageable(ITypeSymbol typeSymbol) return; } - if (operationReturn.Name != "BinaryData") + if (!IsOrImplements(operationReturn, BinaryDataTypeName)) { context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); } } return; } - else if (originalType.Name == "Pageable" || originalType.Name == "AsyncPageable") + else if (IsOrImplements(originalType, PageableTypeName) || IsOrImplements(originalType, AsyncPageableTypeName)) { if (!IsValidPageable(originalType)) { @@ -276,45 +283,44 @@ bool IsValidPageable(ITypeSymbol typeSymbol) context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); } - private static void CheckClientMethodReturnType(ISymbolAnalysisContext context, IMethodSymbol method) + private static bool IsOrImplements(ITypeSymbol typeSymbol, string typeName) { - bool IsOrImplements(ITypeSymbol typeSymbol, string typeName) + if (typeSymbol.Name == typeName) { - if (typeSymbol.Name == typeName) - { - return true; - } - - if (typeSymbol.BaseType != null) - { - return IsOrImplements(typeSymbol.BaseType, typeName); - } + return true; + } - return false; + if (typeSymbol.BaseType != null) + { + return IsOrImplements(typeSymbol.BaseType, typeName); } + return false; + } + + private static void CheckClientMethodReturnType(ISymbolAnalysisContext context, IMethodSymbol method) + { ITypeSymbol originalType = method.ReturnType; ITypeSymbol unwrappedType = method.ReturnType; if (method.ReturnType is INamedTypeSymbol namedTypeSymbol && namedTypeSymbol.IsGenericType && - namedTypeSymbol.Name == "Task") + namedTypeSymbol.Name == TaskTypeName) { unwrappedType = namedTypeSymbol.TypeArguments.Single(); } - if (IsOrImplements(unwrappedType, "Response") || - IsOrImplements(unwrappedType, "NullableResponse") || - IsOrImplements(unwrappedType, "Operation") || - IsOrImplements(originalType, "Pageable") || - IsOrImplements(originalType, "AsyncPageable") || + if (IsOrImplements(unwrappedType, ResponseTypeName) || + IsOrImplements(unwrappedType, NullableResponseTypeName) || + IsOrImplements(unwrappedType, OperationTypeName) || + IsOrImplements(originalType, PageableTypeName) || + IsOrImplements(originalType, AsyncPageableTypeName) || originalType.Name.EndsWith(ClientSuffix)) { return; } context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0015, method.Locations.FirstOrDefault(), originalType.ToDisplayString()), method); - } private bool IsCheckExempt(IMethodSymbol method) @@ -323,7 +329,7 @@ private bool IsCheckExempt(IMethodSymbol method) method.DeclaredAccessibility != Accessibility.Public || method.OverriddenMethod != null || method.IsImplicitlyDeclared || - (method.Name.StartsWith("Get") && method.Name.EndsWith("Client")); + (method.Name.StartsWith("Get") && method.Name.EndsWith(ClientSuffix)); } public override void AnalyzeCore(ISymbolAnalysisContext context) 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 51331f93a7d..3fd4dda7bd4 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs @@ -120,8 +120,8 @@ internal class Descriptors public static DiagnosticDescriptor AZC0018 = new DiagnosticDescriptor( nameof(AZC0018), - "Do ensure protocol method take a RequestContext parameter called context and not take models as parameter type or return type. Do ensure protocol method will not cause ambiguous call with convenience method.", - "Protocol method should have requestContext as the last parameter and don't have model as parameter type or return type. Protocol method should not have optional parameters if ambiguity exists between protocol method and convenience method.", + "Do ensure protocol methods take a RequestContext parameter called `context` and do not take models as parameter or return types. Do ensure protocol methods will not cause an ambiguous overload resolution with convenience methods.", + "Protocol method should have `requestContext` as the last parameter 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); #endregion From 32cef06f33a008b913419b03cfcb439a4d26cb2c Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Mon, 17 Jul 2023 18:13:47 +0800 Subject: [PATCH 04/14] Update --- src/dotnet/Azure.ClientSdk.Analyzers/readme.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 src/dotnet/Azure.ClientSdk.Analyzers/readme.md diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/readme.md b/src/dotnet/Azure.ClientSdk.Analyzers/readme.md new file mode 100644 index 00000000000..a51a2edcf8d --- /dev/null +++ b/src/dotnet/Azure.ClientSdk.Analyzers/readme.md @@ -0,0 +1,7 @@ +## How to test WIP Azure.ClientSdk.Analyzers against azure-sdk-for-net +1. Make changes into a feature branch in azure-sdk-tools. +2. Run [release pipeline](https://dev.azure.com/azure-sdk/internal/_build?definitionId=2945&_a=summary) for the feature branch. +3. Create a draft PR in azure-sdk-for-net bumping to the newly created dev version. +4. If we are satisfied with the results and all other PR comments are handled we can merge the azure-sdk-tools PR. +5. A new dev version will be published with a release from main branch. +6. Update the azure-sdk-for-net PR to use the newest version and merge the azure-sdk-for-net PR. From 2130cd88b73359cb8162474451ce1bd97ccf7215 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Tue, 18 Jul 2023 13:36:16 +0800 Subject: [PATCH 05/14] Update --- .../Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs index 64a24b19211..ecbd156bf51 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs @@ -45,7 +45,7 @@ public virtual bool Equals(IParameterSymbol x, IParameterSymbol y) return x.Name.Equals(y.Name) && TypeSymbolEquals(x.Type, y.Type) && x.IsOptional == y.IsOptional; } - protected bool TypeSymbolEquals(ITypeSymbol x, ITypeSymbol y) + protected virtual bool TypeSymbolEquals(ITypeSymbol x, ITypeSymbol y) { switch (x) { From 6fdd59aa0d1a98ecade7ef820d19243d3b14436f Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Mon, 24 Jul 2023 18:02:42 +0800 Subject: [PATCH 06/14] update --- .../Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs | 5 +++-- .../Azure.ClientSdk.Analyzers/Descriptors.cs | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) 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 d500588f70e..bbc822fa204 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs @@ -197,8 +197,9 @@ private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context if (requestContent == null && method.Parameters.Last().IsOptional) { INamedTypeSymbol type = (INamedTypeSymbol)context.Symbol; - var methodList = type.GetMembers(method.Name).OfType().Where(member => !SymbolEqualityComparer.Default.Equals(member, method)); - var convenienceMethod = FindMethod(methodList, method.TypeParameters, method.Parameters.RemoveAt(method.Parameters.Length - 1), symbol => IsCancellationToken(symbol), ParameterEquivalenceComparerOptionalIgnore.Default); + IEnumerable methodList = type.GetMembers(method.Name).OfType().Where(member => !SymbolEqualityComparer.Default.Equals(member, method)); + ImmutableArray parametersWithoutLast = method.Parameters.RemoveAt(method.Parameters.Length - 1); + 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); 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 3fd4dda7bd4..6da37731416 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs @@ -120,8 +120,8 @@ internal class Descriptors public static DiagnosticDescriptor AZC0018 = new DiagnosticDescriptor( nameof(AZC0018), - "Do ensure protocol methods take a RequestContext parameter called `context` and do not take models as parameter or return types. Do ensure protocol methods will not cause an ambiguous overload resolution with convenience methods.", - "Protocol method should have `requestContext` as the last parameter 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.", + "Invalid protocl 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); #endregion From 082febc8dcb1f08656915140949e69c14dd0b5f7 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Tue, 25 Jul 2023 16:52:33 +0800 Subject: [PATCH 07/14] Update --- .../AZC0018Tests.cs | 104 +----------------- .../AZC0019Tests.cs | 51 +++++++++ .../ClientMethodsAnalyzer.cs | 89 +-------------- .../Azure.ClientSdk.Analyzers/Descriptors.cs | 10 +- .../SymbolAnalyzerBase.cs | 4 +- 5 files changed, 69 insertions(+), 189 deletions(-) create mode 100644 src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0019Tests.cs 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 +} From 6056a2e9c813168d87ee782744b6319fba827caf Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Thu, 27 Jul 2023 12:55:04 +0800 Subject: [PATCH 08/14] update --- .../Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs | 2 +- .../Azure.ClientSdk.Analyzers/Descriptors.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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 b84c873b587..8caf97d2788 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs @@ -121,7 +121,7 @@ private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context INamedTypeSymbol type = (INamedTypeSymbol)context.Symbol; IEnumerable methodList = type.GetMembers(method.Name).OfType().Where(member => !SymbolEqualityComparer.Default.Equals(member, method)); ImmutableArray parametersWithoutLast = method.Parameters.RemoveAt(method.Parameters.Length - 1); - IMethodSymbol convenienceMethod = FindMethod(methodList, method.TypeParameters, parametersWithoutLast, symbol => IsCancellationToken(symbol), ParameterEquivalenceComparerOptionalIgnore.Default); + IMethodSymbol convenienceMethod = FindMethod(methodList, method.TypeParameters, parametersWithoutLast, symbol => IsCancellationToken(symbol)); if (convenienceMethod != null) { context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0019, method.Locations.FirstOrDefault()), method); 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 5ae43a1000f..338e29b27b7 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs @@ -121,13 +121,13 @@ internal class Descriptors public static DiagnosticDescriptor AZC0018 = new DiagnosticDescriptor( nameof(AZC0018), "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.", + "Protocol method should take a RequestContext parameter called `context` and not use a model as parameter or return types.", "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.", + "There will be ambiguous call when user calls with required parameters. All parameters of the protocol method should be required.", "Usage", DiagnosticSeverity.Warning, isEnabledByDefault: true, description: null); #endregion From ece6ad4ac63aa27372978c63c61186cddab86ea0 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Thu, 27 Jul 2023 14:00:45 +0800 Subject: [PATCH 09/14] update --- .../ClientAnalyzerBase.cs | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs index ecbd156bf51..160745be0da 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs @@ -26,26 +26,16 @@ public override void Analyze(ISymbolAnalysisContext context) AnalyzeCore(context); } - protected class ParameterEquivalenceComparerOptionalIgnore : ParameterEquivalenceComparer - { - public static new ParameterEquivalenceComparerOptionalIgnore Default { get; } = new ParameterEquivalenceComparerOptionalIgnore(); - - public override bool Equals(IParameterSymbol x, IParameterSymbol y) - { - return x.Name.Equals(y.Name) && TypeSymbolEquals(x.Type, y.Type); - } - } - protected class ParameterEquivalenceComparer : IEqualityComparer, IEqualityComparer { public static ParameterEquivalenceComparer Default { get; } = new ParameterEquivalenceComparer(); - public virtual bool Equals(IParameterSymbol x, IParameterSymbol y) + public bool Equals(IParameterSymbol x, IParameterSymbol y) { return x.Name.Equals(y.Name) && TypeSymbolEquals(x.Type, y.Type) && x.IsOptional == y.IsOptional; } - protected virtual bool TypeSymbolEquals(ITypeSymbol x, ITypeSymbol y) + private bool TypeSymbolEquals(ITypeSymbol x, ITypeSymbol y) { switch (x) { @@ -96,19 +86,19 @@ protected static IMethodSymbol FindMethod(IEnumerable methodSymbo parameters.SequenceEqual(symbol.Parameters, ParameterEquivalenceComparer.Default)); } - protected static IMethodSymbol FindMethod(IEnumerable methodSymbols, ImmutableArray genericParameters, ImmutableArray parameters, Func lastParameter, ParameterEquivalenceComparer comparer = null) + protected static IMethodSymbol FindMethod(IEnumerable methodSymbols, ImmutableArray genericParameters, ImmutableArray parameters, Func lastParameter) { return methodSymbols.SingleOrDefault(symbol => { - if (!symbol.Parameters.Any() || !genericParameters.SequenceEqual(symbol.TypeParameters, comparer ?? ParameterEquivalenceComparer.Default)) + if (!symbol.Parameters.Any() || !genericParameters.SequenceEqual(symbol.TypeParameters, ParameterEquivalenceComparer.Default)) { return false; } var allButLast = symbol.Parameters.RemoveAt(symbol.Parameters.Length - 1); - return allButLast.SequenceEqual(parameters, comparer ?? ParameterEquivalenceComparer.Default) && lastParameter(symbol.Parameters.Last()); + return allButLast.SequenceEqual(parameters, ParameterEquivalenceComparer.Default) && lastParameter(symbol.Parameters.Last()); }); } From 14554b1f9d478b83dedecad63ef2958ff56a5335 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Wed, 2 Aug 2023 14:49:55 +0800 Subject: [PATCH 10/14] update --- .../AZC0002Tests.cs | 34 +++++--- .../AZC0003Tests.cs | 8 +- .../AZC0004Tests.cs | 81 +++++++++---------- .../AZC0015Tests.cs | 6 +- .../AZC0017Tests.cs | 6 +- .../AZC0018Tests.cs | 2 + .../ClientMethodsAnalyzer.cs | 38 +++++---- 7 files changed, 94 insertions(+), 81 deletions(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs index b63e6dbe8f0..3c5b56b99ed 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs @@ -13,19 +13,21 @@ public class AZC0002Tests public async Task AZC0002ProducedForMethodsWithoutCancellationTokenOrRequestContext() { const string code = @" +using Azure; using System.Threading.Tasks; namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0002:GetAsync|}() + public virtual Response {|AZC0002:GetAsync|}() { return null; } - public virtual void {|AZC0002:Get|}() + public virtual Response {|AZC0002:Get|}() { + return null; } } }"; @@ -38,6 +40,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0002ProducedForMethodsWithWrongNameCancellationToken() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -45,13 +48,14 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0002:GetAsync|}(CancellationToken cancellation = default) + public virtual Response {|AZC0002:GetAsync|}(CancellationToken cancellation = default) { return null; } - public virtual void {|AZC0002:Get|}(CancellationToken cancellation = default) + public virtual Response {|AZC0002:Get|}(CancellationToken cancellation = default) { + return null; } } }"; @@ -65,6 +69,7 @@ public async Task AZC0002ProducedForMethodsWithWrongNameRequestContext() { const string code = @" using Azure; +using Azure.Core; using System.Threading; using System.Threading.Tasks; @@ -72,13 +77,14 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0002:GetAsync|}(RequestContext cancellation = default) + public virtual Response {|AZC0002:GetAsync|}(RequestContext cancellation = default) { return null; } - public virtual void {|AZC0002:Get|}(RequestContext cancellation = default) + public virtual Response {|AZC0002:Get|}(RequestContext cancellation = default) { + return null; } } }"; @@ -91,6 +97,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0002ProducedForMethodsWithNonOptionalCancellationToken() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -98,13 +105,14 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0002:GetAsync|}(CancellationToken cancellationToken) + public virtual Response {|AZC0002:GetAsync|}(CancellationToken cancellationToken) { return null; } - public virtual void {|AZC0002:Get|}(CancellationToken cancellationToken) + public virtual Response {|AZC0002:Get|}(CancellationToken cancellationToken) { + return null; } } }"; @@ -125,13 +133,14 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0002:GetAsync|}(RequestContext context = default, string text = default) + public virtual Response {|AZC0002:GetAsync|}(RequestContext context = default, string text = default) { return null; } - public virtual void {|AZC0002:Get|}(RequestContext context = default, string text = default) + public virtual Response {|AZC0002:Get|}(RequestContext context = default, string text = default) { + return null; } } }"; @@ -152,13 +161,14 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0002:GetAsync|}(CancellationToken cancellationToken = default, string text = default) + public virtual Response {|AZC0002:GetAsync|}(CancellationToken cancellationToken = default, string text = default) { return null; } - public virtual void {|AZC0002:Get|}(CancellationToken cancellationToken = default, string text = default) + public virtual Response {|AZC0002:Get|}(CancellationToken cancellationToken = default, string text = default) { + return null; } } }"; diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0003Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0003Tests.cs index f37e3bef93d..9b7cfe2c483 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0003Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0003Tests.cs @@ -13,6 +13,7 @@ public class AZC0003Tests public async Task AZC0003ProducedForNonVirtualMethods() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -20,13 +21,14 @@ namespace RandomNamespace { public class SomeClient { - public Task {|AZC0003:GetAsync|](CancellationToken cancellationToken = default) + public Response {|AZC0003:GetAsync|](CancellationToken cancellationToken = default) { return null; } - public void {|AZC0003:Get|](CancellationToken cancellationToken = default) + public Response {|AZC0003:Get|](CancellationToken cancellationToken = default) { + return null; } } }"; @@ -95,4 +97,4 @@ await Verifier.CreateAnalyzer(code) .RunAsync(); } } -} \ No newline at end of file +} diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs index 70bc7444df0..efb2d992778 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs @@ -13,6 +13,7 @@ public class AZC0004Tests public async Task AZC0004ProducedForMethodsWithoutSyncAlternative() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -20,7 +21,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0004:GetAsync|}(CancellationToken cancellationToken = default) + public virtual Response {|AZC0004:GetAsync|}(CancellationToken cancellationToken = default) { return null; } @@ -35,6 +36,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004ProducedForMethodsWithoutAsyncAlternative() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -42,7 +44,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0004:Get|}(CancellationToken cancellationToken = default) + public virtual Response {|AZC0004:Get|}(CancellationToken cancellationToken = default) { return null; } @@ -146,11 +148,11 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0004:GetAsync|}(string a, CancellationToken cancellationToken = default) + public virtual Response {|AZC0004:GetAsync|}(string a, CancellationToken cancellationToken = default) { return null; } - public virtual Task {|AZC0004:Get|}(string a, RequestContext context) + public virtual Response {|AZC0004:Get|}(string a, RequestContext context) { return null; } @@ -193,6 +195,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004ProducedForGenericMethodsWithSyncAlternative() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -200,16 +203,17 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(CancellationToken cancellationToken = default) + public virtual Response GetAsync(CancellationToken cancellationToken = default) { return null; } - public virtual void Get(CancellationToken cancellationToken = default) + public virtual Response Get(CancellationToken cancellationToken = default) { + return null; } - public virtual Task {|AZC0004:GetAsync|}(CancellationToken cancellationToken = default) + public virtual Response {|AZC0004:GetAsync|}(CancellationToken cancellationToken = default) { return null; } @@ -229,8 +233,15 @@ public async Task AZC0004NotProducedForGenericMethodsWithSyncAlternative() namespace RandomNamespace { +public class CapacityReservationCollection{ +} public class SomeClient { + public virtual CapacityReservationCollection GetCapacityReservations() + { + return null; + } + public virtual Task GetAsync(CancellationToken cancellationToken = default) { return null; @@ -260,6 +271,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004ProducedForGenericMethodsTakingGenericArgWithoutSyncAlternative() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -267,16 +279,17 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(CancellationToken cancellationToken = default) + public virtual Response GetAsync(CancellationToken cancellationToken = default) { return null; } - public virtual void Get(CancellationToken cancellationToken = default) + public virtual Response Get(CancellationToken cancellationToken = default) { + return null; } - public virtual Task {|AZC0004:GetAsync|}(T item, CancellationToken cancellationToken = default) + public virtual Task {|AZC0004:GetAsync|}(T item, CancellationToken cancellationToken = default) { return null; } @@ -323,32 +336,6 @@ await Verifier.CreateAnalyzer(code) .RunAsync(); } - [Fact] - public async Task AZC0004NProducedForMethodsWithoutArgMatchedSyncAlternative() - { - const string code = @" -using System.Threading; -using System.Threading.Tasks; - -namespace RandomNamespace -{ - public class SomeClient - { - public virtual Task {|AZC0004:GetAsync|}(int sameNameDifferentType, CancellationToken cancellationToken = default) - { - return null; - } - - public virtual void {|AZC0004:Get|}(string sameNameDifferentType, CancellationToken cancellationToken = default) - { - } - } -}"; - await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") - .RunAsync(); - } - [Fact] public async Task AZC0004NotProducedForGenericMethodsTakingGenericExpressionArgWithSyncAlternative() { @@ -381,6 +368,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004ProducedForGenericMethodsTakingGenericExpressionArgWithoutSyncAlternative() { const string code = @" +using Azure; using System; using System.Linq.Expressions; using System.Threading; @@ -390,13 +378,14 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0004:QueryAsync|}(Expression> filter, CancellationToken cancellationToken = default) + public virtual Response {|AZC0004:QueryAsync|}(Expression> filter, CancellationToken cancellationToken = default) { return null; } - public virtual void {|AZC0004:Query|}(Expression> filter, CancellationToken cancellationToken = default) + public virtual Response {|AZC0004:Query|}(Expression> filter, CancellationToken cancellationToken = default) { + return null; } } }"; @@ -442,6 +431,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004ProducedForArrayTypesWithoutSyncAlternative() { const string code = @" +using Azure; using System; using System.IO; using System.Threading; @@ -451,7 +441,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0004:AppendAsync|}( + public virtual Response {|AZC0004:AppendAsync|}( byte[] arr, CancellationToken cancellationToken = default) { @@ -459,10 +449,11 @@ public class SomeClient } - public virtual void {|AZC0004:Append|}( + public virtual Response {|AZC0004:Append|}( string[] arr, CancellationToken cancellationToken = default) { + return null; } } }"; @@ -475,6 +466,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004ProducedForMethodsWithoutSyncAlternativeWithMatchingArgNames() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -482,13 +474,14 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0004:GetAsync|}(int foo, CancellationToken cancellationToken = default) + public virtual Response {|AZC0004:GetAsync|}(int foo, CancellationToken cancellationToken = default) { return null; } - public virtual void {|AZC0004:Get|}(int differentName, CancellationToken cancellationToken = default) + public virtual Response {|AZC0004:Get|}(int differentName, CancellationToken cancellationToken = default) { + return null; } } }"; @@ -532,13 +525,13 @@ namespace RandomNamespace { public class SomeClient { - public SubClient GetSubClient() + public Sub GetSubClient() { return null; } } - public class SubClient + public class Sub { } }"; diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0015Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0015Tests.cs index 462a30629f1..05e5aa8994c 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0015Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0015Tests.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. using System.Threading.Tasks; @@ -9,7 +9,7 @@ namespace Azure.ClientSdk.Analyzers.Tests { public class AZC0015Tests { - [Theory] + [Theory(Skip = "We use return type as an exit criteria to check method, so this test set is not applicable until we find a better criteria.")] [InlineData("public Task> {|AZC0015:ClientMethodAsync|}() { return default; }")] [InlineData("public Task> {|AZC0015:ClientMethodAsync|}() { return default; }")] [InlineData("public int {|AZC0015:ClientMethodAsync|}() { return default; }")] @@ -36,7 +36,7 @@ await Verifier.CreateAnalyzer(code) .RunAsync(); } - [Theory] + [Theory(Skip = "We use return type as an exit criteria to check method, so this test set is not applicable until we find a better criteria.")] [InlineData("public Task> ClientMethodAsync() { return default; }")] [InlineData("public Operation ClientMethodAsync() { return default; }")] [InlineData("public Pageable ClientMethodAsync() { return default; }")] diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0017Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0017Tests.cs index 41edbc10aee..a8f25144260 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0017Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0017Tests.cs @@ -13,6 +13,7 @@ public class AZC0017Tests public async Task AZC0017ProducedForMethodsWithRequestContentParameter() { const string code = @" +using Azure; using Azure.Core; using System.Threading; using System.Threading.Tasks; @@ -21,13 +22,14 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task {|AZC0017:GetAsync|}(RequestContent content, CancellationToken cancellationToken = default) + public virtual Response {|AZC0017:GetAsync|}(RequestContent content, CancellationToken cancellationToken = default) { return null; } - public virtual void {|AZC0017:Get|}(RequestContent content, CancellationToken cancellationToken = default) + public virtual Response {|AZC0017:Get|}(RequestContent content, CancellationToken cancellationToken = default) { + return null; } } }"; 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 7a593bd42dc..956592f0350 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 @@ -21,6 +21,8 @@ public async Task AZC0018NotProducedForCorrectReturnType() namespace RandomNamespace { +public class AssetConversion {} + public class SomeClient { public virtual Task GetResponseAsync(string s, RequestContext context) 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 8caf97d2788..d3be35e253c 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs @@ -221,6 +221,11 @@ private static bool IsOrImplements(ITypeSymbol typeSymbol, string typeName) } private static void CheckClientMethodReturnType(ISymbolAnalysisContext context, IMethodSymbol method) + { + IsClientMethodReturnType(context, method, true); + } + + private static bool IsClientMethodReturnType(ISymbolAnalysisContext context, IMethodSymbol method, bool throwError = false) { ITypeSymbol originalType = method.ReturnType; ITypeSymbol unwrappedType = method.ReturnType; @@ -236,56 +241,55 @@ private static void CheckClientMethodReturnType(ISymbolAnalysisContext context, IsOrImplements(unwrappedType, NullableResponseTypeName) || IsOrImplements(unwrappedType, OperationTypeName) || IsOrImplements(originalType, PageableTypeName) || - IsOrImplements(originalType, AsyncPageableTypeName) || - originalType.Name.EndsWith(ClientSuffix)) + IsOrImplements(originalType, AsyncPageableTypeName)) { - return; + return true; } - context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0015, method.Locations.FirstOrDefault(), originalType.ToDisplayString()), method); + if (throwError) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0015, method.Locations.FirstOrDefault(), originalType.ToDisplayString()), method); + } + return false; } - private bool IsCheckExempt(IMethodSymbol method) + private bool IsCheckExempt(ISymbolAnalysisContext context, IMethodSymbol method) { return method.MethodKind != MethodKind.Ordinary || method.DeclaredAccessibility != Accessibility.Public || method.OverriddenMethod != null || method.IsImplicitlyDeclared || - (method.Name.StartsWith("Get") && method.Name.EndsWith(ClientSuffix)); + !IsClientMethodReturnType(context, method); } public override void AnalyzeCore(ISymbolAnalysisContext context) { INamedTypeSymbol type = (INamedTypeSymbol)context.Symbol; - List visitedSyncMember = new List(); foreach (var member in type.GetMembers()) { - if (member is IMethodSymbol asyncMethodSymbol && !IsCheckExempt(asyncMethodSymbol) && asyncMethodSymbol.Name.EndsWith(AsyncSuffix)) + if (member is IMethodSymbol asyncMethodSymbol && !IsCheckExempt(context, asyncMethodSymbol) && asyncMethodSymbol.Name.EndsWith(AsyncSuffix)) { - CheckClientMethod(context, asyncMethodSymbol); - var syncMemberName = member.Name.Substring(0, member.Name.Length - AsyncSuffix.Length); - var syncMember = FindMethod(type.GetMembers(syncMemberName).OfType(), asyncMethodSymbol.TypeParameters, asyncMethodSymbol.Parameters); if (syncMember == null) { context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0004, member.Locations.First()), member); } - else - { - visitedSyncMember.Add(syncMember); - CheckClientMethod(context, syncMember); - } + + CheckClientMethod(context, asyncMethodSymbol); } - else if (member is IMethodSymbol syncMethodSymbol && !IsCheckExempt(syncMethodSymbol) && !syncMethodSymbol.Name.EndsWith(AsyncSuffix) && !visitedSyncMember.Contains(syncMethodSymbol)) + else if (member is IMethodSymbol syncMethodSymbol && !IsCheckExempt(context, syncMethodSymbol) && !syncMethodSymbol.Name.EndsWith(AsyncSuffix)) { var asyncMemberName = member.Name + AsyncSuffix; var asyncMember = FindMethod(type.GetMembers(asyncMemberName).OfType(), syncMethodSymbol.TypeParameters, syncMethodSymbol.Parameters); + if (asyncMember == null) { context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0004, member.Locations.First()), member); } + + CheckClientMethod(context, syncMethodSymbol); } } } From 040cd84d3c4b7dbc72247a5c6a6ca063bbcef584 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Thu, 3 Aug 2023 15:24:26 +0800 Subject: [PATCH 11/14] update --- .../AZC0002Tests.cs | 38 +++++++++ .../AZC0018Tests.cs | 10 +++ .../ClientMethodsAnalyzer.cs | 79 ++++++++++++++----- .../Azure.ClientSdk.Analyzers/Descriptors.cs | 2 +- 4 files changed, 108 insertions(+), 21 deletions(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs index 3c5b56b99ed..8ebdcb03bfb 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs @@ -295,5 +295,43 @@ await Verifier.CreateAnalyzer(code) .WithDisabledDiagnostics("AZC0018") .RunAsync(); } + + [Fact] + public async Task AZC0002NotProducedIfThereIsAnOverloadWithCancellationToken() + { + const string code = @" +using Azure; +using System.Threading; +using System.Threading.Tasks; + +namespace RandomNamespace +{ + public class SomeClient + { + public virtual Response GetAsync(string s) + { + return null; + } + + public virtual Response Get(string s) + { + return null; + } + + public virtual Response GetAsync(string s, CancellationToken cancellationToken) + { + return null; + } + + public virtual Response Get(string s, CancellationToken cancellationToken) + { + return null; + } + } +}"; + await Verifier.CreateAnalyzer(code) + .WithDisabledDiagnostics("AZC0015") + .RunAsync(); + } } } 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 956592f0350..b9c39094ae9 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 @@ -25,6 +25,16 @@ public class AssetConversion {} public class SomeClient { + public virtual Task> GetHeadAsBooleanAsync(string s, RequestContext context) + { + return null; + } + + public virtual Response GetHeadAsBoolean(string s, RequestContext context) + { + return null; + } + public virtual Task GetResponseAsync(string s, RequestContext context) { return null; 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 d3be35e253c..2080173bdb1 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs @@ -21,6 +21,7 @@ public class ClientMethodsAnalyzer : ClientAnalyzerBase private const string NullableResponseTypeName = "NullableResponse"; private const string OperationTypeName = "Operation"; private const string TaskTypeName = "Task"; + private const string BooleanTypeName = "Boolean"; public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(new[] { @@ -45,7 +46,7 @@ private static bool IsRequestContext(IParameterSymbol parameterSymbol) private static bool IsCancellationToken(IParameterSymbol parameterSymbol) { - return parameterSymbol.Name == "cancellationToken" && parameterSymbol.Type.Name == "CancellationToken" && parameterSymbol.IsOptional; + return parameterSymbol.Name == "cancellationToken" && parameterSymbol.Type.Name == "CancellationToken"; } private static void CheckClientMethod(ISymbolAnalysisContext context, IMethodSymbol member) @@ -67,10 +68,31 @@ static bool IsCancellationOrRequestContext(IParameterSymbol parameterSymbol) if (!isCancellationOrRequestContext) { - context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0002, member.Locations.FirstOrDefault()), member); + var overloadSupportsCancellations = FindMethod( + member.ContainingType.GetMembers(member.Name).OfType(), + member.TypeParameters, + member.Parameters, + p => IsCancellationToken(p)); + + if (overloadSupportsCancellations == null) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0002, member.Locations.FirstOrDefault()), member); + } } else if (IsCancellationToken(lastArgument)) { + if (!lastArgument.IsOptional) + { + var overloadWithCancellationToken = FindMethod( + member.ContainingType.GetMembers(member.Name).OfType(), + member.TypeParameters, + member.Parameters.RemoveAt(member.Parameters.Length - 1)); + + if (overloadWithCancellationToken == null) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0002, member.Locations.FirstOrDefault()), member); + } + } // A convenience method should not have RequestContent as parameter if (member.Parameters.FirstOrDefault(IsRequestContent) != null) { @@ -129,7 +151,7 @@ private static void CheckProtocolMethodParameters(ISymbolAnalysisContext context } } - // A protocol method should not have model as type. Accepted return type: Response, Task, Pageable, AsyncPageable, Operation, Task>, Operation, Task, Operation>, Task>> + // A protocol method should not have model as type. Accepted return type: Response, Task, Response, Task>, Pageable, AsyncPageable, Operation, Task>, Operation, Task, Operation>, Task>> private static void CheckProtocolMethodReturnType(ISymbolAnalysisContext context, IMethodSymbol method) { bool IsValidPageable(ITypeSymbol typeSymbol) @@ -168,7 +190,11 @@ bool IsValidPageable(ITypeSymbol typeSymbol) { if (unwrappedType is INamedTypeSymbol responseTypeSymbol && responseTypeSymbol.IsGenericType) { - context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); + var responseReturn = responseTypeSymbol.TypeArguments.Single(); + if (responseReturn.Name != BooleanTypeName) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0018, method.Locations.FirstOrDefault()), method); + } } return; } @@ -264,32 +290,45 @@ private bool IsCheckExempt(ISymbolAnalysisContext context, IMethodSymbol method) public override void AnalyzeCore(ISymbolAnalysisContext context) { + void CheckSyncAsyncPair(INamedTypeSymbol type, IMethodSymbol method, string methodName) + { + var lastArgument = method.Parameters.LastOrDefault(); + IMethodSymbol methodSymbol = null; + if (lastArgument != null && IsRequestContext(lastArgument)) + { + methodSymbol = FindMethod(type.GetMembers(methodName).OfType(), method.TypeParameters, method.Parameters); + } + else if (lastArgument != null && IsCancellationToken(lastArgument)) + { + methodSymbol = FindMethod(type.GetMembers(methodName).OfType(), method.TypeParameters, method.Parameters.RemoveAt(method.Parameters.Length - 1), p => IsCancellationToken(p)); + } + else + { + return; + } + + if (methodSymbol == null) + { + context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0004, method.Locations.First()), method); + } + } + INamedTypeSymbol type = (INamedTypeSymbol)context.Symbol; foreach (var member in type.GetMembers()) { if (member is IMethodSymbol asyncMethodSymbol && !IsCheckExempt(context, asyncMethodSymbol) && asyncMethodSymbol.Name.EndsWith(AsyncSuffix)) { - var syncMemberName = member.Name.Substring(0, member.Name.Length - AsyncSuffix.Length); - var syncMember = FindMethod(type.GetMembers(syncMemberName).OfType(), asyncMethodSymbol.TypeParameters, asyncMethodSymbol.Parameters); - - if (syncMember == null) - { - context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0004, member.Locations.First()), member); - } - CheckClientMethod(context, asyncMethodSymbol); + + var syncMemberName = member.Name.Substring(0, member.Name.Length - AsyncSuffix.Length); + CheckSyncAsyncPair(type, asyncMethodSymbol, syncMemberName); } else if (member is IMethodSymbol syncMethodSymbol && !IsCheckExempt(context, syncMethodSymbol) && !syncMethodSymbol.Name.EndsWith(AsyncSuffix)) { - var asyncMemberName = member.Name + AsyncSuffix; - var asyncMember = FindMethod(type.GetMembers(asyncMemberName).OfType(), syncMethodSymbol.TypeParameters, syncMethodSymbol.Parameters); - - if (asyncMember == null) - { - context.ReportDiagnostic(Diagnostic.Create(Descriptors.AZC0004, member.Locations.First()), member); - } - CheckClientMethod(context, syncMethodSymbol); + + var asyncMemberName = member.Name + AsyncSuffix; + CheckSyncAsyncPair(type, syncMethodSymbol, asyncMemberName); } } } 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 338e29b27b7..447615e75f0 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs @@ -17,7 +17,7 @@ internal class Descriptors public static DiagnosticDescriptor AZC0002 = new DiagnosticDescriptor( nameof(AZC0002), "DO ensure all service methods, both asynchronous and synchronous, take an optional CancellationToken parameter called cancellationToken or a RequestContext parameter called context.", - "Client method should have an optional CancellationToken (both name and it being optional matters) or a RequestContext as the last parameter.", + "Client method should have an optional CancellationToken called cancellationToken (both name and it being optional matters) or a RequestContext called context as the last parameter.", "Usage", DiagnosticSeverity.Warning, isEnabledByDefault: true, description: null, "https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-service-methods-cancellation" ); From a1d3abbe80193d287db57152e4e58a33fcccdeb0 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Wed, 9 Aug 2023 18:36:10 +0800 Subject: [PATCH 12/14] Update --- .../AZC0002Tests.cs | 62 +++++------ .../AZC0003Tests.cs | 5 +- .../AZC0004Tests.cs | 103 ++++++++++-------- .../AZC0017Tests.cs | 10 +- .../AZC0018Tests.cs | 2 - .../ClientAnalyzerBase.cs | 1 + 6 files changed, 94 insertions(+), 89 deletions(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs index 8ebdcb03bfb..a93ce2b42e8 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0002Tests.cs @@ -20,7 +20,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0002:GetAsync|}() + public virtual Task {|AZC0002:GetAsync|}() { return null; } @@ -32,7 +32,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -48,7 +47,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0002:GetAsync|}(CancellationToken cancellation = default) + public virtual Task {|AZC0002:GetAsync|}(CancellationToken cancellation = default) { return null; } @@ -60,7 +59,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -77,7 +75,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0002:GetAsync|}(RequestContext cancellation = default) + public virtual Task {|AZC0002:GetAsync|}(RequestContext cancellation = default) { return null; } @@ -89,7 +87,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -105,7 +102,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0002:GetAsync|}(CancellationToken cancellationToken) + public virtual Task {|AZC0002:GetAsync|}(CancellationToken cancellationToken) { return null; } @@ -117,7 +114,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -133,7 +129,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0002:GetAsync|}(RequestContext context = default, string text = default) + public virtual Task {|AZC0002:GetAsync|}(RequestContext context = default, string text = default) { return null; } @@ -145,7 +141,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -161,7 +156,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0002:GetAsync|}(CancellationToken cancellationToken = default, string text = default) + public virtual Task {|AZC0002:GetAsync|}(CancellationToken cancellationToken = default, string text = default) { return null; } @@ -173,7 +168,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -181,6 +175,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0002NotProducedForMethodsWithCancellationToken() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -188,18 +183,18 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(CancellationToken cancellationToken = default) + public virtual Task GetAsync(CancellationToken cancellationToken = default) { return null; } - public virtual void Get(CancellationToken cancellationToken = default) + public virtual Response Get(CancellationToken cancellationToken = default) { + return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -215,45 +210,48 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task Get1Async(string s, CancellationToken cancellationToken = default) + public virtual Task Get1Async(string s, CancellationToken cancellationToken = default) { return null; } - public virtual void Get1(string s, CancellationToken cancellationToken = default) + public virtual Response Get1(string s, CancellationToken cancellationToken = default) { + return null; } - public virtual Task Get1Async(string s, RequestContext context) + public virtual Task Get1Async(string s, RequestContext context) { return null; } - public virtual void Get1(string s, RequestContext context) + public virtual Response Get1(string s, RequestContext context) { + return null; } - public virtual Task Get2Async(CancellationToken cancellationToken = default) + public virtual Task Get2Async(CancellationToken cancellationToken = default) { return null; } - public virtual void Get2(CancellationToken cancellationToken = default) + public virtual Response Get2(CancellationToken cancellationToken = default) { + return null; } - public virtual Task Get2Async(RequestContext context) + public virtual Task Get2Async(RequestContext context) { return null; } - public virtual void Get2(RequestContext context) + public virtual Response Get2(RequestContext context) { + return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .WithDisabledDiagnostics("AZC0018") .WithDisabledDiagnostics("AD0001") .RunAsync(); @@ -271,27 +269,28 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(RequestContext context = default) + public virtual Task GetAsync(RequestContext context = default) { return null; } - public virtual void Get(RequestContext context = default) + public virtual Response Get(RequestContext context = default) { + return null; } - public virtual Task Get2Async(RequestContext context) + public virtual Task Get2Async(RequestContext context) { return null; } - public virtual void Get2(RequestContext context) + public virtual Response Get2(RequestContext context) { + return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .WithDisabledDiagnostics("AZC0018") .RunAsync(); } @@ -308,7 +307,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response GetAsync(string s) + public virtual Task GetAsync(string s) { return null; } @@ -318,7 +317,7 @@ public virtual Response Get(string s) return null; } - public virtual Response GetAsync(string s, CancellationToken cancellationToken) + public virtual Task GetAsync(string s, CancellationToken cancellationToken) { return null; } @@ -330,7 +329,6 @@ public virtual Response Get(string s, CancellationToken cancellationToken) } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } } diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0003Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0003Tests.cs index 9b7cfe2c483..f485878e719 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0003Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0003Tests.cs @@ -21,7 +21,7 @@ namespace RandomNamespace { public class SomeClient { - public Response {|AZC0003:GetAsync|](CancellationToken cancellationToken = default) + public Task {|AZC0003:GetAsync|](CancellationToken cancellationToken = default) { return null; } @@ -33,7 +33,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -59,7 +58,6 @@ private void Get(CancellationToken cancellationToken = default) } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -93,7 +91,6 @@ public virtual void Get(CancellationToken cancellationToken = default) } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } } diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs index efb2d992778..7b191fd4f8b 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs @@ -21,14 +21,13 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0004:GetAsync|}(CancellationToken cancellationToken = default) + public virtual Task {|AZC0004:GetAsync|}(CancellationToken cancellationToken = default) { return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -51,7 +50,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -59,6 +57,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004NotProducedForMethodsWithCancellationToken() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -66,18 +65,17 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(CancellationToken cancellationToken = default) + public virtual Task GetAsync(CancellationToken cancellationToken = default) { return null; } - public virtual Task Get(CancellationToken cancellationToken = default) + public virtual Response Get(CancellationToken cancellationToken = default) { return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -93,18 +91,17 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(RequestContext context = null) + public virtual Task GetAsync(RequestContext context = null) { return null; } - public virtual Task Get(RequestContext context = null) + public virtual Response Get(RequestContext context = null) { return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .WithDisabledDiagnostics("AZC0018") .RunAsync(); } @@ -132,7 +129,6 @@ public virtual Response Get(RequestContext context) } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -148,7 +144,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0004:GetAsync|}(string a, CancellationToken cancellationToken = default) + public virtual Task {|AZC0004:GetAsync|}(string a, CancellationToken cancellationToken = default) { return null; } @@ -159,7 +155,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -186,7 +181,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .WithDisabledDiagnostics("AZC0018") .RunAsync(); } @@ -203,7 +197,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response GetAsync(CancellationToken cancellationToken = default) + public virtual Task GetAsync(CancellationToken cancellationToken = default) { return null; } @@ -213,14 +207,13 @@ public virtual Response Get(CancellationToken cancellationToken = default) return null; } - public virtual Response {|AZC0004:GetAsync|}(CancellationToken cancellationToken = default) + public virtual Task {|AZC0004:GetAsync|}(CancellationToken cancellationToken = default) { return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -228,42 +221,36 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004NotProducedForGenericMethodsWithSyncAlternative() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; namespace RandomNamespace { -public class CapacityReservationCollection{ -} public class SomeClient { - public virtual CapacityReservationCollection GetCapacityReservations() + public virtual Task GetAsync(CancellationToken cancellationToken = default) { return null; } - public virtual Task GetAsync(CancellationToken cancellationToken = default) + public virtual Response Get(CancellationToken cancellationToken = default) { return null; } - - public virtual void Get(CancellationToken cancellationToken = default) - { - } - public virtual Task GetAsync(CancellationToken cancellationToken = default) + public virtual Task GetAsync(CancellationToken cancellationToken = default) { return null; } - public virtual Task Get(CancellationToken cancellationToken = default) + public virtual Response Get(CancellationToken cancellationToken = default) { return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -279,7 +266,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response GetAsync(CancellationToken cancellationToken = default) + public virtual Task GetAsync(CancellationToken cancellationToken = default) { return null; } @@ -296,7 +283,6 @@ public virtual Response Get(CancellationToken cancellationToken = default) } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -304,6 +290,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004NotProducedForGenericMethodsTakingGenericArgWithSyncAlternative() { const string code = @" +using Azure; using System.Threading; using System.Threading.Tasks; @@ -311,35 +298,61 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(CancellationToken cancellationToken = default) + public virtual Task GetAsync(CancellationToken cancellationToken = default) { return null; } - public virtual void Get(CancellationToken cancellationToken = default) + public virtual Response Get(CancellationToken cancellationToken = default) { + return null; } - public virtual Task GetAsync(T item, CancellationToken cancellationToken = default) + public virtual Task GetAsync(T item, CancellationToken cancellationToken = default) { return null; } - public virtual Task Get(T item, CancellationToken cancellationToken = default) + public virtual Response Get(T item, CancellationToken cancellationToken = default) { return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } [Fact] + public async Task AZC0004NProducedForMethodsWithoutArgMatchedSyncAlternative() + { + const string code = @" +using Azure; +using System.Threading; +using System.Threading.Tasks; +namespace RandomNamespace +{ + public class SomeClient + { + public virtual Task {|AZC0004:GetAsync|}(int sameNameDifferentType, CancellationToken cancellationToken = default) + { + return null; + } + public virtual Response {|AZC0004:Get|}(string sameNameDifferentType, CancellationToken cancellationToken = default) + { + return null; + } + } +}"; + await Verifier.CreateAnalyzer(code) + .RunAsync(); + } + + [Fact] public async Task AZC0004NotProducedForGenericMethodsTakingGenericExpressionArgWithSyncAlternative() { const string code = @" +using Azure; using System; using System.Linq.Expressions; using System.Threading; @@ -349,18 +362,18 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task QueryAsync(Expression> filter, CancellationToken cancellationToken = default) + public virtual Task QueryAsync(Expression> filter, CancellationToken cancellationToken = default) { return null; } - public virtual void Query(Expression> filter, CancellationToken cancellationToken = default) + public virtual Response Query(Expression> filter, CancellationToken cancellationToken = default) { + return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -378,7 +391,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0004:QueryAsync|}(Expression> filter, CancellationToken cancellationToken = default) + public virtual Task {|AZC0004:QueryAsync|}(Expression> filter, CancellationToken cancellationToken = default) { return null; } @@ -390,7 +403,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -398,6 +410,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0004NotProducedForArrayTypesWithSyncAlternative() { const string code = @" +using Azure; using System; using System.IO; using System.Threading; @@ -407,7 +420,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task AppendAsync( + public virtual Task AppendAsync( byte[] arr, CancellationToken cancellationToken = default) { @@ -415,15 +428,15 @@ public virtual Task AppendAsync( } - public virtual void Append( + public virtual Response Append( byte[] arr, CancellationToken cancellationToken = default) { + return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -441,7 +454,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0004:AppendAsync|}( + public virtual Task {|AZC0004:AppendAsync|}( byte[] arr, CancellationToken cancellationToken = default) { @@ -458,7 +471,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -474,7 +486,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0004:GetAsync|}(int foo, CancellationToken cancellationToken = default) + public virtual Task {|AZC0004:GetAsync|}(int foo, CancellationToken cancellationToken = default) { return null; } @@ -486,7 +498,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0017Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0017Tests.cs index a8f25144260..efa2449cd0a 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0017Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0017Tests.cs @@ -22,7 +22,7 @@ namespace RandomNamespace { public class SomeClient { - public virtual Response {|AZC0017:GetAsync|}(RequestContent content, CancellationToken cancellationToken = default) + public virtual Task {|AZC0017:GetAsync|}(RequestContent content, CancellationToken cancellationToken = default) { return null; } @@ -34,7 +34,6 @@ public class SomeClient } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } @@ -42,6 +41,7 @@ await Verifier.CreateAnalyzer(code) public async Task AZC0017NotProducedForMethodsWithCancellationToken() { const string code = @" +using Azure; using Azure.Core; using System.Threading; using System.Threading.Tasks; @@ -50,18 +50,18 @@ namespace RandomNamespace { public class SomeClient { - public virtual Task GetAsync(string s, CancellationToken cancellationToken = default) + public virtual Task GetAsync(string s, CancellationToken cancellationToken = default) { return null; } - public virtual void Get(string s, CancellationToken cancellationToken = default) + public virtual Response Get(string s, CancellationToken cancellationToken = default) { + return null; } } }"; await Verifier.CreateAnalyzer(code) - .WithDisabledDiagnostics("AZC0015") .RunAsync(); } } 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 b9c39094ae9..3a222160d5a 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 @@ -21,8 +21,6 @@ public async Task AZC0018NotProducedForCorrectReturnType() namespace RandomNamespace { -public class AssetConversion {} - public class SomeClient { public virtual Task> GetHeadAsBooleanAsync(string s, RequestContext context) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs index 160745be0da..2e3480b9910 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs @@ -91,6 +91,7 @@ protected static IMethodSymbol FindMethod(IEnumerable methodSymbo return methodSymbols.SingleOrDefault(symbol => { + if (!symbol.Parameters.Any() || !genericParameters.SequenceEqual(symbol.TypeParameters, ParameterEquivalenceComparer.Default)) { return false; From c79e3e01f4b20be7a8b5e2cfbd991ce33d2d4a64 Mon Sep 17 00:00:00 2001 From: pshao25 <97225342+pshao25@users.noreply.github.com> Date: Thu, 10 Aug 2023 11:30:39 +0800 Subject: [PATCH 13/14] Update --- .../Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs index 7b191fd4f8b..62ccf3ef5f3 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0004Tests.cs @@ -324,7 +324,7 @@ await Verifier.CreateAnalyzer(code) } [Fact] - public async Task AZC0004NProducedForMethodsWithoutArgMatchedSyncAlternative() + public async Task AZC0004ProducedForMethodsWithoutArgMatchedSyncAlternative() { const string code = @" using Azure; From 100da6995bb03b6a49f49d967d014423ed0a0120 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 10 Aug 2023 14:16:10 -0700 Subject: [PATCH 14/14] Update file missed in merge from upstream/main --- .../Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs index 25b20d345fc..3a5b9109216 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientAnalyzerBase.cs @@ -32,7 +32,7 @@ protected class ParameterEquivalenceComparer : IEqualityComparer