Skip to content

Commit

Permalink
Update
Browse files Browse the repository at this point in the history
  • Loading branch information
@pshao25
pshao25 committed Jul 25, 2023
1 parent 6fdd59a commit 082febc
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 189 deletions.
104 changes: 3 additions & 101 deletions src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0018Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ await Verifier.CreateAnalyzer(code)
}

[Fact]
public async Task AZC0018ProducedForMethodsWithNoRequestContentButProtocolAndConvenience()
public async Task AZC0018NotProducedForMethodsWithRequestContentAndOptionalRequestContext()
{
const string code = @"
using Azure;
Expand All @@ -339,110 +339,12 @@ namespace RandomNamespace
{
public class SomeClient
{
public virtual Task<Response> GetAsync(string a, CancellationToken cancellationToken = default)
{
return null;
}
public virtual Response Get(string a, CancellationToken cancellationToken = default)
{
return null;
}
public virtual Task<Response> {|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>(T value, string name)
{
if (value is null)
{
throw new System.ArgumentNullException(name);
}
}
}
}
namespace RandomNamespace
{
public class SomeClient
{
public virtual Task<Response> {|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>(T value, string name)
{
if (value is null)
{
throw new System.ArgumentNullException(name);
}
}
}
}
namespace RandomNamespace
{
public class SomeClient
{
public virtual Task<Response> {|AZC0018:GetAsync|}(RequestContent content, RequestContext context = null)
public virtual Task<Response> 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;
}
Expand Down
51 changes: 51 additions & 0 deletions src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0019Tests.cs
View file
Original file line number Diff line number Diff line change
@@ -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<Azure.ClientSdk.Analyzers.ClientMethodsAnalyzer>;

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<Response> GetAsync(string a, CancellationToken cancellationToken = default)
{
return null;
}
public virtual Response Get(string a, CancellationToken cancellationToken = default)
{
return null;
}
public virtual Task<Response> {|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();
}
}
}
89 changes: 5 additions & 84 deletions src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<InvocationExpressionSyntax>())
{
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";
Expand Down Expand Up @@ -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 =>
Expand All @@ -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<Response>, Pageable<BinaryData>, AsyncPageable<BinaryData>, Operation<BinaryData>, Task<Operation<BinaryData>>, Operation, Task<Operation>, Operation<Pageable<BinaryData>>, Task<Operation<AsyncPageable<BinaryData>>>
Expand Down
10 changes: 8 additions & 2 deletions src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/SymbolAnalyzerBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -81,4 +81,4 @@ public void ReportDiagnostic(Diagnostic diagnostic, ISymbol symbol)
}
}
}
}
}

0 comments on commit 082febc

Please sign in to comment.