From b9d94ce722cc5083056005d20cf50c2e4750d6d0 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/12] Initial plan From 4dcc41747eb14acb291c6733452fd030c5bcb7ff 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/12] 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 38f74bddf37..7f3596fb040 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 @@ -48,5 +48,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 c3fc8efec62..197d5bd24a8 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 @@ -774,6 +774,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 05a455fae5de8eb54ce4099da63d4d240ad0515c 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/12] 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 197d5bd24a8..d9ca18cd3ec 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 @@ -872,6 +872,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; @@ -880,8 +881,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; } @@ -933,7 +949,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 88ab6c206b8..28509417aec 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 @@ -2329,6 +2329,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 51aecdcaae4a861f1ef241e7eab9a37d0e2e64db 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/12] 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 d9ca18cd3ec..eff216fddda 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; @@ -786,17 +787,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; } @@ -816,8 +817,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( @@ -837,29 +837,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; } @@ -867,7 +872,7 @@ protected internal override IReadOnlyList BuildConstructors if (targetConstructor == null) { - return null; + return false; } var targetParameters = targetConstructor.Signature.Parameters; @@ -879,7 +884,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. @@ -891,6 +896,7 @@ protected internal override IReadOnlyList BuildConstructors keptParameter.Description, keptParameter.Type, keptParameter.DefaultValue, + wireInfo: keptParameter.WireInfo, validation: ParameterValidationType.None); } @@ -906,16 +912,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); @@ -928,7 +936,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); @@ -947,16 +955,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) @@ -964,7 +970,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++; } @@ -980,7 +986,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 c2f7d7886094d66165845422ef7e36988ab10979 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/12] 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 28509417aec..d23d9bce6bb 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 @@ -2379,6 +2379,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] @@ -2438,6 +2443,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 255c02e6be66df0971c53e4eb160b37e2cac9932 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/12] 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 ++++----- .../Providers/PartialMethodCustomization.cs | 2 +- .../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 +- 20 files changed, 393 insertions(+), 47 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 eff216fddda..c49d27377e4 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 @@ -811,8 +811,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; } @@ -835,12 +844,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, @@ -886,23 +889,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; @@ -915,18 +906,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/Providers/PartialMethodCustomization.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PartialMethodCustomization.cs index 9d1d98a9785..0337b78c5a3 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 @@ -213,7 +213,7 @@ public static MethodSignature BuildPartialSignature( // Clones a ParameterProvider with a new name (and optionally without its default value) // while preserving all generator metadata. Returns the source unchanged when no change is // needed. - private static ParameterProvider CloneParameterWithName( + internal static ParameterProvider CloneParameterWithName( ParameterProvider source, string newName, bool removeDefault) 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 f0717c007ff..4497c86e746 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 @@ -66,6 +66,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 d23d9bce6bb..4648368d96c 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 @@ -2377,8 +2377,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); @@ -2412,6 +2414,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] @@ -2441,6 +2447,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] @@ -2479,7 +2489,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(); @@ -2522,6 +2532,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 a2059efc456737e65afc603380f2a88e8ae66b99 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/12] 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 c49d27377e4..e0972c15bb9 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 @@ -793,10 +793,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; @@ -804,29 +804,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( @@ -847,6 +843,7 @@ protected internal override IReadOnlyList BuildConstructors private bool TryBuildRestoredConstructor( ConstructorProvider previousConstructor, IReadOnlyList currentConstructors, + Dictionary restorablePropertyLookup, [NotNullWhen(true)] out ConstructorProvider? restoredConstructor) { restoredConstructor = null; @@ -900,7 +897,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; @@ -954,10 +951,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++; } @@ -967,12 +974,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) @@ -980,13 +990,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 de697f4360bd644c8f991f57d92d4a5f877dcea1 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/12] 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 e0972c15bb9..a24bb831e89 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 @@ -826,13 +826,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); } } @@ -897,8 +897,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; } @@ -973,13 +973,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(); @@ -991,44 +984,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 161aa8ababfe747dd52d73c56387c6bf150d3a4f 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/12] 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 a24bb831e89..58ac55a913a 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 @@ -886,31 +886,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 4648368d96c..0862645c522 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 @@ -2376,11 +2376,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 2de5445e2f40270727283cbd267a43041f4c66cc Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 31 Jul 2026 15:39:16 -0500 Subject: [PATCH 10/12] 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 58ac55a913a..f1e2c8d3961 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 @@ -903,16 +903,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 0862645c522..35a0832b33e 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 @@ -2379,8 +2379,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 6f7d60210e8f9fd6873c5f049bf04869e3d607e0 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 31 Jul 2026 15:50:57 -0500 Subject: [PATCH 11/12] 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 f1e2c8d3961..770ec7ec7e3 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 @@ -903,13 +903,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 99dd6e72309ccb6b3ec6f5073ad6b0781f65946d Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 31 Jul 2026 16:00:38 -0500 Subject: [PATCH 12/12] 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 770ec7ec7e3..1bf8d934f9f 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 @@ -983,11 +983,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); } }