From bffe617ceb21836bcef2f85b94f47a86eaa64a20 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:16:37 +0000 Subject: [PATCH 01/19] Initial plan From 4e374c72abd82ade7e1a14b17814740a9456a500 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:28:20 +0000 Subject: [PATCH 02/19] Add back-compat restore for dropped model constructors Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../BackCompatibilityChangeCategory.cs | 6 + .../src/Providers/ModelProvider.cs | 209 ++++++++++++++++++ 2 files changed, 215 insertions(+) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs index 77cc9c53543..6a2aad4f357 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs @@ -54,5 +54,11 @@ public enum BackCompatibilityChangeCategory /// A fixed enum member was re-added to preserve a member that existed in the last contract but is no longer produced by the current spec. EnumMemberAddedFromLastContract, + + /// A back-compat model constructor was re-added to preserve a public constructor that existed in the last contract but is no longer produced by the current spec. + ConstructorAddedFromLastContract, + + /// A back-compat model constructor could not be reconstructed from the last contract and was skipped. + ConstructorAddedFromLastContractSkipped, } } 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 4d9c4eb985e..3c6ee0fa415 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 @@ -775,6 +775,215 @@ protected internal override ConstructorProvider[] BuildConstructors() return [.. constructors]; } + /// + /// Restores previously-published public constructors that the current generation would otherwise + /// drop. The primary scenario is a previously required property becoming optional: the corresponding + /// parameter is removed from the initialization constructor, which is a source-breaking change for + /// callers that construct the model positionally. When the previous public constructor can be safely + /// reconstructed - i.e. every one of its extra parameters still maps to a settable property whose + /// name and type are unchanged (or a property renamed via a codegen customization but keeping the + /// same type) - a back-compat overload is added that chains to the current public constructor and + /// assigns the extra properties. + /// + protected internal override IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable originalConstructors) + { + var constructors = new List(base.BuildConstructorsForBackCompatibility(originalConstructors)); + + if (LastContractView?.Constructors is not { Count: > 0 } previousConstructors) + { + return constructors; + } + + foreach (var previousConstructor in previousConstructors) + { + // Only public constructors are part of the API surface that callers can depend on. + if (!previousConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)) + { + continue; + } + + var previousParameters = previousConstructor.Signature.Parameters; + + // A parameterless constructor is always still generated (or intentionally absent); there is + // nothing to restore and doing so could collide with an existing constructor. + if (previousParameters.Count == 0) + { + continue; + } + + // If a constructor with the same parameters already exists, there is nothing to restore. + if (constructors.Any(c => BackCompatHelper.ParametersMatch(c.Signature.Parameters, previousParameters))) + { + continue; + } + + var restoredConstructor = TryBuildRestoredConstructor(previousConstructor, constructors); + if (restoredConstructor != null) + { + constructors.Add(restoredConstructor); + CodeModelGenerator.Instance.Emitter.Info( + $"Restored constructor '{Name}({string.Join(", ", previousParameters.Select(p => p.Type.ToString()))})' to match last contract.", + BackCompatibilityChangeCategory.ConstructorAddedFromLastContract); + } + else + { + CodeModelGenerator.Instance.Emitter.Info( + $"Could not restore constructor '{Name}({string.Join(", ", previousParameters.Select(p => p.Type.ToString()))})' from the last contract; a property name or type has changed.", + BackCompatibilityChangeCategory.ConstructorAddedFromLastContractSkipped); + } + } + + return constructors; + } + + /// + /// Attempts to reconstruct as a back-compat overload that + /// chains to an existing public constructor. Returns when the constructor + /// cannot be safely restored. + /// + private ConstructorProvider? TryBuildRestoredConstructor( + ConstructorProvider previousConstructor, + IReadOnlyList currentConstructors) + { + var previousParameters = previousConstructor.Signature.Parameters; + + // Find the public constructor to chain to: its parameters must form an in-order subsequence of + // the previous constructor's parameters (matched by name and type name). Prefer the closest one. + ConstructorProvider? targetConstructor = null; + foreach (var candidate in currentConstructors) + { + if (!candidate.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + || candidate.Signature.Parameters.Count >= previousParameters.Count) + { + continue; + } + + if (IsParameterSubsequence(candidate.Signature.Parameters, previousParameters) + && (targetConstructor == null + || candidate.Signature.Parameters.Count > targetConstructor.Signature.Parameters.Count)) + { + targetConstructor = candidate; + } + } + + if (targetConstructor == null) + { + return null; + } + + var targetParameters = targetConstructor.Signature.Parameters; + var restoredParameters = new List(previousParameters.Count); + var extraAssignments = new List<(PropertyProvider Property, ParameterProvider Parameter)>(); + int targetIndex = 0; + + foreach (var previousParameter in previousParameters) + { + if (targetIndex < targetParameters.Count + && ParametersEquivalent(targetParameters[targetIndex], previousParameter)) + { + // Kept parameter: reuse the target constructor's parameter so the chained call lines up. + restoredParameters.Add(targetParameters[targetIndex]); + targetIndex++; + continue; + } + + // Extra parameter: it must map to a settable property whose type is unchanged. + var property = FindRestorableProperty(previousParameter); + if (property == null) + { + return null; + } + + // Preserve the previously published parameter name and type exactly to keep the signature + // source-compatible. Reinstate null validation for non-nullable reference types so the + // restored constructor matches the behavior the property previously had while required. + var restoredParameter = new ParameterProvider( + previousParameter.Name, + previousParameter.Description, + previousParameter.Type, + validation: previousParameter.Type is { IsValueType: false, IsNullable: false } + ? ParameterValidationType.AssertNotNull + : ParameterValidationType.None); + + restoredParameters.Add(restoredParameter); + extraAssignments.Add((property, restoredParameter)); + } + + // Every target parameter must be consumed and at least one extra property must be assigned, + // otherwise the restored constructor would be redundant or would produce an invalid chained call. + if (targetIndex != targetParameters.Count || extraAssignments.Count == 0) + { + return null; + } + + var bodyStatements = new List(extraAssignments.Count); + foreach (var (property, parameter) in extraAssignments) + { + ValueExpression assignee = property.BackingField is null ? property : property.BackingField; + ValueExpression value = parameter; + if (CSharpType.RequiresToList(parameter.Type, property.Type)) + { + value = parameter.Type.IsNullable ? value.NullConditional().ToList() : value.ToList(); + } + + bodyStatements.Add(assignee.Assign(value).Terminate()); + } + + var signature = new ConstructorSignature( + Type, + $"Initializes a new instance of {Type:C}", + MethodSignatureModifiers.Public, + restoredParameters, + initializer: new ConstructorInitializer(false, targetParameters)); + + return new ConstructorProvider(signature, bodyStatements, this); + } + + private static bool ParametersEquivalent(ParameterProvider left, ParameterProvider right) + => left.Name == right.Name && left.Type.AreNamesEqual(right.Type); + + private static bool IsParameterSubsequence( + IReadOnlyList subset, + IReadOnlyList full) + { + int matched = 0; + foreach (var parameter in full) + { + if (matched < subset.Count && ParametersEquivalent(subset[matched], parameter)) + { + matched++; + } + } + + return matched == subset.Count; + } + + /// + /// Finds a settable public property that can receive the value of . + /// The property must have the same type (ignoring nullability) and either the same name or a name that + /// was changed via a codegen customization (matched by ). + /// + private PropertyProvider? FindRestorableProperty(ParameterProvider previousParameter) + { + foreach (var property in Properties) + { + if (!IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null) + { + continue; + } + + var nameMatches = property.AsParameter.Name == previousParameter.Name + || (property.OriginalName != null && property.OriginalName.ToVariableName() == previousParameter.Name); + + if (nameMatches && property.Type.AreNamesEqual(previousParameter.Type)) + { + return property; + } + } + + return null; + } + /// /// Determines if this model should have a dual constructor pattern. /// This is needed when the model shares the same discriminator property name as its base model From 9a3839d8d5931c5ee01a33b57fd93d34d00c8900 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:57:48 +0000 Subject: [PATCH 03/19] Add tests and validation-stripping fix for restored back-compat constructors Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../src/Providers/ModelProvider.cs | 22 +++- .../ModelProviders/ModelProviderTests.cs | 109 ++++++++++++++++++ .../UnrelatedModel.cs | 9 ++ .../MockInputModel.cs | 18 +++ .../MockInputModel.cs | 17 +++ 5 files changed, 172 insertions(+), 3 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenLastContractMissing/UnrelatedModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored/MockInputModel.cs 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 3c6ee0fa415..7260b4e4a65 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 @@ -873,6 +873,7 @@ protected internal override IReadOnlyList BuildConstructors var targetParameters = targetConstructor.Signature.Parameters; var restoredParameters = new List(previousParameters.Count); + var initializerArguments = new List(targetParameters.Count); var extraAssignments = new List<(PropertyProvider Property, ParameterProvider Parameter)>(); int targetIndex = 0; @@ -881,8 +882,23 @@ protected internal override IReadOnlyList BuildConstructors if (targetIndex < targetParameters.Count && ParametersEquivalent(targetParameters[targetIndex], previousParameter)) { - // Kept parameter: reuse the target constructor's parameter so the chained call lines up. - restoredParameters.Add(targetParameters[targetIndex]); + // Kept parameter: it is forwarded to the chained constructor, which performs any + // validation, so drop validation here to avoid emitting a redundant null check. + var keptParameter = targetParameters[targetIndex]; + if (keptParameter.Validation != ParameterValidationType.None) + { + keptParameter = new ParameterProvider( + keptParameter.Name, + keptParameter.Description, + keptParameter.Type, + keptParameter.DefaultValue, + validation: ParameterValidationType.None); + } + + restoredParameters.Add(keptParameter); + // Forward the restored constructor's own parameter to the chained call so the emitted + // variable reference matches the parameter declared on this constructor. + initializerArguments.Add(keptParameter); targetIndex++; continue; } @@ -934,7 +950,7 @@ protected internal override IReadOnlyList BuildConstructors $"Initializes a new instance of {Type:C}", MethodSignatureModifiers.Public, restoredParameters, - initializer: new ConstructorInitializer(false, targetParameters)); + initializer: new ConstructorInitializer(false, initializerArguments)); return new ConstructorProvider(signature, bodyStatements, this); } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index 9b7270835f5..4428e777468 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -2346,6 +2346,115 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual("baseProp", publicConstructor!.Signature.Parameters[0].Name); } + [Test] + public async Task BackCompat_RequiredToOptionalConstructorIsRestored() + { + // "resources" was required in the last contract (so the initialization constructor + // accepted it), but the current spec relaxes it to optional which would otherwise drop + // it from the constructor. The previously published constructor should be restored. + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input | InputModelTypeUsage.Json, + properties: + [ + InputFactory.Property("name", InputPrimitiveType.String, isRequired: true), + InputFactory.Property("resources", InputPrimitiveType.String, isRequired: false), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + + // Before back-compat processing the public constructor only takes "name". + var publicCtorBefore = modelProvider!.Constructors.SingleOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)); + Assert.IsNotNull(publicCtorBefore); + Assert.AreEqual(1, publicCtorBefore!.Signature.Parameters.Count); + + modelProvider.ProcessTypeForBackCompatibility(); + + // After back-compat processing the previously published (name, resources) constructor is restored. + var restoredCtor = modelProvider.Constructors.SingleOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + && c.Signature.Parameters.Count == 2); + Assert.IsNotNull(restoredCtor, "Expected the (name, resources) constructor to be restored for back compat"); + Assert.AreEqual("name", restoredCtor!.Signature.Parameters[0].Name); + Assert.AreEqual("resources", restoredCtor.Signature.Parameters[1].Name); + Assert.IsTrue(restoredCtor.Signature.Parameters[1].Type.Equals(typeof(string))); + + // It chains to the current (name) constructor and assigns the extra property in its body. + var initializer = restoredCtor.Signature.Initializer; + Assert.IsNotNull(initializer); + Assert.IsFalse(initializer!.IsBase); + Assert.AreEqual(1, initializer.Arguments.Count); + Assert.AreEqual("name", initializer.Arguments[0].ToDisplayString()); + + var body = restoredCtor.BodyStatements!.ToDisplayString(); + Assert.IsTrue(body.Contains("Resources = resources"), $"Expected the body to assign Resources, was: {body}"); + // A required non-nullable reference type restores its null validation. + Assert.IsTrue(body.Contains("Argument.AssertNotNull(resources"), $"Expected null validation for resources, was: {body}"); + } + + [Test] + public async Task BackCompat_ConstructorNotRestoredWhenPropertyRemoved() + { + // "resources" existed in the last contract constructor but has been removed entirely from + // the current spec, so the previous constructor cannot be safely restored. + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input | InputModelTypeUsage.Json, + properties: + [ + InputFactory.Property("name", InputPrimitiveType.String, isRequired: true), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + + modelProvider!.ProcessTypeForBackCompatibility(); + + var twoParamPublicCtor = modelProvider.Constructors.FirstOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + && c.Signature.Parameters.Count == 2); + Assert.IsNull(twoParamPublicCtor, "The constructor should not be restored when a property was removed"); + } + + [Test] + public async Task BackCompat_ConstructorNotRestoredWhenLastContractMissing() + { + // No last contract exists for the model, so nothing should be restored. + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input | InputModelTypeUsage.Json, + properties: + [ + InputFactory.Property("name", InputPrimitiveType.String, isRequired: true), + InputFactory.Property("resources", InputPrimitiveType.String, isRequired: false), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + Assert.IsNull(modelProvider!.LastContractView); + + modelProvider.ProcessTypeForBackCompatibility(); + + var twoParamPublicCtor = modelProvider.Constructors.FirstOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + && c.Signature.Parameters.Count == 2); + Assert.IsNull(twoParamPublicCtor); + } + [Test] public async Task TestBuildProperties_WithObjectAdditionalPropertiesBackwardCompatibility() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenLastContractMissing/UnrelatedModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenLastContractMissing/UnrelatedModel.cs new file mode 100644 index 00000000000..78a0f6a28d2 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenLastContractMissing/UnrelatedModel.cs @@ -0,0 +1,9 @@ +namespace Sample.Models +{ + // Note: this last-contract model has a different name than the spec model + // ("MockInputModel"), so no last contract view is found for the model. + public partial class UnrelatedModel + { + public int? Count { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved/MockInputModel.cs new file mode 100644 index 00000000000..69e9aa5a99f --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved/MockInputModel.cs @@ -0,0 +1,18 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + // In the last contract "resources" existed and was part of the constructor, but + // the current spec removes the property entirely. Because there is no matching + // property to assign, the previous constructor cannot be safely restored. + public MockInputModel(string name, string resources) + { + Name = name; + Resources = resources; + } + + public string Name { get; set; } + + public string Resources { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored/MockInputModel.cs new file mode 100644 index 00000000000..bd5ceb0f9ef --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored/MockInputModel.cs @@ -0,0 +1,17 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + // In the last contract, both properties were required so the initialization + // constructor accepted both of them. + public MockInputModel(string name, string resources) + { + Name = name; + Resources = resources; + } + + public string Name { get; set; } + + public string Resources { get; set; } + } +} From 9c940c4c3280819b9e1a1a01cd900f8028fbc214 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 23:15:37 +0000 Subject: [PATCH 04/19] Address review feedback on back-compat constructor restoration Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../src/Providers/ModelProvider.cs | 64 ++++++++++--------- 1 file changed, 35 insertions(+), 29 deletions(-) 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 7260b4e4a65..a4a619fe465 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 @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.Linq; using Microsoft.TypeSpec.Generator.EmitterRpc; @@ -787,17 +788,17 @@ protected internal override ConstructorProvider[] BuildConstructors() /// protected internal override IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable originalConstructors) { - var constructors = new List(base.BuildConstructorsForBackCompatibility(originalConstructors)); - if (LastContractView?.Constructors is not { Count: > 0 } previousConstructors) { - return constructors; + return base.BuildConstructorsForBackCompatibility(originalConstructors); } + var constructors = new List(base.BuildConstructorsForBackCompatibility(originalConstructors)); + foreach (var previousConstructor in previousConstructors) { - // Only public constructors are part of the API surface that callers can depend on. - if (!previousConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)) + // Only public/protected constructors are part of the API surface that callers can depend on. + if (!IsPublicApi(previousConstructor.Signature.Modifiers)) { continue; } @@ -817,8 +818,7 @@ protected internal override IReadOnlyList BuildConstructors continue; } - var restoredConstructor = TryBuildRestoredConstructor(previousConstructor, constructors); - if (restoredConstructor != null) + if (TryBuildRestoredConstructor(previousConstructor, constructors, out var restoredConstructor)) { constructors.Add(restoredConstructor); CodeModelGenerator.Instance.Emitter.Info( @@ -838,29 +838,34 @@ protected internal override IReadOnlyList BuildConstructors /// /// Attempts to reconstruct as a back-compat overload that - /// chains to an existing public constructor. Returns when the constructor - /// cannot be safely restored. + /// chains to an existing public constructor. Returns and sets + /// when the constructor can be safely restored; otherwise + /// returns . /// - private ConstructorProvider? TryBuildRestoredConstructor( + private bool TryBuildRestoredConstructor( ConstructorProvider previousConstructor, - IReadOnlyList currentConstructors) + IReadOnlyList currentConstructors, + [NotNullWhen(true)] out ConstructorProvider? restoredConstructor) { + restoredConstructor = null; var previousParameters = previousConstructor.Signature.Parameters; // Find the public constructor to chain to: its parameters must form an in-order subsequence of - // the previous constructor's parameters (matched by name and type name). Prefer the closest one. + // the previous constructor's parameters. Prefer the closest one. ConstructorProvider? targetConstructor = null; foreach (var candidate in currentConstructors) { - if (!candidate.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + if (!IsPublicApi(candidate.Signature.Modifiers) || candidate.Signature.Parameters.Count >= previousParameters.Count) { continue; } - if (IsParameterSubsequence(candidate.Signature.Parameters, previousParameters) - && (targetConstructor == null - || candidate.Signature.Parameters.Count > targetConstructor.Signature.Parameters.Count)) + // Check whether this candidate would improve on the current target before performing the + // more expensive subsequence lookup. + if ((targetConstructor == null + || candidate.Signature.Parameters.Count > targetConstructor.Signature.Parameters.Count) + && IsParameterSubsequence(candidate.Signature.Parameters, previousParameters)) { targetConstructor = candidate; } @@ -868,7 +873,7 @@ protected internal override IReadOnlyList BuildConstructors if (targetConstructor == null) { - return null; + return false; } var targetParameters = targetConstructor.Signature.Parameters; @@ -880,7 +885,7 @@ protected internal override IReadOnlyList BuildConstructors foreach (var previousParameter in previousParameters) { if (targetIndex < targetParameters.Count - && ParametersEquivalent(targetParameters[targetIndex], previousParameter)) + && targetParameters[targetIndex].Equals(previousParameter)) { // Kept parameter: it is forwarded to the chained constructor, which performs any // validation, so drop validation here to avoid emitting a redundant null check. @@ -892,6 +897,7 @@ protected internal override IReadOnlyList BuildConstructors keptParameter.Description, keptParameter.Type, keptParameter.DefaultValue, + wireInfo: keptParameter.WireInfo, validation: ParameterValidationType.None); } @@ -907,16 +913,18 @@ protected internal override IReadOnlyList BuildConstructors var property = FindRestorableProperty(previousParameter); if (property == null) { - return null; + return false; } // Preserve the previously published parameter name and type exactly to keep the signature - // source-compatible. Reinstate null validation for non-nullable reference types so the - // restored constructor matches the behavior the property previously had while required. + // source-compatible, carrying the wire info from the current property so serialization is + // unchanged. Reinstate null validation for non-nullable reference types so the restored + // constructor matches the behavior the property previously had while required. var restoredParameter = new ParameterProvider( previousParameter.Name, previousParameter.Description, previousParameter.Type, + wireInfo: property.AsParameter.WireInfo, validation: previousParameter.Type is { IsValueType: false, IsNullable: false } ? ParameterValidationType.AssertNotNull : ParameterValidationType.None); @@ -929,7 +937,7 @@ protected internal override IReadOnlyList BuildConstructors // otherwise the restored constructor would be redundant or would produce an invalid chained call. if (targetIndex != targetParameters.Count || extraAssignments.Count == 0) { - return null; + return false; } var bodyStatements = new List(extraAssignments.Count); @@ -948,16 +956,14 @@ protected internal override IReadOnlyList BuildConstructors var signature = new ConstructorSignature( Type, $"Initializes a new instance of {Type:C}", - MethodSignatureModifiers.Public, + previousConstructor.Signature.Modifiers, restoredParameters, initializer: new ConstructorInitializer(false, initializerArguments)); - return new ConstructorProvider(signature, bodyStatements, this); + restoredConstructor = new ConstructorProvider(signature, bodyStatements, this); + return true; } - private static bool ParametersEquivalent(ParameterProvider left, ParameterProvider right) - => left.Name == right.Name && left.Type.AreNamesEqual(right.Type); - private static bool IsParameterSubsequence( IReadOnlyList subset, IReadOnlyList full) @@ -965,7 +971,7 @@ private static bool IsParameterSubsequence( int matched = 0; foreach (var parameter in full) { - if (matched < subset.Count && ParametersEquivalent(subset[matched], parameter)) + if (matched < subset.Count && subset[matched].Equals(parameter)) { matched++; } @@ -981,7 +987,7 @@ private static bool IsParameterSubsequence( /// private PropertyProvider? FindRestorableProperty(ParameterProvider previousParameter) { - foreach (var property in Properties) + foreach (var property in CanonicalView.Properties) { if (!IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null) { From a7950c88904e17783997fb85e64dca63c61654d4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 23:23:09 +0000 Subject: [PATCH 05/19] Add robust TestData-based tests for back-compat constructor restoration Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../ModelProviders/ModelProviderTests.cs | 86 +++++++++++++++++++ ...ValueTypeConstructorParameterIsRestored.cs | 38 ++++++++ .../MockInputModel.cs | 18 ++++ .../MockInputModel.cs | 14 +++ .../MockInputModel.cs | 16 ++++ ...RequiredToOptionalConstructorIsRestored.cs | 40 +++++++++ 6 files changed, 212 insertions(+) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored_LastContract/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index 4428e777468..3870054301f 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -2396,6 +2396,11 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.IsTrue(body.Contains("Resources = resources"), $"Expected the body to assign Resources, was: {body}"); // A required non-nullable reference type restores its null validation. Assert.IsTrue(body.Contains("Argument.AssertNotNull(resources"), $"Expected null validation for resources, was: {body}"); + + // Validate the full generated model, including the restored constructor, against the expected output. + var writer = new TypeProviderWriter(modelProvider); + var file = writer.Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); } [Test] @@ -2455,6 +2460,87 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.IsNull(twoParamPublicCtor); } + [Test] + public async Task BackCompat_OptionalValueTypeConstructorParameterIsRestored() + { + // "count" was a required value type in the last contract, so the initialization constructor + // accepted it. Relaxing it to optional drops it; the previously published constructor is + // restored, and because it is a value type no null validation is emitted. + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input | InputModelTypeUsage.Json, + properties: + [ + InputFactory.Property("name", InputPrimitiveType.String, isRequired: true), + InputFactory.Property("count", InputPrimitiveType.Int32, isRequired: false), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + + modelProvider!.ProcessTypeForBackCompatibility(); + + var restoredCtor = modelProvider.Constructors.SingleOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + && c.Signature.Parameters.Count == 2); + Assert.IsNotNull(restoredCtor, "Expected the (name, count) constructor to be restored for back compat"); + Assert.AreEqual("count", restoredCtor!.Signature.Parameters[1].Name); + // The restored parameter keeps the previously published non-nullable value type. + Assert.IsTrue(restoredCtor.Signature.Parameters[1].Type.Equals(typeof(int))); + Assert.AreEqual(ParameterValidationType.None, restoredCtor.Signature.Parameters[1].Validation); + + var body = restoredCtor.BodyStatements!.ToDisplayString(); + Assert.IsTrue(body.Contains("Count = count"), $"Expected the body to assign Count, was: {body}"); + // Value types never emit a null check. + Assert.IsFalse(body.Contains("AssertNotNull"), $"Did not expect null validation for a value type, was: {body}"); + + var writer = new TypeProviderWriter(modelProvider); + var file = writer.Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); + } + + [Test] + public async Task BackCompat_RenamedPropertyConstructorIsRestored() + { + // The spec property "resources" is renamed to "ResourceList" via a [CodeGenMember] + // customization. The previously published constructor's "resources" parameter must still + // be matched to the renamed property (via its OriginalName) so the constructor is restored. + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input | InputModelTypeUsage.Json, + properties: + [ + InputFactory.Property("name", InputPrimitiveType.String, isRequired: true), + InputFactory.Property("resources", InputPrimitiveType.String, isRequired: false), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + compilation: async () => await Helpers.GetCompilationFromDirectoryAsync(), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync( + method: "BackCompat_RenamedPropertyConstructorIsRestored_LastContract")); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + + modelProvider!.ProcessTypeForBackCompatibility(); + + var restoredCtor = modelProvider.Constructors.SingleOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + && c.Signature.Parameters.Count == 2); + Assert.IsNotNull(restoredCtor, "Expected the (name, resources) constructor to be restored for back compat"); + // The restored parameter keeps the previously published (pre-rename) name. + Assert.AreEqual("resources", restoredCtor!.Signature.Parameters[1].Name); + + // The body assigns the current, renamed property. + var body = restoredCtor.BodyStatements!.ToDisplayString(); + Assert.IsTrue(body.Contains("ResourceList = resources"), $"Expected the body to assign the renamed property, was: {body}"); + } + [Test] public async Task TestBuildProperties_WithObjectAdditionalPropertiesBackwardCompatibility() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs new file mode 100644 index 00000000000..c6e78ea90c8 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs @@ -0,0 +1,38 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; +using Sample; + +namespace Sample.Models +{ + public partial class MockInputModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + public MockInputModel(string name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + Name = name; + } + + internal MockInputModel(string name, int count, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Name = name; + Count = count; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + public MockInputModel(string name, int count) : this(name) + { + Count = count; + } + + public string Name { get; } + + public int Count { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored/MockInputModel.cs new file mode 100644 index 00000000000..0b6a20147c1 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored/MockInputModel.cs @@ -0,0 +1,18 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + // In the last contract "count" was required so the initialization constructor + // accepted it. The current spec relaxes it to optional, which drops it from the + // constructor unless it is restored for back compat. + public MockInputModel(string name, int count) + { + Name = name; + Count = count; + } + + public string Name { get; set; } + + public int Count { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored/MockInputModel.cs new file mode 100644 index 00000000000..e9ed6d5d4a3 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored/MockInputModel.cs @@ -0,0 +1,14 @@ +#nullable disable + +using Sample; +using SampleTypeSpec; +using Microsoft.TypeSpec.Generator.Customizations; + +namespace Sample.Models +{ + public partial class MockInputModel + { + [CodeGenMember("Resources")] + public string ResourceList { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored_LastContract/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored_LastContract/MockInputModel.cs new file mode 100644 index 00000000000..01ac1bdcf99 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored_LastContract/MockInputModel.cs @@ -0,0 +1,16 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + // The previously published constructor used the pre-rename parameter name "resources". + public MockInputModel(string name, string resources) + { + Name = name; + Resources = resources; + } + + public string Name { get; set; } + + public string Resources { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs new file mode 100644 index 00000000000..e217b6c3fb7 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs @@ -0,0 +1,40 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; +using Sample; + +namespace Sample.Models +{ + public partial class MockInputModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + public MockInputModel(string name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + Name = name; + } + + internal MockInputModel(string name, string resources, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Name = name; + Resources = resources; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + public MockInputModel(string name, string resources) : this(name) + { + global::Sample.Argument.AssertNotNull(resources, nameof(resources)); + + Resources = resources; + } + + public string Name { get; } + + public string Resources { get; set; } + } +} From 364600bc45382233277996c505f3958d2fcdf887 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:36:24 +0000 Subject: [PATCH 06/19] Address review: baseline/custom-code ctor skip, reuse CloneParameterWithName, TestData snapshots Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../src/Providers/ModelProvider.cs | 55 ++++----- .../src/Utilities/BackCompatHelper.cs | 29 +++++ .../ModelProviders/ModelProviderTests.cs | 107 +++++++++++++++++- ...uctorNotRestoredWhenLastContractMissing.cs | 33 ++++++ ...nstructorNotRestoredWhenPropertyRemoved.cs | 30 +++++ .../MockInputModel.cs | 4 +- ...otRestoredWhenRemovalAcceptedInBaseline.cs | 33 ++++++ ...tRestoredWhenRemovalAcceptedInBaseline.txt | 1 + ...tRestoredWhenRemovalAcceptedInBaseline.xml | 7 ++ .../MockInputModel.cs | 18 +++ ...ctorNotRestoredWhenReplacedByCustomCode.cs | 33 ++++++ .../MockInputModel.cs | 18 +++ .../MockInputModel.cs | 16 +++ ...ValueTypeConstructorParameterIsRestored.cs | 2 + .../MockInputModel.cs | 4 +- ...at_RenamedPropertyConstructorIsRestored.cs | 38 +++++++ .../MockInputModel.cs | 4 +- ...RequiredToOptionalConstructorIsRestored.cs | 2 +- .../MockInputModel.cs | 4 +- 19 files changed, 392 insertions(+), 46 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenLastContractMissing.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.txt create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.xml create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline_LastContract/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode_LastContract/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs 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 a4a619fe465..4d8a6e60cea 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 @@ -812,8 +812,17 @@ protected internal override IReadOnlyList BuildConstructors continue; } - // If a constructor with the same parameters already exists, there is nothing to restore. - if (constructors.Any(c => BackCompatHelper.ParametersMatch(c.Signature.Parameters, previousParameters))) + // If a constructor with the same parameters already exists - either still generated or + // supplied by custom code (which lives in the canonical view) - there is nothing to restore. + if (constructors.Any(c => BackCompatHelper.ParametersMatch(c.Signature.Parameters, previousParameters)) + || CanonicalView.Constructors.Any(c => BackCompatHelper.ParametersMatch(c.Signature.Parameters, previousParameters))) + { + continue; + } + + // If the removal of this constructor has been accepted in the ApiCompat baseline (in either + // the xml or txt format), the break is intentional and must not be resurrected. + if (BackCompatHelper.IsConstructorRemovalAcceptedInBaseline(this, previousConstructor.Signature)) { continue; } @@ -836,12 +845,6 @@ protected internal override IReadOnlyList BuildConstructors return constructors; } - /// - /// Attempts to reconstruct as a back-compat overload that - /// chains to an existing public constructor. Returns and sets - /// when the constructor can be safely restored; otherwise - /// returns . - /// private bool TryBuildRestoredConstructor( ConstructorProvider previousConstructor, IReadOnlyList currentConstructors, @@ -887,23 +890,11 @@ private bool TryBuildRestoredConstructor( if (targetIndex < targetParameters.Count && targetParameters[targetIndex].Equals(previousParameter)) { - // Kept parameter: it is forwarded to the chained constructor, which performs any - // validation, so drop validation here to avoid emitting a redundant null check. + // Kept parameter: forward the target constructor's parameter (with its existing + // validation) to the chained call so the emitted variable reference matches the + // parameter declared on this constructor. var keptParameter = targetParameters[targetIndex]; - if (keptParameter.Validation != ParameterValidationType.None) - { - keptParameter = new ParameterProvider( - keptParameter.Name, - keptParameter.Description, - keptParameter.Type, - keptParameter.DefaultValue, - wireInfo: keptParameter.WireInfo, - validation: ParameterValidationType.None); - } - restoredParameters.Add(keptParameter); - // Forward the restored constructor's own parameter to the chained call so the emitted - // variable reference matches the parameter declared on this constructor. initializerArguments.Add(keptParameter); targetIndex++; continue; @@ -916,18 +907,14 @@ private bool TryBuildRestoredConstructor( return false; } - // Preserve the previously published parameter name and type exactly to keep the signature - // source-compatible, carrying the wire info from the current property so serialization is - // unchanged. Reinstate null validation for non-nullable reference types so the restored - // constructor matches the behavior the property previously had while required. - var restoredParameter = new ParameterProvider( + // Clone the current property's parameter under the previously published name (dropping any + // default value so the restored positional parameter matches the previous signature). This + // carries the current wire info and validation, keeping serialization and null-checking + // behavior consistent with the property. + var restoredParameter = PartialMethodCustomization.CloneParameterWithName( + property.AsParameter, previousParameter.Name, - previousParameter.Description, - previousParameter.Type, - wireInfo: property.AsParameter.WireInfo, - validation: previousParameter.Type is { IsValueType: false, IsNullable: false } - ? ParameterValidationType.AssertNotNull - : ParameterValidationType.None); + removeDefault: true); restoredParameters.Add(restoredParameter); extraAssignments.Add((property, restoredParameter)); 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 c87ed9c348a..e468b57d7e0 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 @@ -50,6 +50,35 @@ public static bool IsMethodRemovalAcceptedInBaseline(TypeProvider enclosingType, return true; } + /// + /// Returns true when the removal of a previously-published constructor — identified by the + /// enclosing type's fully-qualified name and the exact parameter types — has been accepted in the + /// ApiCompat baseline, in which case back compatibility must not restore it. Constructors are + /// recorded in the baseline as the .ctor member of their declaring type. Emits an + /// informational log entry when a suppression is honored. + /// + public static bool IsConstructorRemovalAcceptedInBaseline(TypeProvider enclosingType, ConstructorSignature previousSignature) + { + var parameterTypes = new CSharpType[previousSignature.Parameters.Count]; + for (int i = 0; i < parameterTypes.Length; i++) + { + parameterTypes[i] = previousSignature.Parameters[i].Type; + } + + if (CodeModelGenerator.Instance.SourceInputModel?.ApiCompatBaseline.IsMethodRemovalSuppressed( + enclosingType.Type.FullyQualifiedName, + ".ctor", + parameterTypes) != true) + { + return false; + } + + CodeModelGenerator.Instance.Emitter.Info( + $"Skipping back-compat for '{enclosingType.Type.FullyQualifiedName}..ctor'; 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. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index 3870054301f..f6fa91ebe7f 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -2394,8 +2394,10 @@ await MockHelpers.LoadMockGeneratorAsync( var body = restoredCtor.BodyStatements!.ToDisplayString(); Assert.IsTrue(body.Contains("Resources = resources"), $"Expected the body to assign Resources, was: {body}"); - // A required non-nullable reference type restores its null validation. - Assert.IsTrue(body.Contains("Argument.AssertNotNull(resources"), $"Expected null validation for resources, was: {body}"); + // The extra parameter is cloned from the now-optional property, so it carries the property's + // (optional) validation - i.e. no null check is emitted for it. + Assert.AreEqual(ParameterValidationType.None, restoredCtor.Signature.Parameters[1].Validation); + Assert.IsFalse(body.Contains("Argument.AssertNotNull(resources"), $"Did not expect null validation for the optional resources parameter, was: {body}"); // Validate the full generated model, including the restored constructor, against the expected output. var writer = new TypeProviderWriter(modelProvider); @@ -2429,6 +2431,10 @@ await MockHelpers.LoadMockGeneratorAsync( c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) && c.Signature.Parameters.Count == 2); Assert.IsNull(twoParamPublicCtor, "The constructor should not be restored when a property was removed"); + + var writer = new TypeProviderWriter(modelProvider); + var file = writer.Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); } [Test] @@ -2458,6 +2464,10 @@ await MockHelpers.LoadMockGeneratorAsync( c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) && c.Signature.Parameters.Count == 2); Assert.IsNull(twoParamPublicCtor); + + var writer = new TypeProviderWriter(modelProvider); + var file = writer.Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); } [Test] @@ -2496,7 +2506,7 @@ await MockHelpers.LoadMockGeneratorAsync( var body = restoredCtor.BodyStatements!.ToDisplayString(); Assert.IsTrue(body.Contains("Count = count"), $"Expected the body to assign Count, was: {body}"); // Value types never emit a null check. - Assert.IsFalse(body.Contains("AssertNotNull"), $"Did not expect null validation for a value type, was: {body}"); + Assert.IsFalse(body.Contains("AssertNotNull(count"), $"Did not expect null validation for a value type, was: {body}"); var writer = new TypeProviderWriter(modelProvider); var file = writer.Write(); @@ -2539,6 +2549,97 @@ await MockHelpers.LoadMockGeneratorAsync( // The body assigns the current, renamed property. var body = restoredCtor.BodyStatements!.ToDisplayString(); Assert.IsTrue(body.Contains("ResourceList = resources"), $"Expected the body to assign the renamed property, was: {body}"); + + var writer = new TypeProviderWriter(modelProvider); + var file = writer.Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); + } + + [TestCase(".txt")] + [TestCase(".xml")] + public async Task BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline(string baselineExtension) + { + // "resources" was required in the last contract and is now optional, which would normally + // cause the previous (name, resources) constructor to be restored. However its removal is + // accepted in the ApiCompat baseline (tested in both the txt and xml formats), so the + // constructor must not be resurrected. + var baseline = Helpers.GetApiCompatBaselineFromFile(fileExtension: baselineExtension); + + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input | InputModelTypeUsage.Json, + properties: + [ + InputFactory.Property("name", InputPrimitiveType.String, isRequired: true), + InputFactory.Property("resources", InputPrimitiveType.String, isRequired: false), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync( + method: "BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline_LastContract"), + apiCompatBaseline: baseline); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + + modelProvider!.ProcessTypeForBackCompatibility(); + + // The (name, resources) constructor removal is accepted in the baseline, so it is not restored. + var restoredCtor = modelProvider.Constructors.FirstOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + && c.Signature.Parameters.Count == 2); + Assert.IsNull(restoredCtor, "The constructor should not be restored when its removal is accepted in the baseline"); + + var writer = new TypeProviderWriter(modelProvider); + var file = writer.Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); + } + + [Test] + public async Task BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode() + { + // "resources" was required in the last contract and is now optional, which would normally + // cause the previous (name, resources) constructor to be restored. Here the user has replaced + // that constructor with their own custom implementation, so the generator must not add a + // colliding back-compat overload. + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input | InputModelTypeUsage.Json, + properties: + [ + InputFactory.Property("name", InputPrimitiveType.String, isRequired: true), + InputFactory.Property("resources", InputPrimitiveType.String, isRequired: false), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + compilation: async () => await Helpers.GetCompilationFromDirectoryAsync( + method: "BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode"), + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync( + method: "BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode_LastContract")); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + + // The custom (name, resources) constructor lives in the canonical view. + var customCtor = modelProvider!.CanonicalView.Constructors.SingleOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + && c.Signature.Parameters.Count == 2); + Assert.IsNotNull(customCtor, "Expected the custom (name, resources) constructor to be present"); + + modelProvider.ProcessTypeForBackCompatibility(); + + // Because the custom code already provides the (name, resources) constructor, the generator + // must not restore a colliding overload of its own. + var restoredCtor = modelProvider.Constructors.FirstOrDefault(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + && c.Signature.Parameters.Count == 2); + Assert.IsNull(restoredCtor, "The constructor should not be restored when it is replaced by custom code"); + + var writer = new TypeProviderWriter(modelProvider); + var file = writer.Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); } [Test] diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenLastContractMissing.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenLastContractMissing.cs new file mode 100644 index 00000000000..61958c1b8b5 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenLastContractMissing.cs @@ -0,0 +1,33 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; +using Sample; + +namespace Sample.Models +{ + public partial class MockInputModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + public MockInputModel(string name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + Name = name; + } + + internal MockInputModel(string name, string resources, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Name = name; + Resources = resources; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + public string Name { get; } + + public string Resources { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved.cs new file mode 100644 index 00000000000..ac6d11111d5 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved.cs @@ -0,0 +1,30 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; +using Sample; + +namespace Sample.Models +{ + public partial class MockInputModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + public MockInputModel(string name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + Name = name; + } + + internal MockInputModel(string name, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Name = name; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + public string Name { get; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved/MockInputModel.cs index 69e9aa5a99f..776ffa9d8d7 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved/MockInputModel.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenPropertyRemoved/MockInputModel.cs @@ -11,8 +11,8 @@ public MockInputModel(string name, string resources) Resources = resources; } - public string Name { get; set; } + public string Name { get; } - public string Resources { get; set; } + public string Resources { get; } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.cs new file mode 100644 index 00000000000..61958c1b8b5 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.cs @@ -0,0 +1,33 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; +using Sample; + +namespace Sample.Models +{ + public partial class MockInputModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + public MockInputModel(string name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + Name = name; + } + + internal MockInputModel(string name, string resources, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Name = name; + Resources = resources; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + public string Name { get; } + + public string Resources { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.txt b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.txt new file mode 100644 index 00000000000..2c4258cf7d3 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.txt @@ -0,0 +1 @@ +MembersMustExist : Member 'public Sample.Models.MockInputModel..ctor(System.String, System.String)' 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/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.xml b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.xml new file mode 100644 index 00000000000..36000f351a8 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline.xml @@ -0,0 +1,7 @@ + + + + CP0002 + M:Sample.Models.MockInputModel.#ctor(System.String,System.String) + + diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline_LastContract/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline_LastContract/MockInputModel.cs new file mode 100644 index 00000000000..c6e9450a34e --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenRemovalAcceptedInBaseline_LastContract/MockInputModel.cs @@ -0,0 +1,18 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + // In the last contract "resources" was required so the initialization constructor + // accepted it. The current spec relaxes it to optional, which would normally cause the + // previous constructor to be restored - but here its removal is accepted in the baseline. + public MockInputModel(string name, string resources) + { + Name = name; + Resources = resources; + } + + public string Name { get; } + + public string Resources { get; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode.cs new file mode 100644 index 00000000000..61958c1b8b5 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode.cs @@ -0,0 +1,33 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; +using Sample; + +namespace Sample.Models +{ + public partial class MockInputModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + public MockInputModel(string name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + Name = name; + } + + internal MockInputModel(string name, string resources, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Name = name; + Resources = resources; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + public string Name { get; } + + public string Resources { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode/MockInputModel.cs new file mode 100644 index 00000000000..4dc7415bd15 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode/MockInputModel.cs @@ -0,0 +1,18 @@ +#nullable disable + +using Sample; +using SampleTypeSpec; + +namespace Sample.Models +{ + public partial class MockInputModel + { + // The user supplies their own (name, resources) constructor, replacing the one the + // generator would otherwise restore for back compat. Restoration must be skipped so the + // generated overload does not collide with this custom code. + public MockInputModel(string name, string resources) : this(name) + { + Resources = resources; + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode_LastContract/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode_LastContract/MockInputModel.cs new file mode 100644 index 00000000000..d088ba570fa --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorNotRestoredWhenReplacedByCustomCode_LastContract/MockInputModel.cs @@ -0,0 +1,16 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + // The previously published constructor accepted the required "resources" parameter. + public MockInputModel(string name, string resources) + { + Name = name; + Resources = resources; + } + + public string Name { get; } + + public string Resources { get; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs index c6e78ea90c8..536d73912ae 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs @@ -28,6 +28,8 @@ internal MockInputModel(string name, int count, global::System.Collections.Gener public MockInputModel(string name, int count) : this(name) { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + Count = count; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored/MockInputModel.cs index 0b6a20147c1..adad3e228ab 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored/MockInputModel.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored/MockInputModel.cs @@ -11,8 +11,8 @@ public MockInputModel(string name, int count) Count = count; } - public string Name { get; set; } + public string Name { get; } - public int Count { get; set; } + public int Count { get; } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs new file mode 100644 index 00000000000..e1f22a9b915 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs @@ -0,0 +1,38 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; +using Sample; + +namespace Sample.Models +{ + public partial class MockInputModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + public MockInputModel(string name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + Name = name; + } + + internal MockInputModel(string name, string resourceList, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Name = name; + ResourceList = resourceList; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + public MockInputModel(string name, string resources) : this(name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + ResourceList = resources; + } + + public string Name { get; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored_LastContract/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored_LastContract/MockInputModel.cs index 01ac1bdcf99..83aedb284da 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored_LastContract/MockInputModel.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored_LastContract/MockInputModel.cs @@ -9,8 +9,8 @@ public MockInputModel(string name, string resources) Resources = resources; } - public string Name { get; set; } + public string Name { get; } - public string Resources { get; set; } + public string Resources { get; } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs index e217b6c3fb7..fc878ffd638 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs @@ -28,7 +28,7 @@ internal MockInputModel(string name, string resources, global::System.Collection public MockInputModel(string name, string resources) : this(name) { - global::Sample.Argument.AssertNotNull(resources, nameof(resources)); + global::Sample.Argument.AssertNotNull(name, nameof(name)); Resources = resources; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored/MockInputModel.cs index bd5ceb0f9ef..bc469c58d83 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored/MockInputModel.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored/MockInputModel.cs @@ -10,8 +10,8 @@ public MockInputModel(string name, string resources) Resources = resources; } - public string Name { get; set; } + public string Name { get; } - public string Resources { get; set; } + public string Resources { get; } } } From 04eb55b7eca3da35c92ee26d78bb1353420902f0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:58:31 +0000 Subject: [PATCH 07/19] Address review: trim comments, reorder baseline check, property lookup dict, ctor baseline tests Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../src/Providers/ModelProvider.cs | 79 ++++++++++++++----- .../SourceInput/ApiCompatBaselineTests.cs | 46 +++++++++++ .../ApiCompatBaselineTests/Baseline.txt | 2 + .../ApiCompatBaselineTests/Baseline.xml | 8 ++ 4 files changed, 114 insertions(+), 21 deletions(-) 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 4d8a6e60cea..227314686a3 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 @@ -794,10 +794,10 @@ protected internal override IReadOnlyList BuildConstructors } var constructors = new List(base.BuildConstructorsForBackCompatibility(originalConstructors)); + var restorablePropertyLookup = BuildRestorablePropertyLookup(); foreach (var previousConstructor in previousConstructors) { - // Only public/protected constructors are part of the API surface that callers can depend on. if (!IsPublicApi(previousConstructor.Signature.Modifiers)) { continue; @@ -805,29 +805,25 @@ protected internal override IReadOnlyList BuildConstructors var previousParameters = previousConstructor.Signature.Parameters; - // A parameterless constructor is always still generated (or intentionally absent); there is - // nothing to restore and doing so could collide with an existing constructor. if (previousParameters.Count == 0) { continue; } - // If a constructor with the same parameters already exists - either still generated or - // supplied by custom code (which lives in the canonical view) - there is nothing to restore. - if (constructors.Any(c => BackCompatHelper.ParametersMatch(c.Signature.Parameters, previousParameters)) - || CanonicalView.Constructors.Any(c => BackCompatHelper.ParametersMatch(c.Signature.Parameters, previousParameters))) + if (BackCompatHelper.IsConstructorRemovalAcceptedInBaseline(this, previousConstructor.Signature)) { continue; } - // If the removal of this constructor has been accepted in the ApiCompat baseline (in either - // the xml or txt format), the break is intentional and must not be resurrected. - if (BackCompatHelper.IsConstructorRemovalAcceptedInBaseline(this, previousConstructor.Signature)) + // If a constructor with the same parameters already exists - either still generated or + // supplied by custom code (which lives in the canonical view) - there is nothing to restore. + if (constructors.Any(c => BackCompatHelper.ParametersMatch(c.Signature.Parameters, previousParameters)) + || CanonicalView.Constructors.Any(c => BackCompatHelper.ParametersMatch(c.Signature.Parameters, previousParameters))) { continue; } - if (TryBuildRestoredConstructor(previousConstructor, constructors, out var restoredConstructor)) + if (TryBuildRestoredConstructor(previousConstructor, constructors, restorablePropertyLookup, out var restoredConstructor)) { constructors.Add(restoredConstructor); CodeModelGenerator.Instance.Emitter.Info( @@ -848,6 +844,7 @@ protected internal override IReadOnlyList BuildConstructors private bool TryBuildRestoredConstructor( ConstructorProvider previousConstructor, IReadOnlyList currentConstructors, + Dictionary restorablePropertyLookup, [NotNullWhen(true)] out ConstructorProvider? restoredConstructor) { restoredConstructor = null; @@ -901,7 +898,7 @@ private bool TryBuildRestoredConstructor( } // Extra parameter: it must map to a settable property whose type is unchanged. - var property = FindRestorableProperty(previousParameter); + var property = FindRestorableProperty(restorablePropertyLookup, previousParameter); if (property == null) { return false; @@ -955,10 +952,20 @@ private static bool IsParameterSubsequence( IReadOnlyList subset, IReadOnlyList full) { + if (subset.Count > full.Count) + { + return false; + } + int matched = 0; foreach (var parameter in full) { - if (matched < subset.Count && subset[matched].Equals(parameter)) + if (matched == subset.Count) + { + break; + } + + if (subset[matched].Equals(parameter)) { matched++; } @@ -968,12 +975,15 @@ private static bool IsParameterSubsequence( } /// - /// Finds a settable public property that can receive the value of . - /// The property must have the same type (ignoring nullability) and either the same name or a name that - /// was changed via a codegen customization (matched by ). + /// Builds a lookup of settable public properties keyed by the name a constructor parameter would + /// use, so a dropped last-contract parameter can be resolved to its property in a single lookup. A + /// property is registered under its current parameter name and, when it was renamed via a codegen + /// customization, also under its (the direct name wins + /// on collision). /// - private PropertyProvider? FindRestorableProperty(ParameterProvider previousParameter) + private Dictionary BuildRestorablePropertyLookup() { + var lookup = new Dictionary(); foreach (var property in CanonicalView.Properties) { if (!IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null) @@ -981,13 +991,40 @@ private static bool IsParameterSubsequence( continue; } - var nameMatches = property.AsParameter.Name == previousParameter.Name - || (property.OriginalName != null && property.OriginalName.ToVariableName() == previousParameter.Name); + lookup[property.AsParameter.Name] = property; + } - if (nameMatches && property.Type.AreNamesEqual(previousParameter.Type)) + foreach (var property in CanonicalView.Properties) + { + if (!IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null + || property.OriginalName == null) { - return property; + continue; } + + var originalVariableName = property.OriginalName.ToVariableName(); + if (!lookup.ContainsKey(originalVariableName)) + { + lookup[originalVariableName] = property; + } + } + + return lookup; + } + + /// + /// Finds a settable public property that can receive the value of . + /// The property is resolved from by the parameter name and + /// must have the same type (ignoring nullability). + /// + private static PropertyProvider? FindRestorableProperty( + Dictionary restorablePropertyLookup, + ParameterProvider previousParameter) + { + if (restorablePropertyLookup.TryGetValue(previousParameter.Name, out var property) + && property.Type.AreNamesEqual(previousParameter.Type)) + { + return property; } return null; diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs index 0d0cfddde73..a02b5b079ff 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs @@ -365,6 +365,52 @@ public void IsMethodRemovalSuppressedMatchesDictionaryWithModelValue() Assert.IsFalse(baseline.IsMethodRemovalSuppressed("Ns.Types", "WithDictionary", [dictionaryOfInt])); } + [Test] + public void IsMethodRemovalSuppressedMatchesConstructor() + { + var baseline = Helpers.GetApiCompatBaselineFromFile(fileExtension: _fileExtension, method: "Baseline"); + + // Ns.Ctors..ctor(System.String, System.Int32) is accepted in the baseline. Constructors are + // recorded under the ".ctor" member name with their exact parameter types. + Assert.IsTrue(baseline.IsMethodRemovalSuppressed( + "Ns.Ctors", + ".ctor", + [new CSharpType(typeof(string)), new CSharpType(typeof(int))])); + + // The same types in a different order are a different constructor signature. + Assert.IsFalse(baseline.IsMethodRemovalSuppressed( + "Ns.Ctors", + ".ctor", + [new CSharpType(typeof(int)), new CSharpType(typeof(string))])); + + // A different arity must not match. + Assert.IsFalse(baseline.IsMethodRemovalSuppressed("Ns.Ctors", ".ctor", [new CSharpType(typeof(string))])); + + // A different parameter type in one slot must not match. + Assert.IsFalse(baseline.IsMethodRemovalSuppressed( + "Ns.Ctors", + ".ctor", + [new CSharpType(typeof(string)), new CSharpType(typeof(bool))])); + + // A different declaring type must not match. + Assert.IsFalse(baseline.IsMethodRemovalSuppressed( + "Ns.Other", + ".ctor", + [new CSharpType(typeof(string)), new CSharpType(typeof(int))])); + } + + [Test] + public void IsMethodRemovalSuppressedMatchesParameterlessConstructor() + { + var baseline = Helpers.GetApiCompatBaselineFromFile(fileExtension: _fileExtension, method: "Baseline"); + + // Ns.Ctors..ctor() has no parameters; the canonical signature is empty on both sides. + Assert.IsTrue(baseline.IsMethodRemovalSuppressed("Ns.Ctors", ".ctor", [])); + + // The parameterless constructor must not match a constructor overload that takes arguments. + Assert.IsFalse(baseline.IsMethodRemovalSuppressed("Ns.Missing", ".ctor", [])); + } + [Test] public void ReferencesSuppressedTypeMatchesDirectType() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.txt b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.txt index 725b47e3795..9769f92a6df 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.txt +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.txt @@ -3,6 +3,8 @@ TypesMustExist : Type 'Azure.AI.Projects.Agents.ProjectsAgentProtocol' does not MembersMustExist : Member 'public Azure.AI.Projects.Agents.ProtocolVersionRecord Azure.AI.Projects.Agents.ProjectsAgentsModelFactory.ProtocolVersionRecord(Azure.AI.Projects.Agents.ProjectsAgentProtocol, System.String)' does not exist in the implementation but it does exist in the contract. MembersMustExist : Member 'public System.Void Ns.Foo.Reset()' does not exist in the implementation but it does exist in the contract. MembersMustExist : Member 'public Ns.Foo..ctor(Ns.Kind, System.String)' does not exist in the implementation but it does exist in the contract. +MembersMustExist : Member 'public Ns.Ctors..ctor(System.String, System.Int32)' does not exist in the implementation but it does exist in the contract. +MembersMustExist : Member 'public Ns.Ctors..ctor()' does not exist in the implementation but it does exist in the contract. MembersMustExist : Member 'public Ns.Kind Ns.Foo.Kind.get()' does not exist in the implementation but it does exist in the contract. MembersMustExist : Member 'public System.Void Ns.Foo.Kind.set(Ns.Kind)' does not exist in the implementation but it does exist in the contract. MembersMustExist : Member 'public System.Void Ns.Foo.Configure(System.Collections.Generic.IDictionary, System.String)' does not exist in the implementation but it does exist in the contract. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.xml b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.xml index 6b8132093b2..b683202d4c6 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.xml +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/TestData/ApiCompatBaselineTests/Baseline.xml @@ -20,6 +20,14 @@ CP0002 M:Ns.Foo.#ctor(Ns.Kind,System.String) + + CP0002 + M:Ns.Ctors.#ctor(System.String,System.Int32) + + + CP0002 + M:Ns.Ctors.#ctor + CP0002 M:Ns.Foo.get_Kind From 040979e323c36a98c4ad44dd956e45c6aa9fdb98 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 31 Jul 2026 19:36:53 +0000 Subject: [PATCH 08/19] Address review: simplify log, merge property lookup loop, inline FindRestorableProperty Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../src/Providers/ModelProvider.cs | 51 ++++--------------- 1 file changed, 10 insertions(+), 41 deletions(-) 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 227314686a3..62f0d768d35 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 @@ -827,13 +827,13 @@ protected internal override IReadOnlyList BuildConstructors { constructors.Add(restoredConstructor); CodeModelGenerator.Instance.Emitter.Info( - $"Restored constructor '{Name}({string.Join(", ", previousParameters.Select(p => p.Type.ToString()))})' to match last contract.", + $"Restored constructor '{Name}({string.Join(", ", previousParameters.Select(p => p.Type.Name))})' to match last contract.", BackCompatibilityChangeCategory.ConstructorAddedFromLastContract); } else { CodeModelGenerator.Instance.Emitter.Info( - $"Could not restore constructor '{Name}({string.Join(", ", previousParameters.Select(p => p.Type.ToString()))})' from the last contract; a property name or type has changed.", + $"Could not restore constructor '{Name}({string.Join(", ", previousParameters.Select(p => p.Type.Name))})' from the last contract; a property name or type has changed.", BackCompatibilityChangeCategory.ConstructorAddedFromLastContractSkipped); } } @@ -898,8 +898,8 @@ private bool TryBuildRestoredConstructor( } // Extra parameter: it must map to a settable property whose type is unchanged. - var property = FindRestorableProperty(restorablePropertyLookup, previousParameter); - if (property == null) + if (!restorablePropertyLookup.TryGetValue(previousParameter.Name, out var property) + || !property.Type.AreNamesEqual(previousParameter.Type)) { return false; } @@ -974,13 +974,6 @@ private static bool IsParameterSubsequence( return matched == subset.Count; } - /// - /// Builds a lookup of settable public properties keyed by the name a constructor parameter would - /// use, so a dropped last-contract parameter can be resolved to its property in a single lookup. A - /// property is registered under its current parameter name and, when it was renamed via a codegen - /// customization, also under its (the direct name wins - /// on collision). - /// private Dictionary BuildRestorablePropertyLookup() { var lookup = new Dictionary(); @@ -992,44 +985,20 @@ private Dictionary BuildRestorablePropertyLookup() } lookup[property.AsParameter.Name] = property; - } - foreach (var property in CanonicalView.Properties) - { - if (!IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null - || property.OriginalName == null) + if (property.OriginalName != null) { - continue; - } - - var originalVariableName = property.OriginalName.ToVariableName(); - if (!lookup.ContainsKey(originalVariableName)) - { - lookup[originalVariableName] = property; + var originalVariableName = property.OriginalName.ToVariableName(); + if (!lookup.ContainsKey(originalVariableName)) + { + lookup[originalVariableName] = property; + } } } return lookup; } - /// - /// Finds a settable public property that can receive the value of . - /// The property is resolved from by the parameter name and - /// must have the same type (ignoring nullability). - /// - private static PropertyProvider? FindRestorableProperty( - Dictionary restorablePropertyLookup, - ParameterProvider previousParameter) - { - if (restorablePropertyLookup.TryGetValue(previousParameter.Name, out var property) - && property.Type.AreNamesEqual(previousParameter.Type)) - { - return property; - } - - return null; - } - /// /// Determines if this model should have a dual constructor pattern. /// This is needed when the model shares the same discriminator property name as its base model From 49c1b453601648d603ccd1217ec1aa8cb9b8221d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 31 Jul 2026 19:55:39 +0000 Subject: [PATCH 09/19] fix(csharp): restore ctor validation semantics in back-compat overloads Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../src/Providers/ModelProvider.cs | 25 +++++++++++-------- .../Providers/PartialMethodCustomization.cs | 13 +++++++--- .../ModelProviders/ModelProviderTests.cs | 8 +++--- ...ValueTypeConstructorParameterIsRestored.cs | 2 -- ...at_RenamedPropertyConstructorIsRestored.cs | 2 +- ...RequiredToOptionalConstructorIsRestored.cs | 2 +- 6 files changed, 29 insertions(+), 23 deletions(-) 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 62f0d768d35..50872bffe34 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 @@ -887,31 +887,34 @@ private bool TryBuildRestoredConstructor( if (targetIndex < targetParameters.Count && targetParameters[targetIndex].Equals(previousParameter)) { - // Kept parameter: forward the target constructor's parameter (with its existing - // validation) to the chained call so the emitted variable reference matches the - // parameter declared on this constructor. - var keptParameter = targetParameters[targetIndex]; + var keptParameter = PartialMethodCustomization.CloneParameterWithName( + targetParameters[targetIndex], + previousParameter.Name, + removeDefault: false, + validation: ParameterValidationType.None); restoredParameters.Add(keptParameter); initializerArguments.Add(keptParameter); targetIndex++; continue; } - // Extra parameter: it must map to a settable property whose type is unchanged. if (!restorablePropertyLookup.TryGetValue(previousParameter.Name, out var property) || !property.Type.AreNamesEqual(previousParameter.Type)) { return false; } - // Clone the current property's parameter under the previously published name (dropping any - // default value so the restored positional parameter matches the previous signature). This - // carries the current wire info and validation, keeping serialization and null-checking - // behavior consistent with the property. + var restoredValidation = previousParameter.Validation != ParameterValidationType.None + ? previousParameter.Validation + : !previousParameter.Type.IsValueType && !previousParameter.Type.IsNullable + ? ParameterValidationType.AssertNotNull + : ParameterValidationType.None; var restoredParameter = PartialMethodCustomization.CloneParameterWithName( - property.AsParameter, + previousParameter, previousParameter.Name, - removeDefault: true); + removeDefault: true, + validation: restoredValidation, + wireInfo: property.AsParameter.WireInfo); restoredParameters.Add(restoredParameter); extraAssignments.Add((property, restoredParameter)); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs index 0337b78c5a3..d186757a9f8 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs @@ -216,9 +216,14 @@ public static MethodSignature BuildPartialSignature( internal static ParameterProvider CloneParameterWithName( ParameterProvider source, string newName, - bool removeDefault) + bool removeDefault, + ParameterValidationType? validation = null, + WireInformation? wireInfo = null) { - if (source.Name == newName && !(removeDefault && source.DefaultValue != null)) + if (source.Name == newName + && !(removeDefault && source.DefaultValue != null) + && (validation == null || validation == source.Validation) + && (wireInfo == null || wireInfo == source.WireInfo)) { return source; } @@ -237,8 +242,8 @@ internal static ParameterProvider CloneParameterWithName( field: source.Field, initializationValue: source.InitializationValue, location: source.Location, - wireInfo: source.WireInfo, - validation: source.Validation, + wireInfo: wireInfo ?? source.WireInfo, + validation: validation ?? source.Validation, inputParameter: source.InputParameter) { SpreadSource = source.SpreadSource, diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index f6fa91ebe7f..ab2192b41fb 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -2393,11 +2393,11 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual("name", initializer.Arguments[0].ToDisplayString()); var body = restoredCtor.BodyStatements!.ToDisplayString(); + Assert.AreEqual(ParameterValidationType.None, restoredCtor.Signature.Parameters[0].Validation); + Assert.IsFalse(body.Contains("Argument.AssertNotNull(name"), $"Did not expect duplicated name validation in restored constructor, was: {body}"); Assert.IsTrue(body.Contains("Resources = resources"), $"Expected the body to assign Resources, was: {body}"); - // The extra parameter is cloned from the now-optional property, so it carries the property's - // (optional) validation - i.e. no null check is emitted for it. - Assert.AreEqual(ParameterValidationType.None, restoredCtor.Signature.Parameters[1].Validation); - Assert.IsFalse(body.Contains("Argument.AssertNotNull(resources"), $"Did not expect null validation for the optional resources parameter, was: {body}"); + Assert.AreEqual(ParameterValidationType.AssertNotNull, restoredCtor.Signature.Parameters[1].Validation); + Assert.IsTrue(body.Contains("Argument.AssertNotNull(resources"), $"Expected null validation for restored resources parameter, was: {body}"); // Validate the full generated model, including the restored constructor, against the expected output. var writer = new TypeProviderWriter(modelProvider); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs index 536d73912ae..c6e78ea90c8 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_OptionalValueTypeConstructorParameterIsRestored.cs @@ -28,8 +28,6 @@ internal MockInputModel(string name, int count, global::System.Collections.Gener public MockInputModel(string name, int count) : this(name) { - global::Sample.Argument.AssertNotNull(name, nameof(name)); - Count = count; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs index e1f22a9b915..89da6014257 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs @@ -28,7 +28,7 @@ internal MockInputModel(string name, string resourceList, global::System.Collect public MockInputModel(string name, string resources) : this(name) { - global::Sample.Argument.AssertNotNull(name, nameof(name)); + global::Sample.Argument.AssertNotNull(resources, nameof(resources)); ResourceList = resources; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs index fc878ffd638..e217b6c3fb7 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs @@ -28,7 +28,7 @@ internal MockInputModel(string name, string resources, global::System.Collection public MockInputModel(string name, string resources) : this(name) { - global::Sample.Argument.AssertNotNull(name, nameof(name)); + global::Sample.Argument.AssertNotNull(resources, nameof(resources)); Resources = resources; } From c58af0e66dba07d6a6c4dc1f86765aa352ead027 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 31 Jul 2026 15:39:16 -0500 Subject: [PATCH 10/19] Follow current property requiredness for restored back-compat ctor params Derive the restored dropped parameter's validation from the current property (property.AsParameter.Validation) instead of restoring the last contract's null-check. A dropped parameter maps to a now-optional property, so its validation is relaxed to match the current model. Also fix an ApiCompatBaselineTests assertion so it verifies the parameterless-vs-overload constructor case its comment describes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e815ae8d-8fc2-4c43-982e-1e98f84f4f54 --- .../src/Providers/ModelProvider.cs | 8 ++------ .../test/Providers/ModelProviders/ModelProviderTests.cs | 5 +++-- .../BackCompat_RenamedPropertyConstructorIsRestored.cs | 2 -- .../BackCompat_RequiredToOptionalConstructorIsRestored.cs | 2 -- .../test/SourceInput/ApiCompatBaselineTests.cs | 5 +++-- 5 files changed, 8 insertions(+), 14 deletions(-) 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 50872bffe34..0341ea08439 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 @@ -904,16 +904,12 @@ private bool TryBuildRestoredConstructor( return false; } - var restoredValidation = previousParameter.Validation != ParameterValidationType.None - ? previousParameter.Validation - : !previousParameter.Type.IsValueType && !previousParameter.Type.IsNullable - ? ParameterValidationType.AssertNotNull - : ParameterValidationType.None; + var restoredParameter = PartialMethodCustomization.CloneParameterWithName( previousParameter, previousParameter.Name, removeDefault: true, - validation: restoredValidation, + validation: property.AsParameter.Validation, wireInfo: property.AsParameter.WireInfo); restoredParameters.Add(restoredParameter); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index ab2192b41fb..e573896ef97 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -2396,8 +2396,9 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual(ParameterValidationType.None, restoredCtor.Signature.Parameters[0].Validation); Assert.IsFalse(body.Contains("Argument.AssertNotNull(name"), $"Did not expect duplicated name validation in restored constructor, was: {body}"); Assert.IsTrue(body.Contains("Resources = resources"), $"Expected the body to assign Resources, was: {body}"); - Assert.AreEqual(ParameterValidationType.AssertNotNull, restoredCtor.Signature.Parameters[1].Validation); - Assert.IsTrue(body.Contains("Argument.AssertNotNull(resources"), $"Expected null validation for restored resources parameter, was: {body}"); + // "resources" is now an optional property, so the restored back-compat overload does not null-check it. + Assert.AreEqual(ParameterValidationType.None, restoredCtor.Signature.Parameters[1].Validation); + Assert.IsFalse(body.Contains("Argument.AssertNotNull(resources"), $"Did not expect null validation for the now-optional resources parameter, was: {body}"); // Validate the full generated model, including the restored constructor, against the expected output. var writer = new TypeProviderWriter(modelProvider); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs index 89da6014257..beb896a14e3 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RenamedPropertyConstructorIsRestored.cs @@ -28,8 +28,6 @@ internal MockInputModel(string name, string resourceList, global::System.Collect public MockInputModel(string name, string resources) : this(name) { - global::Sample.Argument.AssertNotNull(resources, nameof(resources)); - ResourceList = resources; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs index e217b6c3fb7..605759a88bd 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_RequiredToOptionalConstructorIsRestored.cs @@ -28,8 +28,6 @@ internal MockInputModel(string name, string resources, global::System.Collection public MockInputModel(string name, string resources) : this(name) { - global::Sample.Argument.AssertNotNull(resources, nameof(resources)); - Resources = resources; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs index a02b5b079ff..592b12204e2 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/SourceInput/ApiCompatBaselineTests.cs @@ -407,8 +407,9 @@ public void IsMethodRemovalSuppressedMatchesParameterlessConstructor() // Ns.Ctors..ctor() has no parameters; the canonical signature is empty on both sides. Assert.IsTrue(baseline.IsMethodRemovalSuppressed("Ns.Ctors", ".ctor", [])); - // The parameterless constructor must not match a constructor overload that takes arguments. - Assert.IsFalse(baseline.IsMethodRemovalSuppressed("Ns.Missing", ".ctor", [])); + // Ns.Foo only has a constructor overload that takes arguments in the baseline, so querying + // its parameterless constructor must not match that overload. + Assert.IsFalse(baseline.IsMethodRemovalSuppressed("Ns.Foo", ".ctor", [])); } [Test] From 93729fead3c46b3f536190a6fe58d10404678236 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 31 Jul 2026 15:50:57 -0500 Subject: [PATCH 11/19] Clone restored back-compat ctor param from the current property Build the restored dropped parameter from property.AsParameter instead of the last contract's parameter, so its type, validation, and wire info all follow the current (now-optional) property automatically. This removes the per-field overrides and the now-unused wireInfo parameter on CloneParameterWithName. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e815ae8d-8fc2-4c43-982e-1e98f84f4f54 --- .../src/Providers/ModelProvider.cs | 7 ++----- .../src/Providers/PartialMethodCustomization.cs | 8 +++----- 2 files changed, 5 insertions(+), 10 deletions(-) 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 0341ea08439..a81ffde07fe 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 @@ -904,13 +904,10 @@ private bool TryBuildRestoredConstructor( return false; } - var restoredParameter = PartialMethodCustomization.CloneParameterWithName( - previousParameter, + property.AsParameter, previousParameter.Name, - removeDefault: true, - validation: property.AsParameter.Validation, - wireInfo: property.AsParameter.WireInfo); + removeDefault: true); restoredParameters.Add(restoredParameter); extraAssignments.Add((property, restoredParameter)); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs index d186757a9f8..5dbcb7f0aa1 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs @@ -217,13 +217,11 @@ internal static ParameterProvider CloneParameterWithName( ParameterProvider source, string newName, bool removeDefault, - ParameterValidationType? validation = null, - WireInformation? wireInfo = null) + ParameterValidationType? validation = null) { if (source.Name == newName && !(removeDefault && source.DefaultValue != null) - && (validation == null || validation == source.Validation) - && (wireInfo == null || wireInfo == source.WireInfo)) + && (validation == null || validation == source.Validation)) { return source; } @@ -242,7 +240,7 @@ internal static ParameterProvider CloneParameterWithName( field: source.Field, initializationValue: source.InitializationValue, location: source.Location, - wireInfo: wireInfo ?? source.WireInfo, + wireInfo: source.WireInfo, validation: validation ?? source.Validation, inputParameter: source.InputParameter) { From cfcad573b309a090aeeb2075f274b609b6b946cb Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 31 Jul 2026 16:00:38 -0500 Subject: [PATCH 12/19] Use TryAdd for the rename fallback in BuildRestorablePropertyLookup Replace the ContainsKey guard with Dictionary.TryAdd when registering a property's pre-rename name, so a duplicate key can never throw. The direct (current) parameter name still uses an indexer assignment so it keeps precedence over another property's pre-rename name. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e815ae8d-8fc2-4c43-982e-1e98f84f4f54 --- .../src/Providers/ModelProvider.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) 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 a81ffde07fe..dc895bfb035 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 @@ -984,11 +984,7 @@ private Dictionary BuildRestorablePropertyLookup() if (property.OriginalName != null) { - var originalVariableName = property.OriginalName.ToVariableName(); - if (!lookup.ContainsKey(originalVariableName)) - { - lookup[originalVariableName] = property; - } + lookup.TryAdd(property.OriginalName.ToVariableName(), property); } } From 038e1cee5b9134d839f05c4ba0bf82f1e69bad25 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Mon, 3 Aug 2026 10:28:40 -0500 Subject: [PATCH 13/19] Pass through constructors in serialization partial back-compat Override BuildConstructorsForBackCompatibility in MrwSerializationTypeDefinition to return the original constructors, mirroring the existing methods override. The model is emitted as a partial class split across the model and serialization files; both share the same LastContractView, so back-compat restoration would run on each. Keeping the serialization partial's constructors untouched ensures the restored back-compat constructors are only emitted on the model partial, avoiding duplication. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e815ae8d-8fc2-4c43-982e-1e98f84f4f54 --- .../src/Providers/MrwSerializationTypeDefinition.cs | 3 +++ .../src/Providers/ModelProvider.cs | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) 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 5fe0a90bcfc..1d7ea756d1c 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 @@ -125,6 +125,9 @@ public MrwSerializationTypeDefinition(InputModelType inputModel, ModelProvider m protected override IReadOnlyList BuildMethodsForBackCompatibility(IEnumerable originalMethods) => [.. originalMethods]; + protected override IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable originalConstructors) + => [.. originalConstructors]; + private ConstructorProvider SerializationConstructor => _serializationConstructor ??= _model.FullConstructor; private PropertyProvider[] AdditionalProperties => _additionalProperties.Value; 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 dc895bfb035..19c2becea6f 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 @@ -798,7 +798,7 @@ protected internal override IReadOnlyList BuildConstructors foreach (var previousConstructor in previousConstructors) { - if (!IsPublicApi(previousConstructor.Signature.Modifiers)) + if (!MethodProviderHelpers.IsPublicApi(previousConstructor.Signature.Modifiers)) { continue; } @@ -855,7 +855,7 @@ private bool TryBuildRestoredConstructor( ConstructorProvider? targetConstructor = null; foreach (var candidate in currentConstructors) { - if (!IsPublicApi(candidate.Signature.Modifiers) + if (!MethodProviderHelpers.IsPublicApi(candidate.Signature.Modifiers) || candidate.Signature.Parameters.Count >= previousParameters.Count) { continue; @@ -975,7 +975,7 @@ private Dictionary BuildRestorablePropertyLookup() var lookup = new Dictionary(); foreach (var property in CanonicalView.Properties) { - if (!IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null) + if (!MethodProviderHelpers.IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null) { continue; } From 34901ffbff6514a696ee0192f83745377c8d9f19 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Tue, 4 Aug 2026 11:51:12 -0500 Subject: [PATCH 14/19] fix: include custom ctors --- .../ScmModelProvider/ScmModelProviderTests.cs | 40 +++++++ ...estoredRemovesMockingConstructor(Model).cs | 31 +++++ ...emovesMockingConstructor(Serialization).cs | 111 ++++++++++++++++++ .../BaseModel.cs | 10 ++ .../src/Providers/ModelProvider.cs | 67 ++++++++++- .../ModelProviders/ModelProviderTests.cs | 62 ++++++++++ ...essConstructorChainsToPublicConstructor.cs | 34 ++++++ .../MockInputModel.cs | 10 ++ ...Compat_ParameterlessConstructorRestored.cs | 31 +++++ .../BaseModel.cs | 10 ++ 10 files changed, 402 insertions(+), 4 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor(Model).cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor(Serialization).cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor/BaseModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorChainsToPublicConstructor.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorChainsToPublicConstructor/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorRestored.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorRestored/BaseModel.cs diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs index eff1d0ac65b..4aefe7bb911 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs @@ -205,6 +205,46 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); } + [Test] + public async Task BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor() + { + // The last contract published a parameterless `protected BaseModel()`. The current generation + // makes the discriminator required, so the abstract base's initialization constructor now takes a + // parameter and the parameterless constructor is dropped. It is restored, and the generated + // parameterless mocking constructor on the serialization partial is removed to avoid a duplicate. + var derivedInputModel = InputFactory.Model( + "derivedModel", + discriminatedKind: "one", + properties: + [ + InputFactory.Property("kind", InputPrimitiveType.String, isRequired: true, isDiscriminator: true) + ]); + var inputModel = InputFactory.Model( + "baseModel", + properties: + [ + InputFactory.Property("kind", InputPrimitiveType.String, isRequired: true, isDiscriminator: true) + ], + discriminatedModels: new Dictionary() { { "one", derivedInputModel } }); + + await MockHelpers.LoadMockGeneratorAsync( + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync(), + inputModels: () => [inputModel]); + + var model = ScmCodeModelGenerator.Instance.OutputLibrary.TypeProviders + .OfType().Single(t => t.Name == "BaseModel"); + + model.ProcessTypeForBackCompatibility(); + + // The model gains the restored standalone parameterless constructor. + var modelContent = new TypeProviderWriter(model).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile("Model"), modelContent); + + // The serialization partial no longer carries the parameterless mocking constructor (avoids CS0111). + var serializationContent = new TypeProviderWriter(model.SerializationProviders.Single()).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile("Serialization"), serializationContent); + } + [Test] public void TestDynamicModelWithUnionAdditionalProps() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor(Model).cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor(Model).cs new file mode 100644 index 00000000000..621e16a44c3 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor(Model).cs @@ -0,0 +1,31 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; + +namespace Sample.Models +{ + public abstract partial class BaseModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + private protected BaseModel(string kind) + { + Kind = kind; + } + + internal BaseModel(string kind, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Kind = kind; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + protected BaseModel() : this(default) + { + } + + internal string Kind { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor(Serialization).cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor(Serialization).cs new file mode 100644 index 00000000000..31ddf954a8d --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor(Serialization).cs @@ -0,0 +1,111 @@ +// + +#nullable disable + +using System; +using System.ClientModel.Primitives; +using System.Text.Json; +using Sample; + +namespace Sample.Models +{ + [global::System.ClientModel.Primitives.PersistableModelProxyAttribute(typeof(global::Sample.Models.UnknownBaseModel))] + public abstract partial class BaseModel : global::System.ClientModel.Primitives.IJsonModel + { + protected virtual global::Sample.Models.BaseModel PersistableModelCreateCore(global::System.BinaryData data, global::System.ClientModel.Primitives.ModelReaderWriterOptions options) + { + string format = (options.Format == "W") ? ((global::System.ClientModel.Primitives.IPersistableModel)this).GetFormatFromOptions(options) : options.Format; + switch (format) + { + case "J": + using (global::System.Text.Json.JsonDocument document = global::System.Text.Json.JsonDocument.Parse(data, global::Sample.ModelSerializationExtensions.JsonDocumentOptions)) + { + return global::Sample.Models.BaseModel.DeserializeBaseModel(document.RootElement, options); + } + default: + throw new global::System.FormatException($"The model {nameof(global::Sample.Models.BaseModel)} does not support reading '{options.Format}' format."); + } + } + + protected virtual global::System.BinaryData PersistableModelWriteCore(global::System.ClientModel.Primitives.ModelReaderWriterOptions options) + { + string format = (options.Format == "W") ? ((global::System.ClientModel.Primitives.IPersistableModel)this).GetFormatFromOptions(options) : options.Format; + switch (format) + { + case "J": + return global::System.ClientModel.Primitives.ModelReaderWriter.Write(this, options, global::Sample.SampleContext.Default); + default: + throw new global::System.FormatException($"The model {nameof(global::Sample.Models.BaseModel)} does not support writing '{options.Format}' format."); + } + } + + global::System.BinaryData global::System.ClientModel.Primitives.IPersistableModel.Write(global::System.ClientModel.Primitives.ModelReaderWriterOptions options) => this.PersistableModelWriteCore(options); + + global::Sample.Models.BaseModel global::System.ClientModel.Primitives.IPersistableModel.Create(global::System.BinaryData data, global::System.ClientModel.Primitives.ModelReaderWriterOptions options) => this.PersistableModelCreateCore(data, options); + + string global::System.ClientModel.Primitives.IPersistableModel.GetFormatFromOptions(global::System.ClientModel.Primitives.ModelReaderWriterOptions options) => "J"; + + void global::System.ClientModel.Primitives.IJsonModel.Write(global::System.Text.Json.Utf8JsonWriter writer, global::System.ClientModel.Primitives.ModelReaderWriterOptions options) + { + writer.WriteStartObject(); + this.JsonModelWriteCore(writer, options); + writer.WriteEndObject(); + } + + protected virtual void JsonModelWriteCore(global::System.Text.Json.Utf8JsonWriter writer, global::System.ClientModel.Primitives.ModelReaderWriterOptions options) + { + string format = (options.Format == "W") ? ((global::System.ClientModel.Primitives.IPersistableModel)this).GetFormatFromOptions(options) : options.Format; + if ((format != "J")) + { + throw new global::System.FormatException($"The model {nameof(global::Sample.Models.BaseModel)} does not support writing '{format}' format."); + } + writer.WritePropertyName("kind"u8); + writer.WriteStringValue(Kind); + if (((options.Format != "W") && (_additionalBinaryDataProperties != null))) + { + foreach (var item in _additionalBinaryDataProperties) + { + writer.WritePropertyName(item.Key); +#if NET6_0_OR_GREATER + writer.WriteRawValue(item.Value); +#else + using (global::System.Text.Json.JsonDocument document = global::System.Text.Json.JsonDocument.Parse(item.Value)) + { + global::System.Text.Json.JsonSerializer.Serialize(writer, document.RootElement); + } +#endif + } + } + } + + global::Sample.Models.BaseModel global::System.ClientModel.Primitives.IJsonModel.Create(ref global::System.Text.Json.Utf8JsonReader reader, global::System.ClientModel.Primitives.ModelReaderWriterOptions options) => this.JsonModelCreateCore(ref reader, options); + + protected virtual global::Sample.Models.BaseModel JsonModelCreateCore(ref global::System.Text.Json.Utf8JsonReader reader, global::System.ClientModel.Primitives.ModelReaderWriterOptions options) + { + string format = (options.Format == "W") ? ((global::System.ClientModel.Primitives.IPersistableModel)this).GetFormatFromOptions(options) : options.Format; + if ((format != "J")) + { + throw new global::System.FormatException($"The model {nameof(global::Sample.Models.BaseModel)} does not support reading '{format}' format."); + } + using global::System.Text.Json.JsonDocument document = global::System.Text.Json.JsonDocument.ParseValue(ref reader); + return global::Sample.Models.BaseModel.DeserializeBaseModel(document.RootElement, options); + } + + internal static global::Sample.Models.BaseModel DeserializeBaseModel(global::System.Text.Json.JsonElement element, global::System.ClientModel.Primitives.ModelReaderWriterOptions options) + { + if ((element.ValueKind == global::System.Text.Json.JsonValueKind.Null)) + { + return null; + } + if (element.TryGetProperty("kind"u8, out global::System.Text.Json.JsonElement discriminator)) + { + switch (discriminator.GetString()) + { + case "one": + return global::Sample.Models.DerivedModel.DeserializeDerivedModel(element, options); + } + } + return global::Sample.Models.UnknownBaseModel.DeserializeUnknownBaseModel(element, options); + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor/BaseModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor/BaseModel.cs new file mode 100644 index 00000000000..2e50e2419d5 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_ParameterlessConstructorRestoredRemovesMockingConstructor/BaseModel.cs @@ -0,0 +1,10 @@ +namespace Sample.Models +{ + public abstract partial class BaseModel + { + /// Initializes a new instance of BaseModel. + protected BaseModel() + { + } + } +} 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 19c2becea6f..35019f3fb4d 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 @@ -795,6 +795,9 @@ protected internal override IReadOnlyList BuildConstructors var constructors = new List(base.BuildConstructorsForBackCompatibility(originalConstructors)); var restorablePropertyLookup = BuildRestorablePropertyLookup(); + IReadOnlyList candidateConstructors = CustomCodeView?.Constructors is { Count: > 0 } customConstructors + ? [.. constructors, .. customConstructors] + : constructors; foreach (var previousConstructor in previousConstructors) { @@ -805,13 +808,28 @@ protected internal override IReadOnlyList BuildConstructors var previousParameters = previousConstructor.Signature.Parameters; - if (previousParameters.Count == 0) + if (BackCompatHelper.IsConstructorRemovalAcceptedInBaseline(this, previousConstructor.Signature)) { continue; } - if (BackCompatHelper.IsConstructorRemovalAcceptedInBaseline(this, previousConstructor.Signature)) + // A previously published accessible parameterless constructor is dropped when the current + // generation makes a property required. Restore it and drop the generated mocking constructor + // so it is not a duplicate. An accessible parameterless constructor (generated or custom code) + // counts as already present; an inaccessible generated mocking constructor does not. + if (previousParameters.Count == 0) { + if (!constructors.Any(c => c.Signature.Parameters.Count == 0 && MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers)) + && !CanonicalView.Constructors.Any(c => c.Signature.Parameters.Count == 0 && MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers))) + { + var parameterlessConstructor = BuildBackCompatParameterlessConstructor(previousConstructor, candidateConstructors); + RemoveGeneratedMockingConstructor(constructors); + constructors.Add(parameterlessConstructor); + CodeModelGenerator.Instance.Emitter.Info( + $"Restored parameterless constructor '{Name}()' to match last contract.", + BackCompatibilityChangeCategory.ConstructorAddedFromLastContract); + } + continue; } @@ -823,7 +841,7 @@ protected internal override IReadOnlyList BuildConstructors continue; } - if (TryBuildRestoredConstructor(previousConstructor, constructors, restorablePropertyLookup, out var restoredConstructor)) + if (TryBuildRestoredConstructor(previousConstructor, candidateConstructors, restorablePropertyLookup, out var restoredConstructor)) { constructors.Add(restoredConstructor); CodeModelGenerator.Instance.Emitter.Info( @@ -944,6 +962,47 @@ private bool TryBuildRestoredConstructor( return true; } + private ConstructorProvider BuildBackCompatParameterlessConstructor( + ConstructorProvider previousConstructor, + IReadOnlyList currentConstructors) + { + // Prefer the public or protected constructor with the fewest required parameters, then a + // private-protected one; a null target yields a standalone constructor. + const MethodSignatureModifiers privateProtected = MethodSignatureModifiers.Private | MethodSignatureModifiers.Protected; + var target = currentConstructors + .Where(c => c.Signature.Parameters.Count > 0 + && (MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers) || (c.Signature.Modifiers & privateProtected) == privateProtected)) + .MinBy(c => (MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers) ? 0 : 1, c.Signature.Parameters.Count(p => p.DefaultValue is null))); + + ConstructorInitializer? initializer = target is null + ? null + : new ConstructorInitializer(false, [.. target.Signature.Parameters.Select(_ => Snippet.Default)]); + + var signature = new ConstructorSignature( + Type, + $"Initializes a new instance of {Type:C}", + previousConstructor.Signature.Modifiers, + parameters: [], + initializer: initializer); + + return new ConstructorProvider(signature, MethodBodyStatement.Empty, this); + } + + private void RemoveGeneratedMockingConstructor(List constructors) + { + constructors.RemoveAll(c => c.Signature.Parameters.Count == 0); + + foreach (var serializationProvider in SerializationProviders) + { + var serializationConstructors = serializationProvider.Constructors; + if (serializationConstructors.Any(c => c.Signature.Parameters.Count == 0)) + { + serializationProvider.Update( + constructors: [.. serializationConstructors.Where(c => c.Signature.Parameters.Count != 0)]); + } + } + } + private static bool IsParameterSubsequence( IReadOnlyList subset, IReadOnlyList full) @@ -980,7 +1039,7 @@ private Dictionary BuildRestorablePropertyLookup() continue; } - lookup[property.AsParameter.Name] = property; + lookup.TryAdd(property.AsParameter.Name, property); if (property.OriginalName != null) { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index e573896ef97..c8a533c04b0 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -2346,6 +2346,68 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual("baseProp", publicConstructor!.Signature.Parameters[0].Name); } + [Test] + public async Task BackCompat_ParameterlessConstructorRestored() + { + // The last contract published a parameterless `protected BaseModel()`. The current generation + // makes the discriminator required, so the initialization constructor now takes a parameter and + // the parameterless constructor is dropped. It should be restored, chaining to the private-protected + // initialization constructor (no public or protected constructor exists on the abstract base). + var derivedInputModel = InputFactory.Model( + "DerivedModel", + discriminatedKind: "one", + properties: + [ + InputFactory.Property("kind", InputPrimitiveType.String, isRequired: true, isDiscriminator: true) + ]); + var inputModel = InputFactory.Model( + "BaseModel", + properties: + [ + InputFactory.Property("kind", InputPrimitiveType.String, isRequired: true, isDiscriminator: true) + ], + discriminatedModels: new Dictionary() { { "one", derivedInputModel } }); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "BaseModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + + modelProvider!.ProcessTypeForBackCompatibility(); + + var file = new TypeProviderWriter(modelProvider).Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); + } + + [Test] + public async Task BackCompat_ParameterlessConstructorChainsToPublicConstructor() + { + // The last contract published a parameterless `public MockInputModel()`. The current generation + // makes "name" required, so the public initialization constructor now takes it. The restored + // parameterless constructor chains to that public constructor (not the internal full constructor). + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input | InputModelTypeUsage.Json, + properties: + [ + InputFactory.Property("name", InputPrimitiveType.String, isRequired: true) + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider; + Assert.IsNotNull(modelProvider); + + modelProvider!.ProcessTypeForBackCompatibility(); + + var file = new TypeProviderWriter(modelProvider).Write(); + Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); + } + [Test] public async Task BackCompat_RequiredToOptionalConstructorIsRestored() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorChainsToPublicConstructor.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorChainsToPublicConstructor.cs new file mode 100644 index 00000000000..75ea9c12218 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorChainsToPublicConstructor.cs @@ -0,0 +1,34 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; +using Sample; + +namespace Sample.Models +{ + public partial class MockInputModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + public MockInputModel(string name) + { + global::Sample.Argument.AssertNotNull(name, nameof(name)); + + Name = name; + } + + internal MockInputModel(string name, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Name = name; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + public MockInputModel() : this(default) + { + } + + public string Name { get; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorChainsToPublicConstructor/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorChainsToPublicConstructor/MockInputModel.cs new file mode 100644 index 00000000000..bd04c9c92cc --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorChainsToPublicConstructor/MockInputModel.cs @@ -0,0 +1,10 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + /// Initializes a new instance of MockInputModel. + public MockInputModel() + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorRestored.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorRestored.cs new file mode 100644 index 00000000000..621e16a44c3 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorRestored.cs @@ -0,0 +1,31 @@ +// + +#nullable disable + +using System; +using System.Collections.Generic; + +namespace Sample.Models +{ + public abstract partial class BaseModel + { + private protected readonly global::System.Collections.Generic.IDictionary _additionalBinaryDataProperties; + + private protected BaseModel(string kind) + { + Kind = kind; + } + + internal BaseModel(string kind, global::System.Collections.Generic.IDictionary additionalBinaryDataProperties) + { + Kind = kind; + _additionalBinaryDataProperties = additionalBinaryDataProperties; + } + + protected BaseModel() : this(default) + { + } + + internal string Kind { get; set; } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorRestored/BaseModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorRestored/BaseModel.cs new file mode 100644 index 00000000000..2e50e2419d5 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ParameterlessConstructorRestored/BaseModel.cs @@ -0,0 +1,10 @@ +namespace Sample.Models +{ + public abstract partial class BaseModel + { + /// Initializes a new instance of BaseModel. + protected BaseModel() + { + } + } +} From 88060f1a461fc200de9cc67679652c7793dd9874 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:28:12 +0000 Subject: [PATCH 15/19] docs: describe back-compat model constructor restoration Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../generator/docs/backward-compatibility.md | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/packages/http-client-csharp/generator/docs/backward-compatibility.md b/packages/http-client-csharp/generator/docs/backward-compatibility.md index fe1b761caf6..c84868b0418 100644 --- a/packages/http-client-csharp/generator/docs/backward-compatibility.md +++ b/packages/http-client-csharp/generator/docs/backward-compatibility.md @@ -19,6 +19,7 @@ - [API Version Enum](#api-version-enum) - [Non-abstract Base Models](#non-abstract-base-models) - [Model Constructors](#model-constructors) + - [Required Property Becomes Optional](#scenario-required-property-becomes-optional) - [Parameter Naming](#parameter-naming) - [Page Size Parameter Casing Correction](#scenario-page-size-parameter-casing-correction) - [Top Parameter Conversion to MaxCount](#scenario-top-parameter-conversion-to-maxcount) @@ -589,6 +590,59 @@ public abstract partial class SearchIndexerDataIdentity - The modifier is changed from `private protected` to `public` - No additional constructors are generated; only the accessibility is adjusted +#### Scenario: Required Property Becomes Optional + +**Description:** When a required model property becomes optional, the current initialization constructor no longer includes that property. To preserve source compatibility for callers that construct the model positionally, the generator restores the previously published public constructor as an overload. The restored overload chains to the closest current public constructor and assigns the now-optional property. + +**Example:** + +Previous version required both properties: + +```csharp +public partial class Widget +{ + public Widget(string name, string description) + { + Name = name; + Description = description; + } + + public string Name { get; } + public string Description { get; } +} +``` + +Current TypeSpec makes `description` optional: + +```csharp +public partial class Widget +{ + public Widget(string name) + { + Name = name; + } + + public string Name { get; } + public string Description { get; set; } +} +``` + +**Generated Compatibility Result:** + +```csharp +public Widget(string name, string description) : this(name) +{ + Description = description; +} +``` + +**Key Points:** + +- The previous constructor must be public and no generated or custom constructor may already have the same parameters. +- Every parameter removed from the current constructor must map to a public, settable property with the same type. Properties renamed through a code-generation customization are supported. +- The current constructor used for chaining must have parameters that match an in-order subset of the previous constructor's parameters. +- If the constructor removal is accepted in an ApiCompat baseline, the generator does not restore it. + ### Parameter Naming The generator maintains backward compatibility for parameter names to ensure that existing code continues to compile when parameter names are corrected, standardized, or converted to follow naming conventions. From f4de5be1b676450fbbfecfcd06f58a3b4f902522 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:53:16 +0000 Subject: [PATCH 16/19] docs: describe parameterless constructor compatibility Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../generator/docs/backward-compatibility.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/packages/http-client-csharp/generator/docs/backward-compatibility.md b/packages/http-client-csharp/generator/docs/backward-compatibility.md index c84868b0418..d0fe45cb135 100644 --- a/packages/http-client-csharp/generator/docs/backward-compatibility.md +++ b/packages/http-client-csharp/generator/docs/backward-compatibility.md @@ -20,6 +20,7 @@ - [Non-abstract Base Models](#non-abstract-base-models) - [Model Constructors](#model-constructors) - [Required Property Becomes Optional](#scenario-required-property-becomes-optional) + - [Parameterless Constructor Becomes Parameterized](#scenario-parameterless-constructor-becomes-parameterized) - [Parameter Naming](#parameter-naming) - [Page Size Parameter Casing Correction](#scenario-page-size-parameter-casing-correction) - [Top Parameter Conversion to MaxCount](#scenario-top-parameter-conversion-to-maxcount) @@ -643,6 +644,55 @@ public Widget(string name, string description) : this(name) - The current constructor used for chaining must have parameters that match an in-order subset of the previous constructor's parameters. - If the constructor removal is accepted in an ApiCompat baseline, the generator does not restore it. +#### Scenario: Parameterless Constructor Becomes Parameterized + +**Description:** When a model previously exposed an accessible parameterless constructor and a property later becomes required, generation replaces the parameterless constructor with one that accepts the required property. The generator restores the previous parameterless constructor and chains it to an appropriate current constructor with `default` values. + +**Example:** + +Previous version exposed a parameterless constructor: + +```csharp +public partial class Widget +{ + public Widget() + { + } + + public string Name { get; set; } +} +``` + +Current TypeSpec makes `name` required: + +```csharp +public partial class Widget +{ + public Widget(string name) + { + Name = name; + } + + public string Name { get; } +} +``` + +**Generated Compatibility Result:** + +```csharp +public Widget() : this(default) +{ +} +``` + +**Key Points:** + +- The previous parameterless constructor must be accessible and no accessible generated or custom parameterless constructor may already exist. +- The restored constructor retains the previous accessibility. +- The generator prefers an accessible current constructor with the fewest required parameters as the chain target. When necessary, it can chain to a `private protected` initialization constructor. +- The generated parameterless mocking constructor is removed so it does not duplicate the restored constructor. +- If the constructor removal is accepted in an ApiCompat baseline, the generator does not restore it. + ### Parameter Naming The generator maintains backward compatibility for parameter names to ensure that existing code continues to compile when parameter names are corrected, standardized, or converted to follow naming conventions. From d98e70b1514a89e3c24193bb6e7382d563cd05b7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 4 Aug 2026 18:01:16 +0000 Subject: [PATCH 17/19] docs(csharp): use protected constructor back-compat example Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com> --- .../generator/docs/backward-compatibility.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/http-client-csharp/generator/docs/backward-compatibility.md b/packages/http-client-csharp/generator/docs/backward-compatibility.md index d0fe45cb135..f2326a2f624 100644 --- a/packages/http-client-csharp/generator/docs/backward-compatibility.md +++ b/packages/http-client-csharp/generator/docs/backward-compatibility.md @@ -655,7 +655,7 @@ Previous version exposed a parameterless constructor: ```csharp public partial class Widget { - public Widget() + protected Widget() { } @@ -668,7 +668,7 @@ Current TypeSpec makes `name` required: ```csharp public partial class Widget { - public Widget(string name) + protected Widget(string name) { Name = name; } @@ -680,7 +680,7 @@ public partial class Widget **Generated Compatibility Result:** ```csharp -public Widget() : this(default) +protected Widget() : this(default) { } ``` From 4779678868b77da16a4f9faf20934b2adcd09fc4 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Tue, 4 Aug 2026 14:49:41 -0500 Subject: [PATCH 18/19] skip structs --- .../ScmModelProvider/ScmModelProviderTests.cs | 29 +++++++++++++++++++ .../StructModel.cs | 10 +++++++ .../src/Providers/ModelProvider.cs | 2 +- 3 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_StructParameterlessConstructorNotMovedFromSerialization/StructModel.cs diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs index 4aefe7bb911..08a2112faf5 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs @@ -245,6 +245,35 @@ await MockHelpers.LoadMockGeneratorAsync( Assert.AreEqual(Helpers.GetExpectedFromFile("Serialization"), serializationContent); } + [Test] + public async Task BackCompat_StructParameterlessConstructorNotMovedFromSerialization() + { + // A struct always exposes a public parameterless constructor via its serialization (mocking) + // constructor, so the last contract's parameterless constructor is already present. It must not + // be moved onto the model partial, which would be pointless churn with no public API change. + var inputModel = InputFactory.Model( + "structModel", + modelAsStruct: true, + properties: + [ + InputFactory.Property("prop", InputPrimitiveType.String, isRequired: true) + ]); + + await MockHelpers.LoadMockGeneratorAsync( + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync(), + inputModels: () => [inputModel]); + + var model = ScmCodeModelGenerator.Instance.OutputLibrary.TypeProviders + .OfType().Single(t => t.Name == "StructModel"); + + model.ProcessTypeForBackCompatibility(); + + Assert.IsFalse(model.Constructors.Any(c => c.Signature.Parameters.Count == 0), + "Struct model must not gain a parameterless constructor on the model partial."); + Assert.IsTrue(model.SerializationProviders.Single().Constructors.Any(c => c.Signature.Parameters.Count == 0), + "Struct serialization partial must retain its parameterless constructor."); + } + [Test] public void TestDynamicModelWithUnionAdditionalProps() { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_StructParameterlessConstructorNotMovedFromSerialization/StructModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_StructParameterlessConstructorNotMovedFromSerialization/StructModel.cs new file mode 100644 index 00000000000..7aa013b79fe --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/TestData/ScmModelProviderTests/BackCompat_StructParameterlessConstructorNotMovedFromSerialization/StructModel.cs @@ -0,0 +1,10 @@ +namespace Sample.Models +{ + public partial struct StructModel + { + /// Initializes a new instance of StructModel. + public StructModel() + { + } + } +} 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 35019f3fb4d..73d199897e1 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 @@ -817,7 +817,7 @@ protected internal override IReadOnlyList BuildConstructors // generation makes a property required. Restore it and drop the generated mocking constructor // so it is not a duplicate. An accessible parameterless constructor (generated or custom code) // counts as already present; an inaccessible generated mocking constructor does not. - if (previousParameters.Count == 0) + if (!Type.IsStruct && previousParameters.Count == 0) { if (!constructors.Any(c => c.Signature.Parameters.Count == 0 && MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers)) && !CanonicalView.Constructors.Any(c => c.Signature.Parameters.Count == 0 && MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers))) From 93bfb97c23af265fa4a624c999d86570a39e93bf Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Wed, 5 Aug 2026 11:39:01 -0500 Subject: [PATCH 19/19] Fix build: use MethodSignatureHelper.IsPublicApi after main moved the method --- .../src/Providers/ModelProvider.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 575521e530d..43d2559a48b 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 @@ -800,7 +800,7 @@ protected internal override IReadOnlyList BuildConstructors foreach (var previousConstructor in previousConstructors) { - if (!MethodProviderHelpers.IsPublicApi(previousConstructor.Signature.Modifiers)) + if (!MethodSignatureHelper.IsPublicApi(previousConstructor.Signature.Modifiers)) { continue; } @@ -818,8 +818,8 @@ protected internal override IReadOnlyList BuildConstructors // counts as already present; an inaccessible generated mocking constructor does not. if (!Type.IsStruct && previousParameters.Count == 0) { - if (!constructors.Any(c => c.Signature.Parameters.Count == 0 && MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers)) - && !CanonicalView.Constructors.Any(c => c.Signature.Parameters.Count == 0 && MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers))) + if (!constructors.Any(c => c.Signature.Parameters.Count == 0 && MethodSignatureHelper.IsPublicApi(c.Signature.Modifiers)) + && !CanonicalView.Constructors.Any(c => c.Signature.Parameters.Count == 0 && MethodSignatureHelper.IsPublicApi(c.Signature.Modifiers))) { var parameterlessConstructor = BuildBackCompatParameterlessConstructor(previousConstructor, candidateConstructors); RemoveGeneratedMockingConstructor(constructors); @@ -872,7 +872,7 @@ private bool TryBuildRestoredConstructor( ConstructorProvider? targetConstructor = null; foreach (var candidate in currentConstructors) { - if (!MethodProviderHelpers.IsPublicApi(candidate.Signature.Modifiers) + if (!MethodSignatureHelper.IsPublicApi(candidate.Signature.Modifiers) || candidate.Signature.Parameters.Count >= previousParameters.Count) { continue; @@ -970,8 +970,8 @@ private ConstructorProvider BuildBackCompatParameterlessConstructor( const MethodSignatureModifiers privateProtected = MethodSignatureModifiers.Private | MethodSignatureModifiers.Protected; var target = currentConstructors .Where(c => c.Signature.Parameters.Count > 0 - && (MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers) || (c.Signature.Modifiers & privateProtected) == privateProtected)) - .MinBy(c => (MethodProviderHelpers.IsPublicApi(c.Signature.Modifiers) ? 0 : 1, c.Signature.Parameters.Count(p => p.DefaultValue is null))); + && (MethodSignatureHelper.IsPublicApi(c.Signature.Modifiers) || (c.Signature.Modifiers & privateProtected) == privateProtected)) + .MinBy(c => (MethodSignatureHelper.IsPublicApi(c.Signature.Modifiers) ? 0 : 1, c.Signature.Parameters.Count(p => p.DefaultValue is null))); ConstructorInitializer? initializer = target is null ? null @@ -1033,7 +1033,7 @@ private Dictionary BuildRestorablePropertyLookup() var lookup = new Dictionary(); foreach (var property in CanonicalView.Properties) { - if (!MethodProviderHelpers.IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null) + if (!MethodSignatureHelper.IsPublicApi(property.Modifiers) || !property.Body.HasSetter || property.WireInfo == null) { continue; }