From 7f1d2585a172371da261f5add1ef1caf8eb1374e Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Mon, 15 Jun 2026 12:27:39 -0500 Subject: [PATCH 1/4] feat: generalize back compat --- .../src/Providers/ClientProvider.cs | 163 ++------- .../src/Expressions/VariableExpression.cs | 2 +- .../src/Primitives/CodeWriterDeclaration.cs | 10 +- .../src/Providers/ModelFactoryProvider.cs | 90 +---- .../src/Providers/ModelProvider.cs | 72 +--- .../src/Providers/TypeProvider.cs | 123 ++++++- .../src/Utilities/BackCompatHelper.cs | 329 ++++++++++++++++++ .../KeepCtorType.cs | 9 + .../PromoteCtorType.cs | 9 + .../NonAbstractPreservedType.cs | 6 + ...patibilityKeepsUnpublishedParameterName.cs | 18 + .../TestClient.cs | 10 + ...ityRestoredNameDoesNotSplitDeclarations.cs | 19 + .../TestClient.cs | 10 + ...patibilityRestoresPreviousParameterName.cs | 19 + .../TestClient.cs | 10 + ...atibilitySkipsReorderAcceptedInBaseline.cs | 14 + ...tibilitySkipsReorderAcceptedInBaseline.txt | 1 + .../SkipReorderType.cs | 7 + ...viderInheritsParameterReorderBackCompat.cs | 14 + .../CustomReorderType.cs | 7 + ...oviderReorderPreservesValueTypeDefaults.cs | 14 + .../CustomReorderValueTypeDefaultsType.cs | 13 + .../TestClient.cs | 15 + .../test/Providers/TypeProviderTests.cs | 298 ++++++++++++++++ .../test/TestHelpers/TestTypeProvider.cs | 7 +- .../backward-compatibility-extensibility.md | 170 +++++++++ 27 files changed, 1160 insertions(+), 299 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityKeepsModifierOnNonAbstractType/KeepCtorType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityPromotesPrivateProtectedToPublic/PromoteCtorType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildDeclarationModifiersPreservesNonAbstractFromLastContract/NonAbstractPreservedType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName/TestClient.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations/TestClient.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoresPreviousParameterName.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoresPreviousParameterName/TestClient.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline.txt create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline/SkipReorderType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderInheritsParameterReorderBackCompat.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderInheritsParameterReorderBackCompat/CustomReorderType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderReorderPreservesValueTypeDefaults.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderReorderPreservesValueTypeDefaults/CustomReorderValueTypeDefaultsType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/FindPreviousParameterNameLooksUpLastContract/TestClient.cs create mode 100644 packages/http-client-csharp/generator/docs/backward-compatibility-extensibility.md diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs index 947b717b5ec..958cd3adf75 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs @@ -1263,62 +1263,51 @@ protected override ScmMethodProvider[] BuildMethods() protected sealed override IReadOnlyList BuildMethodsForBackCompatibility(IEnumerable originalMethods) { - List materializedMethods = [.. originalMethods]; - if (LastContractView?.Methods == null || LastContractView.Methods.Count == 0) { - return materializedMethods; + return [.. originalMethods]; } - var currentMethodSignatures = BuildCurrentMethodSignatures(materializedMethods); - - ProcessBackCompatForParameterReordering(materializedMethods, currentMethodSignatures); - ProcessBackCompatForNewOptionalParameters(materializedMethods, currentMethodSignatures); + // Snapshot the current signatures before the base reorders any of them in place, so we + // can fix up convenience method bodies that call reordered protocol methods afterwards. + var originalSignatures = new Dictionary(ReferenceEqualityComparer.Instance); + foreach (var method in originalMethods) + { + originalSignatures.TryAdd(method, method.Signature); + } - return materializedMethods; - } + List result = [.. base.BuildMethodsForBackCompatibility(originalMethods)]; - private void ProcessBackCompatForParameterReordering( - IList materializedMethods, - Dictionary currentMethodSignatures) - { + // Determine which existing methods had their parameters reordered by the base and fix up + // convenience method bodies that call the reordered protocol methods. var updatedSignatureToOriginal = new Dictionary(MethodSignature.MethodSignatureComparer); var methodsWithReorderedParams = new List(); - - foreach (var previousMethod in LastContractView!.Methods) + foreach (var (method, originalSignature) in originalSignatures) { - if (!ShouldProcessMethodForBackCompat(previousMethod.Signature, currentMethodSignatures)) + if (method.Signature.Name.Equals(originalSignature.Name) + && !MethodSignatureHelper.HaveSameParametersInSameOrder(method.Signature, originalSignature)) { - continue; - } - - var methodToUpdate = FindMethodWithSameParametersButDifferentOrder( - previousMethod.Signature, - currentMethodSignatures); - - if (methodToUpdate != null && TryReorderCurrentMethodParameters( - methodToUpdate, - previousMethod.Signature, - updatedSignatureToOriginal)) - { - methodsWithReorderedParams.Add(methodToUpdate); - CodeModelGenerator.Instance.Emitter.Debug( - $"Reordered parameters of '{Name}.{methodToUpdate.Signature.Name}' to match last contract.", - BackCompatibilityChangeCategory.MethodParameterReordering); + updatedSignatureToOriginal.TryAdd(method.Signature, originalSignature); + methodsWithReorderedParams.Add(method); } } if (methodsWithReorderedParams.Count > 0) { - UpdateConvenienceMethodsForBackCompat(materializedMethods, methodsWithReorderedParams, updatedSignatureToOriginal); + UpdateConvenienceMethodsForBackCompat(result, methodsWithReorderedParams, updatedSignatureToOriginal); } + + // Add hidden overloads for methods that gained new optional non-body parameters. + ProcessBackCompatForNewOptionalParameters(result, BuildCurrentMethodSignatureMap(result)); + + return result; } - private Dictionary BuildCurrentMethodSignatures(IEnumerable originalMethods) + private Dictionary BuildCurrentMethodSignatureMap(IEnumerable methods) { var allMethods = CustomCodeView?.Methods != null - ? originalMethods.Concat(CustomCodeView.Methods) - : originalMethods; + ? methods.Concat(CustomCodeView.Methods) + : methods; var result = new Dictionary(MethodSignature.MethodSignatureComparer); foreach (var method in allMethods) @@ -1328,86 +1317,6 @@ private Dictionary BuildCurrentMethodSignatures return result; } - private static bool ShouldProcessMethodForBackCompat( - MethodSignature previousSignature, - Dictionary currentMethodSignatures) - { - if (currentMethodSignatures.ContainsKey(previousSignature)) - { - return false; - } - - var modifiers = previousSignature.Modifiers; - return modifiers.HasFlag(MethodSignatureModifiers.Public) || - modifiers.HasFlag(MethodSignatureModifiers.Protected); - } - - private static MethodProvider? FindMethodWithSameParametersButDifferentOrder( - MethodSignature previousSignature, - Dictionary currentMethodSignatures) - { - foreach (var kvp in currentMethodSignatures) - { - var currentSignature = kvp.Key; - if (currentSignature.Name.Equals(previousSignature.Name) - && currentSignature.ReturnType?.AreNamesEqual(previousSignature.ReturnType) == true - && MethodSignatureHelper.ContainsSameParameters(previousSignature, currentSignature)) - { - return kvp.Value; - } - } - - return null; - } - - private bool TryReorderCurrentMethodParameters( - MethodProvider methodToUpdate, - MethodSignature previousSignature, - Dictionary updatedSignatureToOriginal) - { - var currentSignature = methodToUpdate.Signature; - // Early exit: Check if parameters are already in the same order - if (MethodSignatureHelper.HaveSameParametersInSameOrder(currentSignature, previousSignature)) - { - return false; - } - - var parametersByName = currentSignature.Parameters.ToDictionary(p => p.Name.ToVariableName()); - var reorderedParameters = new List(currentSignature.Parameters.Count); - - foreach (var previousParam in previousSignature.Parameters) - { - if (parametersByName.TryGetValue(previousParam.Name, out var matchingParam)) - { - reorderedParameters.Add(matchingParam); - } - } - - if (reorderedParameters.Count != currentSignature.Parameters.Count) - { - return false; - } - - var updatedSignature = new MethodSignature( - currentSignature.Name, - currentSignature.Description, - currentSignature.Modifiers, - currentSignature.ReturnType, - currentSignature.ReturnDescription, - reorderedParameters, - currentSignature.Attributes, - currentSignature.GenericArguments, - currentSignature.GenericParameterConstraints, - currentSignature.ExplicitInterface, - currentSignature.NonDocumentComment); - updatedSignatureToOriginal.TryAdd(updatedSignature, currentSignature); - - UpdateXmlDocProviderForParamReorder(methodToUpdate.XmlDocs, updatedSignature); - methodToUpdate.Update(signature: updatedSignature, xmlDocProvider: methodToUpdate.XmlDocs); - - return true; - } - private ParameterProvider BuildClientEndpointParameter() { _inputEndpointParam = _inputClient.Parameters @@ -1773,28 +1682,6 @@ private static void ReorderMethodInvocationArguments( } } - private static void UpdateXmlDocProviderForParamReorder( - XmlDocProvider xmlDocs, - MethodSignature updatedSignature) - { - var paramDocsByName = xmlDocs.Parameters.ToDictionary(s => s.Parameter.Name); - var reorderedParamDocs = new List(updatedSignature.Parameters.Count); - - foreach (var param in updatedSignature.Parameters) - { - if (paramDocsByName.TryGetValue(param.Name, out var paramDoc)) - { - reorderedParamDocs.Add(paramDoc); - } - } - - if (reorderedParamDocs.Count == xmlDocs.Parameters.Count && - !reorderedParamDocs.SequenceEqual(xmlDocs.Parameters)) - { - xmlDocs.Update(parameters: reorderedParamDocs); - } - } - private void ProcessBackCompatForNewOptionalParameters( List methods, Dictionary currentMethodSignatures) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Expressions/VariableExpression.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Expressions/VariableExpression.cs index 409e5dc2b62..219b72470aa 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Expressions/VariableExpression.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Expressions/VariableExpression.cs @@ -34,7 +34,7 @@ public void Update(CSharpType? type = null, string? name = null) } if (name != null) { - Declaration = new CodeWriterDeclaration(name); + Declaration.Update(name: name); } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/CodeWriterDeclaration.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/CodeWriterDeclaration.cs index 3894d8f1c57..87035d3114c 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/CodeWriterDeclaration.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/CodeWriterDeclaration.cs @@ -33,7 +33,7 @@ public CodeWriterDeclaration(string name) RequestedName = name; } - public string RequestedName { get; } + public string RequestedName { get; private set; } internal string GetActualName(CodeWriter.CodeScope scope) { @@ -49,5 +49,13 @@ internal void SetActualName(string actualName, CodeWriter.CodeScope scope) { _actualNames[scope] = actualName; } + + internal void Update(string? name = null) + { + if (name != null) + { + RequestedName = name; + } + } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelFactoryProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelFactoryProvider.cs index c30d3a00bff..ae2495a64ba 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelFactoryProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelFactoryProvider.cs @@ -12,6 +12,7 @@ using Microsoft.TypeSpec.Generator.Primitives; using Microsoft.TypeSpec.Generator.Snippets; using Microsoft.TypeSpec.Generator.Statements; +using Microsoft.TypeSpec.Generator.Utilities; using static Microsoft.TypeSpec.Generator.Snippets.Snippet; namespace Microsoft.TypeSpec.Generator.Providers @@ -105,7 +106,7 @@ protected internal sealed override IReadOnlyList BuildMethodsFor // change between the previous and current contract is a parameter rename. This // avoids source-breaking changes for callers using named arguments (e.g. when a // property is renamed via @@clientName, spec rename, or naming-rule change). - PreservePreviousParameterNames(factoryMethods); + BackCompatHelper.RestorePreviousParameterNames(this, factoryMethods); HashSet currentMethodSignatures = new List([.. factoryMethods, .. CustomCodeView?.Methods ?? []]) .Select(m => m.Signature) @@ -120,14 +121,8 @@ protected internal sealed override IReadOnlyList BuildMethodsFor // If the removal of this factory method has already been accepted in the ApiCompat // baseline, honor that decision and do not resurrect a compatibility shim for it. - if (CodeModelGenerator.Instance.SourceInputModel?.ApiCompatBaseline.IsMemberSuppressed( - Type.FullyQualifiedName, - previousMethod.Signature.Name, - previousMethod.Signature.Parameters.Count) == true) + if (BackCompatHelper.IsMethodRemovalAcceptedInBaseline(this, previousMethod.Signature)) { - CodeModelGenerator.Instance.Emitter.Info( - $"Skipping back-compat shim for '{Type.FullyQualifiedName}.{previousMethod.Signature.Name}'; removal is accepted in the ApiCompat baseline.", - BackCompatibilityChangeCategory.BaselineAcceptedRemovalSkipped); continue; } @@ -212,85 +207,6 @@ protected internal sealed override IReadOnlyList BuildMethodsFor return [.. factoryMethods]; } - // Preserve original parameter names from the previous contract when a current factory - // method matches a previous one by name + parameter types/order but differs only in - // parameter names. The rename is applied in-place via ParameterProvider.Update which - // also updates the cached variable/argument expressions used by the method body and - // XML docs, so the body and docs serialize with the preserved names automatically. - private void PreservePreviousParameterNames(List currentFactoryMethods) - { - if (LastContractView?.Methods == null) - { - return; - } - - foreach (var previousMethod in LastContractView.Methods) - { - MethodProvider? matchingCurrent = null; - foreach (var current in currentFactoryMethods) - { - // MethodSignatureComparer matches on method name + parameter count + parameter - // types (positional); it does not consider parameter names. So a previous - // method whose only difference from a current method is parameter names will - // still match here. - if (MethodSignature.MethodSignatureComparer.Equals(current.Signature, previousMethod.Signature)) - { - matchingCurrent = current; - break; - } - } - - if (matchingCurrent is null) - { - continue; - } - - var previousParameters = previousMethod.Signature.Parameters; - var currentParameters = matchingCurrent.Signature.Parameters; - if (previousParameters.Count != currentParameters.Count) - { - continue; - } - - for (int i = 0; i < previousParameters.Count; i++) - { - var previousName = previousParameters[i].Name; - var currentParam = currentParameters[i]; - if (string.IsNullOrEmpty(previousName) || currentParam.Name == previousName) - { - continue; - } - - // Skip the rename when applying it would create a name collision with another - // current parameter (e.g. two same-typed parameters in swapped order between - // the previous and current contracts). A positional rename in that case would - // silently swap which parameter feeds which constructor field via name-based - // lookup in GetCtorArgs, producing semantically wrong (and source-breaking) code. - var previousNameToApply = previousName; - bool wouldCollide = false; - for (int j = 0; j < currentParameters.Count; j++) - { - if (j != i && string.Equals(currentParameters[j].Name, previousNameToApply, StringComparison.Ordinal)) - { - wouldCollide = true; - break; - } - } - - if (wouldCollide) - { - continue; - } - - CodeModelGenerator.Instance.Emitter.Debug( - $"Preserved parameter name '{previousName}' on '{Name}.{matchingCurrent.Signature.Name}' from last contract (instead of '{currentParam.Name}').", - BackCompatibilityChangeCategory.ParameterNamePreserved); - - currentParam.Update(name: previousName); - } - } - } - private bool TryBuildCompatibleMethodForPreviousContract( MethodProvider previousMethod, MethodSignature? currentMethodSignature, diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs index 9f28c6bc482..53142b6c74c 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs @@ -27,7 +27,7 @@ protected override FormattableString BuildDescription() { var description = DocHelpers.GetFormattableDescription(_inputModel.Summary, _inputModel.Doc) ?? $"The {Name}."; - if (IsAbstract) + if (DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract)) { _derivedModels = BuildDerivedModels(); var publicDerivedModels = _derivedModels.Where(m => m.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Public)).ToList(); @@ -66,7 +66,6 @@ protected override FormattableString BuildDescription() private ConstructorProvider? _fullConstructor; internal PropertyProvider? DiscriminatorProperty { get; private set; } private ValueExpression DiscriminatorLiteral => Literal(_inputModel.DiscriminatorValue ?? ""); - private bool IsAbstract => _inputModel.DiscriminatorProperty is not null && _inputModel.DiscriminatorValue is null && LastContractView?.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract) != false; public ModelProvider(InputModelType inputModel) : base(inputModel) { @@ -325,7 +324,7 @@ protected override TypeSignatureModifiers BuildDeclarationModifiers() declarationModifiers |= TypeSignatureModifiers.Internal; } - if (IsAbstract) + if (_inputModel.DiscriminatorProperty is not null && _inputModel.DiscriminatorValue is null) { declarationModifiers |= TypeSignatureModifiers.Abstract; } @@ -777,7 +776,7 @@ protected internal override ConstructorProvider[] BuildConstructors() private bool ComputeIsMultiLevelDiscriminator() { // Only applies to non-abstract models with a base model - if (IsAbstract || _inputModel.BaseModel == null) + if (DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract) || _inputModel.BaseModel == null) { return false; } @@ -848,71 +847,6 @@ private ConstructorProvider BuildFullConstructor() this); } - protected internal override IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable originalConstructors) - { - // Only handle the case of changing modifiers on abstract base types - if (!DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract)) - { - return [.. originalConstructors]; - } - - if (LastContractView?.Constructors == null || LastContractView.Constructors.Count == 0) - { - return [.. originalConstructors]; - } - - List constructors = [.. originalConstructors]; - - // Check if the last contract had a public constructor with matching parameters - foreach (var previousConstructor in LastContractView.Constructors) - { - if (!previousConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)) - { - continue; - } - - // Find a matching constructor in the current version by parameter signature - for (int i = 0; i < constructors.Count; i++) - { - var currentConstructor = constructors[i]; - - // Check if parameters match (same count and types) - if (ParametersMatch(currentConstructor.Signature.Parameters, previousConstructor.Signature.Parameters)) - { - // Change the modifier from private protected to public - if (currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Private) && - currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)) - { - currentConstructor.Signature.Update(modifiers: MethodSignatureModifiers.Public); - CodeModelGenerator.Instance.Emitter.Debug( - $"Promoted constructor '{Name}({string.Join(", ", currentConstructor.Signature.Parameters.Select(p => p.Type.ToString()))})' from 'private protected' to 'public' to match last contract.", - BackCompatibilityChangeCategory.ConstructorModifierPreserved); - } - } - } - } - - return [.. constructors]; - } - - private bool ParametersMatch(IReadOnlyList params1, IReadOnlyList params2) - { - if (params1.Count != params2.Count) - { - return false; - } - - for (int i = 0; i < params1.Count; i++) - { - if (!params1[i].Type.AreNamesEqual(params2[i].Type) || params1[i].Name != params2[i].Name) - { - return false; - } - } - - return true; - } - private IEnumerable GetAllBasePropertiesForConstructorInitialization(bool includeAllHierarchyDiscriminator = false) { var properties = new Stack>(); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index 3d71670d5a9..7e15a1f0cf7 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -215,6 +215,16 @@ private TypeSignatureModifiers BuildDeclarationModifiersInternal() severity: EmitterDiagnosticSeverity.Warning); } + // Back-compat: a type that the last contract published as non-abstract must not become + // abstract, which would be a source-breaking change for existing derived types and + // callers. Preserve the previously-published non-abstract shape. + if (modifiers.HasFlag(TypeSignatureModifiers.Abstract) && + LastContractView is { } lastContractView && + !lastContractView.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract)) + { + modifiers &= ~TypeSignatureModifiers.Abstract; + } + // we always add partial when possible if (!modifiers.HasFlag(TypeSignatureModifiers.Enum) && DeclaringTypeProvider is null) { @@ -813,11 +823,120 @@ private static IReadOnlyList VisitNewMembers( protected internal virtual IReadOnlyList? BuildEnumValuesForBackCompatibility(IReadOnlyList originalEnumValues) => null; + /// + /// Returns this type's methods with backward compatibility applied against + /// . The default implementation restores the previous + /// parameter order on a current method when it matches a last-contract method by name and + /// return type with the same parameter set but in a different order. Reordering is done in + /// place, so a method's body (which references its parameters by object) remains valid. + /// Override and call base to extend this behavior; override without calling + /// base to replace it. + /// protected internal virtual IReadOnlyList BuildMethodsForBackCompatibility(IEnumerable originalMethods) - => [.. originalMethods]; + { + var methods = new List(originalMethods); + + if (LastContractView?.Methods is not { Count: > 0 } previousMethods) + { + return methods; + } + + var currentMethodSignatures = BuildCurrentMethodSignatureMap(methods); + + foreach (var previousMethod in previousMethods) + { + if (!BackCompatHelper.ShouldApplyMethodBackCompatibility(previousMethod.Signature, currentMethodSignatures) + || BackCompatHelper.IsMethodRemovalAcceptedInBaseline(this, previousMethod.Signature)) + { + continue; + } + var methodToReorder = BackCompatHelper.FindMethodWithSameParametersDifferentOrder(previousMethod.Signature, currentMethodSignatures); + if (methodToReorder != null && BackCompatHelper.TryRestorePreviousParameterOrder(methodToReorder, previousMethod.Signature)) + { + CodeModelGenerator.Instance.Emitter.Debug( + $"Reordered parameters of '{Name}.{methodToReorder.Signature.Name}' to match last contract.", + BackCompatibilityChangeCategory.MethodParameterReordering); + } + } + + BackCompatHelper.RestorePreviousParameterNames(this, methods); + + return methods; + } + + /// + /// Builds a lookup of the type's current method signatures (including custom code methods) + /// used to match against last-contract methods. + /// + private Dictionary BuildCurrentMethodSignatureMap(IEnumerable methods) + { + var allMethods = CustomCodeView?.Methods != null + ? methods.Concat(CustomCodeView.Methods) + : methods; + + var result = new Dictionary(MethodSignature.MethodSignatureComparer); + foreach (var method in allMethods) + { + result.TryAdd(method.Signature, method); + } + return result; + } + + /// + /// Returns this type's constructors with backward compatibility applied against + /// . The default implementation preserves a previously-published + /// public constructor on an abstract base type: when the current generation would emit a + /// private protected constructor whose parameters match a public constructor in + /// the last contract, the modifier is promoted back to public. Override and call + /// base to extend this behavior. + /// protected internal virtual IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable originalConstructors) - => [.. originalConstructors]; + { + // Only handle the case of changing modifiers on abstract base types. + if (!DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract)) + { + return [.. originalConstructors]; + } + + if (LastContractView?.Constructors == null || LastContractView.Constructors.Count == 0) + { + return [.. originalConstructors]; + } + + List constructors = [.. originalConstructors]; + + // Check if the last contract had a public constructor with matching parameters + foreach (var previousConstructor in LastContractView.Constructors) + { + if (!previousConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)) + { + continue; + } + + // Find a matching constructor in the current version by parameter signature + for (int i = 0; i < constructors.Count; i++) + { + var currentConstructor = constructors[i]; + + // Check if parameters match (same count and types) + if (BackCompatHelper.ParametersMatch(currentConstructor.Signature.Parameters, previousConstructor.Signature.Parameters)) + { + // Change the modifier from private protected to public + if (currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Private) && + currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)) + { + currentConstructor.Signature.Update(modifiers: MethodSignatureModifiers.Public); + CodeModelGenerator.Instance.Emitter.Debug( + $"Promoted constructor '{Name}({string.Join(", ", currentConstructor.Signature.Parameters.Select(p => p.Type.ToString()))})' from 'private protected' to 'public' to match last contract.", + BackCompatibilityChangeCategory.ConstructorModifierPreserved); + } + } + } + } + + return [.. constructors]; + } private IReadOnlyList? _enumValues; diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs new file mode 100644 index 00000000000..05cd639b94c --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs @@ -0,0 +1,329 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.TypeSpec.Generator.EmitterRpc; +using Microsoft.TypeSpec.Generator.Input.Extensions; +using Microsoft.TypeSpec.Generator.Primitives; +using Microsoft.TypeSpec.Generator.Providers; +using Microsoft.TypeSpec.Generator.Statements; + +namespace Microsoft.TypeSpec.Generator.Utilities +{ + /// + /// Shared helpers for applying backward compatibility to generated methods by comparing the + /// current generation against a type's last contract. + /// + internal static class BackCompatHelper + { + /// + /// Returns true when a last-contract method should be considered for back compatibility: + /// it is public or protected and has no exact match among the current signatures. + /// + public static bool ShouldApplyMethodBackCompatibility( + MethodSignature previousSignature, + Dictionary currentMethodSignatures) + { + if (currentMethodSignatures.ContainsKey(previousSignature)) + { + return false; + } + + var modifiers = previousSignature.Modifiers; + return modifiers.HasFlag(MethodSignatureModifiers.Public) || + modifiers.HasFlag(MethodSignatureModifiers.Protected); + } + + /// + /// Returns true when the removal of a previously-published method — identified by the enclosing + /// type's fully-qualified name, the method name, and the parameter count — has been accepted in + /// the ApiCompat baseline, in which case back compatibility must not resurrect or restore it. + /// Emits an informational log entry when a suppression is honored. + /// + public static bool IsMethodRemovalAcceptedInBaseline(TypeProvider enclosingType, MethodSignature previousSignature) + { + if (CodeModelGenerator.Instance.SourceInputModel?.ApiCompatBaseline.IsMemberSuppressed( + enclosingType.Type.FullyQualifiedName, + previousSignature.Name, + previousSignature.Parameters.Count) != true) + { + return false; + } + + CodeModelGenerator.Instance.Emitter.Info( + $"Skipping back-compat for '{enclosingType.Type.FullyQualifiedName}.{previousSignature.Name}'; removal is accepted in the ApiCompat baseline.", + BackCompatibilityChangeCategory.BaselineAcceptedRemovalSkipped); + return true; + } + + /// + /// Finds the current method that has the same parameter set as + /// (matched by name and return type) but in a different order, or null when there is none. + /// + public static MethodProvider? FindMethodWithSameParametersDifferentOrder( + MethodSignature previousSignature, + Dictionary currentMethodSignatures) + { + foreach (var kvp in currentMethodSignatures) + { + var currentSignature = kvp.Key; + if (currentSignature.Name.Equals(previousSignature.Name) + && currentSignature.ReturnType?.AreNamesEqual(previousSignature.ReturnType) == true + && MethodSignatureHelper.ContainsSameParameters(previousSignature, currentSignature)) + { + return kvp.Value; + } + } + + return null; + } + + /// + /// Returns true when two parameter lists match positionally by type name and parameter name + /// (and have the same count). Used to align a current member with its last-contract counterpart. + /// + public static bool ParametersMatch(IReadOnlyList params1, IReadOnlyList params2) + { + if (params1.Count != params2.Count) + { + return false; + } + + for (int i = 0; i < params1.Count; i++) + { + if (!params1[i].Type.AreNamesEqual(params2[i].Type) || params1[i].Name != params2[i].Name) + { + return false; + } + } + + return true; + } + + /// + /// Returns the previously-published name of a parameter whose original (spec) name is + /// , looked up in . When + /// is supplied, the search is scoped to last-contract methods + /// whose name matches it (allowing for a sync/async pair) so a parameter name shared across + /// methods cannot cross-match. Returns null when no match exists. + /// + public static string? FindPreviousParameterName( + TypeProvider? lastContractView, + string originalName, + string? methodName = null) + { + var lastContractMethods = lastContractView?.Methods; + if (lastContractMethods is null || lastContractMethods.Count == 0) + { + return null; + } + + IEnumerable scopedMethods = lastContractMethods; + if (methodName != null) + { + scopedMethods = lastContractMethods.Where(m => + string.Equals(m.Signature.Name, methodName, StringComparison.OrdinalIgnoreCase) || + string.Equals(m.Signature.Name, methodName + "Async", StringComparison.OrdinalIgnoreCase)); + } + + return scopedMethods + .SelectMany(method => method.Signature.Parameters) + .FirstOrDefault(p => string.Equals(p.Name, originalName, StringComparison.OrdinalIgnoreCase)) + ?.Name; + } + + public static void RestorePreviousParameterNames( + TypeProvider enclosingType, + IReadOnlyList currentMethods) + { + var lastContractView = enclosingType.LastContractView; + if (lastContractView?.Methods is not { Count: > 0 } previousMethods) + { + return; + } + + foreach (var method in currentMethods) + { + var modifiers = method.Signature.Modifiers; + if (!modifiers.HasFlag(MethodSignatureModifiers.Public) && !modifiers.HasFlag(MethodSignatureModifiers.Protected)) + { + continue; + } + + var currentParameters = method.Signature.Parameters; + MethodProvider? matchingPrevious = null; + bool matchingPreviousResolved = false; + + for (int i = 0; i < currentParameters.Count; i++) + { + var parameter = currentParameters[i]; + string? preservedName; + + var inputParameter = parameter.InputParameter; + if (inputParameter is not null) + { + if (!string.Equals(parameter.Name, inputParameter.Name, StringComparison.Ordinal)) + { + continue; + } + + var originalName = inputParameter.OriginalName; + if (string.IsNullOrEmpty(originalName)) + { + continue; + } + + preservedName = FindPreviousParameterName(lastContractView, originalName, method.Signature.Name); + } + else + { + // Positional fallback for synthesized parameters (e.g. model factory methods). + if (!matchingPreviousResolved) + { + matchingPrevious = FindMethodWithSameSignatureIgnoringNames(previousMethods, method.Signature); + matchingPreviousResolved = true; + } + + var previousParameters = matchingPrevious?.Signature.Parameters; + preservedName = previousParameters is not null && previousParameters.Count == currentParameters.Count + ? previousParameters[i].Name + : null; + } + + // A casing-only difference is still a source-breaking rename for named arguments, + // so compare case-sensitively and restore the previously-published spelling. + if (string.IsNullOrEmpty(preservedName) || string.Equals(parameter.Name, preservedName, StringComparison.Ordinal)) + { + continue; + } + + // Skip the rename when applying it would collide with another current parameter's + // name (e.g. two same-typed parameters whose order changed between the previous and + // current contracts). A rename in that case would produce a duplicate parameter name + // and, for name-based argument lookups, silently wire the wrong value. + bool wouldCollide = false; + for (int j = 0; j < currentParameters.Count; j++) + { + if (j != i && string.Equals(currentParameters[j].Name, preservedName, StringComparison.Ordinal)) + { + wouldCollide = true; + break; + } + } + + if (wouldCollide) + { + continue; + } + + CodeModelGenerator.Instance.Emitter.Debug( + $"Preserved parameter name '{preservedName}' on '{enclosingType.Name}.{method.Signature.Name}' from last contract (instead of '{parameter.Name}').", + BackCompatibilityChangeCategory.ParameterNamePreserved); + parameter.Update(name: preservedName); + } + } + } + + /// + /// Finds the last-contract method whose signature matches on + /// method name plus parameter count and types (ignoring parameter names), or null when none. + /// + private static MethodProvider? FindMethodWithSameSignatureIgnoringNames( + IReadOnlyList previousMethods, + MethodSignature currentSignature) + { + foreach (var previousMethod in previousMethods) + { + if (MethodSignature.MethodSignatureComparer.Equals(currentSignature, previousMethod.Signature)) + { + return previousMethod; + } + } + + return null; + } + + /// + /// Restores the previous parameter order on in place + /// (updating XML docs). Returns true when a change was made. + /// + public static bool TryRestorePreviousParameterOrder( + MethodProvider methodToReorder, + MethodSignature previousSignature) + { + var currentSignature = methodToReorder.Signature; + if (MethodSignatureHelper.HaveSameParametersInSameOrder(currentSignature, previousSignature)) + { + return false; + } + + var parametersByName = currentSignature.Parameters.ToDictionary(p => p.Name.ToVariableName()); + var reorderedParameters = new List(currentSignature.Parameters.Count); + + foreach (var previousParam in previousSignature.Parameters) + { + if (parametersByName.TryGetValue(previousParam.Name, out var matchingParam)) + { + reorderedParameters.Add(matchingParam); + } + } + + if (reorderedParameters.Count != currentSignature.Parameters.Count) + { + return false; + } + + foreach (var previousParam in previousSignature.Parameters) + { + if (parametersByName.TryGetValue(previousParam.Name, out var matchingParam) + && matchingParam.DefaultValue is not null + && previousParam.DefaultValue is not null) + { + matchingParam.Update(defaultValue: previousParam.DefaultValue); + } + } + + var updatedSignature = new MethodSignature( + currentSignature.Name, + currentSignature.Description, + currentSignature.Modifiers, + currentSignature.ReturnType, + currentSignature.ReturnDescription, + reorderedParameters, + currentSignature.Attributes, + currentSignature.GenericArguments, + currentSignature.GenericParameterConstraints, + currentSignature.ExplicitInterface, + currentSignature.NonDocumentComment); + + UpdateXmlDocProviderForParamReorder(methodToReorder.XmlDocs, updatedSignature); + methodToReorder.Update(signature: updatedSignature, xmlDocProvider: methodToReorder.XmlDocs); + + return true; + } + + private static void UpdateXmlDocProviderForParamReorder( + XmlDocProvider xmlDocs, + MethodSignature updatedSignature) + { + var paramDocsByName = xmlDocs.Parameters.ToDictionary(s => s.Parameter.Name); + var reorderedParamDocs = new List(updatedSignature.Parameters.Count); + + foreach (var param in updatedSignature.Parameters) + { + if (paramDocsByName.TryGetValue(param.Name, out var paramDoc)) + { + reorderedParamDocs.Add(paramDoc); + } + } + + if (reorderedParamDocs.Count == xmlDocs.Parameters.Count && + !reorderedParamDocs.SequenceEqual(xmlDocs.Parameters)) + { + xmlDocs.Update(parameters: reorderedParamDocs); + } + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityKeepsModifierOnNonAbstractType/KeepCtorType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityKeepsModifierOnNonAbstractType/KeepCtorType.cs new file mode 100644 index 00000000000..9b45621f6df --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityKeepsModifierOnNonAbstractType/KeepCtorType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public class KeepCtorType + { + public KeepCtorType(string baseProp) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityPromotesPrivateProtectedToPublic/PromoteCtorType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityPromotesPrivateProtectedToPublic/PromoteCtorType.cs new file mode 100644 index 00000000000..180db065877 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityPromotesPrivateProtectedToPublic/PromoteCtorType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public abstract class PromoteCtorType + { + public PromoteCtorType(string baseProp) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildDeclarationModifiersPreservesNonAbstractFromLastContract/NonAbstractPreservedType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildDeclarationModifiersPreservesNonAbstractFromLastContract/NonAbstractPreservedType.cs new file mode 100644 index 00000000000..688e1888072 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildDeclarationModifiersPreservesNonAbstractFromLastContract/NonAbstractPreservedType.cs @@ -0,0 +1,6 @@ +namespace Test +{ + public class NonAbstractPreservedType + { + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName.cs new file mode 100644 index 00000000000..a1fadf5dd08 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName.cs @@ -0,0 +1,18 @@ +// + +#nullable disable + +using Sample; + +namespace Test +{ + public partial class TestClient + { + public string Foo(string brandNewParam) + { + global::Sample.Argument.AssertNotNullOrEmpty(brandNewParam, nameof(brandNewParam)); + + return brandNewParam; + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName/TestClient.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName/TestClient.cs new file mode 100644 index 00000000000..afe96ffaaf7 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName/TestClient.cs @@ -0,0 +1,10 @@ +namespace Test +{ + /// + /// Previously-published contract that does not contain the "brandNewParam" parameter. + /// + public class TestClient + { + public string Foo(string oldParam) { return null; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations.cs new file mode 100644 index 00000000000..c611ba3d45d --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations.cs @@ -0,0 +1,19 @@ +// + +#nullable disable + +using Sample; + +namespace Test +{ + public partial class TestClient + { + public string Foo(string oldParam) + { + global::Sample.Argument.AssertNotNullOrEmpty(oldParam, nameof(oldParam)); + + this.Validate(oldParam); + return oldParam; + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations/TestClient.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations/TestClient.cs new file mode 100644 index 00000000000..c413df7e7f8 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations/TestClient.cs @@ -0,0 +1,10 @@ +namespace Test +{ + /// + /// Previously-published contract: the parameter was published as "oldParam". + /// + public class TestClient + { + public string Foo(string oldParam) { return null; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoresPreviousParameterName.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoresPreviousParameterName.cs new file mode 100644 index 00000000000..c611ba3d45d --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoresPreviousParameterName.cs @@ -0,0 +1,19 @@ +// + +#nullable disable + +using Sample; + +namespace Test +{ + public partial class TestClient + { + public string Foo(string oldParam) + { + global::Sample.Argument.AssertNotNullOrEmpty(oldParam, nameof(oldParam)); + + this.Validate(oldParam); + return oldParam; + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoresPreviousParameterName/TestClient.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoresPreviousParameterName/TestClient.cs new file mode 100644 index 00000000000..c413df7e7f8 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityRestoresPreviousParameterName/TestClient.cs @@ -0,0 +1,10 @@ +namespace Test +{ + /// + /// Previously-published contract: the parameter was published as "oldParam". + /// + public class TestClient + { + public string Foo(string oldParam) { return null; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline.cs new file mode 100644 index 00000000000..c0647c0cd30 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline.cs @@ -0,0 +1,14 @@ +// + +#nullable disable + +namespace Test +{ + public partial class SkipReorderType + { + public string Foo(int second, string first) + { + return null; + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline.txt b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline.txt new file mode 100644 index 00000000000..7e78b1188d0 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline.txt @@ -0,0 +1 @@ +MembersMustExist : Member 'public System.String Test.SkipReorderType.Foo(System.String, System.Int32)' does not exist in the implementation but it does exist in the contract. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline/SkipReorderType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline/SkipReorderType.cs new file mode 100644 index 00000000000..0374f273cfb --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline/SkipReorderType.cs @@ -0,0 +1,7 @@ +namespace Test +{ + public class SkipReorderType + { + public string Foo(string first, int second) => null; + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderInheritsParameterReorderBackCompat.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderInheritsParameterReorderBackCompat.cs new file mode 100644 index 00000000000..a4c2bd4cf62 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderInheritsParameterReorderBackCompat.cs @@ -0,0 +1,14 @@ +// + +#nullable disable + +namespace Test +{ + public partial class CustomReorderType + { + public string Foo(string first, int second) + { + return null; + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderInheritsParameterReorderBackCompat/CustomReorderType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderInheritsParameterReorderBackCompat/CustomReorderType.cs new file mode 100644 index 00000000000..0c0a8860a69 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderInheritsParameterReorderBackCompat/CustomReorderType.cs @@ -0,0 +1,7 @@ +namespace Test +{ + public class CustomReorderType + { + public string Foo(string first, int second) => null; + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderReorderPreservesValueTypeDefaults.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderReorderPreservesValueTypeDefaults.cs new file mode 100644 index 00000000000..f41eb686ec3 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderReorderPreservesValueTypeDefaults.cs @@ -0,0 +1,14 @@ +// + +#nullable disable + +namespace Test +{ + public partial class CustomReorderValueTypeDefaultsType + { + public string Foo(int count = 0, bool flag = false) + { + return null; + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderReorderPreservesValueTypeDefaults/CustomReorderValueTypeDefaultsType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderReorderPreservesValueTypeDefaults/CustomReorderValueTypeDefaultsType.cs new file mode 100644 index 00000000000..d45afd10149 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/CustomTypeProviderReorderPreservesValueTypeDefaults/CustomReorderValueTypeDefaultsType.cs @@ -0,0 +1,13 @@ +namespace Test +{ + /// + /// Previous contract whose Foo method declares value-type optional parameters with literal + /// defaults (count = 0, flag = false) in the order (count, flag). The current generation + /// declares them in a different order with `default`-keyword defaults; restoring the order must + /// also preserve the previously published literal default representation. + /// + public class CustomReorderValueTypeDefaultsType + { + public string Foo(int count = 0, bool flag = false) => null; + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/FindPreviousParameterNameLooksUpLastContract/TestClient.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/FindPreviousParameterNameLooksUpLastContract/TestClient.cs new file mode 100644 index 00000000000..2c11ceaee1e --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/FindPreviousParameterNameLooksUpLastContract/TestClient.cs @@ -0,0 +1,15 @@ +namespace Test +{ + /// + /// Represents the previously-published contract for a type whose parameters may have since + /// been renamed by the generator. + /// + public class TestClient + { + public void Foo(string oldParam) { } + + public void BarAsync(string oldAsyncParam) { } + + public void Other(string otherParam) { } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index 9ec0b21c32d..9c7e339eb7b 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -12,6 +12,7 @@ using Microsoft.TypeSpec.Generator.Snippets; using Microsoft.TypeSpec.Generator.Statements; using Microsoft.TypeSpec.Generator.Tests.Common; +using Microsoft.TypeSpec.Generator.Utilities; using NUnit.Framework; namespace Microsoft.TypeSpec.Generator.Tests.Providers @@ -57,6 +58,303 @@ public async Task TestLoadLastContractView() Assert.AreEqual("p1", signature.Parameters[0].Name); } + // Validates that a custom TypeProvider inherits the parameter-reordering back-compat behavior + // from the base TypeProvider. + [Test] + public async Task CustomTypeProviderInheritsParameterReorderBackCompat() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // Current generation declares Foo(second, first) — the reverse of the last contract's + // Foo(first, second). + var first = new ParameterProvider("first", $"", new CSharpType(typeof(string))); + var second = new ParameterProvider("second", $"", new CSharpType(typeof(int))); + var currentFoo = new MethodProvider( + new MethodSignature("Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [second, first]), + Snippet.Return(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "CustomReorderType", ns: "Test", methods: [currentFoo]); + + // The custom type does not override BuildMethodsForBackCompatibility; the base default + // restores the previous parameter order in place. + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // Validates that when restoring a previous parameter order, the previously published default + // value representation is preserved (e.g. a value-type parameter keeps its literal `= 0` + // rather than flipping to `= default`). + [Test] + public async Task CustomTypeProviderReorderPreservesValueTypeDefaults() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // Current generation declares Foo(flag, count) with `default`-keyword defaults — the + // reverse of the last contract's Foo(int count = 0, bool flag = false). + var flag = new ParameterProvider("flag", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default); + var count = new ParameterProvider("count", $"", new CSharpType(typeof(int)), defaultValue: Snippet.Default); + var currentFoo = new MethodProvider( + new MethodSignature("Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [flag, count]), + Snippet.Return(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "CustomReorderValueTypeDefaultsType", ns: "Test", methods: [currentFoo]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // Validates that the base BuildMethodsForBackCompatibility default does NOT restore the + // previous parameter order when that removal has been accepted in the ApiCompat baseline. + [Test] + public async Task BuildMethodsForBackCompatibilitySkipsReorderAcceptedInBaseline() + { + var baseline = Helpers.GetApiCompatBaselineFromFile(); + + await MockHelpers.LoadMockGeneratorAsync( + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync(), + apiCompatBaseline: baseline); + + // Current generation declares Foo(second, first) — the reverse of the last contract's + // Foo(first, second) — but the reorder is an accepted removal in the ApiCompat baseline, + // so the base default must leave the current order untouched. + var first = new ParameterProvider("first", $"", new CSharpType(typeof(string))); + var second = new ParameterProvider("second", $"", new CSharpType(typeof(int))); + var currentFoo = new MethodProvider( + new MethodSignature("Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [second, first]), + Snippet.Return(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "SkipReorderType", ns: "Test", methods: [currentFoo]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // Validates that the base TypeProvider generalizes the non-abstract base model back-compat to + // any TypeProvider: a type the current generation would declare abstract is kept non-abstract + // when the last contract published it as a non-abstract class. + [Test] + public async Task BuildDeclarationModifiersPreservesNonAbstractFromLastContract() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var typeProvider = new TestTypeProvider( + name: "NonAbstractPreservedType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Abstract | TypeSignatureModifiers.Class); + + Assert.IsFalse( + typeProvider.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract), + "Expected the abstract modifier to be removed to match the non-abstract last contract."); + } + + // Validates the negative case: without a last contract, a type the current generation declares + // abstract stays abstract (the preservation only applies against a non-abstract last contract). + [Test] + public void BuildDeclarationModifiersKeepsAbstractWithoutLastContract() + { + var typeProvider = new TestTypeProvider( + name: "AbstractNoContractType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Abstract | TypeSignatureModifiers.Class); + + Assert.IsTrue(typeProvider.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract)); + } + + // Validates that the base TypeProvider generalizes the model-constructor back-compat to any + // abstract TypeProvider: a private-protected constructor is promoted to public when the last + // contract published a matching public constructor. + [Test] + public async Task BuildConstructorsForBackCompatibilityPromotesPrivateProtectedToPublic() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var baseProp = new ParameterProvider("baseProp", $"", new CSharpType(typeof(string))); + var currentConstructor = new ConstructorProvider( + new ConstructorSignature( + new CSharpType(typeof(object)), + $"", + MethodSignatureModifiers.Private | MethodSignatureModifiers.Protected, + [baseProp]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider( + name: "PromoteCtorType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Abstract | TypeSignatureModifiers.Class, + constructors: [currentConstructor]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var promoted = typeProvider.Constructors.Single(); + Assert.IsTrue(promoted.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)); + Assert.IsFalse(promoted.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Private)); + Assert.IsFalse(promoted.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)); + } + + // Validates the negative case: a non-abstract type does not get its private-protected + // constructor promoted, even with a matching public constructor in the last contract. + [Test] + public async Task BuildConstructorsForBackCompatibilityKeepsModifierOnNonAbstractType() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var baseProp = new ParameterProvider("baseProp", $"", new CSharpType(typeof(string))); + var currentConstructor = new ConstructorProvider( + new ConstructorSignature( + new CSharpType(typeof(object)), + $"", + MethodSignatureModifiers.Private | MethodSignatureModifiers.Protected, + [baseProp]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider( + name: "KeepCtorType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: [currentConstructor]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var constructor = typeProvider.Constructors.Single(); + Assert.IsTrue(constructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Private)); + Assert.IsTrue(constructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)); + Assert.IsFalse(constructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)); + } + + // Validates the shared lookup that any provider can use to restore a previously-published + // parameter name from its last contract. + [Test] + public async Task FindPreviousParameterNameLooksUpLastContract() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + var lastContractView = new TestTypeProvider(name: "TestClient").LastContractView; + + // The last contract published Foo(string oldParam); scoped to that method it is found. + Assert.AreEqual("oldParam", BackCompatHelper.FindPreviousParameterName(lastContractView, "oldParam", "Foo")); + + // The exact casing from the contract is returned even when the lookup name differs only in casing. + Assert.AreEqual("oldParam", BackCompatHelper.FindPreviousParameterName(lastContractView, "oldparam", "Foo")); + + // The lookup is scoped: "oldParam" is not published on Other. + Assert.IsNull(BackCompatHelper.FindPreviousParameterName(lastContractView, "oldParam", "Other")); + + // An unscoped lookup finds the parameter on any last-contract method. + Assert.AreEqual("oldParam", BackCompatHelper.FindPreviousParameterName(lastContractView, "oldParam")); + + // A parameter that does not exist in the last contract returns null. + Assert.IsNull(BackCompatHelper.FindPreviousParameterName(lastContractView, "missing", "Foo")); + + // A sync method name matches its async counterpart (BarAsync) in the contract. + Assert.AreEqual("oldAsyncParam", BackCompatHelper.FindPreviousParameterName(lastContractView, "oldAsyncParam", "Bar")); + } + + // Validates that the lookup returns null when there is no last contract. + [Test] + public void FindPreviousParameterNameReturnsNullWithoutLastContract() + { + var typeProvider = new TestTypeProvider(name: "TestClient"); + Assert.IsNull(typeProvider.LastContractView); + Assert.IsNull(BackCompatHelper.FindPreviousParameterName(typeProvider.LastContractView, "oldParam", "Foo")); + } + + // Validates that the base BuildMethodsForBackCompatibility default automatically restores a + // previously-published parameter name on any derived type — keyed on the parameter's spec + // original name — and that the rename propagates into the already-built method body. + [Test] + public async Task BuildMethodsForBackCompatibilityRestoresPreviousParameterName() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // A spec parameter "oldParam" that the generator renamed to "newParam". + var inputParameter = InputFactory.QueryParameter("oldParam", InputPrimitiveType.String, isRequired: true); + inputParameter.Update(name: "newParam"); + + var parameter = new ParameterProvider(inputParameter); + var fooMethod = new MethodProvider( + new MethodSignature("Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [parameter]), + new MethodBodyStatement[] + { + // Passing the parameter as an argument to another method exercises propagation + // of the restored name into invocation arguments, not just the return. + Snippet.This.Invoke("Validate", parameter).Terminate(), + Snippet.Return(parameter), + }, + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "TestClient", methods: [fooMethod]); + + // The default has no override here; the base restores the previously-published name and + // the rename propagates into the already-built body. + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // Validates that a parameter whose spec name is not in the last contract keeps its current name. + [Test] + public async Task BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // "brandNewParam" has no counterpart in the last contract (TestClient.Foo(oldParam)). + var inputParameter = InputFactory.QueryParameter("brandNewParam", InputPrimitiveType.String, isRequired: true); + var parameter = new ParameterProvider(inputParameter); + var fooMethod = new MethodProvider( + new MethodSignature("Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [parameter]), + Snippet.Return(parameter), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "TestClient", methods: [fooMethod]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // When the restored name is used both as an argument (AsArgument -> _asArgument) and + // as a variable (-> _asVariable), materializing both cached expressions before the rename, the + // rename must keep them sharing one declaration. Otherwise the writer uniquifies the two + // declarations of the same name into "oldParam0"/"oldParam1", producing non-compiling code. + [Test] + public async Task BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var inputParameter = InputFactory.QueryParameter("oldParam", InputPrimitiveType.String, isRequired: true); + inputParameter.Update(name: "newParam"); + + var parameter = new ParameterProvider(inputParameter); + var fooMethod = new MethodProvider( + new MethodSignature("Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [parameter]), + new MethodBodyStatement[] + { + Snippet.This.Invoke("Validate", parameter.AsArgument()).Terminate(), + Snippet.Return(parameter), + }, + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "TestClient", methods: [fooMethod]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + [Test] public async Task LastContractViewLoadedForRenamedType() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/TestHelpers/TestTypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/TestHelpers/TestTypeProvider.cs index e142850b002..4b339e7bff4 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/TestHelpers/TestTypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/TestHelpers/TestTypeProvider.cs @@ -13,6 +13,7 @@ internal class TestTypeProvider : TypeProvider private readonly TypeSignatureModifiers? _declarationModifiers; private readonly MethodProvider[] _methods; private readonly PropertyProvider[] _properties; + private readonly ConstructorProvider[] _constructors; private readonly string _name; private readonly string _namespace; protected override string BuildRelativeFilePath() => $"{Name}.cs"; @@ -24,6 +25,8 @@ internal class TestTypeProvider : TypeProvider protected internal override PropertyProvider[] BuildProperties() => _properties; protected internal override MethodProvider[] BuildMethods() => _methods; + + protected internal override ConstructorProvider[] BuildConstructors() => _constructors; protected override TypeProvider[] BuildNestedTypes() => NestedTypesInternal ?? base.BuildNestedTypes(); public static readonly TypeProvider Empty = new TestTypeProvider(); @@ -33,11 +36,13 @@ internal TestTypeProvider( TypeSignatureModifiers? declarationModifiers = null, IEnumerable? methods = null, IEnumerable? properties = null, - string? ns = null) + string? ns = null, + IEnumerable? constructors = null) { _declarationModifiers = declarationModifiers; _methods = methods?.ToArray() ?? []; _properties = properties?.ToArray() ?? []; + _constructors = constructors?.ToArray() ?? []; _name = name ?? "TestName"; _namespace = ns ?? "Test"; } diff --git a/packages/http-client-csharp/generator/docs/backward-compatibility-extensibility.md b/packages/http-client-csharp/generator/docs/backward-compatibility-extensibility.md new file mode 100644 index 00000000000..2a39dcca305 --- /dev/null +++ b/packages/http-client-csharp/generator/docs/backward-compatibility-extensibility.md @@ -0,0 +1,170 @@ +# Backward Compatibility Extensibility + +## Problem + +The generator preserves backward compatibility by diffing the current generation against +the last released contract and adjusting members so existing code keeps compiling. Today +this logic is private to each built-in provider (`ClientProvider`, `ModelProvider`, +`ModelFactoryProvider`, `ApiVersionEnumProvider`), so downstream providers that derive +directly from `TypeProvider` — such as the management generator's `ResourceClientProvider` — +cannot reuse it. + +## Solution + +Move the generic algorithms into `TypeProvider` as the **default implementations** of its +back-compat hooks. Any provider then consumes them by inheriting, and customizes by +overriding and calling `base`. On by default; opt out by overriding to return the input +unchanged. + +## Two API families (split by when they run) + +| Family | Shape | Caller / timing | +|---|---|---| +| **`Build…ForBackCompatibility`** | `(IReadOnlyList) → IReadOnlyList` | Framework calls, **after** visitors | +| **`Get…ForBackCompatibility`** | `(member) → value` | **You** call, **while building** a member (before serialization) | + +```csharp +public partial class TypeProvider +{ + public TypeProvider? LastContractView { get; } // already public + + // FAMILY 1 — framework calls after visitors; default applies the standard algorithm. + // Override + call base to extend; override without base to replace. + // Members produced here are NOT re-visited by visitors. + protected internal virtual IReadOnlyList BuildMethodsForBackCompatibility(IEnumerable methods); + protected internal virtual IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable constructors); + protected internal virtual IReadOnlyList? BuildEnumValuesForBackCompatibility(IReadOnlyList enumValues); + + // Materialization seam used by the FAMILY 1 method default. The base builds the synthesized + // overload as a plain MethodProvider whose body delegates to the current method, then hands + // it here. Default returns it unchanged; override to return a provider-specific subtype or + // to apply adjustments (e.g. analyzer suppressions). + protected virtual MethodProvider BuildBackCompatibilityOverload(MethodProvider overload); + + // FAMILY 2 — you call while building; returns the value to use (non-mutating), + // scoped to this type's LastContractView. + protected virtual CSharpType GetPropertyTypeForBackCompatibility(PropertyProvider property); +} +``` + +> The parameter/signature comparison primitives used internally by the default algorithm +> (`MethodSignatureHelper`) are shared source compiled into each generator assembly and remain +> `internal`; downstream providers do not need them because the materialization seam hands them a +> fully-built `MethodProvider`. + +**Defaults:** +- `BuildMethodsForBackCompatibility` — restores previous parameter order; adds a hidden + `[EditorBrowsable(Never)]` overload (delegating to the current method) when new optional + non-body parameters were added. +- `BuildConstructorsForBackCompatibility` — keeps an abstract base type's constructor public + when the last contract exposed a matching public one. +- `BuildEnumValuesForBackCompatibility` — preserves enum members removed since the last contract. +- `GetPropertyTypeForBackCompatibility` — returns the previously published property type when + it changed, else the current type. + +## Pipeline + +```mermaid +flowchart TD + A["1 · EnsureBuilt()
build members + bodies"] --> B["2 · visitors run
(transform members)"] + B --> C["3 · ProcessTypeForBackCompatibility()"] + C --> D["4 · writers emit"] + + A -. "you call" .-> G["Get…ForBackCompatibility
(property type, …)"] + C -. "framework calls" .-> F["Build…ForBackCompatibility
(methods, ctors, enum values)"] + + style G fill:#e8f0fe,stroke:#4285f4 + style F fill:#e6f4ea,stroke:#34a853 +``` + +- **`Get…` is build-time (step 1)** because its value feeds the body/serialization built in + the same step. Running it later would desync a property's declared type from its already- + generated serialization. +- **`Build…` is post-visitor (step 3)** because its transforms are safe to apply last + (reorder is cosmetic, new overloads get a fresh body, modifier/enum changes are + declaration-only) and shouldn't be rewritten by visitors. Members it produces are not + re-visited. + +A `Build…` hook only acts when `LastContractView` has those members and a real diff exists, +so enabling defaults everywhere is safe. + +## Built-in providers after consolidation + +| Provider | Change | +|---|---| +| `ClientProvider` | Inherits the base `BuildMethodsForBackCompatibility` algorithm; keeps a slim override only for SCM-specific concerns (re-typing the synthesized overload as `ScmMethodProvider` + AZC0002 via `BuildBackCompatibilityOverload`, and fixing convenience-method bodies that call reordered protocol methods). | +| `ModelProvider` | Ctor logic → base hook; property-type logic → `GetPropertyTypeForBackCompatibility` (called from `BuildProperties`). | +| `ModelFactoryProvider` | **Keeps its override** (multi-strategy factory logic differs). | +| `ApiVersionEnumProvider` | Enum logic → base hook; **inherits**. | + +## Downstream usage (management generator) + +`ResourceClientProvider` derives from `TypeProvider`, so it inherits all the defaults. + +### 1. Standard method back-compat — free, no code + +Resource operation methods automatically get parameter-order restoration and hidden +overloads for newly added optional parameters: + +```csharp +public partial class ResourceClientProvider : TypeProvider +{ + // No override needed. +} +``` + +### 2. Materialize the synthesized overload as a provider-specific type + +Override the seam to return your own `MethodProvider` subtype or to apply adjustments. The +base has already built the hidden, delegating overload — you only re-wrap or tweak it. (This +is exactly how `ClientProvider` returns an `ScmMethodProvider` and adds its AZC0002 +suppression.) + +```csharp +public partial class ResourceClientProvider : TypeProvider +{ + protected override MethodProvider BuildBackCompatibilityOverload(MethodProvider overload) + { + // overload.Signature is hidden ([EditorBrowsable(Never)], defaults stripped) and + // overload.BodyStatements already delegates to the current method. + return new MyResourceMethodProvider(overload.Signature, overload.BodyStatements!, this, overload.XmlDocs); + } +} +``` + +### 3. Extend or filter the produced method list + +Override `BuildMethodsForBackCompatibility`, call `base` to keep the standard shims, then add +or filter your own: + +```csharp +public partial class ResourceClientProvider : TypeProvider +{ + protected override IReadOnlyList BuildMethodsForBackCompatibility( + IEnumerable methods) + { + var result = base.BuildMethodsForBackCompatibility(methods); // standard shims first + // ...inspect LastContractView and add/adjust resource-specific methods... + return result; + } +} +``` + +The default synthesized overload delegates to the current method, so it works even for +management LRO methods (it forwards to the method that wraps the result in `ArmOperation`). + +### 4. Preserve a property type while building it (build-time) + +Call the getter while constructing the property, before serialization is generated: + +```csharp +var property = TypeFactory.CreateProperty(inputProperty, this); +property.Type = GetPropertyTypeForBackCompatibility(property); // previous type wins if it changed +``` + +## Deferred + +Build-time **parameter-name preservation** (`top`→`maxCount`, page-size casing, +`@@clientName` renames) and **content-type-before-body ordering** stay in `RestClientProvider` +for now. When exposed, they join the `Get…` family as +`GetParameterNameForBackCompatibility(ParameterProvider, MethodProvider)`. From b93ab68970165a8adabb0884e2f2365ec4a0117f Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Mon, 6 Jul 2026 15:11:07 -0500 Subject: [PATCH 2/4] add non-optional param overload --- .../src/Providers/ClientProvider.cs | 189 ++---------------- .../ExtensibleEnumSerializationProvider.cs | 4 + .../MrwSerializationTypeDefinition.cs | 4 + ...ultipartFormDataSerializationDefinition.cs | 3 + .../src/Providers/TypeProvider.cs | 2 + .../src/Utilities/BackCompatHelper.cs | 167 ++++++++++++++++ ...lityAddsOverloadForNewOptionalParameter.cs | 22 ++ .../NewOptionalParamType.cs | 7 + .../test/Providers/TypeProviderTests.cs | 25 +++ 9 files changed, 250 insertions(+), 173 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter/NewOptionalParamType.cs diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs index 958cd3adf75..b6e2d9cc78a 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ClientProvider.cs @@ -1297,23 +1297,24 @@ protected sealed override IReadOnlyList BuildMethodsForBackCompa UpdateConvenienceMethodsForBackCompat(result, methodsWithReorderedParams, updatedSignatureToOriginal); } - // Add hidden overloads for methods that gained new optional non-body parameters. - ProcessBackCompatForNewOptionalParameters(result, BuildCurrentMethodSignatureMap(result)); - - return result; - } - - private Dictionary BuildCurrentMethodSignatureMap(IEnumerable methods) - { - var allMethods = CustomCodeView?.Methods != null - ? methods.Concat(CustomCodeView.Methods) - : methods; - - var result = new Dictionary(MethodSignature.MethodSignatureComparer); - foreach (var method in allMethods) + // The base adds hidden overloads for methods that gained new optional non-body parameters. + // Where such an overload preserves a previous signature whose trailing parameter was a + // CancellationToken, suppress AZC0002: making that parameter optional would create an + // ambiguous call with the current method. + foreach (var method in result) { - result.TryAdd(method.Signature, method); + if (!originalSignatures.ContainsKey(method) && PreviousSignatureEndsWithCancellationToken(method.Signature)) + { + method.Update(suppressions: + [ + new SuppressionStatement( + inner: null, + code: Literal("AZC0002"), + justification: "Back-compat overload preserves the previous method signature where CancellationToken was the trailing parameter. Making it optional would introduce an ambiguous call with the new method.") + ]); + } } + return result; } @@ -1682,82 +1683,6 @@ private static void ReorderMethodInvocationArguments( } } - private void ProcessBackCompatForNewOptionalParameters( - List methods, - Dictionary currentMethodSignatures) - { - var currentMethodsByName = new Dictionary>(); - foreach (var method in currentMethodSignatures.Values) - { - if (method is ScmMethodProvider { Kind: ScmMethodKind.CreateRequest }) - { - continue; - } - - if (!currentMethodsByName.TryGetValue(method.Signature.Name, out var list)) - { - list = []; - currentMethodsByName[method.Signature.Name] = list; - } - list.Add(method); - } - - foreach (var previousMethod in LastContractView!.Methods) - { - var previousSignature = previousMethod.Signature; - - if (!previousSignature.Modifiers.HasFlag(MethodSignatureModifiers.Public) && - !previousSignature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)) - { - continue; - } - - if (currentMethodSignatures.ContainsKey(previousSignature) || - !currentMethodsByName.TryGetValue(previousSignature.Name, out var candidates)) - { - continue; - } - - ScmMethodProvider? matchedCurrent = null; - foreach (var candidate in candidates) - { - if (candidate is ScmMethodProvider { Kind: ScmMethodKind.Convenience or ScmMethodKind.Protocol } scmCandidate && - HasNewOptionalNonBodyParametersOnly(previousSignature, scmCandidate.Signature)) - { - matchedCurrent = scmCandidate; - break; - } - } - - if (matchedCurrent is null) - { - continue; - } - - var overload = BuildBackCompatOverloadForNewOptionalParameters(previousMethod, matchedCurrent); - if (overload == null || !currentMethodSignatures.TryAdd(overload.Signature, overload)) - { - continue; - } - - if (PreviousSignatureEndsWithCancellationToken(previousSignature)) - { - overload.Update(suppressions: - [ - new SuppressionStatement( - inner: null, - code: Literal("AZC0002"), - justification: "Back-compat overload preserves the previous method signature where CancellationToken was the trailing parameter. Making it optional would introduce an ambiguous call with the new method.") - ]); - } - - methods.Add(overload); - CodeModelGenerator.Instance.Emitter.Debug( - $"Added back-compat overload for '{Name}.{previousSignature.Name}' to handle new optional parameter(s) introduced relative to the last contract.", - BackCompatibilityChangeCategory.SvcMethodNewOptionalParameterOverloadAdded); - } - } - private static bool PreviousSignatureEndsWithCancellationToken(MethodSignature previousSignature) { if (previousSignature.Parameters.Count == 0) @@ -1768,87 +1693,5 @@ private static bool PreviousSignatureEndsWithCancellationToken(MethodSignature p var lastParam = previousSignature.Parameters[previousSignature.Parameters.Count - 1]; return new CSharpType.CSharpTypeIgnoreNullableComparer().Equals(lastParam.Type, new CSharpType(typeof(CancellationToken))); } - - // Returns true when currentSignature contains all parameters of previousSignature in the same - // relative order, every "extra" parameter is optional, and none of the extras are body parameters. - private static bool HasNewOptionalNonBodyParametersOnly( - MethodSignature previousSignature, - MethodSignature currentSignature) - { - if (currentSignature.Parameters.Count <= previousSignature.Parameters.Count) - { - return false; - } - - if (previousSignature.ReturnType is null - ? currentSignature.ReturnType is not null - : !previousSignature.ReturnType.AreNamesEqual(currentSignature.ReturnType)) - { - return false; - } - - // Walk current parameters and ensure previous parameters appear in the same relative order - // (matched by variable name and type), with every "extra" parameter being optional and non-body. - int previousIndex = 0; - for (int currentIndex = 0; currentIndex < currentSignature.Parameters.Count; currentIndex++) - { - var currentParam = currentSignature.Parameters[currentIndex]; - - if (previousIndex < previousSignature.Parameters.Count) - { - var previousParam = previousSignature.Parameters[previousIndex]; - if (currentParam.Name.ToVariableName() == previousParam.Name.ToVariableName() && - currentParam.Type.AreNamesEqual(previousParam.Type)) - { - previousIndex++; - continue; - } - } - - if (currentParam.DefaultValue is null) - { - return false; - } - - if (currentParam.Location == ParameterLocation.Body) - { - return false; - } - } - - return previousIndex == previousSignature.Parameters.Count; - } - - private ScmMethodProvider? BuildBackCompatOverloadForNewOptionalParameters( - MethodProvider previousMethod, - ScmMethodProvider currentMethod) - { - var previousSignature = previousMethod.Signature; - var currentSignature = currentMethod.Signature; - - var previousParamsByName = new Dictionary(); - foreach (var p in previousSignature.Parameters) - { - previousParamsByName.TryAdd(p.Name.ToVariableName(), p); - } - - var arguments = new List(currentSignature.Parameters.Count); - foreach (var currentParam in currentSignature.Parameters) - { - var currentParamVariableName = currentParam.Name.ToVariableName(); - ValueExpression value = previousParamsByName.TryGetValue(currentParamVariableName, out var prevParam) - ? prevParam - : (currentParam.DefaultValue ?? Default); - arguments.Add(PositionalReference(currentParamVariableName, value)); - } - - return new ScmMethodProvider( - signature: MethodSignatureHelper.BuildBackCompatMethodSignature(previousSignature, hideMethod: true), - bodyStatements: Return(This.Invoke(currentSignature.Name, arguments)), - enclosingType: this, - methodKind: currentMethod.Kind, - xmlDocProvider: previousMethod.XmlDocs, - serviceMethod: currentMethod.ServiceMethod); - } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ExtensibleEnumSerializationProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ExtensibleEnumSerializationProvider.cs index f3c5a792c95..a026a17b325 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ExtensibleEnumSerializationProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ExtensibleEnumSerializationProvider.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.Collections.Generic; using System.IO; using Microsoft.TypeSpec.Generator.Input; using Microsoft.TypeSpec.Generator.Primitives; @@ -34,6 +35,9 @@ protected override string BuildRelativeFilePath() protected override TypeSignatureModifiers BuildDeclarationModifiers() => _enumProvider.DeclarationModifiers; + protected override IReadOnlyList BuildMethodsForBackCompatibility(IEnumerable originalMethods) + => [.. originalMethods]; + protected override MethodProvider[] BuildMethods() { // for string-based extensible enums, we are using `ToString` as its serialization diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs index 8202a2af405..16ac19e14e9 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs @@ -117,6 +117,10 @@ public MrwSerializationTypeDefinition(InputModelType inputModel, ModelProvider m protected override string BuildNamespace() => _model.Type.Namespace; protected override TypeSignatureModifiers BuildDeclarationModifiers() => _model.DeclarationModifiers; + + protected override IReadOnlyList BuildMethodsForBackCompatibility(IEnumerable originalMethods) + => [.. originalMethods]; + private ConstructorProvider SerializationConstructor => _serializationConstructor ??= _model.FullConstructor; private PropertyProvider[] AdditionalProperties => _additionalProperties.Value; diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MultipartFormDataSerializationDefinition.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MultipartFormDataSerializationDefinition.cs index 88cb97b16e7..3ef2c7c4c62 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MultipartFormDataSerializationDefinition.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MultipartFormDataSerializationDefinition.cs @@ -46,6 +46,9 @@ public MultipartFormDataSerializationDefinition(InputModelType inputModel, Model protected override TypeSignatureModifiers BuildDeclarationModifiers() => _model.DeclarationModifiers; + protected override IReadOnlyList BuildMethodsForBackCompatibility(IEnumerable originalMethods) + => [.. originalMethods]; + protected override string BuildRelativeFilePath() { return Path.Combine("src", "Generated", "Models", $"{Name}.Serialization.Multipart.cs"); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index 7e15a1f0cf7..d0e24b39774 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -862,6 +862,8 @@ protected internal virtual IReadOnlyList BuildMethodsForBackComp BackCompatHelper.RestorePreviousParameterNames(this, methods); + BackCompatHelper.AddOverloadsForNewOptionalParameters(this, methods); + return methods; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs index 05cd639b94c..52b8d742194 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs @@ -5,10 +5,12 @@ using System.Collections.Generic; using System.Linq; using Microsoft.TypeSpec.Generator.EmitterRpc; +using Microsoft.TypeSpec.Generator.Expressions; using Microsoft.TypeSpec.Generator.Input.Extensions; using Microsoft.TypeSpec.Generator.Primitives; using Microsoft.TypeSpec.Generator.Providers; using Microsoft.TypeSpec.Generator.Statements; +using static Microsoft.TypeSpec.Generator.Snippets.Snippet; namespace Microsoft.TypeSpec.Generator.Utilities { @@ -325,5 +327,170 @@ private static void UpdateXmlDocProviderForParamReorder( xmlDocs.Update(parameters: reorderedParamDocs); } } + + /// + /// Adds hidden back-compat overloads for public/protected methods that gained one or more new + /// optional non-body parameters relative to the last contract. Each added overload matches a + /// previously-published signature and delegates to the current method, forwarding the previous + /// arguments and passing default for every new parameter. Renaming a method's parameter + /// set this way is otherwise a source-breaking change for existing callers. + /// + public static void AddOverloadsForNewOptionalParameters(TypeProvider enclosingType, List methods) + { + if (enclosingType.LastContractView?.Methods is not { Count: > 0 } previousMethods) + { + return; + } + + var currentMethods = enclosingType.CustomCodeView?.Methods is { } customMethods + ? methods.Concat(customMethods) + : methods; + if (!currentMethods.Any()) + { + return; + } + + var currentMethodsByName = new Dictionary>(); + foreach (var method in currentMethods) + { + if (!currentMethodsByName.TryGetValue(method.Signature.Name, out var bucket)) + { + bucket = []; + currentMethodsByName[method.Signature.Name] = bucket; + } + bucket.Add(method); + } + + foreach (var previousMethod in previousMethods) + { + var previousSignature = previousMethod.Signature; + if ((!previousSignature.Modifiers.HasFlag(MethodSignatureModifiers.Public) && + !previousSignature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)) || + !currentMethodsByName.TryGetValue(previousSignature.Name, out var candidates)) + { + continue; + } + + // One pass over the same-named methods: bail if the previous signature still exists, + // otherwise remember the first public/protected method that merely gained optional + // non-body parameters. + MethodProvider? matchedCurrent = null; + bool previousStillExists = false; + foreach (var candidate in candidates) + { + if (MethodSignature.MethodSignatureComparer.Equals(candidate.Signature, previousSignature)) + { + previousStillExists = true; + break; + } + + var modifiers = candidate.Signature.Modifiers; + if (matchedCurrent is null && + (modifiers.HasFlag(MethodSignatureModifiers.Public) || modifiers.HasFlag(MethodSignatureModifiers.Protected)) && + HasNewOptionalNonBodyParametersOnly(previousSignature, candidate.Signature)) + { + matchedCurrent = candidate; + } + } + + if (previousStillExists || matchedCurrent is null) + { + continue; + } + + var overload = BuildNewOptionalParameterOverload(enclosingType, previousMethod, matchedCurrent); + candidates.Add(overload); + methods.Add(overload); + CodeModelGenerator.Instance.Emitter.Debug( + $"Added back-compat overload for '{enclosingType.Name}.{previousSignature.Name}' to handle new optional parameter(s) introduced relative to the last contract.", + BackCompatibilityChangeCategory.SvcMethodNewOptionalParameterOverloadAdded); + } + } + + /// + /// Returns true when contains all parameters of + /// in the same relative order (matched by variable name and + /// type) with the same return type, every "extra" current parameter is optional, and none of the + /// extras are body parameters. + /// + public static bool HasNewOptionalNonBodyParametersOnly( + MethodSignature previousSignature, + MethodSignature currentSignature) + { + if (currentSignature.Parameters.Count <= previousSignature.Parameters.Count) + { + return false; + } + + if (previousSignature.ReturnType is null + ? currentSignature.ReturnType is not null + : !previousSignature.ReturnType.AreNamesEqual(currentSignature.ReturnType)) + { + return false; + } + + // Walk current parameters and ensure previous parameters appear in the same relative order + // (matched by variable name and type), with every "extra" parameter being optional and non-body. + int previousIndex = 0; + for (int currentIndex = 0; currentIndex < currentSignature.Parameters.Count; currentIndex++) + { + var currentParam = currentSignature.Parameters[currentIndex]; + + if (previousIndex < previousSignature.Parameters.Count) + { + var previousParam = previousSignature.Parameters[previousIndex]; + if (currentParam.Name.ToVariableName() == previousParam.Name.ToVariableName() && + currentParam.Type.AreNamesEqual(previousParam.Type)) + { + previousIndex++; + continue; + } + } + + if (currentParam.DefaultValue is null) + { + return false; + } + + if (currentParam.Location == ParameterLocation.Body) + { + return false; + } + } + + return previousIndex == previousSignature.Parameters.Count; + } + + private static MethodProvider BuildNewOptionalParameterOverload( + TypeProvider enclosingType, + MethodProvider previousMethod, + MethodProvider currentMethod) + { + var previousSignature = previousMethod.Signature; + var currentSignature = currentMethod.Signature; + + var previousParametersByName = new Dictionary(); + foreach (var parameter in previousSignature.Parameters) + { + previousParametersByName.TryAdd(parameter.Name.ToVariableName(), parameter); + } + + // Build the delegating call: forward each previous parameter and pass default for the new ones. + var arguments = new List(currentSignature.Parameters.Count); + foreach (var currentParam in currentSignature.Parameters) + { + var variableName = currentParam.Name.ToVariableName(); + ValueExpression value = previousParametersByName.TryGetValue(variableName, out var previousParam) + ? previousParam + : currentParam.DefaultValue ?? Default; + arguments.Add(PositionalReference(variableName, value)); + } + + return new MethodProvider( + MethodSignatureHelper.BuildBackCompatMethodSignature(previousSignature, hideMethod: true), + Return(This.Invoke(currentSignature.Name, arguments)), + enclosingType, + previousMethod.XmlDocs); + } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter.cs new file mode 100644 index 00000000000..27ada66502b --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter.cs @@ -0,0 +1,22 @@ +// + +#nullable disable + +using System.ComponentModel; + +namespace Test +{ + public partial class NewOptionalParamType + { + public string GetData(int param1, bool param2 = default) + { + return null; + } + + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Never)] + public string GetData(int param1) + { + return this.GetData(param1: param1, param2: default); + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter/NewOptionalParamType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter/NewOptionalParamType.cs new file mode 100644 index 00000000000..69e7f0621e2 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter/NewOptionalParamType.cs @@ -0,0 +1,7 @@ +namespace Test +{ + public class NewOptionalParamType + { + public string GetData(int param1) => null; + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index 9c7e339eb7b..45d92358f20 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -232,6 +232,31 @@ public async Task BuildConstructorsForBackCompatibilityKeepsModifierOnNonAbstrac Assert.IsFalse(constructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)); } + // Validates that the base TypeProvider generalizes the new-optional-parameter back-compat to any + // TypeProvider: a public method that gained an optional non-body parameter relative to the last + // contract gets a hidden overload matching the previous signature that delegates to the current one. + [Test] + public async Task BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalParameter() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // The last contract published GetData(int param1); the current generation adds an optional + // non-body parameter param2. + var param1 = new ParameterProvider("param1", $"", new CSharpType(typeof(int))); + var param2 = new ParameterProvider("param2", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default); + var getData = new MethodProvider( + new MethodSignature("GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [param1, param2]), + Snippet.Return(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "NewOptionalParamType", ns: "Test", methods: [getData]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + // Validates the shared lookup that any provider can use to restore a previously-published // parameter name from its last contract. [Test] From bf51751ae09fc0569cd2fd1deebeeea009ed9aed Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 10 Jul 2026 10:55:24 -0500 Subject: [PATCH 3/4] generalize fixedEnum backCompat --- .../BackCompatibilityChangeCategory.cs | 3 + .../src/EmitterRpc/Emitter.cs | 1 + .../src/Providers/ApiVersionEnumProvider.cs | 11 +- .../src/Providers/FixedEnumProvider.cs | 109 +++++++-- .../src/Providers/TypeProvider.cs | 18 +- .../src/SourceInput/ApiCompatBaseline.cs | 42 +++- .../src/Utilities/BackCompatHelper.cs | 6 +- .../EnumProviders/EnumProviderTests.cs | 230 +++++++++++++++++- .../MockInputEnum.cs | 11 + .../MockInputEnum.cs | 11 + ...ovedValueNotReaddedWhenBaselineAccepts.txt | 3 + .../MockInputEnum.cs | 0 .../MockInputEnum.cs | 11 + .../MockInputEnum.cs | 11 + .../MockInputEnum.cs | 10 + .../MockInputEnum.cs | 11 + .../test/Providers/TypeProviderTests.cs | 103 +++++++- .../SourceInput/ApiCompatBaselineTests.cs | 20 ++ .../ApiCompatBaselineTests/Baseline.txt | 2 + .../backward-compatibility-extensibility.md | 170 ------------- .../generator/docs/backward-compatibility.md | 88 ++++++- 21 files changed, 643 insertions(+), 228 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumNonContiguousValuesPreserved/MockInputEnum.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedMiddleValueReadded/MockInputEnum.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts.txt rename packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/{BackCompat_IntEnumValueRemoved => BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts}/MockInputEnum.cs (100%) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueReadded/MockInputEnum.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_LastContractExposesEnumMemberValues/MockInputEnum.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_LongEnumRemovedValueReadded/MockInputEnum.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_StringEnumRemovedValueNotReadded/MockInputEnum.cs delete mode 100644 packages/http-client-csharp/generator/docs/backward-compatibility-extensibility.md diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs index 1c322568a7b..38f74bddf37 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs @@ -45,5 +45,8 @@ public enum BackCompatibilityChangeCategory /// A back-compat change was skipped because the removal was accepted in the ApiCompat baseline. BaselineAcceptedRemovalSkipped, + + /// A fixed enum member was re-added to preserve a member that existed in the last contract but is no longer produced by the current spec. + EnumMemberAddedFromLastContract, } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs index e170e8a98a3..83d965b4ed0 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs @@ -182,6 +182,7 @@ public void WriteBufferedMessages() BackCompatibilityChangeCategory.ModelFactoryMethodSkipped => "Model Factory Method Back-Compat Skipped", BackCompatibilityChangeCategory.SvcMethodNewOptionalParameterOverloadAdded => "Method Back-Compat Overload Added For New Optional Parameter", BackCompatibilityChangeCategory.BaselineAcceptedRemovalSkipped => "Back-Compat Skipped For ApiCompat Baseline Accepted Removal", + BackCompatibilityChangeCategory.EnumMemberAddedFromLastContract => "Enum Member Added From Last Contract", _ => category.ToString(), }; diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ApiVersionEnumProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ApiVersionEnumProvider.cs index 642a8bf85e2..6bceafce8a5 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ApiVersionEnumProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ApiVersionEnumProvider.cs @@ -168,15 +168,8 @@ private List BuildApiVersionEnumValuesForBackwardCompatibility(L return currentApiVersions; } - var processedNames = new HashSet(lastContractFields.Select(f => f.Name), StringComparer.OrdinalIgnoreCase); - // Then, add new versions in the wire order - foreach (var currentVersion in currentApiVersions) - { - if (!processedNames.Contains(currentVersion.Name)) - { - allMembers.Add(currentVersion); - } - } + // Then, add new versions in the wire order. + AppendMembersNotInLastContract(currentApiVersions, lastContractFields, allMembers); for (int i = 0; i < allMembers.Count; i++) { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/FixedEnumProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/FixedEnumProvider.cs index 524b3bf2627..31cdabe64f8 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/FixedEnumProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/FixedEnumProvider.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Linq; using Microsoft.TypeSpec.Generator.EmitterRpc; using Microsoft.TypeSpec.Generator.Expressions; @@ -128,29 +129,46 @@ protected override IReadOnlyList BuildEnumValues() { if (currentLookup.TryGetValue(field.Name, out var existingMember)) { + // Preserve the last contract's explicit value for integer enums so members keep + // their exact values. + ValueExpression? initializationValue = null; + var memberValue = existingMember.Value; + if (IsIntValueType && field.InitializationValue is LiteralExpression { Literal: { } lastContractValue }) + { + initializationValue = field.InitializationValue; + memberValue = lastContractValue; + } + var updatedField = new FieldProvider( existingMember.Field.Modifiers, existingMember.Field.Type, existingMember.Name, existingMember.Field.EnclosingType, - existingMember.Field.Description); - allMembers.Add(new EnumTypeMember(existingMember.Name, updatedField, existingMember.Value)); + existingMember.Field.Description, + initializationValue); + allMembers.Add(new EnumTypeMember(existingMember.Name, updatedField, memberValue)); } - } - - // Then, add new members that weren't in the last contract (in their original input order) - var processedNames = new HashSet(lastContractFields.Select(f => f.Name), StringComparer.OrdinalIgnoreCase); - foreach (var current in currentValues) - { - if (!processedNames.Contains(current.Name)) + else if (CodeModelGenerator.Instance.SourceInputModel?.ApiCompatBaseline.IsMemberSuppressed(Type.FullyQualifiedName, field.Name, 0) == true) { - allMembers.Add(current); + CodeModelGenerator.Instance.Emitter.Debug( + $"Skipping re-add of enum member '{Name}.{field.Name}'; the removal is accepted in the ApiCompat baseline.", + BackCompatibilityChangeCategory.BaselineAcceptedRemovalSkipped); + } + else if (TryResurrectRemovedMember(field, out var resurrectedMember)) + { + allMembers.Add(resurrectedMember); + CodeModelGenerator.Instance.Emitter.Debug( + $"Re-added enum member '{field.Name}' to enum '{Name}' to preserve a member from the last contract.", + BackCompatibilityChangeCategory.EnumMemberAddedFromLastContract); } } - // Report a summary-level change only if the relative order of shared members - // was actually altered to match the last contract. - if (!EnumMemberOrderMatches(currentValues, allMembers)) + // Then, add new members that weren't in the last contract (in their original input order). + AppendMembersNotInLastContract(currentValues, lastContractFields, allMembers); + + // Report a reordering only when the relative order of members present in BOTH the + // current values and the resulting set was actually altered. + if (SharedMemberOrderChanged(currentValues, allMembers)) { CodeModelGenerator.Instance.Emitter.Debug( $"Reordered members of enum '{Name}' to match last contract.", @@ -160,22 +178,71 @@ protected override IReadOnlyList BuildEnumValues() return allMembers; } - private static bool EnumMemberOrderMatches( - IReadOnlyList left, - IReadOnlyList right) + private bool TryResurrectRemovedMember(FieldProvider lastContractField, [NotNullWhen(true)] out EnumTypeMember? member) { - if (left.Count != right.Count) + member = null; + if (!IsIntValueType || lastContractField.InitializationValue is not LiteralExpression { Literal: { } literalValue }) { return false; } - for (int i = 0; i < left.Count; i++) + + var field = new FieldProvider( + FieldModifiers.Public | FieldModifiers.Static, + EnumUnderlyingType, + lastContractField.Name, + this, + lastContractField.Description, + Literal(literalValue)); + member = new EnumTypeMember(lastContractField.Name, field, literalValue); + return true; + } + + // Appends the members of that were not present in the last + // contract (i.e. newly introduced by the current spec), preserving their original input order. + // Shared by the fixed-enum and API-version back-compatibility passes. + private protected static void AppendMembersNotInLastContract( + IReadOnlyList currentValues, + IReadOnlyList lastContractFields, + List allMembers) + { + var lastContractNames = new HashSet(lastContractFields.Select(f => f.Name), StringComparer.OrdinalIgnoreCase); + foreach (var current in currentValues) + { + if (!lastContractNames.Contains(current.Name)) + { + allMembers.Add(current); + } + } + } + + private static bool SharedMemberOrderChanged( + IReadOnlyList currentValues, + IReadOnlyList result) + { + var currentNames = new HashSet(currentValues.Count, StringComparer.Ordinal); + foreach (var member in currentValues) { - if (!string.Equals(left[i].Name, right[i].Name, StringComparison.Ordinal)) + currentNames.Add(member.Name); + } + + var index = 0; + foreach (var member in result) + { + if (!currentNames.Contains(member.Name)) + { + continue; + } + + if (index >= currentValues.Count + || !string.Equals(member.Name, currentValues[index].Name, StringComparison.Ordinal)) { - return false; + return true; } + + index++; } - return true; + + return false; } protected internal override FieldProvider[] BuildFields() diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index d0e24b39774..83380c4299b 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -861,7 +861,6 @@ protected internal virtual IReadOnlyList BuildMethodsForBackComp } BackCompatHelper.RestorePreviousParameterNames(this, methods); - BackCompatHelper.AddOverloadsForNewOptionalParameters(this, methods); return methods; @@ -920,19 +919,20 @@ protected internal virtual IReadOnlyList BuildConstructorsF for (int i = 0; i < constructors.Count; i++) { var currentConstructor = constructors[i]; + if (!currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Private) || + !currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)) + { + continue; + } // Check if parameters match (same count and types) if (BackCompatHelper.ParametersMatch(currentConstructor.Signature.Parameters, previousConstructor.Signature.Parameters)) { // Change the modifier from private protected to public - if (currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Private) && - currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)) - { - currentConstructor.Signature.Update(modifiers: MethodSignatureModifiers.Public); - CodeModelGenerator.Instance.Emitter.Debug( - $"Promoted constructor '{Name}({string.Join(", ", currentConstructor.Signature.Parameters.Select(p => p.Type.ToString()))})' from 'private protected' to 'public' to match last contract.", - BackCompatibilityChangeCategory.ConstructorModifierPreserved); - } + currentConstructor.Signature.Update(modifiers: MethodSignatureModifiers.Public); + CodeModelGenerator.Instance.Emitter.Debug( + $"Promoted constructor '{Name}({string.Join(", ", currentConstructor.Signature.Parameters.Select(p => p.Type.ToString()))})' from 'private protected' to 'public' to match last contract.", + BackCompatibilityChangeCategory.ConstructorModifierPreserved); } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/SourceInput/ApiCompatBaseline.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/SourceInput/ApiCompatBaseline.cs index 287c0ead838..5b514d404aa 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/SourceInput/ApiCompatBaseline.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/SourceInput/ApiCompatBaseline.cs @@ -26,6 +26,7 @@ public sealed class ApiCompatBaseline { private const string TypesMustExist = "TypesMustExist"; private const string MembersMustExist = "MembersMustExist"; + private const string EnumValuesMustMatch = "EnumValuesMustMatch"; private readonly HashSet _suppressedTypes; private readonly HashSet _suppressedMembers; @@ -98,11 +99,17 @@ public static ApiCompatBaseline Parse(IEnumerable lines) suppressedTypes.Add(quoted); break; case MembersMustExist: - if (TryParseMember(quoted, out var memberKey)) + if (TryParseMember(quoted, out var memberKey) || TryParseField(quoted, out memberKey)) { suppressedMembers.Add(memberKey); } break; + case EnumValuesMustMatch: + if (TryParseField(quoted, out var enumMemberKey)) + { + suppressedMembers.Add(enumMemberKey); + } + break; // Other rule ids (e.g. CannotRemoveAttribute) do not describe a removed // member/type that the back-compat system would resurrect, so they are ignored. default: @@ -256,6 +263,39 @@ private static bool TryParseMember(string signature, out MemberKey memberKey) return true; } + // Parses an ApiCompat field/enum-value signature (which has no parameter list) of the form: + // [modifiers] FieldType Namespace.DeclaringType.MemberName + // for example: + // Ns.CapacityLevel Ns.CapacityLevel.FiftyThousand + // public Ns.Kind Ns.Foo.Kind + // Enum members are recorded with a parameter count of zero. + private static bool TryParseField(string signature, out MemberKey memberKey) + { + memberKey = default; + + // Field/enum-value signatures never contain a parameter list. + if (signature.Contains('(')) + { + return false; + } + + // The fully-qualified member path is the last whitespace-delimited token (the tokens + // before it are access modifiers and the field/enum type). + var lastSpace = signature.LastIndexOf(' '); + var memberPath = lastSpace >= 0 ? signature.Substring(lastSpace + 1) : signature; + + var lastDot = memberPath.LastIndexOf('.'); + if (lastDot <= 0 || lastDot == memberPath.Length - 1) + { + return false; + } + + var declaringTypeFullName = memberPath.Substring(0, lastDot); + var memberName = memberPath.Substring(lastDot + 1); + memberKey = new MemberKey(declaringTypeFullName, memberName, 0); + return true; + } + private static int CountParameters(string parameterList) { var trimmed = parameterList.Trim(); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs index 52b8d742194..d44de16d724 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs @@ -266,7 +266,7 @@ public static bool TryRestorePreviousParameterOrder( foreach (var previousParam in previousSignature.Parameters) { - if (parametersByName.TryGetValue(previousParam.Name, out var matchingParam)) + if (parametersByName.TryGetValue(previousParam.Name.ToVariableName(), out var matchingParam)) { reorderedParameters.Add(matchingParam); } @@ -279,7 +279,7 @@ public static bool TryRestorePreviousParameterOrder( foreach (var previousParam in previousSignature.Parameters) { - if (parametersByName.TryGetValue(previousParam.Name, out var matchingParam) + if (parametersByName.TryGetValue(previousParam.Name.ToVariableName(), out var matchingParam) && matchingParam.DefaultValue is not null && previousParam.DefaultValue is not null) { @@ -452,7 +452,7 @@ public static bool HasNewOptionalNonBodyParametersOnly( return false; } - if (currentParam.Location == ParameterLocation.Body) + if (currentParam.Location is ParameterLocation.Body or ParameterLocation.Unknown) { return false; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs index 3bbc8cb463e..dd2d2fc63e3 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +// cspell:ignore readded + using System; using System.Linq; using System.Threading.Tasks; @@ -394,9 +396,9 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual("Recover", fields[0].Name); Assert.AreEqual("Default", fields[1].Name); - // No explicit initialization values - compiler auto-assigns based on order - Assert.IsNull(fields[0].InitializationValue); - Assert.IsNull(fields[1].InitializationValue); + // Shared members keep their explicit values from the last contract (Recover = 0, Default = 1). + Assert.AreEqual(0, (fields[0].InitializationValue as LiteralExpression)?.Literal); + Assert.AreEqual(1, (fields[1].InitializationValue as LiteralExpression)?.Literal); } // Validates that int enum member order is preserved and new values are appended @@ -429,18 +431,75 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual("Default", fields[1].Name); Assert.AreEqual("Third", fields[2].Name); - // No explicit initialization values for reordered members - compiler auto-assigns based on order - Assert.IsNull(fields[0].InitializationValue); - Assert.IsNull(fields[1].InitializationValue); + // Shared members keep their explicit values from the last contract (Recover = 0, Default = 1). + Assert.AreEqual(0, (fields[0].InitializationValue as LiteralExpression)?.Literal); + Assert.AreEqual(1, (fields[1].InitializationValue as LiteralExpression)?.Literal); // New value keeps its initialization value from the input var value3 = fields[2].InitializationValue as LiteralExpression; Assert.IsNotNull(value3); Assert.AreEqual(2, value3?.Literal); } - // Validates that removed enum values from last contract are not included + // Verifies that the last contract (reconstructed from Roslyn) exposes each enum member's + // explicit numeric value, so back-compat can preserve non-contiguous values (e.g. 100, 200, + // 500) rather than reassigning positional ordinals. + [Test] + public async Task BackCompat_LastContractExposesEnumMemberValues() + { + await MockHelpers.LoadMockGeneratorAsync( + createCSharpTypeCore: (inputType) => typeof(int), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var input = InputFactory.Int32Enum("mockInputEnum", [ + ("OneHundred", 100), + ("TwoHundred", 200), + ("FiveHundred", 500), + ]); + + var enumType = EnumProvider.Create(input); + enumType.EnsureBuilt(); + + var lastContractFields = enumType.LastContractView?.Fields; + Assert.IsNotNull(lastContractFields); + Assert.AreEqual(3, lastContractFields!.Count); + + var values = lastContractFields.Select(f => (f.InitializationValue as LiteralExpression)?.Literal).ToArray(); + CollectionAssert.AreEqual(new object[] { 100, 200, 500 }, values); + } + + [Test] + public async Task BackCompat_IntEnumNonContiguousValuesPreserved() + { + await MockHelpers.LoadMockGeneratorAsync( + createCSharpTypeCore: (inputType) => typeof(int), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var input = InputFactory.Int32Enum("mockInputEnum", [ + ("OneHundred", 100), + ("TwoHundred", 200), + ("FiveHundred", 500), + ]); + + var enumType = EnumProvider.Create(input); + enumType.EnsureBuilt(); + enumType.ProcessTypeForBackCompatibility(); + + var fields = enumType.Fields; + Assert.AreEqual(3, fields.Count); + Assert.AreEqual("OneHundred", fields[0].Name); + Assert.AreEqual("TwoHundred", fields[1].Name); + Assert.AreEqual("FiveHundred", fields[2].Name); + + // Each member keeps its exact explicit value from the last contract. + Assert.AreEqual(100, (fields[0].InitializationValue as LiteralExpression)?.Literal); + Assert.AreEqual(200, (fields[1].InitializationValue as LiteralExpression)?.Literal); + Assert.AreEqual(500, (fields[2].InitializationValue as LiteralExpression)?.Literal); + } + + // Validates that an integer enum value removed from the current spec is re-added from the + // last contract (preserving its explicit numeric value) to keep the previously shipped API. [Test] - public async Task BackCompat_IntEnumValueRemoved() + public async Task BackCompat_IntEnumRemovedValueReadded() { await MockHelpers.LoadMockGeneratorAsync( createCSharpTypeCore: (inputType) => typeof(int), @@ -459,18 +518,169 @@ await MockHelpers.LoadMockGeneratorAsync( enumType.EnsureBuilt(); enumType.ProcessTypeForBackCompatibility(); + var fields = enumType.Fields; + Assert.AreEqual(3, fields.Count); + + // Order preserved from last contract, and the removed "Third" is re-added at its + // original position. + Assert.AreEqual("Recover", fields[0].Name); + Assert.AreEqual("Default", fields[1].Name); + Assert.AreEqual("Third", fields[2].Name); + + // Shared members keep their explicit values from the last contract (Recover = 0, Default = 1). + Assert.AreEqual(0, (fields[0].InitializationValue as LiteralExpression)?.Literal); + Assert.AreEqual(1, (fields[1].InitializationValue as LiteralExpression)?.Literal); + + // The re-added member keeps its explicit numeric value from the last contract. + var readdedValue = fields[2].InitializationValue as LiteralExpression; + Assert.IsNotNull(readdedValue); + Assert.AreEqual(2, readdedValue?.Literal); + } + + // Validates that a value removed from the MIDDLE of an integer enum is re-added at its + // original position, preserving both surrounding order and its explicit numeric value. + [Test] + public async Task BackCompat_IntEnumRemovedMiddleValueReadded() + { + await MockHelpers.LoadMockGeneratorAsync( + createCSharpTypeCore: (inputType) => typeof(int), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // Last contract: Alpha = 0, Beta = 1, Gamma = 2. Current input removes the middle "Beta". + var input = InputFactory.Int32Enum("mockInputEnum", [ + ("Alpha", 0), + ("Gamma", 2), + ]); + + var enumType = EnumProvider.Create(input); + Assert.IsFalse(enumType is ApiVersionEnumProvider); + + enumType.EnsureBuilt(); + enumType.ProcessTypeForBackCompatibility(); + + var fields = enumType.Fields; + Assert.AreEqual(3, fields.Count); + + // "Beta" is re-added at its original middle position. + Assert.AreEqual("Alpha", fields[0].Name); + Assert.AreEqual("Beta", fields[1].Name); + Assert.AreEqual("Gamma", fields[2].Name); + + // All members keep their explicit last-contract values (Alpha = 0, Beta = 1, Gamma = 2). + Assert.AreEqual(0, (fields[0].InitializationValue as LiteralExpression)?.Literal); + var betaValue = fields[1].InitializationValue as LiteralExpression; + Assert.IsNotNull(betaValue); + Assert.AreEqual(1, betaValue?.Literal); + Assert.AreEqual(2, (fields[2].InitializationValue as LiteralExpression)?.Literal); + } + + // Validates that a value removed from a long-backed integer enum is re-added with its exact + // (potentially > int.MaxValue) numeric value preserved from the last contract. + [Test] + public async Task BackCompat_LongEnumRemovedValueReadded() + { + await MockHelpers.LoadMockGeneratorAsync( + createCSharpTypeCore: (inputType) => typeof(long), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // Last contract: Small = 0, Large = 5000000000 (> int.MaxValue). Current removes "Large". + var input = InputFactory.Int64Enum("mockInputEnum", [ + ("Small", 0), + ]); + + var enumType = EnumProvider.Create(input); + Assert.IsFalse(enumType is ApiVersionEnumProvider); + + enumType.EnsureBuilt(); + enumType.ProcessTypeForBackCompatibility(); + var fields = enumType.Fields; Assert.AreEqual(2, fields.Count); - // Order should be preserved from last contract for members that still exist + Assert.AreEqual("Small", fields[0].Name); + Assert.AreEqual("Large", fields[1].Name); + + // The shared member keeps its explicit last-contract value (Small = 0, as a long). + Assert.AreEqual(0L, (fields[0].InitializationValue as LiteralExpression)?.Literal); + + // The re-added member preserves its long value exactly (not truncated to int). + var largeValue = fields[1].InitializationValue as LiteralExpression; + Assert.IsNotNull(largeValue); + Assert.AreEqual(5000000000L, largeValue?.Literal); + Assert.IsInstanceOf(largeValue?.Literal); + } + + // Validates that removed members are NOT re-added for string-backed enums, because the + // serialized wire value is not recoverable from the last contract (only the C# ordinal is). + [Test] + public async Task BackCompat_StringEnumRemovedValueNotReadded() + { + await MockHelpers.LoadMockGeneratorAsync( + createCSharpTypeCore: (inputType) => typeof(string), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // Last contract: Recover, Default, Third. Current input removes "Third". + var input = InputFactory.StringEnum("mockInputEnum", [ + ("Default", "default"), + ("Recover", "recover"), + ]); + + var enumType = EnumProvider.Create(input); + Assert.IsFalse(enumType is ApiVersionEnumProvider); + + enumType.EnsureBuilt(); + enumType.ProcessTypeForBackCompatibility(); + + var fields = enumType.Fields; + + // "Third" must NOT be re-added for a string-based enum; only the two current members remain. + Assert.AreEqual(2, fields.Count); Assert.AreEqual("Recover", fields[0].Name); Assert.AreEqual("Default", fields[1].Name); + Assert.IsFalse(fields.Any(f => f.Name == "Third")); - // No explicit initialization values - compiler auto-assigns based on order + // String-based enum members never carry an explicit initialization value. Assert.IsNull(fields[0].InitializationValue); Assert.IsNull(fields[1].InitializationValue); } + // Validates that a removed integer enum member is NOT re-added when its removal is accepted + // in the ApiCompat baseline (here recorded as an EnumValuesMustMatch suppression), so the + // generator honors the intentional removal instead of resurrecting it. + [Test] + public async Task BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts() + { + var baseline = Helpers.GetApiCompatBaselineFromFile(); + + await MockHelpers.LoadMockGeneratorAsync( + createCSharpTypeCore: (inputType) => typeof(int), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync(), + apiCompatBaseline: baseline); + + // Last contract: Recover, Default, Third. Current input removes "Third", but the baseline + // accepts that removal, so it must NOT be re-added. + var input = InputFactory.Int32Enum("mockInputEnum", [ + ("Default", 0), + ("Recover", 1), + ]); + + var enumType = EnumProvider.Create(input); + Assert.IsFalse(enumType is ApiVersionEnumProvider); + + enumType.EnsureBuilt(); + enumType.ProcessTypeForBackCompatibility(); + + var fields = enumType.Fields; + Assert.AreEqual(2, fields.Count); + Assert.IsFalse(fields.Any(f => f.Name == "Third")); + Assert.AreEqual("Recover", fields[0].Name); + Assert.AreEqual("Default", fields[1].Name); + + // The surviving shared members keep their explicit last-contract values (Recover = 0, Default = 1). + Assert.AreEqual(0, (fields[0].InitializationValue as LiteralExpression)?.Literal); + Assert.AreEqual(1, (fields[1].InitializationValue as LiteralExpression)?.Literal); + } + // Validates that string enum order is also preserved from last contract [Test] public async Task BackCompat_StringEnumOrderPreserved() diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumNonContiguousValuesPreserved/MockInputEnum.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumNonContiguousValuesPreserved/MockInputEnum.cs new file mode 100644 index 00000000000..6a1ca3ff51b --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumNonContiguousValuesPreserved/MockInputEnum.cs @@ -0,0 +1,11 @@ +#nullable disable + +namespace Sample.Models +{ + public enum MockInputEnum + { + OneHundred = 100, + TwoHundred = 200, + FiveHundred = 500, + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedMiddleValueReadded/MockInputEnum.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedMiddleValueReadded/MockInputEnum.cs new file mode 100644 index 00000000000..4c8303d7c42 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedMiddleValueReadded/MockInputEnum.cs @@ -0,0 +1,11 @@ +#nullable disable + +namespace Sample.Models +{ + public enum MockInputEnum + { + Alpha = 0, + Beta = 1, + Gamma = 2, + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts.txt b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts.txt new file mode 100644 index 00000000000..6902bc71e1e --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts.txt @@ -0,0 +1,3 @@ +# The Third enum member was intentionally removed during migration; suppress the difference so the +# back-compat system honors the removal instead of re-adding it. +EnumValuesMustMatch : Enum value 'Sample.Models.MockInputEnum Sample.Models.MockInputEnum.Third' is (System.Int32)2 in the implementation but (System.Int32)2 in the contract. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumValueRemoved/MockInputEnum.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts/MockInputEnum.cs similarity index 100% rename from packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumValueRemoved/MockInputEnum.cs rename to packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts/MockInputEnum.cs diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueReadded/MockInputEnum.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueReadded/MockInputEnum.cs new file mode 100644 index 00000000000..e93e6f85a4a --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueReadded/MockInputEnum.cs @@ -0,0 +1,11 @@ +#nullable disable + +namespace Sample.Models +{ + public enum MockInputEnum + { + Recover = 0, + Default = 1, + Third = 2, + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_LastContractExposesEnumMemberValues/MockInputEnum.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_LastContractExposesEnumMemberValues/MockInputEnum.cs new file mode 100644 index 00000000000..6a1ca3ff51b --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_LastContractExposesEnumMemberValues/MockInputEnum.cs @@ -0,0 +1,11 @@ +#nullable disable + +namespace Sample.Models +{ + public enum MockInputEnum + { + OneHundred = 100, + TwoHundred = 200, + FiveHundred = 500, + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_LongEnumRemovedValueReadded/MockInputEnum.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_LongEnumRemovedValueReadded/MockInputEnum.cs new file mode 100644 index 00000000000..507bb126c3f --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_LongEnumRemovedValueReadded/MockInputEnum.cs @@ -0,0 +1,10 @@ +#nullable disable + +namespace Sample.Models +{ + public enum MockInputEnum : long + { + Small = 0, + Large = 5000000000, + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_StringEnumRemovedValueNotReadded/MockInputEnum.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_StringEnumRemovedValueNotReadded/MockInputEnum.cs new file mode 100644 index 00000000000..e93e6f85a4a --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_StringEnumRemovedValueNotReadded/MockInputEnum.cs @@ -0,0 +1,11 @@ +#nullable disable + +namespace Sample.Models +{ + public enum MockInputEnum + { + Recover = 0, + Default = 1, + Third = 2, + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index 45d92358f20..208089106c1 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -243,7 +243,7 @@ public async Task BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalPara // The last contract published GetData(int param1); the current generation adds an optional // non-body parameter param2. var param1 = new ParameterProvider("param1", $"", new CSharpType(typeof(int))); - var param2 = new ParameterProvider("param2", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default); + var param2 = new ParameterProvider("param2", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default, location: ParameterLocation.Query); var getData = new MethodProvider( new MethodSignature("GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [param1, param2]), Snippet.Return(Snippet.Null), @@ -293,6 +293,103 @@ public void FindPreviousParameterNameReturnsNullWithoutLastContract() Assert.IsNull(BackCompatHelper.FindPreviousParameterName(typeProvider.LastContractView, "oldParam", "Foo")); } + [Test] + public async Task TryRestorePreviousParameterOrderMatchesNonCanonicalParameterNames() + { + await MockHelpers.LoadMockGeneratorAsync(); + + // Current generation declares Foo(second_param, first_param) — the reverse of the + // previous Foo(first_param, second_param). The raw names are snake_case, so matching + // requires normalizing both sides via ToVariableName. + var firstParam = new ParameterProvider("first_param", $"", new CSharpType(typeof(string))); + var secondParam = new ParameterProvider("second_param", $"", new CSharpType(typeof(int))); + var currentFoo = new MethodProvider( + new MethodSignature("Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [secondParam, firstParam]), + Snippet.Return(Snippet.Null), + new TestTypeProvider()); + + var previousSignature = new MethodSignature( + "Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", + [ + new ParameterProvider("first_param", $"", new CSharpType(typeof(string))), + new ParameterProvider("second_param", $"", new CSharpType(typeof(int))), + ]); + + var reordered = BackCompatHelper.TryRestorePreviousParameterOrder(currentFoo, previousSignature); + + Assert.IsTrue(reordered); + CollectionAssert.AreEqual( + new[] { "first_param", "second_param" }, + currentFoo.Signature.Parameters.Select(p => p.Name).ToArray()); + } + + // Validates that the default-value preservation performed while reordering also matches by + // the normalized parameter identifier, restoring the previously-published default onto the + // current parameter even when the raw name is snake_case. + [Test] + public async Task TryRestorePreviousParameterOrderPreservesPreviousDefaultForNonCanonicalNames() + { + await MockHelpers.LoadMockGeneratorAsync(); + + // Current declares Foo(second_param = default, first_param) — reverse of previous order. + var firstParam = new ParameterProvider("first_param", $"", new CSharpType(typeof(string))); + var secondParam = new ParameterProvider("second_param", $"", new CSharpType(typeof(int)), defaultValue: Snippet.Default); + var currentFoo = new MethodProvider( + new MethodSignature("Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [secondParam, firstParam]), + Snippet.Return(Snippet.Null), + new TestTypeProvider()); + + // The previously-published second_param carried a distinct default representation. + var previousDefault = Snippet.Literal(0); + var previousSignature = new MethodSignature( + "Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", + [ + new ParameterProvider("first_param", $"", new CSharpType(typeof(string))), + new ParameterProvider("second_param", $"", new CSharpType(typeof(int)), defaultValue: previousDefault), + ]); + + var reordered = BackCompatHelper.TryRestorePreviousParameterOrder(currentFoo, previousSignature); + + Assert.IsTrue(reordered); + CollectionAssert.AreEqual( + new[] { "first_param", "second_param" }, + currentFoo.Signature.Parameters.Select(p => p.Name).ToArray()); + + // The current second_param's default was restored from the previous contract. + var restoredSecond = currentFoo.Signature.Parameters.Single(p => p.Name == "second_param"); + Assert.AreSame(previousDefault, restoredSecond.DefaultValue); + } + + // Validates that a new optional parameter with an Unknown location is not treated as a + // non-body parameter, so no back-compat overload is produced for it. + [Test] + public void HasNewOptionalNonBodyParametersOnlyRejectsUnknownLocation() + { + var previousSignature = new MethodSignature( + "GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", + [new ParameterProvider("param1", $"", new CSharpType(typeof(int)))]); + + // A new optional parameter whose location is Unknown must not qualify. + var unknownLocationSignature = new MethodSignature( + "GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", + [ + new ParameterProvider("param1", $"", new CSharpType(typeof(int))), + new ParameterProvider("param2", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default, location: ParameterLocation.Unknown), + ]); + + Assert.IsFalse(BackCompatHelper.HasNewOptionalNonBodyParametersOnly(previousSignature, unknownLocationSignature)); + + // The same shape with an explicit non-body location does qualify. + var queryLocationSignature = new MethodSignature( + "GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", + [ + new ParameterProvider("param1", $"", new CSharpType(typeof(int))), + new ParameterProvider("param2", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default, location: ParameterLocation.Query), + ]); + + Assert.IsTrue(BackCompatHelper.HasNewOptionalNonBodyParametersOnly(previousSignature, queryLocationSignature)); + } + // Validates that the base BuildMethodsForBackCompatibility default automatically restores a // previously-published parameter name on any derived type — keyed on the parameter's spec // original name — and that the rename propagates into the already-built method body. @@ -351,8 +448,8 @@ public async Task BuildMethodsForBackCompatibilityKeepsUnpublishedParameterName( // When the restored name is used both as an argument (AsArgument -> _asArgument) and // as a variable (-> _asVariable), materializing both cached expressions before the rename, the - // rename must keep them sharing one declaration. Otherwise the writer uniquifies the two - // declarations of the same name into "oldParam0"/"oldParam1", producing non-compiling code. + // rename must keep them sharing one declaration. Otherwise the writer renames the two + // declarations of the same name to "oldParam0"/"oldParam1", producing non-compiling code. [Test] public async Task BuildMethodsForBackCompatibilityRestoredNameDoesNotSplitDeclarations() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs index b50f507ce00..a524a955e4c 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs @@ -92,6 +92,26 @@ public void CountsGenericParametersAsSingleArgument() Assert.IsTrue(baseline.IsMemberSuppressed("Ns.Foo", "Configure", 2)); } + [Test] + public void ParsesMembersMustExistField() + { + var baseline = Helpers.GetApiCompatBaselineFromFile(method: "Baseline"); + + // A removed field/enum member has no parameter list; it is recorded with arity 0. + Assert.IsTrue(baseline.IsMemberSuppressed("Ns.Foo", "LegacyField", 0)); + } + + [Test] + public void ParsesEnumValuesMustMatch() + { + var baseline = Helpers.GetApiCompatBaselineFromFile(method: "Baseline"); + + // An accepted enum value difference suppresses back-compat handling for that member. + Assert.IsTrue(baseline.IsMemberSuppressed("Ns.CapacityLevel", "FiftyThousand", 0)); + // A different member on the same enum must not match. + Assert.IsFalse(baseline.IsMemberSuppressed("Ns.CapacityLevel", "OneHundred", 0)); + } + [Test] public void IgnoresUnknownRulesAndMalformedLines() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.txt b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.txt index f2e71a155c5..52fd15fee3d 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.txt +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.txt @@ -6,3 +6,5 @@ MembersMustExist : Member 'public Ns.Foo..ctor(Ns.Kind, System.String)' does not MembersMustExist : Member 'public Ns.Kind Ns.Foo.Kind.get()' does not exist in the implementation but it does exist in the contract. MembersMustExist : Member 'public System.Void Ns.Foo.Kind.set(Ns.Kind)' does not exist in the implementation but it does exist in the contract. MembersMustExist : Member 'public System.Void Ns.Foo.Configure(System.Collections.Generic.IDictionary, System.String)' does not exist in the implementation but it does exist in the contract. +MembersMustExist : Member 'public Ns.Kind Ns.Foo.LegacyField' does not exist in the implementation but it does exist in the contract. +EnumValuesMustMatch : Enum value 'Ns.CapacityLevel Ns.CapacityLevel.FiftyThousand' is (System.Int32)10 in the implementation but (System.Int32)50000 in the contract. diff --git a/packages/http-client-csharp/generator/docs/backward-compatibility-extensibility.md b/packages/http-client-csharp/generator/docs/backward-compatibility-extensibility.md deleted file mode 100644 index 2a39dcca305..00000000000 --- a/packages/http-client-csharp/generator/docs/backward-compatibility-extensibility.md +++ /dev/null @@ -1,170 +0,0 @@ -# Backward Compatibility Extensibility - -## Problem - -The generator preserves backward compatibility by diffing the current generation against -the last released contract and adjusting members so existing code keeps compiling. Today -this logic is private to each built-in provider (`ClientProvider`, `ModelProvider`, -`ModelFactoryProvider`, `ApiVersionEnumProvider`), so downstream providers that derive -directly from `TypeProvider` — such as the management generator's `ResourceClientProvider` — -cannot reuse it. - -## Solution - -Move the generic algorithms into `TypeProvider` as the **default implementations** of its -back-compat hooks. Any provider then consumes them by inheriting, and customizes by -overriding and calling `base`. On by default; opt out by overriding to return the input -unchanged. - -## Two API families (split by when they run) - -| Family | Shape | Caller / timing | -|---|---|---| -| **`Build…ForBackCompatibility`** | `(IReadOnlyList) → IReadOnlyList` | Framework calls, **after** visitors | -| **`Get…ForBackCompatibility`** | `(member) → value` | **You** call, **while building** a member (before serialization) | - -```csharp -public partial class TypeProvider -{ - public TypeProvider? LastContractView { get; } // already public - - // FAMILY 1 — framework calls after visitors; default applies the standard algorithm. - // Override + call base to extend; override without base to replace. - // Members produced here are NOT re-visited by visitors. - protected internal virtual IReadOnlyList BuildMethodsForBackCompatibility(IEnumerable methods); - protected internal virtual IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable constructors); - protected internal virtual IReadOnlyList? BuildEnumValuesForBackCompatibility(IReadOnlyList enumValues); - - // Materialization seam used by the FAMILY 1 method default. The base builds the synthesized - // overload as a plain MethodProvider whose body delegates to the current method, then hands - // it here. Default returns it unchanged; override to return a provider-specific subtype or - // to apply adjustments (e.g. analyzer suppressions). - protected virtual MethodProvider BuildBackCompatibilityOverload(MethodProvider overload); - - // FAMILY 2 — you call while building; returns the value to use (non-mutating), - // scoped to this type's LastContractView. - protected virtual CSharpType GetPropertyTypeForBackCompatibility(PropertyProvider property); -} -``` - -> The parameter/signature comparison primitives used internally by the default algorithm -> (`MethodSignatureHelper`) are shared source compiled into each generator assembly and remain -> `internal`; downstream providers do not need them because the materialization seam hands them a -> fully-built `MethodProvider`. - -**Defaults:** -- `BuildMethodsForBackCompatibility` — restores previous parameter order; adds a hidden - `[EditorBrowsable(Never)]` overload (delegating to the current method) when new optional - non-body parameters were added. -- `BuildConstructorsForBackCompatibility` — keeps an abstract base type's constructor public - when the last contract exposed a matching public one. -- `BuildEnumValuesForBackCompatibility` — preserves enum members removed since the last contract. -- `GetPropertyTypeForBackCompatibility` — returns the previously published property type when - it changed, else the current type. - -## Pipeline - -```mermaid -flowchart TD - A["1 · EnsureBuilt()
build members + bodies"] --> B["2 · visitors run
(transform members)"] - B --> C["3 · ProcessTypeForBackCompatibility()"] - C --> D["4 · writers emit"] - - A -. "you call" .-> G["Get…ForBackCompatibility
(property type, …)"] - C -. "framework calls" .-> F["Build…ForBackCompatibility
(methods, ctors, enum values)"] - - style G fill:#e8f0fe,stroke:#4285f4 - style F fill:#e6f4ea,stroke:#34a853 -``` - -- **`Get…` is build-time (step 1)** because its value feeds the body/serialization built in - the same step. Running it later would desync a property's declared type from its already- - generated serialization. -- **`Build…` is post-visitor (step 3)** because its transforms are safe to apply last - (reorder is cosmetic, new overloads get a fresh body, modifier/enum changes are - declaration-only) and shouldn't be rewritten by visitors. Members it produces are not - re-visited. - -A `Build…` hook only acts when `LastContractView` has those members and a real diff exists, -so enabling defaults everywhere is safe. - -## Built-in providers after consolidation - -| Provider | Change | -|---|---| -| `ClientProvider` | Inherits the base `BuildMethodsForBackCompatibility` algorithm; keeps a slim override only for SCM-specific concerns (re-typing the synthesized overload as `ScmMethodProvider` + AZC0002 via `BuildBackCompatibilityOverload`, and fixing convenience-method bodies that call reordered protocol methods). | -| `ModelProvider` | Ctor logic → base hook; property-type logic → `GetPropertyTypeForBackCompatibility` (called from `BuildProperties`). | -| `ModelFactoryProvider` | **Keeps its override** (multi-strategy factory logic differs). | -| `ApiVersionEnumProvider` | Enum logic → base hook; **inherits**. | - -## Downstream usage (management generator) - -`ResourceClientProvider` derives from `TypeProvider`, so it inherits all the defaults. - -### 1. Standard method back-compat — free, no code - -Resource operation methods automatically get parameter-order restoration and hidden -overloads for newly added optional parameters: - -```csharp -public partial class ResourceClientProvider : TypeProvider -{ - // No override needed. -} -``` - -### 2. Materialize the synthesized overload as a provider-specific type - -Override the seam to return your own `MethodProvider` subtype or to apply adjustments. The -base has already built the hidden, delegating overload — you only re-wrap or tweak it. (This -is exactly how `ClientProvider` returns an `ScmMethodProvider` and adds its AZC0002 -suppression.) - -```csharp -public partial class ResourceClientProvider : TypeProvider -{ - protected override MethodProvider BuildBackCompatibilityOverload(MethodProvider overload) - { - // overload.Signature is hidden ([EditorBrowsable(Never)], defaults stripped) and - // overload.BodyStatements already delegates to the current method. - return new MyResourceMethodProvider(overload.Signature, overload.BodyStatements!, this, overload.XmlDocs); - } -} -``` - -### 3. Extend or filter the produced method list - -Override `BuildMethodsForBackCompatibility`, call `base` to keep the standard shims, then add -or filter your own: - -```csharp -public partial class ResourceClientProvider : TypeProvider -{ - protected override IReadOnlyList BuildMethodsForBackCompatibility( - IEnumerable methods) - { - var result = base.BuildMethodsForBackCompatibility(methods); // standard shims first - // ...inspect LastContractView and add/adjust resource-specific methods... - return result; - } -} -``` - -The default synthesized overload delegates to the current method, so it works even for -management LRO methods (it forwards to the method that wraps the result in `ArmOperation`). - -### 4. Preserve a property type while building it (build-time) - -Call the getter while constructing the property, before serialization is generated: - -```csharp -var property = TypeFactory.CreateProperty(inputProperty, this); -property.Type = GetPropertyTypeForBackCompatibility(property); // previous type wins if it changed -``` - -## Deferred - -Build-time **parameter-name preservation** (`top`→`maxCount`, page-size casing, -`@@clientName` renames) and **content-type-before-body ordering** stay in `RestClientProvider` -for now. When exposed, they join the `Get…` family as -`GetParameterNameForBackCompatibility(ParameterProvider, MethodProvider)`. diff --git a/packages/http-client-csharp/generator/docs/backward-compatibility.md b/packages/http-client-csharp/generator/docs/backward-compatibility.md index 75a6fe512ee..6ca83258649 100644 --- a/packages/http-client-csharp/generator/docs/backward-compatibility.md +++ b/packages/http-client-csharp/generator/docs/backward-compatibility.md @@ -12,6 +12,10 @@ - [New Property Added Together with a Rename](#scenario-new-property-added-together-with-a-rename) - [Model Properties](#model-properties) - [AdditionalProperties Type Preservation](#additionalproperties-type-preservation) + - [Fixed Enum Members](#fixed-enum-members) + - [Explicit (Non-contiguous) Values Preserved](#scenario-explicit-non-contiguous-values-preserved) + - [Removed Integer Enum Member Re-added](#scenario-removed-integer-enum-member-re-added) + - [Baseline-Accepted Removal Honored](#scenario-baseline-accepted-removal-honored) - [API Version Enum](#api-version-enum) - [Non-abstract Base Models](#non-abstract-base-models) - [Model Constructors](#model-constructors) @@ -42,13 +46,14 @@ When generating code, the generator can optionally receive a compiled assembly f Sometimes a breaking change (such as removing a model or a model factory method) is intentional and has already been reviewed and accepted. In the Azure SDK, such accepted breaking changes are recorded in an [ApiCompat](https://github.com/dotnet/sdk/tree/main/src/Compatibility) baseline (suppression) file, conventionally located at `eng/apicompatbaselines/.txt`. -Without awareness of these files, the back-compat system would resurrect every member present in the last contract — re-introducing the very API that was intentionally removed (and potentially referencing types that no longer exist). To prevent this, the generator discovers the baseline file by walking up from the project directory and parses its `TypesMustExist` and `MembersMustExist` suppressions. The back-compat code then consults the baseline in two ways: +Without awareness of these files, the back-compat system would resurrect every member present in the last contract — re-introducing the very API that was intentionally removed (and potentially referencing types that no longer exist). To prevent this, the generator discovers the baseline file by walking up from the project directory and parses its `TypesMustExist`, `MembersMustExist`, and `EnumValuesMustMatch` suppressions. The back-compat code then consults the baseline in the following ways: - **Skipping resurrected members:** before regenerating a compatibility shim for a removed member, it checks whether the removal has been accepted in the baseline (matched by declaring-type full name, member name, and parameter count) and, if so, skips it. - **Allowing property type changes:** the back-compat system normally preserves a property's previous (last-contract) type to avoid a breaking change. When that previous type — or a type nested within it (e.g. the element type of a collection) — has itself been intentionally removed and accepted in the baseline, preserving it would reference a now-deleted type. In that case the generator allows the property to take its current (spec) type instead. +- **Skipping re-added enum members:** before re-adding a fixed enum member that exists in the last contract but was dropped from the current spec, it checks whether that removal — or an accepted value difference recorded as `EnumValuesMustMatch` — has been accepted in the baseline (matched by the declaring enum's fully-qualified name and the member name) and, if so, omits the member. > [!NOTE] -> Baseline awareness is currently wired into the model factory back-compat path (`ModelFactoryProvider`) and the model property type-preservation path (`ModelProvider`). The other back-compat consumers (`ModelProvider` constructors, the enum providers, `ClientProvider`, and `RestClientProvider`) can be made baseline-aware in the same way as a follow-up. +> Baseline awareness is currently wired into the base method back-compat path in `TypeProvider` (the parameter reorder/removal loop, inherited by `ClientProvider` and also used directly by `ModelFactoryProvider`), the model property type-preservation path (`ModelProvider`), and the fixed enum member path (`FixedEnumProvider`). The remaining back-compat consumers (`ModelProvider` constructors and `RestClientProvider`) can be made baseline-aware in the same way as a follow-up. ## Supported Scenarios @@ -363,6 +368,85 @@ public partial class MyModel - For object types, serialization uses `Utf8JsonWriter.WriteObjectValue()` to handle arbitrary values - Binary compatibility is fully maintained - existing client code continues to work without recompilation +### Fixed Enum Members + +Fixed enums (C# `enum` types) preserve their previously shipped members and values by comparing the current spec against the last contract. Because the underlying value of an integer-backed enum member is part of its public API, the generator keeps those values stable across regenerations, aligns member order to the last contract, and re-adds members that were dropped from the current spec. + +> [!NOTE] +> These behaviors apply to **integer-backed** fixed enums (`int`/`long`). + +#### Scenario: Explicit (Non-contiguous) Values Preserved + +**Description:** When an integer enum's members carry explicit, non-contiguous values, the generator preserves each member's exact value from the last contract instead of reassigning positional ordinals. + +**Example:** + +Previous version (GA surface): + +```csharp +public enum CapacityReservationLevel +{ + OneHundred = 100, + TwoHundred = 200, + FiveHundred = 500, +} +``` + +**Generated Result:** The members keep their exact values (`100`, `200`, `500`) rather than being reassigned to `0`, `1`, `2`. + +#### Scenario: Removed Integer Enum Member Re-added + +**Description:** When an integer enum member present in the last contract is dropped from the current spec, the generator re-adds it — at its original position and with its original explicit value — to avoid removing a previously shipped member. + +**Example:** + +Previous version: + +```csharp +public enum SampleEnum +{ + Alpha = 0, + Beta = 1, + Gamma = 2, +} +``` + +Current TypeSpec removes `Beta`: + +```csharp +public enum SampleEnum +{ + Alpha = 0, + Gamma = 2, +} +``` + +**Generated Result:** `Beta` is re-added at its original position with its original value: + +```csharp +public enum SampleEnum +{ + Alpha = 0, + Beta = 1, + Gamma = 2, +} +``` + +**Key Points:** + +- Members shared between the current spec and the last contract are ordered to match the last contract and keep their last-contract values +- New members introduced by the current spec are appended after the last-contract members, keeping their spec values +- The re-added member's underlying value is read from the previously published assembly's metadata (no debug symbols required) + +#### Scenario: Baseline-Accepted Removal Honored + +**Description:** When the removal of an enum member has been intentionally accepted in the [ApiCompat baseline](#apicompat-baseline-awareness) — recorded as a `MembersMustExist` removal or an `EnumValuesMustMatch` value-difference suppression — the generator honors that decision and does **not** re-add the member. + +**Key Points:** + +- Suppressed members are matched by the declaring enum's fully-qualified name and the member name +- This lets a library intentionally drop a previously shipped enum member once the removal is reviewed and recorded in the baseline + ### API Version Enum Service version enums maintain backward compatibility by preserving version values from previous releases. From a03e3115550616a2658afab2b4b0b0adcfed5815 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Mon, 13 Jul 2026 17:02:12 -0500 Subject: [PATCH 4/4] pr feedback --- .../src/Providers/FixedEnumProvider.cs | 11 ++- .../src/Utilities/BackCompatHelper.cs | 15 +++- .../EnumProviders/EnumProviderTests.cs | 42 ++++++++++ ...ChangedValueHonoredWhenBaselineAccepts.txt | 4 + .../MockInputEnum.cs | 10 +++ ...ovedValueNotReaddedWhenBaselineAccepts.txt | 5 +- ...dsStaticOverloadForNewOptionalParameter.cs | 22 +++++ .../StaticOverloadType.cs | 7 ++ ...AddsVoidOverloadForNewOptionalParameter.cs | 21 +++++ .../VoidOverloadType.cs | 7 ++ ...kipsOverloadForBaselineAcceptedRemoval.txt | 1 + .../SkipOverloadType.cs | 7 ++ .../test/Providers/TypeProviderTests.cs | 80 +++++++++++++++++++ 13 files changed, 224 insertions(+), 8 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts.txt create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts/MockInputEnum.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter/StaticOverloadType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter/VoidOverloadType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval.txt create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval/SkipOverloadType.cs diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/FixedEnumProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/FixedEnumProvider.cs index 31cdabe64f8..854ea4a6b6a 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/FixedEnumProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/FixedEnumProvider.cs @@ -129,11 +129,14 @@ protected override IReadOnlyList BuildEnumValues() { if (currentLookup.TryGetValue(field.Name, out var existingMember)) { - // Preserve the last contract's explicit value for integer enums so members keep - // their exact values. - ValueExpression? initializationValue = null; + // By default, preserve the last contract's explicit value for integer enums so + // members keep their exact values. If the baseline accepts a value change for this + // member (EnumValuesMustMatch), honor the current value instead of restoring the old. + ValueExpression? initializationValue = existingMember.Field.InitializationValue; var memberValue = existingMember.Value; - if (IsIntValueType && field.InitializationValue is LiteralExpression { Literal: { } lastContractValue }) + if (IsIntValueType + && field.InitializationValue is LiteralExpression { Literal: { } lastContractValue } + && CodeModelGenerator.Instance.SourceInputModel?.ApiCompatBaseline.IsMemberSuppressed(Type.FullyQualifiedName, field.Name, 0) != true) { initializationValue = field.InitializationValue; memberValue = lastContractValue; diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs index d44de16d724..72bcebb2ba8 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs @@ -393,7 +393,7 @@ public static void AddOverloadsForNewOptionalParameters(TypeProvider enclosingTy } } - if (previousStillExists || matchedCurrent is null) + if (previousStillExists || matchedCurrent is null || IsMethodRemovalAcceptedInBaseline(enclosingType, previousSignature)) { continue; } @@ -486,11 +486,22 @@ private static MethodProvider BuildNewOptionalParameterOverload( arguments.Add(PositionalReference(variableName, value)); } + var invocationTarget = currentSignature.Modifiers.HasFlag(MethodSignatureModifiers.Static) + ? Static(enclosingType.Type) + : This; + var delegatingCall = invocationTarget.Invoke(currentSignature.Name, arguments); + MethodBodyStatement body = IsVoidReturnType(currentSignature.ReturnType) + ? delegatingCall.Terminate() + : Return(delegatingCall); + return new MethodProvider( MethodSignatureHelper.BuildBackCompatMethodSignature(previousSignature, hideMethod: true), - Return(This.Invoke(currentSignature.Name, arguments)), + body, enclosingType, previousMethod.XmlDocs); } + + private static bool IsVoidReturnType(CSharpType? returnType) => + returnType is null || (returnType.IsFrameworkType && returnType.FrameworkType == typeof(void)); } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs index dd2d2fc63e3..fe18ff059a3 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs @@ -681,6 +681,48 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual(1, (fields[1].InitializationValue as LiteralExpression)?.Literal); } + // Validates that when a shared integer enum member's value was intentionally changed and the + // baseline accepts it (recorded as an EnumValuesMustMatch suppression), back-compat honors the + // CURRENT value instead of restoring the old last-contract value. + [Test] + public async Task BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts() + { + var baseline = Helpers.GetApiCompatBaselineFromFile(); + + await MockHelpers.LoadMockGeneratorAsync( + createCSharpTypeCore: (inputType) => typeof(int), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync(), + apiCompatBaseline: baseline); + + // Last contract: Recover = 0, Default = 5. Current changes Default to 1; the baseline accepts + // the value change, so back-compat must keep the current value (1) rather than restoring 5. + var input = InputFactory.Int32Enum("mockInputEnum", [ + ("Default", 1), + ("Recover", 0), + ]); + + var enumType = EnumProvider.Create(input); + Assert.IsFalse(enumType is ApiVersionEnumProvider); + + enumType.EnsureBuilt(); + enumType.ProcessTypeForBackCompatibility(); + + var fields = enumType.Fields; + Assert.AreEqual(2, fields.Count); + + // Order is preserved from the last contract: Recover first, Default second. + Assert.AreEqual("Recover", fields[0].Name); + Assert.AreEqual("Default", fields[1].Name); + + // Recover was unchanged, so it keeps its (identical) value of 0. + Assert.AreEqual(0, (fields[0].InitializationValue as LiteralExpression)?.Literal); + + // Default's accepted value change is honored: the CURRENT value (1) is kept, not the old (5). + var defaultValue = fields[1].InitializationValue as LiteralExpression; + Assert.IsNotNull(defaultValue); + Assert.AreEqual(1, defaultValue?.Literal); + } + // Validates that string enum order is also preserved from last contract [Test] public async Task BackCompat_StringEnumOrderPreserved() diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts.txt b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts.txt new file mode 100644 index 00000000000..ea63b24a4c6 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts.txt @@ -0,0 +1,4 @@ +# The Default member's value was intentionally changed from 5 (contract) to 1 (implementation). +# Accepting the difference in the baseline means back-compat must honor the new value instead of +# restoring the old one. +EnumValuesMustMatch : Enum value 'Sample.Models.MockInputEnum Sample.Models.MockInputEnum.Default' is (System.Int32)1 in the implementation but (System.Int32)5 in the contract. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts/MockInputEnum.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts/MockInputEnum.cs new file mode 100644 index 00000000000..cc5d8599434 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumChangedValueHonoredWhenBaselineAccepts/MockInputEnum.cs @@ -0,0 +1,10 @@ +#nullable disable + +namespace Sample.Models +{ + public enum MockInputEnum + { + Recover = 0, + Default = 5, + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts.txt b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts.txt index 6902bc71e1e..a49039dcdac 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts.txt +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/TestData/EnumProviderTests/BackCompat_IntEnumRemovedValueNotReaddedWhenBaselineAccepts.txt @@ -1,3 +1,4 @@ # The Third enum member was intentionally removed during migration; suppress the difference so the -# back-compat system honors the removal instead of re-adding it. -EnumValuesMustMatch : Enum value 'Sample.Models.MockInputEnum Sample.Models.MockInputEnum.Third' is (System.Int32)2 in the implementation but (System.Int32)2 in the contract. +# back-compat system honors the removal instead of re-adding it. ApiCompat reports a removed enum +# member as a MembersMustExist difference. +MembersMustExist : Member 'public Sample.Models.MockInputEnum Sample.Models.MockInputEnum.Third' does not exist in the implementation but it does exist in the contract. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter.cs new file mode 100644 index 00000000000..4c768fd2b46 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter.cs @@ -0,0 +1,22 @@ +// + +#nullable disable + +using System.ComponentModel; + +namespace Test +{ + public partial class StaticOverloadType + { + public static string GetData(int param1, bool param2 = default) + { + return null; + } + + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Never)] + public static string GetData(int param1) + { + return global::Test.StaticOverloadType.GetData(param1: param1, param2: default); + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter/StaticOverloadType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter/StaticOverloadType.cs new file mode 100644 index 00000000000..514ea6bdb9c --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter/StaticOverloadType.cs @@ -0,0 +1,7 @@ +namespace Test +{ + public class StaticOverloadType + { + public static string GetData(int param1) => null; + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter.cs new file mode 100644 index 00000000000..027f0f2b3cd --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter.cs @@ -0,0 +1,21 @@ +// + +#nullable disable + +using System.ComponentModel; + +namespace Test +{ + public partial class VoidOverloadType + { + public void DoWork(int param1, bool param2 = default) + { + } + + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Never)] + public void DoWork(int param1) + { + this.DoWork(param1: param1, param2: default); + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter/VoidOverloadType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter/VoidOverloadType.cs new file mode 100644 index 00000000000..e4890ff568f --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter/VoidOverloadType.cs @@ -0,0 +1,7 @@ +namespace Test +{ + public class VoidOverloadType + { + public void DoWork(int param1) { } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval.txt b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval.txt new file mode 100644 index 00000000000..ec668690582 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval.txt @@ -0,0 +1 @@ +MembersMustExist : Member 'public System.String Test.SkipOverloadType.GetData(System.Int32)' does not exist in the implementation but it does exist in the contract. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval/SkipOverloadType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval/SkipOverloadType.cs new file mode 100644 index 00000000000..1e1b8d634c4 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval/SkipOverloadType.cs @@ -0,0 +1,7 @@ +namespace Test +{ + public class SkipOverloadType + { + public string GetData(int param1) => null; + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index 208089106c1..5c2d2a087d8 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -257,6 +257,86 @@ public async Task BuildMethodsForBackCompatibilityAddsOverloadForNewOptionalPara Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); } + // TypeProvider: a STATIC method that gained an optional non-body parameter gets a hidden overload + // whose body delegates through the declaring type (not `this`), since a static method cannot use + // an instance receiver. + [Test] + public async Task BuildMethodsForBackCompatibilityAddsStaticOverloadForNewOptionalParameter() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // The last contract published static GetData(int param1); the current generation adds an + // optional non-body parameter param2. + var param1 = new ParameterProvider("param1", $"", new CSharpType(typeof(int))); + var param2 = new ParameterProvider("param2", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default, location: ParameterLocation.Query); + var getData = new MethodProvider( + new MethodSignature("GetData", $"", MethodSignatureModifiers.Public | MethodSignatureModifiers.Static, new CSharpType(typeof(string)), $"", [param1, param2]), + Snippet.Return(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "StaticOverloadType", ns: "Test", methods: [getData]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // TypeProvider: a VOID method that gained an optional non-body parameter gets a hidden overload + // whose body invokes the current method as a statement (no `return`), since a void method has no + // value to return. + [Test] + public async Task BuildMethodsForBackCompatibilityAddsVoidOverloadForNewOptionalParameter() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // The last contract published void DoWork(int param1); the current generation adds an + // optional non-body parameter param2. + var param1 = new ParameterProvider("param1", $"", new CSharpType(typeof(int))); + var param2 = new ParameterProvider("param2", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default, location: ParameterLocation.Query); + var doWork = new MethodProvider( + new MethodSignature("DoWork", $"", MethodSignatureModifiers.Public, null, $"", [param1, param2]), + MethodBodyStatement.Empty, + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "VoidOverloadType", ns: "Test", methods: [doWork]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // TypeProvider: when a previous signature's removal is accepted in the ApiCompat baseline, the + // optional-parameter overload pass must NOT resurrect it even though the replacement method added + // optional parameters. + [Test] + public async Task BuildMethodsForBackCompatibilitySkipsOverloadForBaselineAcceptedRemoval() + { + var baseline = Helpers.GetApiCompatBaselineFromFile(); + + await MockHelpers.LoadMockGeneratorAsync( + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync(), + apiCompatBaseline: baseline); + + // The last contract published GetData(int param1); the current generation adds an optional + // param2. The baseline accepts the removal of GetData(int), so no overload must be added. + var param1 = new ParameterProvider("param1", $"", new CSharpType(typeof(int))); + var param2 = new ParameterProvider("param2", $"", new CSharpType(typeof(bool)), defaultValue: Snippet.Default, location: ParameterLocation.Query); + var getData = new MethodProvider( + new MethodSignature("GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [param1, param2]), + Snippet.Return(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "SkipOverloadType", ns: "Test", methods: [getData]); + + typeProvider.ProcessTypeForBackCompatibility(); + + // Only the current method remains; the accepted removal is not resurrected as an overload. + Assert.AreEqual(1, typeProvider.Methods.Count); + Assert.AreEqual(2, typeProvider.Methods[0].Signature.Parameters.Count); + } + // Validates the shared lookup that any provider can use to restore a previously-published // parameter name from its last contract. [Test]