Preserve model factory back-compat parameter optionality

[Copilot]

Model-factory compatibility overloads either made every parameter required or retained fully optional reordered signatures, breaking callers or creating ambiguous overloads.

Changes

• Compute the minimum required positional prefix across all surviving generated and custom overloads.
• Preserve trailing defaults once parameter types distinguish the compatibility overload.
• Preserve all defaults when no competing overload exists.
• Cover required/optional boundaries, fully optional reorders, custom overloads, and nullable value types.

CompatibilityModel(
    string id,
    string name,
    bool? enabled,
    string description = default);

• Fixes [http-client-csharp] Preserve model factory back-compat parameter optionality when unambiguous #11667

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@typespec/http-client-csharp@11703

commit: 14fde4f

[Copilot]

Pull request overview

This PR updates the http-client-csharp generator’s model-factory back-compat signature shaping so compatibility overloads preserve optional trailing parameters when doing so does not introduce ambiguous overload resolution, while still requiring the minimum positional prefix needed to disambiguate against surviving generated/custom overloads.

Changes:

• Extend MethodSignatureHelper.BuildBackCompatMethodSignature to compute and enforce a minimum “required parameter prefix” based on competing overload signatures.
• Update ModelFactoryProvider to pass the relevant competing overload set (including custom overloads) when constructing back-compat shims.
• Add/adjust unit tests and golden test data to cover reordered overloads, fully-optional overloads, custom overload coexistence, and nullable value-type disambiguation.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Shared/MethodSignatureHelper.cs	Adds overload-aware logic to require only the minimum disambiguating parameter prefix while preserving trailing defaults when safe.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelFactoryProvider.cs	Computes and supplies the current competing overload set (including custom methods) when generating back-compat model factory methods.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Shared/MethodSignatureHelperTests.cs	Adds focused unit tests validating minimum-prefix computation across multiple overloads and value-type nullability distinctions.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelFactories/ModelFactoryProviderTests.cs	Adds integration tests asserting required/optional boundaries for generated back-compat factory methods across key scenarios.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelFactories/TestData/ModelFactoryProviderTests/BackCompatibility_UnknownDiscriminatorReturnTypeOverloadIsGenerated.cs	Updates expected output to preserve optional parameters when no competing overload exists for the compatibility overload.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelFactories/TestData/ModelFactoryProviderTests/BackCompatibility_ReorderedRequiredParametersPreserveOptionalTrailingParameters/SampleNamespaceModelFactory.cs	New test asset representing a prior contract with required prefix + optional trailing defaults.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelFactories/TestData/ModelFactoryProviderTests/BackCompatibility_ReorderedFullyOptionalParametersRequireMinimumPrefix/SampleNamespaceModelFactory.cs	New test asset representing a fully-optional prior contract reorder scenario.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelFactories/TestData/ModelFactoryProviderTests/BackCompatibility_ReorderedCustomOverloadRequiresMinimumPrefix(Last)/SampleNamespaceModelFactory.cs	New “last contract” asset used to validate interplay with custom overloads.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelFactories/TestData/ModelFactoryProviderTests/BackCompatibility_ReorderedCustomOverloadRequiresMinimumPrefix(Custom)/SampleNamespaceModelFactory.cs	New “custom” asset used to validate coexistence/minimum-prefix computation against custom overload signatures.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot It doesn't seem our fix is working as intended in the test azure library Azure/azure-sdk-for-net#62209. The pr shows that the SiteContainerData model factory back co...

Confirmed. The Azure diff showed the prior required prefix was being promoted through every trailing optional parameter. Fixed in 890dd3e: overlapping all-optional current overloads now retain an existing required prefix and preserve the prior trailing defaults. Added helper and model-factory snapshot regressions for that reordered shape.

[Jorge Rangel (jorgerangel-msft)]

Copilot

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (2)

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

• BuildBackCompatMethodSignature(previous, hideMethod, currentMethodSignatures, ...) currently calls RequireMinimumParameterPrefix(previousMethodSignature, ...), which mutates the last-contract signature in place (it nulls DefaultValue on its ParameterProvider instances). In ModelFactoryProvider those previous signatures are reused while processing other overloads, so this introduces order-dependent behavior and can affect later comparisons.

Consider applying the required-prefix adjustment only to the new back-compat signature (leaving the previous-contract signature unchanged).

        internal static MethodSignature BuildBackCompatMethodSignature(
            MethodSignature previousMethodSignature,
            bool hideMethod,
            IReadOnlyList<MethodSignature> currentMethodSignatures,
            bool shouldNotBeAsync = false)

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

• The XML doc for RequireMinimumParameterPrefix says that when no overloads are supplied "every parameter becomes required", but the implementation only forces all parameters required when currentMethodSignatures is null (an empty list results in requiredParameterCount = 0). Updating the wording would avoid misleading future maintainers.

        /// 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>

[JoshLove-msft]

Found a source-compatibility regression in the overload analysis.

[JoshLove-msft]

[P1] Do not treat last-contract overloads as new competitors

Adding every other compatiblePreviousMethod to the analysis set can remove defaults even when there is no surviving current/custom overload with that method name. Those overloads already coexisted in the published contract. For example, after a model/factory rename, prior overloads Old(string id, string description = default) and Old(string name = default) make the first overload fully required because the second is a positional prefix. That breaks the previously valid named call Old(id: "x"); reversing declaration order can additionally make Old() invalid because signatures are mutated in place. Only surviving current/custom overloads should drive optionality promotion. Please add a regression with multiple removed or renamed prior overloads and named calls.

--generated by Copilot

[Jorge Rangel (jorgerangel-msft)]

Addressed. Published last-contract overloads are no longer treated as new competitors for one another. Compatibility overloads retain their published defaults; required-prefix promotion is applied to newly generated overloads, while only non-published custom overloads can still constrain a compatibility overload.

Added BackCompatibility_CoexistingPreviousOverloadsKeepPublishedOptionality and its reversed-declaration-order counterpart to verify that the wider published overload keeps its trailing defaults and that processing is order-independent.

🤖 Generated by Jorge's Copilot

Review state corrected; PR reviews should be posted as comments, not changes requested.

[Copilot]

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.

Suppressed comments (1)

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

• GetMinimumRequiredParameterCount uses CSharpType.AreNamesEqual(...) to detect the first distinguishing parameter type, but AreNamesEqual ignores nullability. In C#, T vs T? for value types are distinct overloads (see MethodSignatureBase.cs:179-187), so nullable value type differences should be treated as a distinguishing type here as well. Otherwise, overloads that differ only by nullable value type will be treated as identical and may force all parameters to become required unnecessarily, breaking the intended “minimum required prefix” behavior.

            for (int i = 0; i < overlappingParameterCount; i++)
            {
                if (!targetMethodSignature.Parameters[i].Type.AreNamesEqual(competingMethodSignature.Parameters[i].Type))
                {
                    return Math.Max(i + 1, targetMinimumArgumentCount);
                }

[Copilot]

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.

Suppressed comments (2)

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Shared/MethodSignatureHelperTests.cs:12

• using Microsoft.TypeSpec.Generator.Tests.Common; is unused in this test file (no Helpers/other symbols referenced), which can introduce avoidable warnings and makes the imports noisier.
  packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Shared/MethodSignatureHelper.cs:88
• The XML doc for RequireMinimumParameterPrefix says that when “no overloads are supplied” every parameter becomes required, but the implementation only does that when currentMethodSignatures is null. When an empty list is passed (e.g., “no competing overload exists”), GetMinimumRequiredParameterCount returns 0 and defaults are preserved. Updating the doc would prevent callers from misusing null vs empty lists.

        /// <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.