Preserve model factory back-compat parameter optionality

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelFactoryProvider.cs

@@ -102,6 +102,7 @@ protected internal sealed override IReadOnlyList<MethodProvider> BuildMethodsFor
                 return [.. originalMethods];
             }
 
+            IReadOnlyList<MethodProvider> customFactoryMethods = CustomCodeView?.Methods ?? [];
             List<MethodProvider> factoryMethods = [.. originalMethods];
 
             // Preserve the original parameter names on current factory methods when the only
@@ -110,18 +111,48 @@ protected internal sealed override IReadOnlyList<MethodProvider> BuildMethodsFor
             // property is renamed via @@clientName, spec rename, or naming-rule change).
             BackCompatHelper.RestorePreviousParameterNames(this, factoryMethods);
 
-            HashSet<MethodSignature> currentMethodSignatures = new List<MethodProvider>([.. factoryMethods, .. CustomCodeView?.Methods ?? []])
-               .Select(m => m.Signature)
-               .ToHashSet(MethodSignature.MethodSignatureComparer);
+            var allFactoryMethods = factoryMethods
+               .Concat(customFactoryMethods)
+               .ToList();
+            HashSet<MethodSignature> currentMethodSignatures = allFactoryMethods
+                .Select(m => m.Signature)
+                .ToHashSet(MethodSignature.MethodSignatureComparer);
 
+            var compatiblePreviousMethods = new List<MethodProvider>();
+            HashSet<MethodSignature> previousPublicSignatures = new(MethodSignature.MethodSignatureComparer);
             foreach (var previousMethod in LastContractView.Methods)
             {
-                if (!MethodSignatureHelper.IsPublicApi(previousMethod.Signature.Modifiers) ||
-                    currentMethodSignatures.Contains(previousMethod.Signature))
+                if (!MethodSignatureHelper.IsPublicApi(previousMethod.Signature.Modifiers))
                 {
                     continue;
                 }
 
+                // Record every public previous signature, including the ones skipped below, because
+                // a current overload that still matches one of them must never be removed.
+                previousPublicSignatures.Add(previousMethod.Signature);
+
+                if (currentMethodSignatures.Contains(previousMethod.Signature))
+                {
+                    // A method with this signature is already present, but it may have been restored
+                    // from the last contract by a library visitor before this pass ran.
+                    var restoredOverload = factoryMethods.FirstOrDefault(m =>
+                        MethodSignature.MethodSignatureComparer.Equals(m.Signature, previousMethod.Signature)
+                        && m.Signature.Attributes.Any(a =>
+                            a.Type is { IsFrameworkType: true }
+                            && a.Type.FrameworkType == typeof(System.ComponentModel.EditorBrowsableAttribute)));
+                    if (restoredOverload is not null)
+                    {
+                        MethodSignatureHelper.RequireMinimumParameterPrefix(
+                            restoredOverload.Signature,
+                            GetCurrentOverloadSignatures(
+                                allFactoryMethods,
+                                restoredOverload.Signature.Name,
+                                restoredOverload));
+                    }
+
+                    continue;
+                }
+
                 // If the removal of this factory method has already been accepted in the ApiCompat
                 // baseline, honor that decision and do not resurrect a compatibility shim for it.
                 if (BackCompatHelper.IsMethodRemovalAcceptedInBaseline(this, previousMethod.Signature))
@@ -138,55 +169,85 @@ protected internal sealed override IReadOnlyList<MethodProvider> BuildMethodsFor
                     continue;
                 }
 
+                compatiblePreviousMethods.Add(previousMethod);
+            }
+
+            foreach (var previousMethod in compatiblePreviousMethods)
+            {
                 List<MethodSignature> currentOverloads = [];
                 bool foundCompatibleOverload = false;
+                var currentOverloadSignatures = GetCurrentOverloadSignatures(
+                    allFactoryMethods,
+                    previousMethod.Signature.Name);
 
                 // Attempt to find an updated method in the current contract to call
-                foreach (var currentMethodSignature in currentMethodSignatures)
+                foreach (var currentMethodSignature in currentOverloadSignatures)
                 {
-                    if (currentMethodSignature.Name.Equals(previousMethod.Signature.Name))
+                    if (MethodSignatureHelper.HaveSameParametersInSameOrder(currentMethodSignature, previousMethod.Signature))
                     {
-                        if (MethodSignatureHelper.HaveSameParametersInSameOrder(currentMethodSignature, previousMethod.Signature))
-                        {
-                            foundCompatibleOverload = true;
-                            break;
-                        }
-
-                        currentOverloads.Add(currentMethodSignature);
+                        foundCompatibleOverload = true;
+                        break;
                     }
+
+                    currentOverloads.Add(currentMethodSignature);
                 }
 
                 if (foundCompatibleOverload)
                 {
                     continue;
                 }
 
+                var compatibilityOverloadSignatures = GetCompatibilityOverloadSignatures(
+                    currentOverloadSignatures,
+                    compatiblePreviousMethods,
+                    previousMethod);
+
                 foreach (var currentOverload in currentOverloads)
                 {
-                    // If the parameter ordering is the only difference, just use the previous method
+                    // If the parameter ordering is the only difference, just use the previous method.
                     if (MethodSignatureHelper.ContainsSameParameters(previousMethod.Signature, currentOverload)
-                        && TryBuildCompatibleMethodForPreviousContract(previousMethod, currentOverload, false, out MethodProvider? replacedMethod))
+                        && !previousPublicSignatures.Contains(currentOverload))
                     {
-                        factoryMethods.Add(replacedMethod);
-
                         var factoryMethodToRemove = factoryMethods
                             .FirstOrDefault(m => MethodSignature.MethodSignatureComparer.Equals(m.Signature, currentOverload));
-                        if (factoryMethodToRemove != null)
+                        var coexistingOverloads = GetCurrentOverloadSignatures(
+                            allFactoryMethods,
+                            previousMethod.Signature.Name,
+                            factoryMethodToRemove);
+                        var coexistingCompatibilityOverloads = GetCompatibilityOverloadSignatures(
+                            coexistingOverloads,
+                            compatiblePreviousMethods,
+                            previousMethod);
+                        if (TryBuildCompatibleMethodForPreviousContract(
+                            previousMethod,
+                            currentOverload,
+                            false,
+                            coexistingCompatibilityOverloads,
+                            out MethodProvider? replacedMethod))
                         {
-                            factoryMethods.Remove(factoryMethodToRemove);
-                        }
+                            factoryMethods.Add(replacedMethod);
 
-                        CodeModelGenerator.Instance.Emitter.Debug(
-                            $"Replaced model factory method '{Name}.{currentOverload.Name}' with previous parameter order from last contract.",
-                            BackCompatibilityChangeCategory.ModelFactoryMethodReplaced);
+                            if (factoryMethodToRemove != null)
+                            {
+                                factoryMethods.Remove(factoryMethodToRemove);
+                            }
 
-                        foundCompatibleOverload = true;
-                        break;
+                            CodeModelGenerator.Instance.Emitter.Debug(
+                                $"Replaced model factory method '{Name}.{currentOverload.Name}' with previous parameter order from last contract.",
+                                BackCompatibilityChangeCategory.ModelFactoryMethodReplaced);
+                            foundCompatibleOverload = true;
+                            break;
+                        }
                     }
 
-                    if (TryBuildCompatibleMethodForPreviousContract(previousMethod, currentOverload, true, out replacedMethod))
+                    if (TryBuildCompatibleMethodForPreviousContract(
+                        previousMethod,
+                        currentOverload,
+                        true,
+                        compatibilityOverloadSignatures,
+                        out var hiddenMethod))
                     {
-                        factoryMethods.Add(replacedMethod);
+                        factoryMethods.Add(hiddenMethod);
                         CodeModelGenerator.Instance.Emitter.Debug(
                             $"Added back-compat overload for model factory method '{Name}.{previousMethod.Signature.Name}' delegating to '{currentOverload.Name}'.",
                             BackCompatibilityChangeCategory.ModelFactoryMethodAdded);
@@ -201,7 +262,12 @@ protected internal sealed override IReadOnlyList<MethodProvider> BuildMethodsFor
                 }
 
                 // If no compatible overload found, try to add the previous method by instantiating the model directly.
-                if (TryBuildCompatibleMethodForPreviousContract(previousMethod, null, true, out var builtMethod))
+                if (TryBuildCompatibleMethodForPreviousContract(
+                    previousMethod,
+                    null,
+                    true,
+                    compatibilityOverloadSignatures,
+                    out var builtMethod))
                 {
                     factoryMethods.Add(builtMethod);
                     CodeModelGenerator.Instance.Emitter.Debug(
@@ -219,6 +285,35 @@ protected internal sealed override IReadOnlyList<MethodProvider> BuildMethodsFor
             return [.. factoryMethods];
         }
 
+        private static IReadOnlyList<MethodSignature> GetCompatibilityOverloadSignatures(
+            IReadOnlyList<MethodSignature> currentOverloadSignatures,
+            IReadOnlyList<MethodProvider> compatiblePreviousMethods,
+            MethodProvider previousMethod)
+        {
+            HashSet<MethodSignature> overloadSignatures = new(MethodSignature.MethodSignatureComparer);
+            foreach (var compatiblePreviousMethod in compatiblePreviousMethods)
+            {
+                if (!ReferenceEquals(compatiblePreviousMethod, previousMethod))
+                {
+                    overloadSignatures.Add(compatiblePreviousMethod.Signature);
+                }
+            }
+
+            overloadSignatures.UnionWith(currentOverloadSignatures);
+            return [.. overloadSignatures];
+        }
+
+        private IReadOnlyList<MethodSignature> GetCurrentOverloadSignatures(
+            IEnumerable<MethodProvider> methods,
+            string methodName,
+            MethodProvider? methodToExclude = null)
+        {
+            return methods
+                .Where(m => (methodToExclude is null || !ReferenceEquals(m, methodToExclude)) && m.Signature.Name == methodName)
+                .Select(m => m.Signature)
+                .ToList();
+        }
+
         internal static IReadOnlyList<string> GetUnavailableSignatureTypes(MethodSignature signature)
         {
             var unavailableTypes = new HashSet<string>(StringComparer.Ordinal);
@@ -310,6 +405,7 @@ private bool TryBuildCompatibleMethodForPreviousContract(
             MethodProvider previousMethod,
             MethodSignature? currentMethodSignature,
             bool hideMethod,
+            IReadOnlyList<MethodSignature> currentOverloadSignatures,
             [NotNullWhen(true)] out MethodProvider? builtMethod)
         {
             builtMethod = null;
@@ -345,7 +441,10 @@ private bool TryBuildCompatibleMethodForPreviousContract(
             {
                 var callToOverload = Return(new InvokeMethodExpression(null, currentMethodSignature, arguments));
                 builtMethod = new MethodProvider(
-                    MethodSignatureHelper.BuildBackCompatMethodSignature(previousMethod.Signature, hideMethod),
+                    MethodSignatureHelper.BuildBackCompatMethodSignature(
+                        previousMethod.Signature,
+                        hideMethod,
+                        currentMethodSignatures: currentOverloadSignatures),
                     callToOverload,
                     this,
                     previousMethod.XmlDocs);
@@ -356,7 +455,10 @@ private bool TryBuildCompatibleMethodForPreviousContract(
             MethodBodyStatements body = ConstructMethodBody(previousMethod.Signature, modelToInstantiate);
 
             builtMethod = new MethodProvider(
-                MethodSignatureHelper.BuildBackCompatMethodSignature(previousMethod.Signature, hideMethod),
+                MethodSignatureHelper.BuildBackCompatMethodSignature(
+                    previousMethod.Signature,
+                    hideMethod,
+                    currentMethodSignatures: currentOverloadSignatures),
                 body,
                 this,
                 previousMethod.XmlDocs);

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Shared/MethodSignatureHelper.cs

@@ -57,17 +57,55 @@ internal static bool HaveSameParametersInSameOrder(MethodSignature method1, Meth
             return true;
         }
 
-        internal static MethodSignature BuildBackCompatMethodSignature(MethodSignature previousMethodSignature, bool hideMethod, bool shouldNotBeAsync = false)
+        internal static MethodSignature BuildBackCompatMethodSignature(
+            MethodSignature previousMethodSignature,
+            bool hideMethod,
+            bool shouldNotBeAsync = false)
         {
             if (hideMethod)
             {
-                // make all parameter required to avoid ambiguous call sites if necessary
-                foreach (var param in previousMethodSignature.Parameters)
-                {
-                    param.DefaultValue = null;
-                }
+                RequireMinimumParameterPrefix(previousMethodSignature);
+            }
+
+            return CreateBackCompatSignature(previousMethodSignature, hideMethod, shouldNotBeAsync);
+        }
+
+        internal static MethodSignature BuildBackCompatMethodSignature(
+            MethodSignature previousMethodSignature,
+            bool hideMethod,
+            IReadOnlyList<MethodSignature> currentMethodSignatures,
+            bool shouldNotBeAsync = false)
+        {
+            RequireMinimumParameterPrefix(previousMethodSignature, currentMethodSignatures);
+
+            return CreateBackCompatSignature(previousMethodSignature, hideMethod, shouldNotBeAsync);
+        }
+
+        /// <summary>
+        /// Removes the default values from the leading parameters of <paramref name="signature"/> so it
+        /// can no longer be called with fewer arguments than the prefix that distinguishes it from
+        /// <paramref name="currentMethodSignatures"/>. When no overloads are supplied there is nothing to
+        /// compare against and every parameter becomes required.
+        /// </summary>
+        internal static void RequireMinimumParameterPrefix(
+            MethodSignature signature,
+            IReadOnlyList<MethodSignature>? currentMethodSignatures = null)
+        {
+            int requiredParameterCount = currentMethodSignatures is null
+                ? signature.Parameters.Count
+                : GetMinimumRequiredParameterCount(signature, currentMethodSignatures);
+
+            for (int i = 0; i < requiredParameterCount; i++)
+            {
+                signature.Parameters[i].DefaultValue = null;
             }
+        }
 
+        private static MethodSignature CreateBackCompatSignature(
+            MethodSignature previousMethodSignature,
+            bool hideMethod,
+            bool shouldNotBeAsync)
+        {
             var modifiers = shouldNotBeAsync
                 ? previousMethodSignature.Modifiers & ~MethodSignatureModifiers.Async
                 : previousMethodSignature.Modifiers;
@@ -85,6 +123,82 @@ internal static MethodSignature BuildBackCompatMethodSignature(MethodSignature p
                 Attributes: attributes);
         }
 
+        private static int GetMinimumRequiredParameterCount(
+            MethodSignature previousMethodSignature,
+            IReadOnlyList<MethodSignature> currentMethodSignatures)
+        {
+            int requiredParameterCount = 0;
+            foreach (var currentMethodSignature in currentMethodSignatures)
+            {
+                if (currentMethodSignature.Name == previousMethodSignature.Name)
+                {
+                    requiredParameterCount = Math.Max(
+                        requiredParameterCount,
+                        GetMinimumRequiredParameterCount(previousMethodSignature, currentMethodSignature));
+                }
+            }
+
+            return requiredParameterCount;
+        }
+
+        private static int GetMinimumRequiredParameterCount(
+            MethodSignature previousMethodSignature,
+            MethodSignature currentMethodSignature)
+        {
+            if (currentMethodSignature.Parameters.Any(p => p.IsRef || p.IsOut))
+            {
+                return 0;
+            }
+
+            int previousMinimumArgumentCount = GetMinimumArgumentCount(previousMethodSignature);
+            int currentMinimumArgumentCount = GetMinimumArgumentCount(currentMethodSignature);
+            int currentMaximumArgumentCount = currentMethodSignature.Parameters.Any(p => p.IsParams)
+                ? int.MaxValue
+                : currentMethodSignature.Parameters.Count;
+
+            // No argument count can reach both overloads, so the previous signature is already
+            // unambiguous and keeps the optionality it was published with.
+            if (Math.Max(previousMinimumArgumentCount, currentMinimumArgumentCount) >
+                Math.Min(previousMethodSignature.Parameters.Count, currentMaximumArgumentCount))
+            {
+                return 0;
+            }
+
+            // Require only the prefix up to and including the first position whose parameter type
+            // differs. Any call supplying that many arguments can no longer bind to the current
+            // overload, so every trailing parameter keeps the optionality it had previously.
+            int overlappingParameterCount = Math.Min(
+                previousMethodSignature.Parameters.Count,
+                currentMethodSignature.Parameters.Count);
+            for (int i = 0; i < overlappingParameterCount; i++)
+            {
+                if (!previousMethodSignature.Parameters[i].Type.AreNamesEqual(currentMethodSignature.Parameters[i].Type))
+                {
+                    return Math.Max(i + 1, previousMinimumArgumentCount);
+                }
+            }
+
+            // The shorter signature is a positional prefix of the other, so no argument count
+            // distinguishes them. Fall back to requiring every parameter.
+            return previousMethodSignature.Parameters.Count;
+        }
+
+        private static int GetMinimumArgumentCount(MethodSignature methodSignature)
+        {
+            int count = 0;
+            foreach (var parameter in methodSignature.Parameters)
+            {
+                if (parameter.DefaultValue is not null || parameter.IsParams)
+                {
+                    break;
+                }
+
+                count++;
+            }
+
+            return count;
+        }
+
         private sealed class ParameterProviderVariableNameComparer : IEqualityComparer<ParameterProvider>
         {
             public bool Equals(ParameterProvider? x, ParameterProvider? y)