From 34d99ecbc2aab565bf1c3b4188ec2d97db5f755f Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Mon, 10 Aug 2026 03:26:50 +0000 Subject: [PATCH 1/7] fix(http-client-csharp): restore constructor parameter names Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eefce9bd-b052-451e-9208-2781435428a1 --- ...tor-parameter-names-2026-08-10-03-24-00.md | 7 + .../src/Providers/TypeProvider.cs | 71 +++++--- .../src/Utilities/BackCompatHelper.cs | 48 ++++- .../ConstructorParameterTypeMismatchType.cs | 9 + .../ConstructorParameterCasingType.cs | 9 + .../ConstructorParameterRotationType.cs | 9 + .../SimpleConstructorParameterRenameType.cs | 9 + .../ConstructorParameterRenameType.cs | 9 + .../test/Providers/TypeProviderTests.cs | 164 ++++++++++++++++++ 9 files changed, 304 insertions(+), 31 deletions(-) create mode 100644 .chronus/changes/fix-csharp-constructor-parameter-names-2026-08-10-03-24-00.md create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityDoesNotRenameParametersWhenTypesDiffer/ConstructorParameterTypeMismatchType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresParameterNameCasing/ConstructorParameterCasingType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresRotatedParameterNames/ConstructorParameterRotationType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresSimpleParameterRename/SimpleConstructorParameterRenameType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresSwappedParameterNames/ConstructorParameterRenameType.cs diff --git a/.chronus/changes/fix-csharp-constructor-parameter-names-2026-08-10-03-24-00.md b/.chronus/changes/fix-csharp-constructor-parameter-names-2026-08-10-03-24-00.md new file mode 100644 index 00000000000..67c3691e089 --- /dev/null +++ b/.chronus/changes/fix-csharp-constructor-parameter-names-2026-08-10-03-24-00.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@typespec/http-client-csharp" +--- + +Restore constructor parameter names from the previous contract to preserve named-argument compatibility. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index b881b47e65c..b48815e5ac4 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -1046,20 +1046,13 @@ private Dictionary BuildCurrentMethodSignatureM /// /// Returns this type's constructors with backward compatibility applied against - /// . The default implementation preserves a previously-published - /// public constructor on an abstract base type: when the current generation would emit a - /// private protected constructor whose parameters match a public constructor in - /// the last contract, the modifier is promoted back to public. Override and call - /// base to extend this behavior. + /// . The default implementation restores parameter names on + /// matching public or protected constructors. It also preserves a previously-published public + /// constructor on an abstract base type by promoting a matching private protected + /// constructor back to public. Override and call base to extend this behavior. /// protected internal virtual IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable originalConstructors) { - // Only handle the case of changing modifiers on abstract base types. - if (!DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract)) - { - return [.. originalConstructors]; - } - if (LastContractView?.Constructors == null || LastContractView.Constructors.Count == 0) { return [.. originalConstructors]; @@ -1067,32 +1060,56 @@ protected internal virtual IReadOnlyList BuildConstructorsF List constructors = [.. originalConstructors]; - // Check if the last contract had a public constructor with matching parameters foreach (var previousConstructor in LastContractView.Constructors) { - if (!previousConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)) + var previousModifiers = previousConstructor.Signature.Modifiers; + if (!previousModifiers.HasFlag(MethodSignatureModifiers.Public) + && !previousModifiers.HasFlag(MethodSignatureModifiers.Protected)) { continue; } - // Find a matching constructor in the current version by parameter signature - for (int i = 0; i < constructors.Count; i++) + var currentConstructor = constructors.FirstOrDefault(c => + BackCompatHelper.ParameterTypesMatch(c.Signature.Parameters, previousConstructor.Signature.Parameters)); + if (currentConstructor == null) { - var currentConstructor = constructors[i]; - if (!currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Private) || - !currentConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)) - { - continue; - } + continue; + } - // Check if parameters match (same count and types) - if (BackCompatHelper.ParametersMatch(currentConstructor.Signature.Parameters, previousConstructor.Signature.Parameters)) + var currentModifiers = currentConstructor.Signature.Modifiers; + if (DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract) + && previousModifiers.HasFlag(MethodSignatureModifiers.Public) + && currentModifiers.HasFlag(MethodSignatureModifiers.Private) + && currentModifiers.HasFlag(MethodSignatureModifiers.Protected)) + { + currentConstructor.Signature.Update(modifiers: MethodSignatureModifiers.Public); + currentModifiers = MethodSignatureModifiers.Public; + CodeModelGenerator.Instance.Emitter.Debug( + $"Promoted constructor '{Name}({string.Join(", ", currentConstructor.Signature.Parameters.Select(p => p.Type.ToString()))})' from 'private protected' to 'public' to match last contract.", + BackCompatibilityChangeCategory.ConstructorModifierPreserved); + } + + if (!currentModifiers.HasFlag(MethodSignatureModifiers.Public) + && !currentModifiers.HasFlag(MethodSignatureModifiers.Protected)) + { + continue; + } + + var renamedParameters = currentConstructor.Signature.Parameters + .Zip(previousConstructor.Signature.Parameters) + .Where(pair => !string.Equals(pair.First.Name, pair.Second.Name, StringComparison.Ordinal)) + .Select(pair => (OldName: pair.First.Name, NewName: pair.Second.Name)) + .ToArray(); + + if (BackCompatHelper.TryRestorePreviousParameterNames( + currentConstructor.Signature.Parameters, + previousConstructor.Signature.Parameters)) + { + foreach (var (oldName, newName) in renamedParameters) { - // Change the modifier from private protected to public - currentConstructor.Signature.Update(modifiers: MethodSignatureModifiers.Public); CodeModelGenerator.Instance.Emitter.Debug( - $"Promoted constructor '{Name}({string.Join(", ", currentConstructor.Signature.Parameters.Select(p => p.Type.ToString()))})' from 'private protected' to 'public' to match last contract.", - BackCompatibilityChangeCategory.ConstructorModifierPreserved); + $"Preserved parameter name '{newName}' on constructor '{Name}' from last contract (instead of '{oldName}').", + BackCompatibilityChangeCategory.ParameterNamePreserved); } } } 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 000c664e000..33c84135b3d 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 @@ -102,10 +102,9 @@ public static bool IsConstructorRemovalAcceptedInBaseline(TypeProvider enclosing } /// - /// Returns true when two parameter lists match positionally by type name and parameter name - /// (and have the same count). Used to align a current member with its last-contract counterpart. + /// Returns true when two parameter lists match positionally by type name and have the same count. /// - public static bool ParametersMatch(IReadOnlyList params1, IReadOnlyList params2) + public static bool ParameterTypesMatch(IReadOnlyList params1, IReadOnlyList params2) { if (params1.Count != params2.Count) { @@ -114,7 +113,7 @@ public static bool ParametersMatch(IReadOnlyList params1, IRe for (int i = 0; i < params1.Count; i++) { - if (!params1[i].Type.AreNamesEqual(params2[i].Type) || params1[i].Name != params2[i].Name) + if (!params1[i].Type.AreNamesEqual(params2[i].Type)) { return false; } @@ -123,6 +122,47 @@ public static bool ParametersMatch(IReadOnlyList params1, IRe return true; } + /// + /// Restores parameter names positionally from a matching previous signature. All target names + /// are validated before any parameter is updated so swaps and rotations are applied atomically. + /// + public static bool TryRestorePreviousParameterNames( + IReadOnlyList currentParameters, + IReadOnlyList previousParameters) + { + if (!ParameterTypesMatch(currentParameters, previousParameters)) + { + return false; + } + + var previousNames = previousParameters.Select(p => p.Name).ToArray(); + if (previousNames.Distinct(StringComparer.Ordinal).Count() != previousNames.Length) + { + return false; + } + + bool changed = false; + for (int i = 0; i < currentParameters.Count; i++) + { + if (!string.Equals(currentParameters[i].Name, previousNames[i], StringComparison.Ordinal)) + { + changed = true; + } + } + + if (!changed) + { + return false; + } + + for (int i = 0; i < currentParameters.Count; i++) + { + currentParameters[i].Update(name: previousNames[i]); + } + + return true; + } + /// /// Returns the previously-published name of a parameter whose original (spec) name is /// , looked up in . When diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityDoesNotRenameParametersWhenTypesDiffer/ConstructorParameterTypeMismatchType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityDoesNotRenameParametersWhenTypesDiffer/ConstructorParameterTypeMismatchType.cs new file mode 100644 index 00000000000..4325a63730a --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityDoesNotRenameParametersWhenTypesDiffer/ConstructorParameterTypeMismatchType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public class ConstructorParameterTypeMismatchType + { + public ConstructorParameterTypeMismatchType(int oldName) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresParameterNameCasing/ConstructorParameterCasingType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresParameterNameCasing/ConstructorParameterCasingType.cs new file mode 100644 index 00000000000..844a54371fd --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresParameterNameCasing/ConstructorParameterCasingType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public class ConstructorParameterCasingType + { + public ConstructorParameterCasingType(string vmwareSiteId) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresRotatedParameterNames/ConstructorParameterRotationType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresRotatedParameterNames/ConstructorParameterRotationType.cs new file mode 100644 index 00000000000..c33972822c3 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresRotatedParameterNames/ConstructorParameterRotationType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public class ConstructorParameterRotationType + { + public ConstructorParameterRotationType(string first, string second, string third) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresSimpleParameterRename/SimpleConstructorParameterRenameType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresSimpleParameterRename/SimpleConstructorParameterRenameType.cs new file mode 100644 index 00000000000..fd2bcfc2922 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresSimpleParameterRename/SimpleConstructorParameterRenameType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public class SimpleConstructorParameterRenameType + { + public SimpleConstructorParameterRenameType(string oldName) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresSwappedParameterNames/ConstructorParameterRenameType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresSwappedParameterNames/ConstructorParameterRenameType.cs new file mode 100644 index 00000000000..6d70c7d40a0 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRestoresSwappedParameterNames/ConstructorParameterRenameType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public class ConstructorParameterRenameType + { + public ConstructorParameterRenameType(int minimumCount, int maximumCount) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index 0d702ffcc2f..c7bcb488857 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -232,6 +232,170 @@ public async Task BuildConstructorsForBackCompatibilityKeepsModifierOnNonAbstrac Assert.IsFalse(constructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)); } + [Test] + public async Task BuildConstructorsForBackCompatibilityRestoresSwappedParameterNames() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var currentConstructor = new ConstructorProvider( + new ConstructorSignature( + new CSharpType(typeof(object)), + $"", + MethodSignatureModifiers.Public, + [ + new ParameterProvider("maximumCount", $"", new CSharpType(typeof(int))), + new ParameterProvider("minimumCount", $"", new CSharpType(typeof(int))), + ]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider( + name: "ConstructorParameterRenameType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: [currentConstructor]); + + typeProvider.ProcessTypeForBackCompatibility(); + + Assert.That( + typeProvider.Constructors.Single().Signature.Parameters.Select(p => p.Name), + Is.EqualTo(new[] { "minimumCount", "maximumCount" })); + } + + [Test] + public async Task BuildConstructorsForBackCompatibilityRestoresSimpleParameterRename() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var parameter = new ParameterProvider("newName", $"The renamed value.", new CSharpType(typeof(string))); + _ = parameter.AsVariable(); + var currentConstructor = new ConstructorProvider( + new ConstructorSignature( + new CSharpType(typeof(object)), + $"", + MethodSignatureModifiers.Public, + [parameter]), + Snippet.This.Property("Value").Assign(parameter).Terminate(), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider( + name: "SimpleConstructorParameterRenameType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: [currentConstructor]); + + typeProvider.ProcessTypeForBackCompatibility(); + + Assert.Multiple(() => + { + Assert.AreEqual("oldName", parameter.Name); + Assert.AreEqual("this.Value = oldName;\n", currentConstructor.BodyStatements!.ToDisplayString()); + Assert.AreEqual("oldName", currentConstructor.XmlDocs.Parameters.Single().Parameter.Name); + }); + } + + [Test] + public async Task BuildConstructorsForBackCompatibilityRestoresParameterNameCasing() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var currentConstructor = new ConstructorProvider( + new ConstructorSignature( + new CSharpType(typeof(object)), + $"", + MethodSignatureModifiers.Public, + [new ParameterProvider("vMwareSiteId", $"", new CSharpType(typeof(string)))]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider( + name: "ConstructorParameterCasingType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: [currentConstructor]); + + typeProvider.ProcessTypeForBackCompatibility(); + + Assert.AreEqual("vmwareSiteId", typeProvider.Constructors.Single().Signature.Parameters.Single().Name); + } + + [Test] + public async Task BuildConstructorsForBackCompatibilityRestoresRotatedParameterNames() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var currentConstructor = new ConstructorProvider( + new ConstructorSignature( + new CSharpType(typeof(object)), + $"", + MethodSignatureModifiers.Public, + [ + new ParameterProvider("third", $"", new CSharpType(typeof(string))), + new ParameterProvider("first", $"", new CSharpType(typeof(string))), + new ParameterProvider("second", $"", new CSharpType(typeof(string))), + ]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider( + name: "ConstructorParameterRotationType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: [currentConstructor]); + + typeProvider.ProcessTypeForBackCompatibility(); + + Assert.That( + typeProvider.Constructors.Single().Signature.Parameters.Select(p => p.Name), + Is.EqualTo(new[] { "first", "second", "third" })); + } + + [Test] + public async Task BuildConstructorsForBackCompatibilityDoesNotRenameParametersWhenTypesDiffer() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var currentConstructor = new ConstructorProvider( + new ConstructorSignature( + new CSharpType(typeof(object)), + $"", + MethodSignatureModifiers.Public, + [new ParameterProvider("newName", $"", new CSharpType(typeof(string)))]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider( + name: "ConstructorParameterTypeMismatchType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: [currentConstructor]); + + typeProvider.ProcessTypeForBackCompatibility(); + + Assert.AreEqual("newName", typeProvider.Constructors.Single().Signature.Parameters.Single().Name); + } + + [Test] + public void TryRestorePreviousParameterNamesRejectsDuplicateFinalNames() + { + ParameterProvider[] currentParameters = + [ + new("first", $"", new CSharpType(typeof(string))), + new("second", $"", new CSharpType(typeof(string))), + ]; + ParameterProvider[] previousParameters = + [ + new("duplicate", $"", new CSharpType(typeof(string))), + new("duplicate", $"", new CSharpType(typeof(string))), + ]; + + Assert.Multiple(() => + { + Assert.IsFalse(BackCompatHelper.TryRestorePreviousParameterNames(currentParameters, previousParameters)); + Assert.That(currentParameters.Select(p => p.Name), Is.EqualTo(new[] { "first", "second" })); + }); + } + // Validates that the base TypeProvider generalizes the new-optional-parameter back-compat to any // TypeProvider: a public method that gained an optional non-body parameter relative to the last // contract gets a hidden overload matching the previous signature that delegates to the current one. From 29a692721fea1bf2c02907c4360b5fb38ebe55c1 Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Mon, 10 Aug 2026 03:33:17 +0000 Subject: [PATCH 2/7] chore: remove C# emitter changelog entry Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eefce9bd-b052-451e-9208-2781435428a1 --- ...harp-constructor-parameter-names-2026-08-10-03-24-00.md | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 .chronus/changes/fix-csharp-constructor-parameter-names-2026-08-10-03-24-00.md diff --git a/.chronus/changes/fix-csharp-constructor-parameter-names-2026-08-10-03-24-00.md b/.chronus/changes/fix-csharp-constructor-parameter-names-2026-08-10-03-24-00.md deleted file mode 100644 index 67c3691e089..00000000000 --- a/.chronus/changes/fix-csharp-constructor-parameter-names-2026-08-10-03-24-00.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -changeKind: fix -packages: - - "@typespec/http-client-csharp" ---- - -Restore constructor parameter names from the previous contract to preserve named-argument compatibility. From 4926a2f938043e039387e764588cb40171ae3cac Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Mon, 10 Aug 2026 03:49:45 +0000 Subject: [PATCH 3/7] fix(http-client-csharp): retain parameter matcher Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eefce9bd-b052-451e-9208-2781435428a1 --- .../src/Utilities/BackCompatHelper.cs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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 33c84135b3d..95e1544c542 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 @@ -122,6 +122,27 @@ public static bool ParameterTypesMatch(IReadOnlyList params1, return true; } + /// + /// Returns true when two parameter lists match positionally by type name and parameter name. + /// + public static bool ParametersMatch(IReadOnlyList params1, IReadOnlyList params2) + { + if (!ParameterTypesMatch(params1, params2)) + { + return false; + } + + for (int i = 0; i < params1.Count; i++) + { + if (params1[i].Name != params2[i].Name) + { + return false; + } + } + + return true; + } + /// /// Restores parameter names positionally from a matching previous signature. All target names /// are validated before any parameter is updated so swaps and rotations are applied atomically. From 511601b934086f8e4159359e48222907fd2cb9a6 Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Mon, 10 Aug 2026 04:14:13 +0000 Subject: [PATCH 4/7] fix(http-client-csharp): preserve constructor parameter semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eefce9bd-b052-451e-9208-2781435428a1 --- .../src/Providers/TypeProvider.cs | 18 +-- .../src/Utilities/BackCompatHelper.cs | 109 ++++++++++++++++-- .../test/Providers/TypeProviderTests.cs | 41 +++++-- 3 files changed, 139 insertions(+), 29 deletions(-) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index b48815e5ac4..bfedc988bcf 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -1095,20 +1095,20 @@ protected internal virtual IReadOnlyList BuildConstructorsF continue; } - var renamedParameters = currentConstructor.Signature.Parameters - .Zip(previousConstructor.Signature.Parameters) - .Where(pair => !string.Equals(pair.First.Name, pair.Second.Name, StringComparison.Ordinal)) - .Select(pair => (OldName: pair.First.Name, NewName: pair.Second.Name)) + var changedPositions = currentConstructor.Signature.Parameters + .Zip(previousConstructor.Signature.Parameters, (current, previous) => (current, previous)) + .Select((pair, position) => (pair.current.Name, PreviousName: pair.previous.Name, Position: position)) + .Where(change => !string.Equals(change.Name, change.PreviousName, StringComparison.Ordinal)) .ToArray(); - if (BackCompatHelper.TryRestorePreviousParameterNames( - currentConstructor.Signature.Parameters, - previousConstructor.Signature.Parameters)) + if (BackCompatHelper.TryRestorePreviousConstructorParameters( + currentConstructor, + previousConstructor)) { - foreach (var (oldName, newName) in renamedParameters) + foreach (var (currentName, previousName, position) in changedPositions) { CodeModelGenerator.Instance.Emitter.Debug( - $"Preserved parameter name '{newName}' on constructor '{Name}' from last contract (instead of '{oldName}').", + $"Restored parameter '{previousName}' at position {position} on constructor '{Name}' from last contract (current generated name was '{currentName}').", BackCompatibilityChangeCategory.ParameterNamePreserved); } } 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 95e1544c542..39685bd71a9 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 @@ -144,13 +144,16 @@ public static bool ParametersMatch(IReadOnlyList params1, IRe } /// - /// Restores parameter names positionally from a matching previous signature. All target names - /// are validated before any parameter is updated so swaps and rotations are applied atomically. + /// Restores constructor parameters from a matching previous signature. Existing parameter + /// objects are reordered when their names were permuted so their semantic associations remain + /// intact; simple and casing-only renames update the existing object in place. /// - public static bool TryRestorePreviousParameterNames( - IReadOnlyList currentParameters, - IReadOnlyList previousParameters) + public static bool TryRestorePreviousConstructorParameters( + ConstructorProvider currentConstructor, + ConstructorProvider previousConstructor) { + var currentParameters = currentConstructor.Signature.Parameters; + var previousParameters = previousConstructor.Signature.Parameters; if (!ParameterTypesMatch(currentParameters, previousParameters)) { return false; @@ -162,23 +165,109 @@ public static bool TryRestorePreviousParameterNames( return false; } - bool changed = false; + var restoredParametersByPosition = new Dictionary(); + var usedParameters = new HashSet(ReferenceEqualityComparer.Instance); + + // Reserve parameters whose current names identify their semantic target. This ensures + // swaps and rotations move the parameter objects instead of renaming their meanings. for (int i = 0; i < currentParameters.Count; i++) { - if (!string.Equals(currentParameters[i].Name, previousNames[i], StringComparison.Ordinal)) + var matchingParameters = currentParameters + .Where(p => !usedParameters.Contains(p) + && string.Equals(p.Name, previousNames[i], StringComparison.Ordinal) + && p.Type.AreNamesEqual(previousParameters[i].Type)) + .Take(2) + .ToArray(); + + if (matchingParameters.Length == 1) { - changed = true; + restoredParametersByPosition.Add(i, matchingParameters[0]); + usedParameters.Add(matchingParameters[0]); } } + // Preserve semantic matches whose only difference is casing before falling back to + // position-based renames. + for (int i = 0; i < currentParameters.Count; i++) + { + if (restoredParametersByPosition.ContainsKey(i)) + { + continue; + } + + var matchingParameters = currentParameters + .Where(p => !usedParameters.Contains(p) + && string.Equals(p.Name, previousNames[i], StringComparison.OrdinalIgnoreCase) + && p.Type.AreNamesEqual(previousParameters[i].Type)) + .Take(2) + .ToArray(); + + if (matchingParameters.Length == 1) + { + restoredParametersByPosition.Add(i, matchingParameters[0]); + usedParameters.Add(matchingParameters[0]); + } + } + + // Fill simple and casing-only renames positionally. If that parameter was already moved, + // proceed only when the remaining type match is unambiguous. + for (int i = 0; i < currentParameters.Count; i++) + { + if (restoredParametersByPosition.ContainsKey(i)) + { + continue; + } + + var currentAtPosition = currentParameters[i]; + if (!usedParameters.Contains(currentAtPosition) + && currentAtPosition.Type.AreNamesEqual(previousParameters[i].Type)) + { + restoredParametersByPosition.Add(i, currentAtPosition); + usedParameters.Add(currentAtPosition); + continue; + } + + var matchingParameters = currentParameters + .Where(p => !usedParameters.Contains(p) && p.Type.AreNamesEqual(previousParameters[i].Type)) + .Take(2) + .ToArray(); + if (matchingParameters.Length != 1) + { + return false; + } + + restoredParametersByPosition.Add(i, matchingParameters[0]); + usedParameters.Add(matchingParameters[0]); + } + + var restoredParameters = Enumerable.Range(0, currentParameters.Count) + .Select(i => restoredParametersByPosition[i]) + .ToArray(); + bool changed = restoredParameters + .Where((parameter, i) => + !ReferenceEquals(parameter, currentParameters[i]) + || !string.Equals(parameter.Name, previousNames[i], StringComparison.Ordinal)) + .Any(); if (!changed) { return false; } - for (int i = 0; i < currentParameters.Count; i++) + var reorderedParamDocs = restoredParameters + .Select(parameter => currentConstructor.XmlDocs.Parameters + .FirstOrDefault(doc => ReferenceEquals(doc.Parameter, parameter))) + .ToArray(); + + for (int i = 0; i < restoredParameters.Length; i++) + { + restoredParameters[i].Update(name: previousNames[i]); + } + + currentConstructor.Signature.Update(parameters: restoredParameters); + if (reorderedParamDocs.Length == currentConstructor.XmlDocs.Parameters.Count + && reorderedParamDocs.All(doc => doc != null)) { - currentParameters[i].Update(name: previousNames[i]); + currentConstructor.XmlDocs.Update(parameters: reorderedParamDocs!); } return true; diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index c7bcb488857..a5226503c73 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -237,16 +237,19 @@ public async Task BuildConstructorsForBackCompatibilityRestoresSwappedParameterN { await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + var maximumCount = new ParameterProvider("maximumCount", $"The maximum count.", new CSharpType(typeof(int))); + var minimumCount = new ParameterProvider("minimumCount", $"The minimum count.", new CSharpType(typeof(int))); var currentConstructor = new ConstructorProvider( new ConstructorSignature( new CSharpType(typeof(object)), $"", MethodSignatureModifiers.Public, - [ - new ParameterProvider("maximumCount", $"", new CSharpType(typeof(int))), - new ParameterProvider("minimumCount", $"", new CSharpType(typeof(int))), - ]), - Snippet.ThrowExpression(Snippet.Null), + [maximumCount, minimumCount]), + new MethodBodyStatement[] + { + Snippet.This.Property("MaximumCount").Assign(maximumCount).Terminate(), + Snippet.This.Property("MinimumCount").Assign(minimumCount).Terminate(), + }, new TestTypeProvider()); var typeProvider = new TestTypeProvider( @@ -257,9 +260,19 @@ public async Task BuildConstructorsForBackCompatibilityRestoresSwappedParameterN typeProvider.ProcessTypeForBackCompatibility(); - Assert.That( - typeProvider.Constructors.Single().Signature.Parameters.Select(p => p.Name), - Is.EqualTo(new[] { "minimumCount", "maximumCount" })); + var constructor = typeProvider.Constructors.Single(); + Assert.Multiple(() => + { + Assert.That(constructor.Signature.Parameters, Is.EqualTo(new[] { minimumCount, maximumCount })); + Assert.AreEqual("The minimum count.", constructor.Signature.Parameters[0].Description.Format); + Assert.AreEqual("The maximum count.", constructor.Signature.Parameters[1].Description.Format); + Assert.AreEqual( + "this.MaximumCount = maximumCount;\nthis.MinimumCount = minimumCount;\n", + constructor.BodyStatements!.ToDisplayString()); + Assert.That( + constructor.XmlDocs.Parameters.Select(p => p.Parameter), + Is.EqualTo(new[] { minimumCount, maximumCount })); + }); } [Test] @@ -376,7 +389,7 @@ public async Task BuildConstructorsForBackCompatibilityDoesNotRenameParametersWh } [Test] - public void TryRestorePreviousParameterNamesRejectsDuplicateFinalNames() + public void TryRestorePreviousConstructorParametersRejectsDuplicateFinalNames() { ParameterProvider[] currentParameters = [ @@ -388,10 +401,18 @@ public void TryRestorePreviousParameterNamesRejectsDuplicateFinalNames() new("duplicate", $"", new CSharpType(typeof(string))), new("duplicate", $"", new CSharpType(typeof(string))), ]; + var currentConstructor = new ConstructorProvider( + new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, currentParameters), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); + var previousConstructor = new ConstructorProvider( + new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, previousParameters), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); Assert.Multiple(() => { - Assert.IsFalse(BackCompatHelper.TryRestorePreviousParameterNames(currentParameters, previousParameters)); + Assert.IsFalse(BackCompatHelper.TryRestorePreviousConstructorParameters(currentConstructor, previousConstructor)); Assert.That(currentParameters.Select(p => p.Name), Is.EqualTo(new[] { "first", "second" })); }); } From 872695c039b17eeed4578d18fc831e6e602f03c1 Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Mon, 10 Aug 2026 04:53:25 +0000 Subject: [PATCH 5/7] fix(http-client-csharp): match nullable constructor overloads Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eefce9bd-b052-451e-9208-2781435428a1 --- .../src/Utilities/BackCompatHelper.cs | 5 ++- .../ConstructorNullableOverloadType.cs | 9 ++++++ .../test/Providers/TypeProviderTests.cs | 32 +++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityMatchesValueTypeNullability/ConstructorNullableOverloadType.cs 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 39685bd71a9..356e4230160 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 @@ -113,7 +113,10 @@ public static bool ParameterTypesMatch(IReadOnlyList params1, for (int i = 0; i < params1.Count; i++) { - if (!params1[i].Type.AreNamesEqual(params2[i].Type)) + var type1 = params1[i].Type; + var type2 = params2[i].Type; + if (!type1.AreNamesEqual(type2) + || (type1.IsValueType && type1.IsNullable != type2.IsNullable)) { return false; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityMatchesValueTypeNullability/ConstructorNullableOverloadType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityMatchesValueTypeNullability/ConstructorNullableOverloadType.cs new file mode 100644 index 00000000000..a8c394ae117 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityMatchesValueTypeNullability/ConstructorNullableOverloadType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public class ConstructorNullableOverloadType + { + public ConstructorNullableOverloadType(int? previousNullable) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index a5226503c73..0ec9b26bce5 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -388,6 +388,38 @@ public async Task BuildConstructorsForBackCompatibilityDoesNotRenameParametersWh Assert.AreEqual("newName", typeProvider.Constructors.Single().Signature.Parameters.Single().Name); } + [Test] + public async Task BuildConstructorsForBackCompatibilityMatchesValueTypeNullability() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var nonNullableParameter = new ParameterProvider("nonNullableCurrent", $"", new CSharpType(typeof(int))); + var nullableParameter = new ParameterProvider("nullableCurrent", $"", new CSharpType(typeof(int?))); + var typeProvider = new TestTypeProvider( + name: "ConstructorNullableOverloadType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: + [ + new ConstructorProvider( + new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, [nonNullableParameter]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()), + new ConstructorProvider( + new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, [nullableParameter]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()), + ]); + + typeProvider.ProcessTypeForBackCompatibility(); + + Assert.Multiple(() => + { + Assert.AreEqual("nonNullableCurrent", nonNullableParameter.Name); + Assert.AreEqual("previousNullable", nullableParameter.Name); + }); + } + [Test] public void TryRestorePreviousConstructorParametersRejectsDuplicateFinalNames() { From 17036ec1530239a0d066ef20b836f119e1e6a344 Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Mon, 10 Aug 2026 07:12:17 +0000 Subject: [PATCH 6/7] fix(http-client-csharp): match nested constructor types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eefce9bd-b052-451e-9208-2781435428a1 --- .../src/Utilities/BackCompatHelper.cs | 50 ++++++++++++--- .../ConstructorNestedNullableOverloadType.cs | 11 ++++ ...ConstructorMixedNullablePermutationType.cs | 9 +++ .../test/Providers/TypeProviderTests.cs | 62 +++++++++++++++++++ 4 files changed, 122 insertions(+), 10 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityMatchesNestedValueTypeNullability/ConstructorNestedNullableOverloadType.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRejectsMixedNullabilityPermutation/ConstructorMixedNullablePermutationType.cs 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 356e4230160..7107d701304 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 @@ -113,10 +113,27 @@ public static bool ParameterTypesMatch(IReadOnlyList params1, for (int i = 0; i < params1.Count; i++) { - var type1 = params1[i].Type; - var type2 = params2[i].Type; - if (!type1.AreNamesEqual(type2) - || (type1.IsValueType && type1.IsNullable != type2.IsNullable)) + if (!TypesMatchForOverload(params1[i].Type, params2[i].Type)) + { + return false; + } + } + + return true; + } + + private static bool TypesMatchForOverload(CSharpType type1, CSharpType type2) + { + if (!type1.AreNamesEqual(type2) + || (type1.IsValueType && type1.IsNullable != type2.IsNullable) + || type1.Arguments.Count != type2.Arguments.Count) + { + return false; + } + + for (int i = 0; i < type1.Arguments.Count; i++) + { + if (!TypesMatchForOverload(type1.Arguments[i], type2.Arguments[i])) { return false; } @@ -177,13 +194,17 @@ public static bool TryRestorePreviousConstructorParameters( { var matchingParameters = currentParameters .Where(p => !usedParameters.Contains(p) - && string.Equals(p.Name, previousNames[i], StringComparison.Ordinal) - && p.Type.AreNamesEqual(previousParameters[i].Type)) + && string.Equals(p.Name, previousNames[i], StringComparison.Ordinal)) .Take(2) .ToArray(); if (matchingParameters.Length == 1) { + if (!TypesMatchForOverload(matchingParameters[0].Type, previousParameters[i].Type)) + { + return false; + } + restoredParametersByPosition.Add(i, matchingParameters[0]); usedParameters.Add(matchingParameters[0]); } @@ -200,13 +221,17 @@ public static bool TryRestorePreviousConstructorParameters( var matchingParameters = currentParameters .Where(p => !usedParameters.Contains(p) - && string.Equals(p.Name, previousNames[i], StringComparison.OrdinalIgnoreCase) - && p.Type.AreNamesEqual(previousParameters[i].Type)) + && string.Equals(p.Name, previousNames[i], StringComparison.OrdinalIgnoreCase)) .Take(2) .ToArray(); if (matchingParameters.Length == 1) { + if (!TypesMatchForOverload(matchingParameters[0].Type, previousParameters[i].Type)) + { + return false; + } + restoredParametersByPosition.Add(i, matchingParameters[0]); usedParameters.Add(matchingParameters[0]); } @@ -223,7 +248,7 @@ public static bool TryRestorePreviousConstructorParameters( var currentAtPosition = currentParameters[i]; if (!usedParameters.Contains(currentAtPosition) - && currentAtPosition.Type.AreNamesEqual(previousParameters[i].Type)) + && TypesMatchForOverload(currentAtPosition.Type, previousParameters[i].Type)) { restoredParametersByPosition.Add(i, currentAtPosition); usedParameters.Add(currentAtPosition); @@ -231,7 +256,7 @@ public static bool TryRestorePreviousConstructorParameters( } var matchingParameters = currentParameters - .Where(p => !usedParameters.Contains(p) && p.Type.AreNamesEqual(previousParameters[i].Type)) + .Where(p => !usedParameters.Contains(p) && TypesMatchForOverload(p.Type, previousParameters[i].Type)) .Take(2) .ToArray(); if (matchingParameters.Length != 1) @@ -246,6 +271,11 @@ public static bool TryRestorePreviousConstructorParameters( var restoredParameters = Enumerable.Range(0, currentParameters.Count) .Select(i => restoredParametersByPosition[i]) .ToArray(); + if (!ParameterTypesMatch(restoredParameters, previousParameters)) + { + return false; + } + bool changed = restoredParameters .Where((parameter, i) => !ReferenceEquals(parameter, currentParameters[i]) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityMatchesNestedValueTypeNullability/ConstructorNestedNullableOverloadType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityMatchesNestedValueTypeNullability/ConstructorNestedNullableOverloadType.cs new file mode 100644 index 00000000000..622de4aa4f0 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityMatchesNestedValueTypeNullability/ConstructorNestedNullableOverloadType.cs @@ -0,0 +1,11 @@ +using System.Collections.Generic; + +namespace Test +{ + public class ConstructorNestedNullableOverloadType + { + public ConstructorNestedNullableOverloadType(List previousNullableItems) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRejectsMixedNullabilityPermutation/ConstructorMixedNullablePermutationType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRejectsMixedNullabilityPermutation/ConstructorMixedNullablePermutationType.cs new file mode 100644 index 00000000000..fe575d52459 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildConstructorsForBackCompatibilityRejectsMixedNullabilityPermutation/ConstructorMixedNullablePermutationType.cs @@ -0,0 +1,9 @@ +namespace Test +{ + public class ConstructorMixedNullablePermutationType + { + public ConstructorMixedNullablePermutationType(int nonNullable, int? nullable) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index 0ec9b26bce5..6b5c17a6098 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -420,6 +420,68 @@ public async Task BuildConstructorsForBackCompatibilityMatchesValueTypeNullabili }); } + [Test] + public async Task BuildConstructorsForBackCompatibilityMatchesNestedValueTypeNullability() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var nonNullableParameter = new ParameterProvider("nonNullableCurrent", $"", new CSharpType(typeof(List))); + var nullableParameter = new ParameterProvider("nullableCurrent", $"", new CSharpType(typeof(List))); + var typeProvider = new TestTypeProvider( + name: "ConstructorNestedNullableOverloadType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: + [ + new ConstructorProvider( + new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, [nonNullableParameter]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()), + new ConstructorProvider( + new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, [nullableParameter]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()), + ]); + + typeProvider.ProcessTypeForBackCompatibility(); + + Assert.Multiple(() => + { + Assert.AreEqual("nonNullableCurrent", nonNullableParameter.Name); + Assert.AreEqual("previousNullableItems", nullableParameter.Name); + }); + } + + [Test] + public async Task BuildConstructorsForBackCompatibilityRejectsMixedNullabilityPermutation() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var nullable = new ParameterProvider("nullable", $"", new CSharpType(typeof(int))); + var nonNullable = new ParameterProvider("nonNullable", $"", new CSharpType(typeof(int?))); + var currentConstructor = new ConstructorProvider( + new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, [nullable, nonNullable]), + Snippet.ThrowExpression(Snippet.Null), + new TestTypeProvider()); + var typeProvider = new TestTypeProvider( + name: "ConstructorMixedNullablePermutationType", + ns: "Test", + declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, + constructors: [currentConstructor]); + + typeProvider.ProcessTypeForBackCompatibility(); + + Assert.Multiple(() => + { + Assert.That(currentConstructor.Signature.Parameters, Is.EqualTo(new[] { nullable, nonNullable })); + Assert.That( + currentConstructor.Signature.Parameters.Select(p => p.Name), + Is.EqualTo(new[] { "nullable", "nonNullable" })); + Assert.IsFalse(currentConstructor.Signature.Parameters[0].Type.IsNullable); + Assert.IsTrue(currentConstructor.Signature.Parameters[1].Type.IsNullable); + }); + } + [Test] public void TryRestorePreviousConstructorParametersRejectsDuplicateFinalNames() { From 8a5f57c261fadff8142900f2e5f4074cb0c5e9f6 Mon Sep 17 00:00:00 2001 From: Wei Hu Date: Mon, 10 Aug 2026 10:21:11 +0000 Subject: [PATCH 7/7] fix(http-client-csharp): reconcile constructor contracts early Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eefce9bd-b052-451e-9208-2781435428a1 --- .../src/Providers/ModelProvider.cs | 104 ++++-- .../Providers/PartialMethodCustomization.cs | 6 +- .../src/Providers/TypeProvider.cs | 73 +---- .../src/Utilities/BackCompatHelper.cs | 234 -------------- .../Utilities/ConstructorBackCompatHelper.cs | 295 ++++++++++++++++++ .../src/Utilities/TypeSymbolExtensions.cs | 5 +- .../ModelProviders/ModelProviderTests.cs | 116 +++++++ .../MockInputModel.cs | 7 +- .../MockInputModel.cs | 9 + .../MockInputModel.cs | 9 + .../Models.cs | 17 + .../test/Providers/TypeProviderTests.cs | 212 +++++++++++-- .../Utilities/TypeSymbolExtensionsTests.cs | 23 ++ 13 files changed, 747 insertions(+), 363 deletions(-) create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ConstructorBackCompatHelper.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorParameterRenameIsAppliedBeforeBody/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorParameterSwapIsAppliedBeforeBody/MockInputModel.cs create mode 100644 packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_DerivedInitializerUsesRestoredBaseParameterOrder/Models.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 43d2559a48b..78b0d3d9e6d 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 @@ -733,7 +733,8 @@ protected internal override ConstructorProvider[] BuildConstructors() { if (_inputModel.IsUnknownDiscriminatorModel) { - return [FullConstructor]; + _initializationConstructor = FullConstructor; + return [_initializationConstructor]; } // Build the standard single initialization constructor @@ -742,7 +743,10 @@ protected internal override ConstructorProvider[] BuildConstructors() : _inputModel.Usage.HasFlag(InputModelTypeUsage.Input) ? MethodSignatureModifiers.Public : MethodSignatureModifiers.Internal; - var (constructorParameters, constructorInitializer) = BuildConstructorParameters(true); + var (constructorParameters, constructorInitializer) = BuildConstructorParameters( + true, + accessibility, + reconcileWithLastContract: true); var constructor = new ConstructorProvider( signature: new ConstructorSignature( @@ -756,11 +760,14 @@ protected internal override ConstructorProvider[] BuildConstructors() GetPropertyInitializers(true, parameters: constructorParameters) }, this); + _initializationConstructor = constructor; var constructors = new List { constructor }; // Add FullConstructor if parameters are different - if (!constructorParameters.SequenceEqual(FullConstructor.Signature.Parameters)) + if (!ConstructorBackCompatHelper.HaveSameParameterIdentity( + constructorParameters, + FullConstructor.Signature.Parameters)) { constructors.Add(FullConstructor); } @@ -775,6 +782,17 @@ protected internal override ConstructorProvider[] BuildConstructors() return [.. constructors]; } + private ConstructorProvider? _initializationConstructor; + internal ConstructorProvider InitializationConstructor + { + get + { + _ = Constructors; + return _initializationConstructor + ?? throw new InvalidOperationException($"Initialization constructor for '{Name}' was not built."); + } + } + /// /// Restores previously-published public constructors that the current generation would otherwise /// drop. The primary scenario is a previously required property becoming optional: the corresponding @@ -800,14 +818,8 @@ protected internal override IReadOnlyList BuildConstructors foreach (var previousConstructor in previousConstructors) { - if (!MethodSignatureHelper.IsPublicApi(previousConstructor.Signature.Modifiers)) - { - continue; - } - var previousParameters = previousConstructor.Signature.Parameters; - - if (BackCompatHelper.IsConstructorRemovalAcceptedInBaseline(this, previousConstructor.Signature)) + if (!ConstructorBackCompatHelper.IsEligiblePreviousConstructor(this, previousConstructor)) { continue; } @@ -834,8 +846,8 @@ protected internal override IReadOnlyList BuildConstructors // 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 (constructors.Any(c => ConstructorBackCompatHelper.HaveSameParameterIdentity(c.Signature.Parameters, previousParameters)) + || CanonicalView.Constructors.Any(c => ConstructorBackCompatHelper.HaveSameParameterIdentity(c.Signature.Parameters, previousParameters))) { continue; } @@ -1174,7 +1186,10 @@ private IEnumerable GetAllBaseFieldsForConstructorInitialization( } private (IReadOnlyList Parameters, ConstructorInitializer? Initializer) BuildConstructorParameters( - bool isInitializationConstructor, bool includeDiscriminatorParameter = false) + bool isInitializationConstructor, + MethodSignatureModifiers modifiers = MethodSignatureModifiers.None, + bool includeDiscriminatorParameter = false, + bool reconcileWithLastContract = false) { var baseParameters = new List(); var constructorParameters = new List(); @@ -1222,6 +1237,16 @@ private IEnumerable GetAllBaseFieldsForConstructorInitialization( p.Property is null || (!overriddenProperties.Contains(p.Property!) && (!p.Property.IsDiscriminator || !isInitializationConstructor || (includeDiscriminatorParameter && IsMultiLevelDiscriminator))))); + if (reconcileWithLastContract + && ConstructorBackCompatHelper.TryRestoreInitializationParameters( + this, + modifiers, + constructorParameters, + out var restoredParameters)) + { + constructorParameters = [.. restoredParameters]; + } + // construct the initializer using the parameters from base signature ConstructorInitializer? constructorInitializer = null; if (BaseModelProvider != null) @@ -1245,8 +1270,18 @@ p.Property is null } else { - // Standard base constructor call - constructorInitializer = new ConstructorInitializer(true, [.. baseParameters.Select(p => GetExpressionForCtor(p, overriddenProperties, isInitializationConstructor, constructorParameters))]); + // Build the call from the reconciled base signature so a restored parameter + // order is reflected in every derived constructor call. + var baseSignatureParameters = isInitializationConstructor && !HasBaseModelProviderCycle() + ? BaseModelProvider.InitializationConstructor.Signature.Parameters + : baseParameters; + constructorInitializer = new ConstructorInitializer( + true, + [.. baseSignatureParameters.Select(p => GetExpressionForCtor( + p, + overriddenProperties, + isInitializationConstructor, + constructorParameters))]); } } else @@ -1446,16 +1481,41 @@ private MethodBodyStatement GetPropertyInitializers( IReadOnlyList? parameters = null) { List methodBodyStatements = new(CanonicalView.Properties.Count + CanonicalView.Fields.Count + 1); - Dictionary parameterMap = parameters?.ToDictionary(p => p.Name) ?? []; + Dictionary propertyParameterMap = []; + Dictionary fieldParameterMap = []; + if (parameters is not null) + { + foreach (var parameter in parameters) + { + if (parameter.Property is not null) + { + propertyParameterMap.TryAdd(parameter.Property, parameter); + } + else if (parameter.Field is not null) + { + fieldParameterMap.TryAdd(parameter.Field, parameter); + } + } + } foreach (var property in CanonicalView.Properties) { - CreatePropertyAssignmentStatement(isPrimaryConstructor, methodBodyStatements, parameterMap, property); + CreatePropertyAssignmentStatement( + isPrimaryConstructor, + methodBodyStatements, + propertyParameterMap, + fieldParameterMap, + property); } foreach (var field in CanonicalView.Fields) { - CreatePropertyAssignmentStatement(isPrimaryConstructor, methodBodyStatements, parameterMap, field: field); + CreatePropertyAssignmentStatement( + isPrimaryConstructor, + methodBodyStatements, + propertyParameterMap, + fieldParameterMap, + field: field); } // If discriminator is defined as optional in the base model, but we have an expression for it, assign it in the @@ -1504,7 +1564,8 @@ private MethodBodyStatement GetPropertyInitializers( private void CreatePropertyAssignmentStatement( bool isPrimaryConstructor, List methodBodyStatements, - Dictionary parameterMap, + Dictionary propertyParameterMap, + Dictionary fieldParameterMap, PropertyProvider? property = default, FieldProvider? field = default) { @@ -1537,7 +1598,10 @@ private void CreatePropertyAssignmentStatement( var type = property?.Type ?? field!.Type; - if (parameterMap.TryGetValue(property?.AsParameter.Name ?? field!.AsParameter.Name, out var parameter) || Type.IsStruct) + var hasParameter = property is not null + ? propertyParameterMap.TryGetValue(property, out var parameter) + : fieldParameterMap.TryGetValue(field!, out parameter); + if (hasParameter || Type.IsStruct) { if (parameter != null) { 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 5dbcb7f0aa1..1409d988c7b 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,9 +217,11 @@ internal static ParameterProvider CloneParameterWithName( ParameterProvider source, string newName, bool removeDefault, - ParameterValidationType? validation = null) + ParameterValidationType? validation = null, + bool forceClone = false) { - if (source.Name == newName + if (!forceClone + && source.Name == newName && !(removeDefault && source.DefaultValue != null) && (validation == null || validation == source.Validation)) { diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index bfedc988bcf..091ed6fba69 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -1045,77 +1045,12 @@ private Dictionary BuildCurrentMethodSignatureM } /// - /// Returns this type's constructors with backward compatibility applied against - /// . The default implementation restores parameter names on - /// matching public or protected constructors. It also preserves a previously-published public - /// constructor on an abstract base type by promoting a matching private protected - /// constructor back to public. Override and call base to extend this behavior. + /// Returns this type's constructors with non-structural backward compatibility applied against + /// . Structural constructor compatibility is applied by the owning + /// provider before constructor bodies and callers are materialized. /// protected internal virtual IReadOnlyList BuildConstructorsForBackCompatibility(IEnumerable originalConstructors) - { - if (LastContractView?.Constructors == null || LastContractView.Constructors.Count == 0) - { - return [.. originalConstructors]; - } - - List constructors = [.. originalConstructors]; - - foreach (var previousConstructor in LastContractView.Constructors) - { - var previousModifiers = previousConstructor.Signature.Modifiers; - if (!previousModifiers.HasFlag(MethodSignatureModifiers.Public) - && !previousModifiers.HasFlag(MethodSignatureModifiers.Protected)) - { - continue; - } - - var currentConstructor = constructors.FirstOrDefault(c => - BackCompatHelper.ParameterTypesMatch(c.Signature.Parameters, previousConstructor.Signature.Parameters)); - if (currentConstructor == null) - { - continue; - } - - var currentModifiers = currentConstructor.Signature.Modifiers; - if (DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract) - && previousModifiers.HasFlag(MethodSignatureModifiers.Public) - && currentModifiers.HasFlag(MethodSignatureModifiers.Private) - && currentModifiers.HasFlag(MethodSignatureModifiers.Protected)) - { - currentConstructor.Signature.Update(modifiers: MethodSignatureModifiers.Public); - currentModifiers = MethodSignatureModifiers.Public; - CodeModelGenerator.Instance.Emitter.Debug( - $"Promoted constructor '{Name}({string.Join(", ", currentConstructor.Signature.Parameters.Select(p => p.Type.ToString()))})' from 'private protected' to 'public' to match last contract.", - BackCompatibilityChangeCategory.ConstructorModifierPreserved); - } - - if (!currentModifiers.HasFlag(MethodSignatureModifiers.Public) - && !currentModifiers.HasFlag(MethodSignatureModifiers.Protected)) - { - continue; - } - - var changedPositions = currentConstructor.Signature.Parameters - .Zip(previousConstructor.Signature.Parameters, (current, previous) => (current, previous)) - .Select((pair, position) => (pair.current.Name, PreviousName: pair.previous.Name, Position: position)) - .Where(change => !string.Equals(change.Name, change.PreviousName, StringComparison.Ordinal)) - .ToArray(); - - if (BackCompatHelper.TryRestorePreviousConstructorParameters( - currentConstructor, - previousConstructor)) - { - foreach (var (currentName, previousName, position) in changedPositions) - { - CodeModelGenerator.Instance.Emitter.Debug( - $"Restored parameter '{previousName}' at position {position} on constructor '{Name}' from last contract (current generated name was '{currentName}').", - BackCompatibilityChangeCategory.ParameterNamePreserved); - } - } - } - - return [.. constructors]; - } + => ConstructorBackCompatHelper.ApplyLateCompatibility(this, originalConstructors); private IReadOnlyList? _enumValues; diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs index 7107d701304..46e85d15279 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs @@ -50,35 +50,6 @@ 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. @@ -101,211 +72,6 @@ public static bool IsConstructorRemovalAcceptedInBaseline(TypeProvider enclosing return null; } - /// - /// Returns true when two parameter lists match positionally by type name and have the same count. - /// - public static bool ParameterTypesMatch(IReadOnlyList params1, IReadOnlyList params2) - { - if (params1.Count != params2.Count) - { - return false; - } - - for (int i = 0; i < params1.Count; i++) - { - if (!TypesMatchForOverload(params1[i].Type, params2[i].Type)) - { - return false; - } - } - - return true; - } - - private static bool TypesMatchForOverload(CSharpType type1, CSharpType type2) - { - if (!type1.AreNamesEqual(type2) - || (type1.IsValueType && type1.IsNullable != type2.IsNullable) - || type1.Arguments.Count != type2.Arguments.Count) - { - return false; - } - - for (int i = 0; i < type1.Arguments.Count; i++) - { - if (!TypesMatchForOverload(type1.Arguments[i], type2.Arguments[i])) - { - return false; - } - } - - return true; - } - - /// - /// Returns true when two parameter lists match positionally by type name and parameter name. - /// - public static bool ParametersMatch(IReadOnlyList params1, IReadOnlyList params2) - { - if (!ParameterTypesMatch(params1, params2)) - { - return false; - } - - for (int i = 0; i < params1.Count; i++) - { - if (params1[i].Name != params2[i].Name) - { - return false; - } - } - - return true; - } - - /// - /// Restores constructor parameters from a matching previous signature. Existing parameter - /// objects are reordered when their names were permuted so their semantic associations remain - /// intact; simple and casing-only renames update the existing object in place. - /// - public static bool TryRestorePreviousConstructorParameters( - ConstructorProvider currentConstructor, - ConstructorProvider previousConstructor) - { - var currentParameters = currentConstructor.Signature.Parameters; - var previousParameters = previousConstructor.Signature.Parameters; - if (!ParameterTypesMatch(currentParameters, previousParameters)) - { - return false; - } - - var previousNames = previousParameters.Select(p => p.Name).ToArray(); - if (previousNames.Distinct(StringComparer.Ordinal).Count() != previousNames.Length) - { - return false; - } - - var restoredParametersByPosition = new Dictionary(); - var usedParameters = new HashSet(ReferenceEqualityComparer.Instance); - - // Reserve parameters whose current names identify their semantic target. This ensures - // swaps and rotations move the parameter objects instead of renaming their meanings. - for (int i = 0; i < currentParameters.Count; i++) - { - var matchingParameters = currentParameters - .Where(p => !usedParameters.Contains(p) - && string.Equals(p.Name, previousNames[i], StringComparison.Ordinal)) - .Take(2) - .ToArray(); - - if (matchingParameters.Length == 1) - { - if (!TypesMatchForOverload(matchingParameters[0].Type, previousParameters[i].Type)) - { - return false; - } - - restoredParametersByPosition.Add(i, matchingParameters[0]); - usedParameters.Add(matchingParameters[0]); - } - } - - // Preserve semantic matches whose only difference is casing before falling back to - // position-based renames. - for (int i = 0; i < currentParameters.Count; i++) - { - if (restoredParametersByPosition.ContainsKey(i)) - { - continue; - } - - var matchingParameters = currentParameters - .Where(p => !usedParameters.Contains(p) - && string.Equals(p.Name, previousNames[i], StringComparison.OrdinalIgnoreCase)) - .Take(2) - .ToArray(); - - if (matchingParameters.Length == 1) - { - if (!TypesMatchForOverload(matchingParameters[0].Type, previousParameters[i].Type)) - { - return false; - } - - restoredParametersByPosition.Add(i, matchingParameters[0]); - usedParameters.Add(matchingParameters[0]); - } - } - - // Fill simple and casing-only renames positionally. If that parameter was already moved, - // proceed only when the remaining type match is unambiguous. - for (int i = 0; i < currentParameters.Count; i++) - { - if (restoredParametersByPosition.ContainsKey(i)) - { - continue; - } - - var currentAtPosition = currentParameters[i]; - if (!usedParameters.Contains(currentAtPosition) - && TypesMatchForOverload(currentAtPosition.Type, previousParameters[i].Type)) - { - restoredParametersByPosition.Add(i, currentAtPosition); - usedParameters.Add(currentAtPosition); - continue; - } - - var matchingParameters = currentParameters - .Where(p => !usedParameters.Contains(p) && TypesMatchForOverload(p.Type, previousParameters[i].Type)) - .Take(2) - .ToArray(); - if (matchingParameters.Length != 1) - { - return false; - } - - restoredParametersByPosition.Add(i, matchingParameters[0]); - usedParameters.Add(matchingParameters[0]); - } - - var restoredParameters = Enumerable.Range(0, currentParameters.Count) - .Select(i => restoredParametersByPosition[i]) - .ToArray(); - if (!ParameterTypesMatch(restoredParameters, previousParameters)) - { - return false; - } - - bool changed = restoredParameters - .Where((parameter, i) => - !ReferenceEquals(parameter, currentParameters[i]) - || !string.Equals(parameter.Name, previousNames[i], StringComparison.Ordinal)) - .Any(); - if (!changed) - { - return false; - } - - var reorderedParamDocs = restoredParameters - .Select(parameter => currentConstructor.XmlDocs.Parameters - .FirstOrDefault(doc => ReferenceEquals(doc.Parameter, parameter))) - .ToArray(); - - for (int i = 0; i < restoredParameters.Length; i++) - { - restoredParameters[i].Update(name: previousNames[i]); - } - - currentConstructor.Signature.Update(parameters: restoredParameters); - if (reorderedParamDocs.Length == currentConstructor.XmlDocs.Parameters.Count - && reorderedParamDocs.All(doc => doc != null)) - { - currentConstructor.XmlDocs.Update(parameters: reorderedParamDocs!); - } - - return true; - } - /// /// Returns the previously-published name of a parameter whose original (spec) name is /// , looked up in . When diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ConstructorBackCompatHelper.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ConstructorBackCompatHelper.cs new file mode 100644 index 00000000000..70a4509c38a --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ConstructorBackCompatHelper.cs @@ -0,0 +1,295 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.TypeSpec.Generator.EmitterRpc; +using Microsoft.TypeSpec.Generator.Primitives; +using Microsoft.TypeSpec.Generator.Providers; + +namespace Microsoft.TypeSpec.Generator.Utilities +{ + /// + /// Owns constructor-specific compatibility policy. Structural changes are planned before a + /// constructor is materialized; late processing is limited to accessibility preservation. + /// + internal static class ConstructorBackCompatHelper + { + public static bool TryRestoreInitializationParameters( + ModelProvider model, + MethodSignatureModifiers currentModifiers, + IReadOnlyList currentParameters, + out IReadOnlyList restoredParameters) + { + restoredParameters = currentParameters; + if (currentModifiers.HasFlag(MethodSignatureModifiers.Static) + || (!MethodSignatureHelper.IsPublicApi(currentModifiers) && !IsPrivateProtected(currentModifiers)) + || model.LastContractView?.Constructors is not { Count: > 0 } previousConstructors) + { + return false; + } + + IReadOnlyList? candidate = null; + foreach (var previousConstructor in previousConstructors) + { + if (!IsEligiblePreviousConstructor(model, previousConstructor) + || !TryCreateRestoredParameters(currentParameters, previousConstructor.Signature.Parameters, out var restored)) + { + continue; + } + + // Multiple viable previous overloads make the intended contract ambiguous. + if (candidate is not null) + { + return false; + } + candidate = restored; + candidate = restored; + } + + if (candidate is null) + { + return false; + } + + restoredParameters = candidate; + for (int i = 0; i < currentParameters.Count; i++) + { + if (!string.Equals(currentParameters[i].Name, restoredParameters[i].Name, StringComparison.Ordinal) + || !ReferenceEquals(currentParameters[i], restoredParameters[i])) + { + CodeModelGenerator.Instance.Emitter.Debug( + $"Restored parameter '{restoredParameters[i].Name}' at position {i} on constructor '{model.Name}' from last contract (current generated name was '{currentParameters[i].Name}').", + BackCompatibilityChangeCategory.ParameterNamePreserved); + } + } + + return true; + } + + public static IReadOnlyList ApplyLateCompatibility( + TypeProvider type, + IEnumerable originalConstructors) + { + List constructors = [.. originalConstructors]; + if (!type.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Abstract) + || type.LastContractView?.Constructors is not { Count: > 0 } previousConstructors) + { + return constructors; + } + + foreach (var previousConstructor in previousConstructors) + { + if (!IsEligiblePreviousConstructor(type, previousConstructor)) + { + continue; + } + + var currentConstructor = constructors.FirstOrDefault(c => + IsPrivateProtected(c.Signature.Modifiers) + && HaveSameParameterIdentity(c.Signature.Parameters, previousConstructor.Signature.Parameters)); + if (currentConstructor is null) + { + continue; + } + + var restoredModifiers = previousConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public) + ? MethodSignatureModifiers.Public + : MethodSignatureModifiers.Protected; + currentConstructor.Signature.Update(modifiers: restoredModifiers); + CodeModelGenerator.Instance.Emitter.Debug( + $"Promoted constructor '{type.Name}({string.Join(", ", currentConstructor.Signature.Parameters.Select(p => p.Type.ToString()))})' from 'private protected' to '{restoredModifiers.ToString().ToLowerInvariant()}' to match last contract.", + BackCompatibilityChangeCategory.ConstructorModifierPreserved); + } + + return constructors; + } + + public static bool IsEligiblePreviousConstructor(TypeProvider type, ConstructorProvider previousConstructor) + { + if (!MethodSignatureHelper.IsPublicApi(previousConstructor.Signature.Modifiers) + || previousConstructor.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Static)) + { + return false; + } + + return !IsRemovalAcceptedInBaseline(type, previousConstructor.Signature); + } + + public static bool HaveSameParameterIdentity( + IReadOnlyList first, + IReadOnlyList second) + { + if (first.Count != second.Count) + { + return false; + } + + for (int i = 0; i < first.Count; i++) + { + if (!HaveSameParameterIdentity(first[i], second[i])) + { + return false; + } + } + + return true; + } + + internal static bool TryCreateRestoredParameters( + IReadOnlyList currentParameters, + IReadOnlyList previousParameters, + out IReadOnlyList restoredParameters) + { + restoredParameters = currentParameters; + if (currentParameters.Count != previousParameters.Count + || previousParameters.Select(p => p.Name).Distinct(StringComparer.Ordinal).Count() != previousParameters.Count) + { + return false; + } + + int matchingNameCount = previousParameters.Count(previous => + currentParameters.Any(current => string.Equals(current.Name, previous.Name, StringComparison.OrdinalIgnoreCase))); + if (matchingNameCount != 0 && matchingNameCount != previousParameters.Count) + { + return false; + } + + var orderedCurrentParameters = new ParameterProvider[currentParameters.Count]; + if (matchingNameCount == previousParameters.Count) + { + var used = new HashSet(ReferenceEqualityComparer.Instance); + for (int i = 0; i < previousParameters.Count; i++) + { + var matches = currentParameters + .Where(current => !used.Contains(current) + && string.Equals(current.Name, previousParameters[i].Name, StringComparison.OrdinalIgnoreCase)) + .Take(2) + .ToArray(); + if (matches.Length != 1 || !HaveSameParameterIdentity(matches[0], previousParameters[i])) + { + return false; + } + + orderedCurrentParameters[i] = matches[0]; + used.Add(matches[0]); + } + } + else + { + for (int i = 0; i < currentParameters.Count; i++) + { + if (!HaveSameParameterIdentity(currentParameters[i], previousParameters[i])) + { + return false; + } + + orderedCurrentParameters[i] = currentParameters[i]; + } + } + + bool changed = orderedCurrentParameters + .Where((parameter, i) => + !ReferenceEquals(parameter, currentParameters[i]) + || !string.Equals(parameter.Name, previousParameters[i].Name, StringComparison.Ordinal)) + .Any(); + if (!changed + || orderedCurrentParameters.Any(p => p.Attributes.Count > 0 || p.InitializationValue is not null)) + { + return false; + } + + var candidate = orderedCurrentParameters + .Select((parameter, i) => PartialMethodCustomization.CloneParameterWithName( + parameter, + previousParameters[i].Name, + removeDefault: false, + forceClone: true)) + .ToArray(); + if (!HaveSameParameterIdentity(candidate, previousParameters) || !HasLegalParameterOrder(candidate)) + { + return false; + } + + restoredParameters = candidate; + return true; + } + + private static bool HaveSameParameterIdentity(ParameterProvider first, ParameterProvider second) + => TypesMatchForOverload(first.Type, second.Type) + && first.IsRef == second.IsRef + && first.IsOut == second.IsOut + && first.IsIn == second.IsIn + && first.IsParams == second.IsParams; + + private static bool TypesMatchForOverload(CSharpType first, CSharpType second) + { + if (!first.AreNamesEqual(second) + || (first.IsValueType && first.IsNullable != second.IsNullable)) + { + return false; + } + + for (int i = 0; i < first.Arguments.Count; i++) + { + if (!TypesMatchForOverload(first.Arguments[i], second.Arguments[i])) + { + return false; + } + } + + return true; + } + + private static bool HasLegalParameterOrder(IReadOnlyList parameters) + { + bool sawOptional = false; + for (int i = 0; i < parameters.Count; i++) + { + var parameter = parameters[i]; + if (parameter.IsParams) + { + if (i != parameters.Count - 1 || parameter.DefaultValue is not null) + { + return false; + } + + continue; + } + + if (parameter.DefaultValue is not null) + { + sawOptional = true; + } + else if (sawOptional) + { + return false; + } + } + + return true; + } + + private static bool IsPrivateProtected(MethodSignatureModifiers modifiers) + => modifiers.HasFlag(MethodSignatureModifiers.Private) + && modifiers.HasFlag(MethodSignatureModifiers.Protected); + + private static bool IsRemovalAcceptedInBaseline(TypeProvider type, ConstructorSignature previousSignature) + { + var parameterTypes = previousSignature.Parameters.Select(p => p.Type).ToArray(); + if (CodeModelGenerator.Instance.SourceInputModel?.ApiCompatBaseline.IsMethodRemovalSuppressed( + type.Type.FullyQualifiedName, + ".ctor", + parameterTypes) != true) + { + return false; + } + + CodeModelGenerator.Instance.Emitter.Info( + $"Skipping back-compat for '{type.Type.FullyQualifiedName}..ctor'; removal is accepted in the ApiCompat baseline.", + BackCompatibilityChangeCategory.BaselineAcceptedRemovalSkipped); + return true; + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/TypeSymbolExtensions.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/TypeSymbolExtensions.cs index 9e8c4a6c493..533936bb21a 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/TypeSymbolExtensions.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/TypeSymbolExtensions.cs @@ -121,7 +121,10 @@ public static string GetFullyQualifiedName(this ITypeSymbol typeSymbol) // Handle array types if (typeSymbol is IArrayTypeSymbol arrayTypeSymbol) { - return GetFullyQualifiedName(arrayTypeSymbol.ElementType) + "[]"; + return GetFullyQualifiedName(arrayTypeSymbol.ElementType) + + "[" + + new string(',', arrayTypeSymbol.Rank - 1) + + "]"; } // Handle tuples & generic types if (typeSymbol is INamedTypeSymbol namedTypeSymbol) 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 15a9ffec1c0..b68d3aa804a 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 @@ -2299,6 +2299,122 @@ protected override TypeProvider[] BuildSerializationProviders() } } + [Test] + public async Task BackCompat_ConstructorParameterSwapIsAppliedBeforeBody() + { + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input, + properties: + [ + InputFactory.Property("maximumCount", InputPrimitiveType.Int32, isRequired: true), + InputFactory.Property("minimumCount", InputPrimitiveType.Int32, isRequired: true), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var model = (ModelProvider)CodeModelGenerator.Instance.OutputLibrary.TypeProviders + .Single(t => t.Name == "MockInputModel"); + var constructor = model.Constructors.Single(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)); + + Assert.Multiple(() => + { + Assert.That( + constructor.Signature.Parameters.Select(p => p.Name), + Is.EqualTo(new[] { "minimumCount", "maximumCount" })); + Assert.That( + constructor.Signature.Parameters.Select(p => p.Property?.Name), + Is.EqualTo(new[] { "MinimumCount", "MaximumCount" })); + Assert.AreEqual( + "MaximumCount = maximumCount;\nMinimumCount = minimumCount;\n", + constructor.BodyStatements!.ToDisplayString()); + Assert.AreEqual("maximumCount", model.Properties.Single(p => p.Name == "MaximumCount").AsParameter.Name); + Assert.AreEqual("minimumCount", model.Properties.Single(p => p.Name == "MinimumCount").AsParameter.Name); + Assert.AreEqual( + 1, + model.Constructors.Count(c => ConstructorBackCompatHelper.HaveSameParameterIdentity( + c.Signature.Parameters, + constructor.Signature.Parameters))); + }); + } + + [Test] + public async Task BackCompat_ConstructorParameterRenameIsAppliedBeforeBody() + { + var inputModel = InputFactory.Model( + "MockInputModel", + usage: InputModelTypeUsage.Input, + properties: + [ + InputFactory.Property("newName", InputPrimitiveType.String, isRequired: true), + ]); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [inputModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var model = (ModelProvider)CodeModelGenerator.Instance.OutputLibrary.TypeProviders + .Single(t => t.Name == "MockInputModel"); + var constructor = model.Constructors.Single(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)); + + Assert.Multiple(() => + { + Assert.AreEqual("oldName", constructor.Signature.Parameters.Single().Name); + Assert.That(constructor.BodyStatements!.ToDisplayString(), Does.Contain("NewName = oldName;")); + Assert.AreEqual("newName", model.Properties.Single().AsParameter.Name); + Assert.AreEqual( + 1, + model.Constructors.Count(c => ConstructorBackCompatHelper.HaveSameParameterIdentity( + c.Signature.Parameters, + constructor.Signature.Parameters))); + }); + } + + [Test] + public async Task BackCompat_DerivedInitializerUsesRestoredBaseParameterOrder() + { + var baseModel = InputFactory.Model( + "BaseModel", + usage: InputModelTypeUsage.Input, + properties: + [ + InputFactory.Property("maximumCount", InputPrimitiveType.Int32, isRequired: true), + InputFactory.Property("minimumCount", InputPrimitiveType.Int32, isRequired: true), + ]); + var derivedModel = InputFactory.Model( + "DerivedModel", + usage: InputModelTypeUsage.Input, + properties: + [ + InputFactory.Property("label", InputPrimitiveType.String, isRequired: true), + ], + baseModel: baseModel); + + await MockHelpers.LoadMockGeneratorAsync( + inputModelTypes: [baseModel, derivedModel], + lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + var derived = (ModelProvider)CodeModelGenerator.Instance.OutputLibrary.TypeProviders + .Single(t => t.Name == "DerivedModel"); + var constructor = derived.Constructors.Single(c => + c.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Public)); + + Assert.Multiple(() => + { + Assert.That( + constructor.Signature.Parameters.Select(p => p.Name), + Is.EqualTo(new[] { "minimumCount", "maximumCount", "label" })); + Assert.IsTrue(constructor.Signature.Initializer!.IsBase); + Assert.That( + constructor.Signature.Initializer.Arguments.Select(a => a.ToDisplayString()), + Is.EqualTo(new[] { "minimumCount", "maximumCount" })); + }); + } + [Test] public async Task BackCompat_AbstractTypeConstructorAccessibility() { 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 index 4dc7415bd15..c674bd737a2 100644 --- 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 @@ -7,12 +7,13 @@ namespace Sample.Models { public partial class MockInputModel { - // The user supplies their own (name, resources) constructor, replacing the one the + // The user supplies their own (name, resourceList) constructor with the same overload + // identity as the previous (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) + public MockInputModel(string name, string resourceList) : this(name) { - Resources = resources; + Resources = resourceList; } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorParameterRenameIsAppliedBeforeBody/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorParameterRenameIsAppliedBeforeBody/MockInputModel.cs new file mode 100644 index 00000000000..078b9a972e8 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorParameterRenameIsAppliedBeforeBody/MockInputModel.cs @@ -0,0 +1,9 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + public MockInputModel(string oldName) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorParameterSwapIsAppliedBeforeBody/MockInputModel.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorParameterSwapIsAppliedBeforeBody/MockInputModel.cs new file mode 100644 index 00000000000..05ca872cda9 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ConstructorParameterSwapIsAppliedBeforeBody/MockInputModel.cs @@ -0,0 +1,9 @@ +namespace Sample.Models +{ + public partial class MockInputModel + { + public MockInputModel(int minimumCount, int maximumCount) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_DerivedInitializerUsesRestoredBaseParameterOrder/Models.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_DerivedInitializerUsesRestoredBaseParameterOrder/Models.cs new file mode 100644 index 00000000000..5cefc297e3b --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_DerivedInitializerUsesRestoredBaseParameterOrder/Models.cs @@ -0,0 +1,17 @@ +namespace Sample.Models +{ + public partial class BaseModel + { + public BaseModel(int minimumCount, int maximumCount) + { + } + } + + public partial class DerivedModel : BaseModel + { + public DerivedModel(int minimumCount, int maximumCount, string label) + : base(minimumCount, maximumCount) + { + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs index 6b5c17a6098..fa22c37312a 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TypeProviderTests.cs @@ -258,20 +258,19 @@ public async Task BuildConstructorsForBackCompatibilityRestoresSwappedParameterN declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, constructors: [currentConstructor]); - typeProvider.ProcessTypeForBackCompatibility(); + var previousParameters = typeProvider.LastContractView!.Constructors.Single().Signature.Parameters; + Assert.IsTrue(ConstructorBackCompatHelper.TryCreateRestoredParameters( + currentConstructor.Signature.Parameters, + previousParameters, + out var restoredParameters)); - var constructor = typeProvider.Constructors.Single(); Assert.Multiple(() => { - Assert.That(constructor.Signature.Parameters, Is.EqualTo(new[] { minimumCount, maximumCount })); - Assert.AreEqual("The minimum count.", constructor.Signature.Parameters[0].Description.Format); - Assert.AreEqual("The maximum count.", constructor.Signature.Parameters[1].Description.Format); - Assert.AreEqual( - "this.MaximumCount = maximumCount;\nthis.MinimumCount = minimumCount;\n", - constructor.BodyStatements!.ToDisplayString()); - Assert.That( - constructor.XmlDocs.Parameters.Select(p => p.Parameter), - Is.EqualTo(new[] { minimumCount, maximumCount })); + Assert.That(restoredParameters.Select(p => p.Name), Is.EqualTo(new[] { "minimumCount", "maximumCount" })); + Assert.AreEqual("The minimum count.", restoredParameters[0].Description.Format); + Assert.AreEqual("The maximum count.", restoredParameters[1].Description.Format); + Assert.AreNotSame(minimumCount, restoredParameters[0]); + Assert.That(currentConstructor.Signature.Parameters, Is.EqualTo(new[] { maximumCount, minimumCount })); }); } @@ -297,13 +296,17 @@ public async Task BuildConstructorsForBackCompatibilityRestoresSimpleParameterRe declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, constructors: [currentConstructor]); - typeProvider.ProcessTypeForBackCompatibility(); + var previousParameters = typeProvider.LastContractView!.Constructors.Single().Signature.Parameters; + Assert.IsTrue(ConstructorBackCompatHelper.TryCreateRestoredParameters( + currentConstructor.Signature.Parameters, + previousParameters, + out var restoredParameters)); Assert.Multiple(() => { - Assert.AreEqual("oldName", parameter.Name); - Assert.AreEqual("this.Value = oldName;\n", currentConstructor.BodyStatements!.ToDisplayString()); - Assert.AreEqual("oldName", currentConstructor.XmlDocs.Parameters.Single().Parameter.Name); + Assert.AreEqual("oldName", restoredParameters.Single().Name); + Assert.AreEqual("newName", parameter.Name); + Assert.AreNotSame(parameter, restoredParameters.Single()); }); } @@ -327,9 +330,13 @@ public async Task BuildConstructorsForBackCompatibilityRestoresParameterNameCasi declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, constructors: [currentConstructor]); - typeProvider.ProcessTypeForBackCompatibility(); + var previousParameters = typeProvider.LastContractView!.Constructors.Single().Signature.Parameters; + Assert.IsTrue(ConstructorBackCompatHelper.TryCreateRestoredParameters( + currentConstructor.Signature.Parameters, + previousParameters, + out var restoredParameters)); - Assert.AreEqual("vmwareSiteId", typeProvider.Constructors.Single().Signature.Parameters.Single().Name); + Assert.AreEqual("vmwareSiteId", restoredParameters.Single().Name); } [Test] @@ -356,10 +363,14 @@ public async Task BuildConstructorsForBackCompatibilityRestoresRotatedParameterN declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, constructors: [currentConstructor]); - typeProvider.ProcessTypeForBackCompatibility(); + var previousParameters = typeProvider.LastContractView!.Constructors.Single().Signature.Parameters; + Assert.IsTrue(ConstructorBackCompatHelper.TryCreateRestoredParameters( + currentConstructor.Signature.Parameters, + previousParameters, + out var restoredParameters)); Assert.That( - typeProvider.Constructors.Single().Signature.Parameters.Select(p => p.Name), + restoredParameters.Select(p => p.Name), Is.EqualTo(new[] { "first", "second", "third" })); } @@ -383,9 +394,13 @@ public async Task BuildConstructorsForBackCompatibilityDoesNotRenameParametersWh declarationModifiers: TypeSignatureModifiers.Public | TypeSignatureModifiers.Class, constructors: [currentConstructor]); - typeProvider.ProcessTypeForBackCompatibility(); + var previousParameters = typeProvider.LastContractView!.Constructors.Single().Signature.Parameters; - Assert.AreEqual("newName", typeProvider.Constructors.Single().Signature.Parameters.Single().Name); + Assert.IsFalse(ConstructorBackCompatHelper.TryCreateRestoredParameters( + currentConstructor.Signature.Parameters, + previousParameters, + out _)); + Assert.AreEqual("newName", currentConstructor.Signature.Parameters.Single().Name); } [Test] @@ -411,12 +426,23 @@ public async Task BuildConstructorsForBackCompatibilityMatchesValueTypeNullabili new TestTypeProvider()), ]); - typeProvider.ProcessTypeForBackCompatibility(); + var previousNullableParameters = typeProvider.LastContractView!.Constructors + .Single(c => c.Signature.Parameters.Single().Type.IsNullable) + .Signature.Parameters; + Assert.IsFalse(ConstructorBackCompatHelper.TryCreateRestoredParameters( + [nonNullableParameter], + previousNullableParameters, + out _)); + Assert.IsTrue(ConstructorBackCompatHelper.TryCreateRestoredParameters( + [nullableParameter], + previousNullableParameters, + out var restoredParameters)); Assert.Multiple(() => { Assert.AreEqual("nonNullableCurrent", nonNullableParameter.Name); - Assert.AreEqual("previousNullable", nullableParameter.Name); + Assert.AreEqual("previousNullable", restoredParameters.Single().Name); + Assert.AreEqual("nullableCurrent", nullableParameter.Name); }); } @@ -443,12 +469,23 @@ public async Task BuildConstructorsForBackCompatibilityMatchesNestedValueTypeNul new TestTypeProvider()), ]); - typeProvider.ProcessTypeForBackCompatibility(); + var previousNullableParameters = typeProvider.LastContractView!.Constructors + .Single(c => c.Signature.Parameters.Single().Type.Arguments.Single().IsNullable) + .Signature.Parameters; + Assert.IsFalse(ConstructorBackCompatHelper.TryCreateRestoredParameters( + [nonNullableParameter], + previousNullableParameters, + out _)); + Assert.IsTrue(ConstructorBackCompatHelper.TryCreateRestoredParameters( + [nullableParameter], + previousNullableParameters, + out var restoredParameters)); Assert.Multiple(() => { Assert.AreEqual("nonNullableCurrent", nonNullableParameter.Name); - Assert.AreEqual("previousNullableItems", nullableParameter.Name); + Assert.AreEqual("previousNullableItems", restoredParameters.Single().Name); + Assert.AreEqual("nullableCurrent", nullableParameter.Name); }); } @@ -483,7 +520,7 @@ public async Task BuildConstructorsForBackCompatibilityRejectsMixedNullabilityPe } [Test] - public void TryRestorePreviousConstructorParametersRejectsDuplicateFinalNames() + public void ConstructorParameterRestorationRejectsDuplicateFinalNames() { ParameterProvider[] currentParameters = [ @@ -495,19 +532,126 @@ public void TryRestorePreviousConstructorParametersRejectsDuplicateFinalNames() new("duplicate", $"", new CSharpType(typeof(string))), new("duplicate", $"", new CSharpType(typeof(string))), ]; - var currentConstructor = new ConstructorProvider( - new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, currentParameters), + Assert.Multiple(() => + { + Assert.IsFalse(ConstructorBackCompatHelper.TryCreateRestoredParameters( + currentParameters, + previousParameters, + out _)); + Assert.That(currentParameters.Select(p => p.Name), Is.EqualTo(new[] { "first", "second" })); + }); + } + + [Test] + public void ConstructorParameterRestorationRejectsPartialNameOverlap() + { + ParameterProvider[] currentParameters = + [ + new("second", $"", new CSharpType(typeof(string))), + new("replacement", $"", new CSharpType(typeof(string))), + new("third", $"", new CSharpType(typeof(string))), + ]; + ParameterProvider[] previousParameters = + [ + new("first", $"", new CSharpType(typeof(string))), + new("second", $"", new CSharpType(typeof(string))), + new("third", $"", new CSharpType(typeof(string))), + ]; + + Assert.IsFalse(ConstructorBackCompatHelper.TryCreateRestoredParameters( + currentParameters, + previousParameters, + out _)); + } + + [Test] + public void ConstructorParameterIdentityIncludesRefKindsAndParams() + { + var value = new ParameterProvider("value", $"", new CSharpType(typeof(int))); + var byRef = new ParameterProvider("value", $"", new CSharpType(typeof(int)), isRef: true); + var byIn = new ParameterProvider("value", $"", new CSharpType(typeof(int)), isIn: true); + var byOut = new ParameterProvider("value", $"", new CSharpType(typeof(int)), isOut: true); + var values = new ParameterProvider("values", $"", new CSharpType(typeof(int[])), isParams: true); + var array = new ParameterProvider("values", $"", new CSharpType(typeof(int[]))); + + Assert.Multiple(() => + { + Assert.IsFalse(ConstructorBackCompatHelper.HaveSameParameterIdentity([value], [byRef])); + Assert.IsFalse(ConstructorBackCompatHelper.HaveSameParameterIdentity([value], [byIn])); + Assert.IsFalse(ConstructorBackCompatHelper.HaveSameParameterIdentity([value], [byOut])); + Assert.IsFalse(ConstructorBackCompatHelper.HaveSameParameterIdentity([values], [array])); + }); + } + + [Test] + public void ConstructorParameterIdentityIncludesArrayRankAndNestedNullability() + { + var vector = new ParameterProvider("value", $"", new CSharpType(typeof(int[]))); + var matrix = new ParameterProvider("value", $"", new CSharpType(typeof(int[,]))); + var nestedNonNullable = new ParameterProvider("value", $"", new CSharpType(typeof(List))); + var nestedNullable = new ParameterProvider("value", $"", new CSharpType(typeof(List))); + + Assert.Multiple(() => + { + Assert.IsFalse(ConstructorBackCompatHelper.HaveSameParameterIdentity([vector], [matrix])); + Assert.IsFalse(ConstructorBackCompatHelper.HaveSameParameterIdentity([nestedNonNullable], [nestedNullable])); + }); + } + + [Test] + public void ConstructorParameterRestorationRejectsIllegalOptionalOrderAndAttributes() + { + var required = new ParameterProvider("required", $"", new CSharpType(typeof(string))); + var optional = new ParameterProvider("optional", $"", new CSharpType(typeof(string)), defaultValue: Snippet.Default); + ParameterProvider[] previousParameters = + [ + new("optional", $"", new CSharpType(typeof(string))), + new("required", $"", new CSharpType(typeof(string)), defaultValue: Snippet.Default), + ]; + + Assert.IsFalse(ConstructorBackCompatHelper.TryCreateRestoredParameters( + [required, optional], + previousParameters, + out _)); + + var attributed = new ParameterProvider( + "newName", + $"", + new CSharpType(typeof(string)), + attributes: [new AttributeStatement(typeof(ObsoleteAttribute))]); + Assert.IsFalse(ConstructorBackCompatHelper.TryCreateRestoredParameters( + [attributed], + [new ParameterProvider("oldName", $"", new CSharpType(typeof(string)))], + out _)); + } + + [Test] + public async Task ConstructorCompatibilityExcludesStaticAndPrivateProtectedContracts() + { + await MockHelpers.LoadMockGeneratorAsync(); + var typeProvider = new TestTypeProvider(); + var parameter = new ParameterProvider("value", $"", new CSharpType(typeof(string))); + var staticConstructor = new ConstructorProvider( + new ConstructorSignature( + typeof(object), + $"", + MethodSignatureModifiers.Public | MethodSignatureModifiers.Static, + [parameter]), Snippet.ThrowExpression(Snippet.Null), - new TestTypeProvider()); - var previousConstructor = new ConstructorProvider( - new ConstructorSignature(typeof(object), $"", MethodSignatureModifiers.Public, previousParameters), + typeProvider); + var privateProtectedConstructor = new ConstructorProvider( + new ConstructorSignature( + typeof(object), + $"", + MethodSignatureModifiers.Private | MethodSignatureModifiers.Protected, + [parameter]), Snippet.ThrowExpression(Snippet.Null), - new TestTypeProvider()); + typeProvider); Assert.Multiple(() => { - Assert.IsFalse(BackCompatHelper.TryRestorePreviousConstructorParameters(currentConstructor, previousConstructor)); - Assert.That(currentParameters.Select(p => p.Name), Is.EqualTo(new[] { "first", "second" })); + Assert.IsFalse(ConstructorBackCompatHelper.IsEligiblePreviousConstructor(typeProvider, staticConstructor)); + Assert.IsFalse(ConstructorBackCompatHelper.IsEligiblePreviousConstructor(typeProvider, privateProtectedConstructor)); }); } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/TypeSymbolExtensionsTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/TypeSymbolExtensionsTests.cs index b031193c398..91b45cd7179 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/TypeSymbolExtensionsTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/TypeSymbolExtensionsTests.cs @@ -151,6 +151,29 @@ public class Container Assert.AreEqual("System.Collections.Generic.IReadOnlyList`1", name); } + [Test] + public void MultidimensionalArrayFullyQualifiedNamePreservesRank() + { + var compilation = CSharpCompilation.Create( + "TestAssembly", + [CSharpSyntaxTree.ParseText(""" + namespace Sample + { + public class Container + { + public int[,] Matrix { get; } + } + } + """)], + [MetadataReference.CreateFromFile(typeof(object).Assembly.Location)]); + var property = compilation.GetTypeByMetadataName("Sample.Container")! + .GetMembers("Matrix") + .OfType() + .Single(); + + Assert.AreEqual("System.Int32[,]", property.Type.GetFullyQualifiedName()); + } + private static IPropertySymbol GetPropertySymbol(Compilation compilation, string containerName, string propertyName) { var typeSymbol = compilation.GetTypeByMetadataName($"Sample.{containerName}");