Add rules for convenience method and protocol method #6934
Conversation
pshao25
requested review from
jsquire,
pallavit,
JoshLove-msft,
christothes,
annelo-msft,
KrzysztofCwalina,
tg-msft,
heaths and
m-nash
as code owners
src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs
Outdated
Show resolved
Hide resolved
src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs
Outdated
Show resolved
Hide resolved
src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs
Outdated
Show resolved
Hide resolved
src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs
Outdated
Show resolved
Hide resolved
| if (unwrappedType is INamedTypeSymbol responseTypeSymbol && responseTypeSymbol.IsGenericType) | ||
| { | ||
| var responseReturn = responseTypeSymbol.TypeArguments.Single(); | ||
| if (responseReturn.Name != BooleanTypeName) |
There was a problem hiding this comment.
Just curious - why is Response<bool> special cased here?
There was a problem hiding this comment.
If users set flag head-as-boolean, then the HEAD method will return Response<bool> instead of Response.
| if ((!IsOrImplements(typeSymbol, PageableTypeName)) && (!IsOrImplements(typeSymbol, AsyncPageableTypeName))) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Is this check necessary? If we ever return false here, it would be because we called this for the wrong method type. Both call sites to this method do the same if check before calling.
There was a problem hiding this comment.
Emmm, I could remove it, but my initial thought is from the method IsValidPageable itself perspective, I should check if it is a pageable type because it is called IsValidPageable, otherwise if an operation type is passed mistakenly, it will still return true.
| return; | ||
| } | ||
|
|
||
| if (!IsOrImplements(operationReturn, BinaryDataTypeName)) |
There was a problem hiding this comment.
Does this mean operations can only return BinaryData types?
There was a problem hiding this comment.
Yes. You could see all the accepted return types from the comments of this method:
Response, Task<Response>, Response<bool>, Task<Response<bool>>, Pageable<BinaryData>, AsyncPageable<BinaryData>, Operation<BinaryData>, Task<Operation<BinaryData>>, Operation, Task<Operation>, Operation<Pageable<BinaryData>>, Task<Operation<AsyncPageable<BinaryData>>>
src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ClientMethodsAnalyzer.cs
Show resolved
Hide resolved
| // A protocol method should not have model as parameter. If it has ambiguity with convenience method, it should have required RequestContext. | ||
| // 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) |
There was a problem hiding this comment.
would a better name for this method be something like - CheckforAmbiguousMethodPair ?
There was a problem hiding this comment.
But this method not only checks the ambiguity, but also the input parameters and output parameters of protocol methods.
Overall, the logic is:
Regen PR: Azure/azure-sdk-for-net#38603