[azsdk-cli] Add compile-time analyzer to enforce Tool method return types#11565
[azsdk-cli] Add compile-time analyzer to enforce Tool method return types#11565
Conversation
Co-authored-by: benbp <1020379+benbp@users.noreply.github.com>
Co-authored-by: benbp <1020379+benbp@users.noreply.github.com>
tools/azsdk-cli/Azure.Sdk.Tools.Cli.Analyzer/EnforceToolsReturnTypes.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli.Analyzer/EnforceToolsReturnTypes.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: benbp <1020379+benbp@users.noreply.github.com>
tools/azsdk-cli/Azure.Sdk.Tools.Cli.Analyzer/EnforceToolsReturnTypes.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: benbp <1020379+benbp@users.noreply.github.com>
6e5d6c8 to
d2b51bc
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new Roslyn analyzer to enforce that all public non-static methods in the Azure.Sdk.Tools.Cli.Tools namespace return only approved types at compile time, addressing the issue of inconsistent return types across tool methods.
- Adds new
EnforceToolsReturnTypesAnalyzer(MCP003) to validate tool method return types - Restructures tool namespaces to consolidate all tools under
Azure.Sdk.Tools.Cli.Tools - Updates .editorconfig to enforce braces on single-line if statements
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/azsdk-cli/Azure.Sdk.Tools.Cli.Analyzer/EnforceToolsReturnTypes.cs | New analyzer implementation that validates tool method return types |
| tools/azsdk-cli/Azure.Sdk.Tools.Cli.Analyzer/README.md | Documentation for the new MCP003 analyzer with usage examples |
| tools/azsdk-cli/Azure.Sdk.Tools.Cli/Tools/*/Tool.cs | Namespace consolidation moving all tools to Azure.Sdk.Tools.Cli.Tools |
| tools/azsdk-cli/Azure.Sdk.Tools.Cli/Commands/SharedOptions.cs | Removal of specific tool namespace imports after consolidation |
| Multiple tool and service files | Code style updates adding braces to single-line if statements |
| tools/azsdk-cli/.editorconfig | Style enforcement configuration updates |
| Test files | Updated imports to reflect namespace changes |
| tools/azsdk-cli/Azure.Sdk.Tools.Cli/Azure.Sdk.Tools.Cli.csproj | Enabled code style enforcement in build |
| case SpecialType.System_String: | ||
| case SpecialType.System_Boolean: | ||
| case SpecialType.System_Byte: | ||
| case SpecialType.System_Char: // NOTE: this seems to be matching against 'string' for some reason |
There was a problem hiding this comment.
The comment suggests System_Char is matching against 'string', which indicates a potential logic error. System_Char should match char types, not string. If this is unexpected behavior, the logic needs investigation and the comment should be clarified or the case removed if incorrect.
| case SpecialType.System_Char: // NOTE: this seems to be matching against 'string' for some reason |
| (namedType.ConstructedFrom?.ToDisplayString() == "System.Threading.Tasks.Task<T>" || | ||
| namedType.Name == "Task" && namedType.ContainingNamespace?.ToDisplayString() == "System.Threading.Tasks")) |
There was a problem hiding this comment.
[nitpick] The Task detection logic checks both the constructed form string and separate name/namespace conditions. This is redundant and could be simplified to use either the ToDisplayString() approach or the name/namespace approach consistently.
| (namedType.ConstructedFrom?.ToDisplayString() == "System.Threading.Tasks.Task<T>" || | |
| namedType.Name == "Task" && namedType.ContainingNamespace?.ToDisplayString() == "System.Threading.Tasks")) | |
| namedType.ConstructedFrom?.ToDisplayString() == "System.Threading.Tasks.Task<T>") |
| var ienumerableInterface = returnType.AllInterfaces.FirstOrDefault( | ||
| i => i.IsGenericType && | ||
| i.ConstructedFrom?.ToDisplayString() == "System.Collections.Generic.IEnumerable<T>"); | ||
|
|
||
| if (ienumerableInterface != null && ienumerableInterface.TypeArguments.Length > 0) | ||
| { | ||
| var elementType = ienumerableInterface.TypeArguments[0]; |
There was a problem hiding this comment.
Using FirstOrDefault() with a complex predicate can be inefficient. Consider using a more specific lookup or caching the constructed generic type definition for IEnumerable to improve performance when analyzing large codebases.
| var ienumerableInterface = returnType.AllInterfaces.FirstOrDefault( | |
| i => i.IsGenericType && | |
| i.ConstructedFrom?.ToDisplayString() == "System.Collections.Generic.IEnumerable<T>"); | |
| if (ienumerableInterface != null && ienumerableInterface.TypeArguments.Length > 0) | |
| { | |
| var elementType = ienumerableInterface.TypeArguments[0]; | |
| var ienumerableTypeSymbol = compilation.GetTypeByMetadataName("System.Collections.Generic.IEnumerable`1"); | |
| if (ienumerableTypeSymbol == null) | |
| { | |
| return false; | |
| } | |
| var ienumerableInterface = returnType.AllInterfaces | |
| .FirstOrDefault(i => i is INamedTypeSymbol named && | |
| named.IsGenericType && | |
| SymbolEqualityComparer.Default.Equals(named.OriginalDefinition, ienumerableTypeSymbol)); | |
| if (ienumerableInterface != null && ((INamedTypeSymbol)ienumerableInterface).TypeArguments.Length > 0) | |
| { | |
| var elementType = ((INamedTypeSymbol)ienumerableInterface).TypeArguments[0]; |
This PR implements a new Roslyn analyzer to enforce that all public non-static methods in the
Azure.Sdk.Tools.Cli.Toolsnamespace return only approved types at compile time.Problem
Tool methods were returning inconsistent types, including raw collections and custom objects that don't follow the established response pattern. This made the API inconsistent and harder to consume.
Solution
Added a new analyzer
EnforceToolsReturnTypesAnalyzer(MCP003) that enforces the following return type rules:Allowed Return Types
Azure.Sdk.Tools.Cli.Models.ResponseValidationResponse,DefaultCommandResponse,LogAnalysisResponse, etc.string,int,bool,DateTime, etc.Task<T>andValueTask<T>where T is any allowed type aboveTaskfor async methods that don't return valuesAnalyzer Features
GetCommand()andHandleCommand()Current Violations Detected
The analyzer identifies 6 methods that need to be updated:
List<FailedTestRunResponse>should return a Response containing the listList<int>orList<string>should return a Response containing the collectionIList<string>should return a Response containing the collectionExample
The analyzer provides immediate feedback with error message:
All existing tests pass and the analyzer correctly validates both positive and negative cases without false positives.
Fixes #11564.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.