From aa710958da15c7dcb2e591201f15533d7aa18754 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Mon, 15 Jun 2026 12:27:39 -0500 Subject: [PATCH 1/2] 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 f8ee8744e32..d7e3c605229 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 @@ -1770,28 +1679,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 430b3fbd6eb..395155335f7 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 @@ -37,7 +37,7 @@ public void Update(CSharpType? type = null, string? name = null, bool? isRef = n } if (name != null) { - Declaration = new CodeWriterDeclaration(name); + Declaration.Update(name: name); } if (isRef != null) { 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 82c7bf1ce6ed0338e5c85132248af6df00556bb2 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Mon, 6 Jul 2026 15:11:07 -0500 Subject: [PATCH 2/2] add non-optional param overload --- .../eng/scripts/RegenPreview.ps1 | 4 + .../eng/scripts/RegenPreview.psm1 | 17 +- .../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 +++ 11 files changed, 267 insertions(+), 177 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/eng/scripts/RegenPreview.ps1 b/packages/http-client-csharp/eng/scripts/RegenPreview.ps1 index 5ae94403980..8c59d618a39 100644 --- a/packages/http-client-csharp/eng/scripts/RegenPreview.ps1 +++ b/packages/http-client-csharp/eng/scripts/RegenPreview.ps1 @@ -843,6 +843,10 @@ try { $tempDir = Join-Path $engFolder "temp-package-update" New-Item -ItemType Directory -Path $tempDir -Force | Out-Null + # Write a local .npmrc that points the default registry at the public + # dev feed. + Set-Content (Join-Path $tempDir ".npmrc") "registry=https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-js/npm/registry/`n" -Encoding utf8 + # Helper function to update emitter package files $updateEmitterPackage = { param($PackagePath, $EmitterJsonName, $LockJsonName) diff --git a/packages/http-client-csharp/eng/scripts/RegenPreview.psm1 b/packages/http-client-csharp/eng/scripts/RegenPreview.psm1 index 03d43effaae..6ab640ff923 100644 --- a/packages/http-client-csharp/eng/scripts/RegenPreview.psm1 +++ b/packages/http-client-csharp/eng/scripts/RegenPreview.psm1 @@ -82,23 +82,25 @@ function Update-GeneratorPackage { } # Step 2: Install dependencies, clean, and build + # Force the default registry to the public azure-sdk-for-js feed. + $publicRegistry = "https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-js/npm/registry/" Push-Location $GeneratorPath try { Write-Host "Installing dependencies..." -ForegroundColor Gray if ($UseNpmCi) { - $installOutput = & npm install --package-lock-only 2>&1 + $installOutput = & npm install --package-lock-only --registry $publicRegistry 2>&1 if ($LASTEXITCODE -ne 0) { Write-Host $installOutput -ForegroundColor Red throw "Failed to update package-lock.json" } - $ciOutput = & npm ci 2>&1 + $ciOutput = & npm ci --registry $publicRegistry 2>&1 if ($LASTEXITCODE -ne 0) { Write-Host $ciOutput -ForegroundColor Red throw "Failed to install dependencies" } } else { - $installOutput = & npm install 2>&1 + $installOutput = & npm install --registry $publicRegistry 2>&1 if ($LASTEXITCODE -ne 0) { Write-Host $installOutput -ForegroundColor Red throw "Failed to run npm install" @@ -261,6 +263,11 @@ function Update-MgmtGenerator { $tempDir = Join-Path $EngFolder "temp-mgmt-package-update" New-Item -ItemType Directory -Path $tempDir -Force | Out-Null + # Point the default registry at the public azure-sdk-for-js feed so dependency + # resolution doesn't fall back to the authenticated machine-global proxy + # (packagefeedproxy), which fails with E401 for @typespec/@azure-tools packages. + Set-Content (Join-Path $tempDir ".npmrc") "registry=https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-js/npm/registry/`n" -Encoding utf8 + try { $tempPackageJson = Join-Path $tempDir "package.json" @@ -901,7 +908,9 @@ function Update-AzureSpectorScenarios { Write-Host "Installing dependencies..." -ForegroundColor Gray Push-Location $AzureGeneratorPath try { - $installOutput = & npm install 2>&1 + # Use the public azure-sdk-for-js feed to avoid the authenticated + # machine-global proxy (packagefeedproxy) failing on uncached deps. + $installOutput = & npm install --registry "https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-js/npm/registry/" 2>&1 if ($LASTEXITCODE -ne 0) { Write-Host $installOutput -ForegroundColor Red throw "npm install failed in Azure generator" 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 d7e3c605229..0856f134e07 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; } @@ -1679,82 +1680,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) @@ -1765,87 +1690,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]