From 106b06f40c7df135811e8efdf3b58fe3be340172 Mon Sep 17 00:00:00 2001 From: Michael Nash Date: Wed, 3 Jun 2026 16:50:39 -0700 Subject: [PATCH] Remove ModelFactoryAnalyzer (AZC0035) - migrated to azure-sdk-for-net The ModelFactoryAnalyzer was migrated to Azure/azure-sdk-for-net (sdk/tools/Azure.SdkAnalyzers) in PR #59647, which also fixed an AZC0035 false-positive on output models from external assemblies. Removing the duplicate analyzer here so AZC0035 only fires from one source. APIView does not depend on AZC0035. - Delete ModelFactoryAnalyzer.cs and AZC0035Tests.cs - Remove AZC0035 DiagnosticDescriptor from Descriptors.cs - AnalyzerPrompts.Client.cs reference to AZC0035 retained (text-only prompt template; diagnostic still emitted by azure-sdk-for-net's analyzer) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AZC0035Tests.cs | 579 ------------------ .../Azure.ClientSdk.Analyzers/Descriptors.cs | 9 - .../ModelFactoryAnalyzer.cs | 333 ---------- 3 files changed, 921 deletions(-) delete mode 100644 src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0035Tests.cs delete mode 100644 src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ModelFactoryAnalyzer.cs diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0035Tests.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0035Tests.cs deleted file mode 100644 index 637d0aa950a..00000000000 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers.Tests/AZC0035Tests.cs +++ /dev/null @@ -1,579 +0,0 @@ -// 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 ModelFactoryAnalyzerTests - { - [Theory] - [InlineData("Response")] - [InlineData("Task>")] - [InlineData("NullableResponse")] - [InlineData("Operation")] - [InlineData("Task>")] - [InlineData("Pageable")] - [InlineData("AsyncPageable")] - public async Task AZC0035_ProducedWhenOutputModelMissingFromModelFactory(string returnType) - { - string code = $@" -using Azure; -using System.Threading.Tasks; - -namespace Azure.Test -{{ - public class {{|AZC0035:TestModel|}} - {{ - private TestModel() {{ }} - public string Name {{ get; }} - }} - - public class TestClient - {{ - public virtual {returnType} GetTestModel() - {{ - return null; - }} - }} -}}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_NotProducedWhenOutputModelHasCorrespondingModelFactory() - { - const string code = @" -using Azure; -using System.Threading.Tasks; - -namespace Azure.Test -{ - public class TestModel1 - { - } - - public class TestModel2 - { - } - - public class TestClient - { - public virtual Response GetTestModel1() - { - return null; - } - - public virtual Task> GetTestModel2Async() - { - return null; - } - } - - public static class TestModelFactory - { - public static TestModel1 TestModel1() - { - return null; - } - - public static TestModel2 TestModel2() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_IgnoresNonClientClassesAndBuiltInTypes() - { - const string code = @" -using Azure; -using System.Threading.Tasks; - -namespace Azure.Test -{ - public class TestModel - { - } - - public class SomeService - { - public virtual Response GetTestModel() - { - return null; - } - } - - public class TestClient - { - public virtual Response GetResponse() - { - return null; - } - - public virtual Response GetString() - { - return null; - } - - public virtual Response GetInt() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_IgnoresEmptyClassesWithPublicConstructors() - { - const string code = @" -using Azure; -using System.Threading.Tasks; - -namespace Azure.Test -{ - // Empty class with implicit public constructor - should NOT be flagged - public class EmptyModel - { - } - - // Empty class with explicit public constructor - should NOT be flagged - public class EmptyModelExplicit - { - public EmptyModelExplicit() { } - } - - public class TestClient - { - public virtual Response GetEmptyModel() - { - return null; - } - - public virtual Response GetEmptyModelExplicit() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_FlagsEmptyClassesWithNoPublicConstructor() - { - const string code = @" -using Azure; -using System.Threading.Tasks; - -namespace Azure.Test -{ - // Empty class with no public constructor - should be flagged - public class {|AZC0035:EmptyModelPrivate|} - { - private EmptyModelPrivate() { } - } - - public class TestClient - { - public virtual Response GetEmptyModel() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_FlagsTypesWithPartiallySettableProperties() - { - const string code = @" -using Azure; -using System.Threading.Tasks; - -namespace Azure.Test -{ - // This has a public constructor but not all properties can be set - should be flagged - public class {|AZC0035:ExampleModel|} - { - public int Age { get; } - public string Name { get; } - - public ExampleModel(string name) - { - Name = name; - } - } - - public class TestClient - { - public virtual Response GetExampleModel() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_RealWorldExamples() - { - const string code = @" -using Azure; -using System; -using System.Threading.Tasks; - -namespace Azure.Storage.Blobs -{ - // Client type - should be ignored - public class BlobContainerClient - { - public string Name { get; set; } - public string AccountName { get; set; } - } - - // Easily instantiable via setters - should be ignored - public class BlobServiceProperties - { - public string Version { get; set; } - public bool LoggingEnabled { get; set; } - public int RetentionDays { get; set; } - } - - // Easily instantiable via constructor - should be ignored - public class BlobImmutabilityPolicy - { - public BlobImmutabilityPolicy(string policyType, int retentionDays) - { - PolicyType = policyType; - RetentionDays = retentionDays; - } - - public string PolicyType { get; } - public int RetentionDays { get; } - } - - // Easily instantiable via constructor + setters - should be ignored - public class ReleasedObjectInfo - { - public ReleasedObjectInfo(string objectId) - { - ObjectId = objectId; - } - - public string ObjectId { get; } - public string Status { get; set; } - public DateTime? ReleasedAt { get; set; } - } - - // Should still be flagged - private constructor - public class {|AZC0035:PrivateConstructorModel|} - { - private PrivateConstructorModel() { } - public string Name { get; set; } - } - - // Should still be flagged - read-only properties without constructor params - public class {|AZC0035:ReadOnlyModel|} - { - public string Name { get; } - public int Value { get; } - } - - public class TestClient - { - public virtual Response GetBlobContainer() => null; - public virtual Response GetServiceProperties() => null; - public virtual Response GetImmutabilityPolicy() => null; - public virtual Response GetReleasedObjectInfo() => null; - public virtual Response GetPrivateConstructorModel() => null; - public virtual Response GetReadOnlyModel() => null; - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_IgnoresSystemTypes() - { - const string code = @" -using Azure; -using System; -using System.Threading; -using System.Threading.Tasks; - -namespace System -{ - // System.BinaryData - should be ignored - public class BinaryData - { - public string Content { get; } - } -} - -namespace System.Threading -{ - // System.Threading.Thread - should be ignored - public class Thread - { - public string Name { get; set; } - public bool IsAlive { get; } - } -} - -namespace Azure.Test -{ - public class TestClient - { - // These should NOT produce diagnostics because BinaryData and Thread are System types - public virtual Response GetBinaryData() - { - return null; - } - - public virtual Task> GetThreadAsync() - { - return null; - } - - // This should also work with generic unwrapping - public virtual Task> GetOperationOfBinaryDataAsync() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_AllowsSystemClientModelTypes() - { - const string code = @" -using Azure; -using System.Threading.Tasks; - -namespace System.ClientModel -{ - // Types in System.ClientModel should still be checked - this one should be flagged - public class {|AZC0035:CustomClientModel|} - { - private CustomClientModel() { } - public string Name { get; } - } - - // This one should not be flagged - has public constructor with settable properties - public class AllowedClientModel - { - public AllowedClientModel(string name) - { - Name = name; - } - - public string Name { get; } - } -} - -namespace System.ClientModel.Primitives -{ - // Types in System.ClientModel subnamespaces should also be checked now with the fix - public class {|AZC0035:PrimitiveModel|} - { - private PrimitiveModel() { } - public string Value { get; } - } -} - -namespace Azure.Test -{ - public class TestClient - { - public virtual Response GetCustomClientModel() - { - return null; - } - - public virtual Response GetAllowedClientModel() - { - return null; - } - - public virtual Response GetPrimitiveModel() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_GenericUnwrappingWithSystemTypes() - { - const string code = @" -using Azure; -using System; -using System.Threading.Tasks; - -namespace System -{ - public class BinaryData - { - public string Content { get; } - } -} - -namespace Azure.Test -{ - // This should be flagged because it's not a System type and has no public constructor - public class {|AZC0035:CustomModel|} - { - private CustomModel() { } - public string Name { get; } - } - - public class TestClient - { - // All these should NOT be flagged because BinaryData is a System type - // even when wrapped in generics - unwrapping should still work correctly - public virtual Task> GetBinaryDataInTaskResponseAsync() - { - return null; - } - - public virtual Task> GetBinaryDataInTaskOperationAsync() - { - return null; - } - - public virtual Task> GetBinaryDataInTaskPageableAsync() - { - return null; - } - - // This SHOULD be flagged because CustomModel is not a System type - public virtual Task> GetCustomModelAsync() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - [Fact] - public async Task AZC0035_SystemClientModelGenericTypes() - { - const string code = @" -using Azure; -using System.Threading.Tasks; - -namespace System.ClientModel -{ - // System.ClientModel type that should be flagged - public class {|AZC0035:ClientResult|} - { - private ClientResult() { } - public string Value { get; } - } - - // System.ClientModel type that should NOT be flagged (easily constructible) - public class EasyClientModel - { - public string Name { get; set; } - public int Value { get; set; } - } -} - -namespace Azure.Test -{ - public class TestClient - { - // ClientResult should be flagged even though it's in System.ClientModel - public virtual Response GetClientResult() - { - return null; - } - - // EasyClientModel should NOT be flagged - public virtual Response GetEasyClientModel() - { - return null; - } - - // Test unwrapping with System.ClientModel types - public virtual Task> GetClientResultAsync() - { - return null; - } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - - - [Fact] - public async Task AZC0035_OnlyAnalyzesSourceDefinedClientTypes() - { - const string code = @" -using Azure; -using System.Threading.Tasks; - -namespace Azure.Test -{ - // Source-defined model that should be flagged - public class {|AZC0035:MyCustomModel|} - { - private MyCustomModel() { } - public string Value { get; } - } - - // Source-defined client - should be analyzed - public class MyClient - { - public virtual Response GetModel() - { - return null; - } - } - - // Source-defined model factory - should be analyzed - public static class MyModelFactory - { - // This factory method doesn't cover MyCustomModel, so MyCustomModel should be flagged - public static SomeOtherModel SomeOtherModel() - { - return null; - } - } - - public class SomeOtherModel - { - public string Name { get; set; } - } -}"; - - await Verifier.CreateAnalyzer(code).RunAsync(); - } - } -} 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 d97b98a6e55..76d2c4094ac 100644 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs +++ b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/Descriptors.cs @@ -179,15 +179,6 @@ internal class Descriptors true, "Type names should not conflict with other SDK and .NET platform types."); - public static readonly DiagnosticDescriptor AZC0035 = new DiagnosticDescriptor( - nameof(AZC0035), - "Output model type should have a corresponding model factory method", - "Output model type '{0}' should have a corresponding method in a model factory class. Add a static method that returns '{0}' to a class ending with 'ModelFactory'.", - DiagnosticCategory.Usage, - DiagnosticSeverity.Warning, - true, - "Output model types returned from client methods should have corresponding model factory methods for mocking support."); - public static readonly DiagnosticDescriptor AZC0036 = new DiagnosticDescriptor( nameof(AZC0036), "Improper model name suffix", diff --git a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ModelFactoryAnalyzer.cs b/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ModelFactoryAnalyzer.cs deleted file mode 100644 index 4e5cd42710a..00000000000 --- a/src/dotnet/Azure.ClientSdk.Analyzers/Azure.ClientSdk.Analyzers/ModelFactoryAnalyzer.cs +++ /dev/null @@ -1,333 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -using System; -using System.Collections.Generic; -using System.Collections.Immutable; -using System.Linq; -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.Diagnostics; - -namespace Azure.ClientSdk.Analyzers -{ - [DiagnosticAnalyzer(LanguageNames.CSharp)] - public class ModelFactoryAnalyzer : DiagnosticAnalyzer - { - private const string ClientSuffix = "Client"; - private const string ModelFactorySuffix = "ModelFactory"; - - private const string AzureNamespace = "Azure"; - private const string SystemClientModelNamespace = "System.ClientModel"; - private const string SystemNamespace = "System"; - private const string PageableTypeName = "Pageable"; - private const string AsyncPageableTypeName = "AsyncPageable"; - private const string ResponseTypeName = "Response"; - private const string NullableResponseTypeName = "NullableResponse"; - private const string OperationTypeName = "Operation"; - private const string TaskTypeName = "Task"; - private const string ClientResultTypeName = "ClientResult"; - private const string CollectionResultTypeName = "CollectionResult"; - private const string AsyncCollectionResultTypeName = "AsyncCollectionResult"; - private const string PageableOperationTypeName = "PageableOperation"; - - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create( - Descriptors.AZC0035 - ); - - public override void Initialize(AnalysisContext context) - { - context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); - context.EnableConcurrentExecution(); - context.RegisterCompilationAction(AnalyzeCompilation); - } - - private static void AnalyzeCompilation(CompilationAnalysisContext context) - { - var outputModels = new HashSet(SymbolEqualityComparer.Default); - var modelFactoryMethods = new HashSet(SymbolEqualityComparer.Default); - - // Find all client classes and extract output models from their methods - // Only process types defined in the current source tree (not dependencies) - foreach (var namedType in GetAllTypes(context.Compilation.GlobalNamespace)) - { - if (!SymbolEqualityComparer.Default.Equals(namedType.ContainingAssembly, context.Compilation.Assembly)) - { - continue; - } - - if (IsClientType(namedType)) - { - ExtractOutputModelsFromClientType(namedType, outputModels); - } - else if (IsModelFactoryType(namedType)) - { - ExtractReturnTypesFromModelFactory(namedType, modelFactoryMethods); - } - } - - // Check if all output models have corresponding model factory methods - foreach (var outputModel in outputModels) - { - if (!modelFactoryMethods.Contains(outputModel)) - { - // Find a location to report the diagnostic - use the first location of the type - var location = outputModel.Locations.FirstOrDefault(); - if (location != null) - { - var diagnostic = Diagnostic.Create( - Descriptors.AZC0035, - location, - outputModel.Name); - context.ReportDiagnostic(diagnostic); - } - } - } - } - - private static bool IsClientType(INamedTypeSymbol namedType) - { - return namedType.TypeKind == TypeKind.Class && - namedType.Name.EndsWith(ClientSuffix) && - namedType.DeclaredAccessibility == Accessibility.Public; - } - - private static bool IsModelFactoryType(INamedTypeSymbol namedType) - { - return namedType.TypeKind == TypeKind.Class && - namedType.Name.EndsWith(ModelFactorySuffix) && - namedType.IsStatic && - namedType.DeclaredAccessibility == Accessibility.Public; - } - - private static void ExtractOutputModelsFromClientType(INamedTypeSymbol clientType, HashSet outputModels) - { - foreach (var member in clientType.GetMembers()) - { - if (member is IMethodSymbol method && method.DeclaredAccessibility == Accessibility.Public) - { - // Skip property accessors as they are not client methods - if (method.AssociatedSymbol is IPropertySymbol) - { - continue; - } - - var outputModel = ExtractOutputModelFromReturnType(method.ReturnType); - if (outputModel != null) - { - outputModels.Add(outputModel); - } - } - } - } - - private static void ExtractReturnTypesFromModelFactory(INamedTypeSymbol factoryType, HashSet modelFactoryMethods) - { - foreach (var member in factoryType.GetMembers()) - { - if (member is IMethodSymbol method && - method.DeclaredAccessibility == Accessibility.Public && - method.IsStatic) - { - // Add the return type of the factory method - modelFactoryMethods.Add(method.ReturnType); - } - } - } - - private static ITypeSymbol ExtractOutputModelFromReturnType(ITypeSymbol returnType) - { - ITypeSymbol unwrappedType = returnType; - - // Unwrap Task - if (returnType is INamedTypeSymbol namedType && - namedType.IsGenericType && - namedType.Name == TaskTypeName) - { - unwrappedType = namedType.TypeArguments.FirstOrDefault(); - if (unwrappedType == null) return null; - } - - ITypeSymbol modelType = null; - - // Check for Azure client method return types and extract the model type - if (IsOrImplements(unwrappedType, ResponseTypeName, AzureNamespace) || - IsOrImplements(unwrappedType, NullableResponseTypeName, AzureNamespace) || - IsOrImplements(unwrappedType, ClientResultTypeName, SystemClientModelNamespace) || - IsOrImplements(unwrappedType, OperationTypeName, AzureNamespace) || - IsOrImplements(unwrappedType, PageableTypeName, AzureNamespace) || - IsOrImplements(unwrappedType, AsyncPageableTypeName, AzureNamespace) || - IsOrImplements(unwrappedType, CollectionResultTypeName, SystemClientModelNamespace) || - IsOrImplements(unwrappedType, AsyncCollectionResultTypeName, SystemClientModelNamespace) || - IsOrImplements(unwrappedType, PageableOperationTypeName, AzureNamespace)) - { - if (unwrappedType is INamedTypeSymbol genericType && genericType.IsGenericType) - { - modelType = genericType.TypeArguments.FirstOrDefault(); - } - } - - // Only return user-defined types, not built-in types - if (modelType != null && IsUserDefinedModelType(modelType)) - { - return modelType; - } - - return null; - } - - private static bool IsUserDefinedModelType(ITypeSymbol typeSymbol) - { - // Filter out System types, unless defined in System.ClientModel - var containingNamespace = typeSymbol.ContainingNamespace?.ToString() ?? string.Empty; - - if (containingNamespace == SystemNamespace || containingNamespace.StartsWith($"{SystemNamespace}.") - && containingNamespace != SystemClientModelNamespace - && (!containingNamespace.StartsWith($"{SystemClientModelNamespace}."))) - { - return false; - } - - // Filter out built-in types - if (typeSymbol.SpecialType != SpecialType.None) - { - return false; - } - - // Only consider class and struct types, but not enums - if (typeSymbol.TypeKind != TypeKind.Class && typeSymbol.TypeKind != TypeKind.Struct) - { - return false; - } - - // Filter out client types - they are not models - if (typeSymbol.Name.EndsWith(ClientSuffix)) - { - return false; - } - - // Filter out types that can be easily instantiated (have public constructors with all properties settable) - if (CanBeConstructedUsingPublicApis(typeSymbol)) - { - return false; - } - - return true; - } - - private static bool CanBeConstructedUsingPublicApis(ITypeSymbol typeSymbol) - { - if (!(typeSymbol is INamedTypeSymbol namedType)) - { - return false; - } - - // Check if there is at least one public constructor - var publicConstructors = namedType.Constructors.Where(c => c.DeclaredAccessibility == Accessibility.Public).ToList(); - if (!publicConstructors.Any()) - { - return false; - } - - // Get all instance properties - var properties = namedType.GetMembers() - .OfType() - .Where(p => !p.IsStatic) - .ToList(); - - // If there are no properties, it can be constructed via public constructor - if (!properties.Any()) - { - return true; - } - - // Get properties that don't have public setters - these must be set via constructor - var propertiesNeedingConstructor = properties - .Where(p => p.SetMethod?.DeclaredAccessibility != Accessibility.Public) - .ToList(); - - // If all properties have public setters, the type can be constructed - if (!propertiesNeedingConstructor.Any()) - { - return true; - } - - // Check if at least one public constructor can set all properties that need constructor parameters - foreach (var constructor in publicConstructors) - { - bool allPropertiesCanBeSet = true; - - foreach (var property in propertiesNeedingConstructor) - { - // Check if this constructor has a parameter that can set this property - bool hasMatchingParameter = constructor.Parameters.Any(p => - string.Equals(p.Name, property.Name, StringComparison.OrdinalIgnoreCase) || - string.Equals(p.Name, property.Name.Substring(0, 1).ToLower() + property.Name.Substring(1), StringComparison.Ordinal)); - - if (!hasMatchingParameter) - { - allPropertiesCanBeSet = false; - break; - } - } - - // If this constructor can set all required properties, the type can be constructed - if (allPropertiesCanBeSet) - { - return true; - } - } - - // No constructor can set all required properties - return false; - } - - private static bool IsOrImplements(ITypeSymbol typeSymbol, string typeName, string namespaceName) - { - if (typeSymbol.Name == typeName && GetFullNamespaceName(typeSymbol.ContainingNamespace) == namespaceName) - { - return true; - } - - if (typeSymbol.BaseType != null) - { - return IsOrImplements(typeSymbol.BaseType, typeName, namespaceName); - } - - return false; - } - - private static string GetFullNamespaceName(INamespaceSymbol namespaceSymbol) - { - if (namespaceSymbol.IsGlobalNamespace) - { - return ""; - } - - var parts = new List(); - var current = namespaceSymbol; - while (current != null && !current.IsGlobalNamespace) - { - parts.Insert(0, current.Name); - current = current.ContainingNamespace; - } - - return string.Join(".", parts); - } - - private static IEnumerable GetAllTypes(INamespaceSymbol namespaceSymbol) - { - foreach (var type in namespaceSymbol.GetTypeMembers()) - { - yield return type; - } - - foreach (var childNamespace in namespaceSymbol.GetNamespaceMembers()) - { - foreach (var type in GetAllTypes(childNamespace)) - { - yield return type; - } - } - } - } -}