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: 94550ea

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

[github-actions]

No changes needing a change description found.

[Jorge Rangel (jorgerangel-msft)]

Copilot I'd like to see more unit tests with back compat + custom code where the resulting generated signature does not have all the parameters as required, only a subset of them. In addition, we should be ensuring we are keeping perf in mind for all changes. The back compat processing is getting quite complex and difficult to follow, so we should avoid overcomplicating things if possible

[Copilot]

Copilot I'd like to see more unit tests with back compat + custom code where the resulting generated signature does not have all the parameters as required, only a subset of them. In addition, we sho...

Addressed in c4d1cde. Added custom-overload coverage that preserves trailing optional parameters, and cached the combined factory method list.

[Jorge Rangel (jorgerangel-msft)]

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 compat method is still being generated with all params as required, and another overload with all params optional. The previous GA signature is public static SiteContainerData SiteContainerData(ResourceIdentifier id = null, string name = null, ResourceType resourceType = null, SystemData systemData = null, string image = null, string targetPort = null, bool? isMain = null, string startUpCommand = null, SiteContainerAuthType? authType = null, string userName = null, string passwordSecret = null, string userManagedIdentityClientId = null, DateTimeOffset? createdOn = null, DateTimeOffset? lastModifiedOn = null, IEnumerable<SiteContainerVolumeMount> volumeMounts = null, bool? inheritAppSettingsAndConnectionStrings = null, IEnumerable<WebAppEnvironmentVariable> environmentVariables = null, string kind = null); . And the custom code that was removed in the pr shows what the generated shape should've been for the back compat method. Validate the pr diff, confirm if my analysis is correct, and work on correcting this in the generator as part of this pr

[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

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