diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs index 38f74bddf37..77cc9c53543 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs @@ -43,6 +43,12 @@ public enum BackCompatibilityChangeCategory /// A back-compat overload of a client method was added because new optional non-body parameter(s) were introduced relative to the last contract. SvcMethodNewOptionalParameterOverloadAdded, + /// A back-compat overload of a client method was added because a value-type parameter's nullability was removed (e.g. T? -> T) relative to the last contract. + SvcMethodParameterNullabilityChangeOverloadAdded, + + /// A back-compat reduced-arity overload of a client method was added because a nullable parameter changed from optional to required relative to the last contract. + SvcMethodParameterOptionalityRestorationOverloadAdded, + /// A back-compat change was skipped because the removal was accepted in the ApiCompat baseline. BaselineAcceptedRemovalSkipped, diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs index 83d965b4ed0..7c073c93c99 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs @@ -181,6 +181,8 @@ public void WriteBufferedMessages() BackCompatibilityChangeCategory.ModelFactoryMethodAdded => "Model Factory Method Added For Back-Compat", BackCompatibilityChangeCategory.ModelFactoryMethodSkipped => "Model Factory Method Back-Compat Skipped", BackCompatibilityChangeCategory.SvcMethodNewOptionalParameterOverloadAdded => "Method Back-Compat Overload Added For New Optional Parameter", + BackCompatibilityChangeCategory.SvcMethodParameterNullabilityChangeOverloadAdded => "Method Back-Compat Overload Added For Parameter Nullability Change", + BackCompatibilityChangeCategory.SvcMethodParameterOptionalityRestorationOverloadAdded => "Method Back-Compat Overload Added For Parameter Optionality Change", BackCompatibilityChangeCategory.BaselineAcceptedRemovalSkipped => "Back-Compat Skipped For ApiCompat Baseline Accepted Removal", BackCompatibilityChangeCategory.EnumMemberAddedFromLastContract => "Enum Member Added From Last Contract", _ => category.ToString(), diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/CSharpType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/CSharpType.cs index 961075449e8..f12dc823010 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/CSharpType.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/CSharpType.cs @@ -642,6 +642,25 @@ public bool AreNamesEqual(CSharpType? other) return true; } + /// + /// Checks whether two instances have equal names, optionally also requiring + /// their nullability to match. Unlike , this compares only + /// names (and generic argument names), which is useful when comparing against a type read from source + /// that may not carry the same metadata (for example an extensible enum, read back as a struct). + /// + /// The instance to compare to. + /// When true, the two types must also have the same value. + /// true if the names (and, when requested, nullability) are equal; false otherwise. + internal bool AreNamesEqual(CSharpType? other, bool checkNullability) + { + if (!AreNamesEqual(other)) + { + return false; + } + + return !checkNullability || IsNullable == other!.IsNullable; + } + private bool IsNameMatch(CSharpType other) { if (string.IsNullOrEmpty(Namespace)) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/MethodSignatureBase.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/MethodSignatureBase.cs index af687f4bd73..dd1adca822f 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/MethodSignatureBase.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/MethodSignatureBase.cs @@ -87,10 +87,18 @@ public void Update(string? name = default, FormattableString? description = defa } } - public static readonly IEqualityComparer SignatureComparer = new MethodSignatureBaseEqualityComparer(); + public static readonly IEqualityComparer SignatureComparer = new MethodSignatureBaseEqualityComparer(checkNullability: false); + internal static readonly IEqualityComparer SignatureComparerIncludingNullability = new MethodSignatureBaseEqualityComparer(checkNullability: true); private class MethodSignatureBaseEqualityComparer : IEqualityComparer { + private readonly bool _checkNullability; + + public MethodSignatureBaseEqualityComparer(bool checkNullability) + { + _checkNullability = checkNullability; + } + public bool Equals(MethodSignatureBase? x, MethodSignatureBase? y) { if (ReferenceEquals(x, y)) @@ -160,7 +168,7 @@ public bool Equals(MethodSignatureBase? x, MethodSignatureBase? y) for (int i = 0; i < x.Parameters.Count; i++) { - if (!x.Parameters[i].Type.AreNamesEqual(y.Parameters[i].Type)) + if (!x.Parameters[i].Type.AreNamesEqual(y.Parameters[i].Type, _checkNullability)) { return false; } 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 83380c4299b..0c5924d9f9b 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 @@ -861,7 +861,7 @@ protected internal virtual IReadOnlyList BuildMethodsForBackComp } BackCompatHelper.RestorePreviousParameterNames(this, methods); - BackCompatHelper.AddOverloadsForNewOptionalParameters(this, methods); + BackCompatHelper.AddBackCompatOverloads(this, methods); return methods; } 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 33ad107cb95..a378a60e97e 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 @@ -3,12 +3,14 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using System.Linq; using Microsoft.TypeSpec.Generator.EmitterRpc; using Microsoft.TypeSpec.Generator.Expressions; using Microsoft.TypeSpec.Generator.Input.Extensions; using Microsoft.TypeSpec.Generator.Primitives; using Microsoft.TypeSpec.Generator.Providers; +using Microsoft.TypeSpec.Generator.Snippets; using Microsoft.TypeSpec.Generator.Statements; using static Microsoft.TypeSpec.Generator.Snippets.Snippet; @@ -33,9 +35,7 @@ public static bool ShouldApplyMethodBackCompatibility( return false; } - var modifiers = previousSignature.Modifiers; - return modifiers.HasFlag(MethodSignatureModifiers.Public) || - modifiers.HasFlag(MethodSignatureModifiers.Protected); + return (previousSignature.Modifiers & (MethodSignatureModifiers.Public | MethodSignatureModifiers.Protected)) != 0; } /// @@ -335,13 +335,16 @@ private static void UpdateXmlDocProviderForParamReorder( } /// - /// Adds hidden back-compat overloads for public/protected methods that gained one or more new - /// optional non-body parameters relative to the last contract. Each added overload matches a - /// previously-published signature and delegates to the current method, forwarding the previous - /// arguments and passing default for every new parameter. Renaming a method's parameter - /// set this way is otherwise a source-breaking change for existing callers. + /// Adds hidden back-compat overloads for public/protected methods whose signature changed in a + /// recoverable way relative to the last contract: either one or more new optional non-body + /// parameters were added, or a value-type parameter's nullability was removed (T? became + /// T). Both scenarios are detected in a single scan of the current methods so they are not + /// re-classified once per scenario, and each produces a hidden overload reproducing the + /// previously-published signature that delegates to the current method. When both scenarios apply + /// to the same previous signature, the new-optional-parameter overload is preferred because it + /// forwards the shared parameters directly rather than unwrapping a nullable value type. /// - public static void AddOverloadsForNewOptionalParameters(TypeProvider enclosingType, List methods) + public static void AddBackCompatOverloads(TypeProvider enclosingType, List methods) { if (enclosingType.LastContractView?.Methods is not { Count: > 0 } previousMethods) { @@ -351,6 +354,7 @@ public static void AddOverloadsForNewOptionalParameters(TypeProvider enclosingTy var currentMethods = enclosingType.CustomCodeView?.Methods is { } customMethods ? methods.Concat(customMethods) : methods; + if (!currentMethods.Any()) { return; @@ -370,49 +374,110 @@ public static void AddOverloadsForNewOptionalParameters(TypeProvider enclosingTy foreach (var previousMethod in previousMethods) { var previousSignature = previousMethod.Signature; - if ((!previousSignature.Modifiers.HasFlag(MethodSignatureModifiers.Public) && - !previousSignature.Modifiers.HasFlag(MethodSignatureModifiers.Protected)) || + if ((previousSignature.Modifiers & (MethodSignatureModifiers.Public | MethodSignatureModifiers.Protected)) == 0 || !currentMethodsByName.TryGetValue(previousSignature.Name, out var candidates)) { continue; } - // One pass over the same-named methods: bail if the previous signature still exists, - // otherwise remember the first public/protected method that merely gained optional - // non-body parameters. - MethodProvider? matchedCurrent = null; - bool previousStillExists = false; + bool previousStillExistsIgnoringNullability = false; + bool previousStillExistsIncludingNullability = false; + MethodProvider? newOptionalMatch = null; + MethodProvider? nullabilityMatch = null; + MethodProvider? optionalityMatch = null; + foreach (var candidate in candidates) { - if (MethodSignature.MethodSignatureComparer.Equals(candidate.Signature, previousSignature)) + var candidateSignature = candidate.Signature; + bool candidateIsAccessible = (candidateSignature.Modifiers & (MethodSignatureModifiers.Public | MethodSignatureModifiers.Protected)) != 0; + + if (MethodSignatureBase.SignatureComparerIncludingNullability.Equals(candidateSignature, previousSignature)) { - previousStillExists = true; + if (candidateIsAccessible && IsSingleNullableParameterOptionalToRequired(previousSignature, candidateSignature)) + { + optionalityMatch = candidate; + } + else + { + previousStillExistsIncludingNullability = true; + } break; } - var modifiers = candidate.Signature.Modifiers; - if (matchedCurrent is null && - (modifiers.HasFlag(MethodSignatureModifiers.Public) || modifiers.HasFlag(MethodSignatureModifiers.Protected)) && - HasNewOptionalNonBodyParametersOnly(previousSignature, candidate.Signature)) + if (MethodSignature.MethodSignatureComparer.Equals(candidateSignature, previousSignature)) + { + previousStillExistsIgnoringNullability = true; + } + + // The remaining scenarios add a public/protected shim, so only accessible candidates matter. + if (!candidateIsAccessible) + { + continue; + } + + if (newOptionalMatch is null && HasNewOptionalNonBodyParametersOnly(previousSignature, candidateSignature)) + { + newOptionalMatch = candidate; + } + + if (nullabilityMatch is null && HasRelaxedNullableValueTypeParametersOnly(previousSignature, candidateSignature)) { - matchedCurrent = candidate; + nullabilityMatch = candidate; } } - if (previousStillExists || matchedCurrent is null || IsMethodRemovalAcceptedInBaseline(enclosingType, previousSignature)) + if (previousStillExistsIncludingNullability) { continue; } - var overload = BuildNewOptionalParameterOverload(enclosingType, previousMethod, matchedCurrent); + // Prefer, in order: the optionality-restoration overload (identical types, a nullable + // parameter became required), then the new-optional-parameter overload (forwards shared + // parameters directly), then the nullability overload (which unwraps the previously-nullable + // value type with .Value). The new-optional scenario only applies when the signature no + // longer exists even when parameter nullability is ignored. + bool canAddNewOptional = !previousStillExistsIgnoringNullability && newOptionalMatch is not null; + if (optionalityMatch is null && !canAddNewOptional && nullabilityMatch is null) + { + continue; + } + + if (IsMethodRemovalAcceptedInBaseline(enclosingType, previousSignature)) + { + continue; + } + + var (overload, category, reason) = true switch + { + _ when optionalityMatch is not null => ( + BuildOptionalityRestorationOverload(enclosingType, previousMethod, optionalityMatch), + BackCompatibilityChangeCategory.SvcMethodParameterOptionalityRestorationOverloadAdded, + "to restore a nullable parameter that changed from optional to required relative to the last contract."), + _ when canAddNewOptional => ( + BuildNewOptionalParameterOverload(enclosingType, previousMethod, newOptionalMatch!), + BackCompatibilityChangeCategory.SvcMethodNewOptionalParameterOverloadAdded, + "to handle new optional parameter(s) introduced relative to the last contract."), + _ => ( + BuildChangedParameterNullabilityOverload(enclosingType, previousMethod, nullabilityMatch!), + BackCompatibilityChangeCategory.SvcMethodParameterNullabilityChangeOverloadAdded, + "to preserve a parameter whose nullability was removed relative to the last contract."), + }; + candidates.Add(overload); methods.Add(overload); CodeModelGenerator.Instance.Emitter.Debug( - $"Added back-compat overload for '{enclosingType.Name}.{previousSignature.Name}' to handle new optional parameter(s) introduced relative to the last contract.", - BackCompatibilityChangeCategory.SvcMethodNewOptionalParameterOverloadAdded); + $"Added back-compat overload for '{enclosingType.Name}.{previousSignature.Name}' {reason}", + category); } } + // Returns true when both signatures have the same return type (matched by name); a null return type + // matches only another null return type. + private static bool ReturnTypesMatch(MethodSignature previous, MethodSignature current) => + previous.ReturnType is null + ? current.ReturnType is null + : previous.ReturnType.AreNamesEqual(current.ReturnType); + /// /// Returns true when contains all parameters of /// in the same relative order (matched by variable name and @@ -428,9 +493,7 @@ public static bool HasNewOptionalNonBodyParametersOnly( return false; } - if (previousSignature.ReturnType is null - ? currentSignature.ReturnType is not null - : !previousSignature.ReturnType.AreNamesEqual(currentSignature.ReturnType)) + if (!ReturnTypesMatch(previousSignature, currentSignature)) { return false; } @@ -485,46 +548,237 @@ private static MethodProvider BuildNewOptionalParameterOverload( // Named argument syntax cannot be combined with 'ref'/'out', so when any parameter is passed by // reference every argument is forwarded positionally instead (the arguments are already built in // the current method's parameter order). - bool hasByRefParameter = false; - foreach (var currentParam in currentSignature.Parameters) - { - if (currentParam.IsRef || currentParam.IsOut) - { - hasByRefParameter = true; - break; - } - } + bool hasByRefParameter = currentSignature.Parameters.Any(p => p.IsRef || p.IsOut); var arguments = new List(currentSignature.Parameters.Count); + var nullGuards = new List(); foreach (var currentParam in currentSignature.Parameters) { var variableName = currentParam.Name.ToVariableName(); ValueExpression value = previousParametersByName.TryGetValue(variableName, out var previousParam) - ? previousParam + ? ForwardParameter(previousParam, currentParam.Type, nullGuards) : currentParam.DefaultValue ?? Default; - if (hasByRefParameter) - { - arguments.Add(currentParam.IsRef || currentParam.IsOut - ? value.AsArgument(isRef: currentParam.IsRef, isOut: currentParam.IsOut) - : value); - } - else + AddForwardedArgument(arguments, currentParam, value, hasByRefParameter); + } + + var body = BuildDelegatingBody(enclosingType, currentSignature, arguments, nullGuards); + + // Preserve the previous parameter optionality when every previous parameter is required in the + // current method: the current method then cannot bind a call that omits a trailing parameter, so + // this hidden overload can keep its optional defaults without creating an ambiguous call site, + // and callers that omitted an optional parameter still compile. Otherwise the defaults are + // stripped (BuildBackCompatMethodSignature does this) to avoid an ambiguous call with the current + // method over a shared optional parameter (e.g. a trailing optional CancellationToken); in that + // case the current method itself still serves the omitting callers. + int requiredCurrentParameterCount = currentSignature.Parameters.Count(p => p.DefaultValue is null); + var preservedDefaults = requiredCurrentParameterCount >= previousSignature.Parameters.Count + ? previousSignature.Parameters.Select(p => p.DefaultValue).ToArray() + : null; + + // The shim delegates without awaiting, so it must not be declared 'async'. + var signature = MethodSignatureHelper.BuildBackCompatMethodSignature(previousSignature, hideMethod: true, shouldNotBeAsync: true); + + if (preservedDefaults is not null) + { + for (int i = 0; i < signature.Parameters.Count; i++) { - arguments.Add(PositionalReference(variableName, value)); + signature.Parameters[i].DefaultValue = preservedDefaults[i]; } } + return new MethodProvider( + signature, + body, + enclosingType, + previousMethod.XmlDocs); + } + + // Forwards a previous parameter to the current method. When the parameter's value-type nullability + // was removed (T? -> T) it is unwrapped with .Value and a null-guard is appended (T? does not + // implicitly convert to T, and the guard turns a null argument into a clear ArgumentNullException); + // otherwise it is forwarded unchanged. + private static ValueExpression ForwardParameter( + ParameterProvider previousParam, + CSharpType currentParamType, + List nullGuards) + { + if (IsNullabilityRelaxedValueType(previousParam.Type, currentParamType)) + { + nullGuards.Add(ArgumentSnippets.AssertNotNull(previousParam)); + return previousParam.Property(nameof(Nullable.Value)); + } + + return previousParam; + } + + private static void AddForwardedArgument( + List arguments, + ParameterProvider currentParam, + ValueExpression value, + bool hasByRefParameter) + { + if (hasByRefParameter) + { + arguments.Add(currentParam.IsRef || currentParam.IsOut + ? value.AsArgument(isRef: currentParam.IsRef, isOut: currentParam.IsOut) + : value); + } + else + { + arguments.Add(PositionalReference(currentParam.Name.ToVariableName(), value)); + } + } + + // Builds the shim body that forwards to the current method: any argument null-guards followed by the + // delegating call (returned directly, or terminated for a void return). + private static MethodBodyStatement BuildDelegatingBody( + TypeProvider enclosingType, + MethodSignature currentSignature, + IReadOnlyList arguments, + List? nullGuards = null) + { var invocationTarget = currentSignature.Modifiers.HasFlag(MethodSignatureModifiers.Static) ? Static(enclosingType.Type) : This; var delegatingCall = invocationTarget.Invoke(currentSignature.Name, arguments); - MethodBodyStatement body = IsVoidReturnType(currentSignature.ReturnType) + var returnType = currentSignature.ReturnType; + bool returnsVoid = returnType is null || (returnType.IsFrameworkType && returnType.FrameworkType == typeof(void)); + MethodBodyStatement delegatingStatement = returnsVoid ? delegatingCall.Terminate() : Return(delegatingCall); - // The shim delegates without awaiting, so it must not be declared 'async'. - var signature = MethodSignatureHelper.BuildBackCompatMethodSignature(previousSignature, hideMethod: true, shouldNotBeAsync: true); + if (nullGuards is { Count: > 0 }) + { + nullGuards.Add(delegatingStatement); + return nullGuards; + } + + return delegatingStatement; + } + + // Builds a hidden (EditorBrowsable.Never) overload signature from a previous signature, replacing + // its parameters and clearing the 'async' modifier (the shim delegates without awaiting). + private static MethodSignature BuildHiddenOverloadSignature( + MethodSignature previousSignature, + IReadOnlyList parameters) + { + return new MethodSignature( + previousSignature.Name, + previousSignature.Description, + previousSignature.Modifiers & ~MethodSignatureModifiers.Async, + previousSignature.ReturnType, + previousSignature.ReturnDescription, + parameters, + Attributes: [new(typeof(EditorBrowsableAttribute), FrameworkEnumValue(EditorBrowsableState.Never))]); + } + + /// + /// Returns true when has the same name, parameter count, and + /// return type as , every parameter matches by name and type, + /// and at least one parameter differs only in that a nullable value type (T?) became its + /// non-nullable form (T). + /// + public static bool HasRelaxedNullableValueTypeParametersOnly( + MethodSignature previousSignature, + MethodSignature currentSignature) + { + if (previousSignature.Parameters.Count != currentSignature.Parameters.Count) + { + return false; + } + + if (!ReturnTypesMatch(previousSignature, currentSignature)) + { + return false; + } + + bool foundNullabilityChange = false; + for (int i = 0; i < currentSignature.Parameters.Count; i++) + { + var previousParam = previousSignature.Parameters[i]; + var currentParam = currentSignature.Parameters[i]; + + if (previousParam.Name.ToVariableName() != currentParam.Name.ToVariableName()) + { + return false; + } + + // An unchanged parameter (identical type, including nullability). + if (previousParam.Type.AreNamesEqual(currentParam.Type, checkNullability: true)) + { + continue; + } + + // A value-type parameter whose nullability was removed (T? -> T). + if (IsNullabilityRelaxedValueType(previousParam.Type, currentParam.Type)) + { + // A ref/out parameter cannot be forwarded through .Value + if (currentParam.IsRef || currentParam.IsOut || previousParam.IsRef || previousParam.IsOut) + { + return false; + } + + foundNullabilityChange = true; + continue; + } + + // Any other difference disqualifies the method from this scenario. + return false; + } + + return foundNullabilityChange; + } + + private static bool IsNullabilityRelaxedValueType(CSharpType previousType, CSharpType currentType) + => previousType is { IsValueType: true, IsNullable: true } + && currentType is { IsValueType: true, IsNullable: false } + && previousType.AreNamesEqual(currentType); + + private static MethodProvider BuildChangedParameterNullabilityOverload( + TypeProvider enclosingType, + MethodProvider previousMethod, + MethodProvider currentMethod) + { + var previousSignature = previousMethod.Signature; + var currentSignature = currentMethod.Signature; + + // Making the changed (nullability-relaxed) parameter required avoids an ambiguous call site + // with the current (non-nullable) method, but that is only necessary when the current + // parameter is itself optional. When the current parameter is required, the previous optional + // default is preserved: the current required overload wins for concrete values while this + // optional overload still binds for omitted/null callers, so omit-callers do not break. + // To keep a valid required-before-optional ordering, strip defaults from every parameter up to + // and including the last one that must become required. + int lastRequiredIndex = -1; + for (int i = 0; i < previousSignature.Parameters.Count; i++) + { + if (IsNullabilityRelaxedValueType(previousSignature.Parameters[i].Type, currentSignature.Parameters[i].Type) + && currentSignature.Parameters[i].DefaultValue is not null) + { + lastRequiredIndex = i; + } + } + + for (int i = 0; i <= lastRequiredIndex; i++) + { + previousSignature.Parameters[i].DefaultValue = null; + } + + // Build the delegating call, unwrapping each previously-nullable value-type argument with .Value + // (guarded first) to match the current non-nullable parameter type. + bool hasByRefParameter = currentSignature.Parameters.Any(p => p.IsRef || p.IsOut); + var arguments = new List(currentSignature.Parameters.Count); + var nullGuards = new List(); + for (int i = 0; i < currentSignature.Parameters.Count; i++) + { + var currentParam = currentSignature.Parameters[i]; + var value = ForwardParameter(previousSignature.Parameters[i], currentParam.Type, nullGuards); + AddForwardedArgument(arguments, currentParam, value, hasByRefParameter); + } + + var body = BuildDelegatingBody(enclosingType, currentSignature, arguments, nullGuards); + var signature = BuildHiddenOverloadSignature(previousSignature, previousSignature.Parameters); return new MethodProvider( signature, @@ -533,7 +787,139 @@ private static MethodProvider BuildNewOptionalParameterOverload( previousMethod.XmlDocs); } - private static bool IsVoidReturnType(CSharpType? returnType) => - returnType is null || (returnType.IsFrameworkType && returnType.FrameworkType == typeof(void)); + // Given two signatures already known to have equal parameter count and types (including nullability), + // e.g. verified via SignatureComparerIncludingNullability, returns true when their only difference is + // that exactly one nullable parameter changed from optional to required and dropping it in a + // reduced-arity overload stays unambiguous (its type differs from every other parameter's type). + internal static bool IsSingleNullableParameterOptionalToRequired( + MethodSignature previousSignature, + MethodSignature currentSignature) + { + if (!ReturnTypesMatch(previousSignature, currentSignature)) + { + return false; + } + + int becameRequiredIndex = -1; + for (int i = 0; i < currentSignature.Parameters.Count; i++) + { + var previousParam = previousSignature.Parameters[i]; + var currentParam = currentSignature.Parameters[i]; + + if (previousParam.Name.ToVariableName() != currentParam.Name.ToVariableName()) + { + return false; + } + + if (previousParam.DefaultValue is not null && currentParam.DefaultValue is null) + { + // Only a nullable parameter is in scope, and only a single one so the reduced-arity + // overload stays unambiguous. + if (!currentParam.Type.IsNullable || becameRequiredIndex != -1) + { + return false; + } + becameRequiredIndex = i; + } + } + + if (becameRequiredIndex == -1) + { + return false; + } + + // The reduced-arity overload drops the became-required parameter; a single positional argument + // could bind to both overloads if any remaining parameter shares its (underlying) type. + var droppedType = currentSignature.Parameters[becameRequiredIndex].Type; + for (int i = 0; i < currentSignature.Parameters.Count; i++) + { + if (i != becameRequiredIndex && droppedType.AreNamesEqual(currentSignature.Parameters[i].Type)) + { + return false; + } + } + + return true; + } + + private static MethodProvider BuildOptionalityRestorationOverload( + TypeProvider enclosingType, + MethodProvider previousMethod, + MethodProvider currentMethod) + { + var previousSignature = previousMethod.Signature; + var currentSignature = currentMethod.Signature; + + // Find the single nullable parameter that changed from optional to required. + int droppedIndex = -1; + for (int i = 0; i < currentSignature.Parameters.Count; i++) + { + if (previousSignature.Parameters[i].DefaultValue is not null && currentSignature.Parameters[i].DefaultValue is null) + { + droppedIndex = i; + break; + } + } + + var droppedParameter = previousSignature.Parameters[droppedIndex]; + + // The overload reproduces the previous signature without the dropped parameter, preserving the + // previous optionality of the remaining parameters. + var shimParameters = previousSignature.Parameters.Where((_, i) => i != droppedIndex).ToList(); + + // Delegate to the current (required) method, supplying the dropped parameter's previous default. + bool hasByRefParameter = currentSignature.Parameters.Any(p => p.IsRef || p.IsOut); + var arguments = new List(currentSignature.Parameters.Count); + for (int i = 0; i < currentSignature.Parameters.Count; i++) + { + var currentParam = currentSignature.Parameters[i]; + ValueExpression value = i == droppedIndex + ? droppedParameter.DefaultValue ?? Default + : previousSignature.Parameters[i]; + AddForwardedArgument(arguments, currentParam, value, hasByRefParameter); + } + + var body = BuildDelegatingBody(enclosingType, currentSignature, arguments); + var signature = BuildHiddenOverloadSignature(previousSignature, shimParameters); + var xmlDocs = BuildXmlDocsWithoutParameter(previousMethod.XmlDocs, droppedParameter.Name.ToVariableName()); + + return new MethodProvider( + signature, + body, + enclosingType, + xmlDocs); + } + + // Rebuilds an XML doc provider without any reference to the dropped parameter: its <param> + // entry is removed and every <exception> that referenced it is rebuilt without that + // <paramref> (exceptions that referenced only the dropped parameter are removed entirely). + // This avoids stale-doc compile errors (CS1572/CS1734) on the reduced-arity overload. + private static XmlDocProvider BuildXmlDocsWithoutParameter(XmlDocProvider docs, string droppedVariableName) + { + var filteredParameters = docs.Parameters + .Where(p => p.Parameter.Name.ToVariableName() != droppedVariableName) + .ToList(); + + var filteredExceptions = new List(docs.Exceptions.Count); + foreach (var exceptionDoc in docs.Exceptions) + { + var remaining = exceptionDoc.Parameters + .Where(p => p.Name.ToVariableName() != droppedVariableName) + .ToList(); + + // Drop an exception that referenced only the removed parameter; keep an unrelated one as-is; + // otherwise rebuild it without the removed paramref. + if (exceptionDoc.Parameters.Count > 0 && remaining.Count == 0) + { + continue; + } + + filteredExceptions.Add(remaining.Count == exceptionDoc.Parameters.Count + ? exceptionDoc + : new XmlDocExceptionStatement(exceptionDoc.ExceptionType, remaining)); + } + + return new XmlDocProvider(docs.Summary, filteredParameters, filteredExceptions, docs.Returns, docs.Inherit); + } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNullableParameterThatBecameRequired.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNullableParameterThatBecameRequired.cs new file mode 100644 index 00000000000..9e9e4914eb0 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNullableParameterThatBecameRequired.cs @@ -0,0 +1,26 @@ +// + +#nullable disable + +using System.ComponentModel; +using Sample; +using Sample.Models; + +namespace Test +{ + public partial class OptionalityChangeType + { + public string GetData(string data, global::Sample.Models.FileFormatType? value, bool? flag = default) + { + global::Sample.Argument.AssertNotNullOrEmpty(data, nameof(data)); + + return data; + } + + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Never)] + public string GetData(string data, bool? flag = default) + { + return this.GetData(data: data, value: default, flag: flag); + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNullableParameterThatBecameRequired/OptionalityChangeType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNullableParameterThatBecameRequired/OptionalityChangeType.cs new file mode 100644 index 00000000000..943dcbd3805 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForNullableParameterThatBecameRequired/OptionalityChangeType.cs @@ -0,0 +1,14 @@ +namespace Sample.Models +{ + public readonly partial struct FileFormatType + { + } +} + +namespace Test +{ + public class OptionalityChangeType + { + public string GetData(string data, global::Sample.Models.FileFormatType? value = default, bool? flag = default) => null; + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForRelaxedNullableValueTypeParameter.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForRelaxedNullableValueTypeParameter.cs new file mode 100644 index 00000000000..468ed35e192 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForRelaxedNullableValueTypeParameter.cs @@ -0,0 +1,27 @@ +// + +#nullable disable + +using System.ComponentModel; +using Sample; +using Sample.Models; + +namespace Test +{ + public partial class NullabilityChangeType + { + public string GetData(string data, global::Sample.Models.FileFormatType value, bool? flag = default) + { + global::Sample.Argument.AssertNotNullOrEmpty(data, nameof(data)); + + return data; + } + + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Never)] + public string GetData(string data, global::Sample.Models.FileFormatType? value = default, bool? flag = default) + { + global::Sample.Argument.AssertNotNull(value, nameof(value)); + return this.GetData(data: data, value: value.Value, flag: flag); + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForRelaxedNullableValueTypeParameter/NullabilityChangeType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForRelaxedNullableValueTypeParameter/NullabilityChangeType.cs new file mode 100644 index 00000000000..cafb879e2a6 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityAddsOverloadForRelaxedNullableValueTypeParameter/NullabilityChangeType.cs @@ -0,0 +1,14 @@ +namespace Sample.Models +{ + public readonly partial struct FileFormatType + { + } +} + +namespace Test +{ + public class NullabilityChangeType + { + public string GetData(string data, global::Sample.Models.FileFormatType? value = default, bool? flag = default) => null; + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityUnwrapsRelaxedNullableSharedParameterInOptionalOverload.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityUnwrapsRelaxedNullableSharedParameterInOptionalOverload.cs new file mode 100644 index 00000000000..2a7bb51bb10 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityUnwrapsRelaxedNullableSharedParameterInOptionalOverload.cs @@ -0,0 +1,27 @@ +// + +#nullable disable + +using System.ComponentModel; +using Sample; +using Sample.Models; + +namespace Test +{ + public partial class NullabilityAndOptionalChangeType + { + public string GetData(string data, global::Sample.Models.FileFormatType value, bool? newFlag = default) + { + global::Sample.Argument.AssertNotNullOrEmpty(data, nameof(data)); + + return data; + } + + [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Never)] + public string GetData(string data, global::Sample.Models.FileFormatType? value = default) + { + global::Sample.Argument.AssertNotNull(value, nameof(value)); + return this.GetData(data: data, value: value.Value, newFlag: default); + } + } +} diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityUnwrapsRelaxedNullableSharedParameterInOptionalOverload/NullabilityAndOptionalChangeType.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityUnwrapsRelaxedNullableSharedParameterInOptionalOverload/NullabilityAndOptionalChangeType.cs new file mode 100644 index 00000000000..8dd1c94dee9 --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/TestData/TypeProviderTests/BuildMethodsForBackCompatibilityUnwrapsRelaxedNullableSharedParameterInOptionalOverload/NullabilityAndOptionalChangeType.cs @@ -0,0 +1,14 @@ +namespace Sample.Models +{ + public readonly partial struct FileFormatType + { + } +} + +namespace Test +{ + public class NullabilityAndOptionalChangeType + { + public string GetData(string data, global::Sample.Models.FileFormatType? value = default) => null; + } +} 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 34e119f008a..f8294e1fac6 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 @@ -478,6 +478,98 @@ public async Task BuildMethodsForBackCompatibilityPreservesOutKeyword() Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); } + // An extensible-enum parameter whose nullability was removed (FileFormatType? -> FileFormatType) + // gets a hidden overload with the previous nullable signature that delegates with .Value. Since the + // current parameter is required, the previous optional default is preserved so omit-callers still + // compile. + [Test] + public async Task BuildMethodsForBackCompatibilityAddsOverloadForRelaxedNullableValueTypeParameter() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // The last contract published GetData(string data, FileFormatType? value = default, bool flag = + // default); the current generation makes the extensible-enum 'value' non-nullable and required. + // The required string 'data' produces an argument assertion in the method body. + var typeFactory = CodeModelGenerator.Instance.TypeFactory; + var enumInput = InputFactory.StringEnum("fileFormatType", [("json", "json"), ("xml", "xml")], isExtensible: true, usage: InputModelTypeUsage.Input); + var data = typeFactory.CreateParameter(InputFactory.QueryParameter("data", InputPrimitiveType.String, isRequired: true))!; + var value = typeFactory.CreateParameter(InputFactory.QueryParameter("value", enumInput, isRequired: true))!; + var flag = typeFactory.CreateParameter(InputFactory.QueryParameter("flag", InputPrimitiveType.Boolean))!; + var getData = new MethodProvider( + new MethodSignature("GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [data, value, flag]), + Snippet.Return(data), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "NullabilityChangeType", ns: "Test", methods: [getData]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // Cross-pass interaction: when a shared parameter's nullability was removed AND the method also + // gained a new optional parameter, the new-optional-parameter overload pass forwards the shared + // parameter with .Value, and the nullability pass does not add a duplicate overload. + [Test] + public async Task BuildMethodsForBackCompatibilityUnwrapsRelaxedNullableSharedParameterInOptionalOverload() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // Last contract: GetData(string data, FileFormatType? value = default). Current makes the + // extensible-enum 'value' non-nullable and adds a new optional parameter 'newFlag'. The required + // string 'data' produces an argument assertion in the method body. + var typeFactory = CodeModelGenerator.Instance.TypeFactory; + var enumInput = InputFactory.StringEnum("fileFormatType", [("json", "json"), ("xml", "xml")], isExtensible: true, usage: InputModelTypeUsage.Input); + var data = typeFactory.CreateParameter(InputFactory.QueryParameter("data", InputPrimitiveType.String, isRequired: true))!; + var value = typeFactory.CreateParameter(InputFactory.QueryParameter("value", enumInput, isRequired: true))!; + var newFlag = typeFactory.CreateParameter(InputFactory.QueryParameter("newFlag", InputPrimitiveType.Boolean))!; + var getData = new MethodProvider( + new MethodSignature("GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [data, value, newFlag]), + Snippet.Return(data), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "NullabilityAndOptionalChangeType", ns: "Test", methods: [getData]); + + typeProvider.ProcessTypeForBackCompatibility(); + + // Exactly one back-compat overload is added (no duplicate across the two passes). + Assert.AreEqual(2, typeProvider.Methods.Count); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + + // A nullable parameter that changed from optional to required (same type, FileFormatType? value = + // default -> FileFormatType? value) gets a hidden reduced-arity overload that drops the parameter + // and delegates with its previous default, so omit-callers still compile without an ambiguous call + // site. + [Test] + public async Task BuildMethodsForBackCompatibilityAddsOverloadForNullableParameterThatBecameRequired() + { + await MockHelpers.LoadMockGeneratorAsync(lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync()); + + // Last contract: GetData(string data, FileFormatType? value = default, bool flag = default). + // Current keeps the extensible-enum 'value' nullable but makes it required. The required string + // 'data' produces an argument assertion in the method body. + var typeFactory = CodeModelGenerator.Instance.TypeFactory; + var enumInput = InputFactory.StringEnum("fileFormatType", [("json", "json"), ("xml", "xml")], isExtensible: true, usage: InputModelTypeUsage.Input); + var data = typeFactory.CreateParameter(InputFactory.QueryParameter("data", InputPrimitiveType.String, isRequired: true))!; + var value = typeFactory.CreateParameter(InputFactory.QueryParameter("value", new InputNullableType(enumInput), isRequired: true))!; + var flag = typeFactory.CreateParameter(InputFactory.QueryParameter("flag", InputPrimitiveType.Boolean))!; + var getData = new MethodProvider( + new MethodSignature("GetData", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", [data, value, flag]), + Snippet.Return(data), + new TestTypeProvider()); + + var typeProvider = new TestTypeProvider(name: "OptionalityChangeType", ns: "Test", methods: [getData]); + + typeProvider.ProcessTypeForBackCompatibility(); + + var actual = new TypeProviderWriter(typeProvider).Write().Content; + Assert.AreEqual(Helpers.GetExpectedFromFile(), actual); + } + // TypeProvider: when a previous signature's removal is accepted in the ApiCompat baseline, the // optional-parameter overload pass must NOT resurrect it even though the replacement method added // optional parameters. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/BackCompatHelperTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/BackCompatHelperTests.cs new file mode 100644 index 00000000000..f06a12083be --- /dev/null +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/BackCompatHelperTests.cs @@ -0,0 +1,113 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using Microsoft.TypeSpec.Generator.Input; +using Microsoft.TypeSpec.Generator.Primitives; +using Microsoft.TypeSpec.Generator.Providers; +using Microsoft.TypeSpec.Generator.Tests.Common; +using Microsoft.TypeSpec.Generator.Utilities; +using NUnit.Framework; + +namespace Microsoft.TypeSpec.Generator.Tests.Utilities +{ + public class BackCompatHelperTests + { + [SetUp] + public void Setup() + { + MockHelpers.LoadMockGenerator(); + } + + // Detection: a nullable value-type parameter that became non-nullable qualifies. + [Test] + public void HasRelaxedNullableValueTypeParametersOnlyMatchesNullableToNonNullableValueType() + { + var typeFactory = CodeModelGenerator.Instance.TypeFactory; + ParameterProvider Param(string name, InputType type) => typeFactory.CreateParameter(InputFactory.QueryParameter(name, type, isRequired: true))!; + MethodSignature Sig(params ParameterProvider[] parameters) => new MethodSignature( + "Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", parameters); + + var previous = Sig(Param("value", new InputNullableType(InputPrimitiveType.Int32)), Param("flag", InputPrimitiveType.Boolean)); + var current = Sig(Param("value", InputPrimitiveType.Int32), Param("flag", InputPrimitiveType.Boolean)); + + Assert.IsTrue(BackCompatHelper.HasRelaxedNullableValueTypeParametersOnly(previous, current)); + } + + // Detection guards: only the nullable-value-type -> non-nullable direction on the same underlying + // type qualifies. Reference-type nullability, the widening direction, a different underlying type, + // and an unchanged signature must all be rejected. + [Test] + public void HasRelaxedNullableValueTypeParametersOnlyRejectsOtherChanges() + { + var typeFactory = CodeModelGenerator.Instance.TypeFactory; + MethodSignature Sig(InputType parameterType) => new MethodSignature( + "Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", + [typeFactory.CreateParameter(InputFactory.QueryParameter("value", parameterType, isRequired: true))!]); + + // Reference-type nullability cannot form distinct overloads (string? and string collide). + Assert.IsFalse(BackCompatHelper.HasRelaxedNullableValueTypeParametersOnly( + Sig(new InputNullableType(InputPrimitiveType.String)), Sig(InputPrimitiveType.String))); + + // The widening direction (non-nullable -> nullable) is not handled. + Assert.IsFalse(BackCompatHelper.HasRelaxedNullableValueTypeParametersOnly( + Sig(InputPrimitiveType.Int32), Sig(new InputNullableType(InputPrimitiveType.Int32)))); + + // A different underlying value type is not a nullability-only change. + Assert.IsFalse(BackCompatHelper.HasRelaxedNullableValueTypeParametersOnly( + Sig(new InputNullableType(InputPrimitiveType.Int32)), Sig(InputPrimitiveType.Int64))); + + // An unchanged signature has no nullability change. + Assert.IsFalse(BackCompatHelper.HasRelaxedNullableValueTypeParametersOnly( + Sig(InputPrimitiveType.Int32), Sig(InputPrimitiveType.Int32))); + + // A ref/out parameter cannot be forwarded through .Value, so a nullability change on one is + // rejected rather than producing an un-compilable shim (ref int? -> ref int). + MethodSignature RefSig(CSharpType parameterType) => new MethodSignature( + "Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", + [new ParameterProvider("value", $"", parameterType, isRef: true)]); + Assert.IsFalse(BackCompatHelper.HasRelaxedNullableValueTypeParametersOnly( + RefSig(new CSharpType(typeof(int), isNullable: true)), RefSig(new CSharpType(typeof(int))))); + } + + // Detection: a nullable parameter that changed from optional to required (same type) qualifies. + [Test] + public void IsSingleNullableParameterOptionalToRequiredMatchesOptionalToRequiredNullable() + { + var typeFactory = CodeModelGenerator.Instance.TypeFactory; + ParameterProvider Opt(string name, InputType type) => typeFactory.CreateParameter(InputFactory.QueryParameter(name, type, isRequired: false))!; + ParameterProvider Req(string name, InputType type) => typeFactory.CreateParameter(InputFactory.QueryParameter(name, type, isRequired: true))!; + MethodSignature Sig(params ParameterProvider[] parameters) => new MethodSignature( + "Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", parameters); + + var previous = Sig(Opt("value", new InputNullableType(InputPrimitiveType.Int32)), Opt("flag", InputPrimitiveType.Boolean)); + var current = Sig(Req("value", new InputNullableType(InputPrimitiveType.Int32)), Opt("flag", InputPrimitiveType.Boolean)); + + Assert.IsTrue(BackCompatHelper.IsSingleNullableParameterOptionalToRequired(previous, current)); + } + + // Detection guards (parameter types are assumed already equal): a non-nullable parameter, an + // unchanged optionality, and a drop that would be ambiguous must all be rejected. + [Test] + public void IsSingleNullableParameterOptionalToRequiredRejectsOtherChanges() + { + var typeFactory = CodeModelGenerator.Instance.TypeFactory; + ParameterProvider Opt(string name, InputType type) => typeFactory.CreateParameter(InputFactory.QueryParameter(name, type, isRequired: false))!; + ParameterProvider Req(string name, InputType type) => typeFactory.CreateParameter(InputFactory.QueryParameter(name, type, isRequired: true))!; + MethodSignature Sig(params ParameterProvider[] parameters) => new MethodSignature( + "Foo", $"", MethodSignatureModifiers.Public, new CSharpType(typeof(string)), $"", parameters); + + // A non-nullable parameter becoming required is not in scope (only nullable parameters qualify). + Assert.IsFalse(BackCompatHelper.IsSingleNullableParameterOptionalToRequired( + Sig(Opt("value", InputPrimitiveType.Int32)), Sig(Req("value", InputPrimitiveType.Int32)))); + + // Unchanged optionality has nothing to restore. + Assert.IsFalse(BackCompatHelper.IsSingleNullableParameterOptionalToRequired( + Sig(Opt("value", new InputNullableType(InputPrimitiveType.Int32))), Sig(Opt("value", new InputNullableType(InputPrimitiveType.Int32))))); + + // Dropping the parameter would be ambiguous when another parameter shares its underlying type. + Assert.IsFalse(BackCompatHelper.IsSingleNullableParameterOptionalToRequired( + Sig(Opt("a", new InputNullableType(InputPrimitiveType.Int32)), Opt("b", new InputNullableType(InputPrimitiveType.Int32))), + Sig(Req("a", new InputNullableType(InputPrimitiveType.Int32)), Opt("b", new InputNullableType(InputPrimitiveType.Int32))))); + } + } +} diff --git a/packages/http-client-csharp/generator/docs/backward-compatibility.md b/packages/http-client-csharp/generator/docs/backward-compatibility.md index 6ca83258649..fe1b761caf6 100644 --- a/packages/http-client-csharp/generator/docs/backward-compatibility.md +++ b/packages/http-client-csharp/generator/docs/backward-compatibility.md @@ -27,6 +27,8 @@ - [Content-Type Before Body Preserved from Last Contract](#scenario-content-type-before-body-preserved-from-last-contract) - [Client Methods](#client-methods) - [New Optional Non-Body Parameter Added to a Service Method](#scenario-new-optional-non-body-parameter-added-to-a-service-method) + - [Value-Type Parameter Nullability Removed](#scenario-value-type-parameter-nullability-removed) + - [Nullable Optional Parameter Became Required](#scenario-nullable-optional-parameter-became-required) ## Overview @@ -964,3 +966,75 @@ public virtual Task GetDataAsync(int p1, BinaryContent body, Reque ``` The back-compat overloads are hidden from IntelliSense via `[EditorBrowsable(EditorBrowsableState.Never)]`, have all default values stripped to avoid ambiguous call sites with the current methods, and delegate to the current method passing `default` for each new parameter. + +#### Scenario: Value-Type Parameter Nullability Removed + +**Description:** When a value-type parameter that was nullable in the last contract (`T?`) becomes non-nullable (`T`) in the current generation, the generator emits a hidden back-compat overload that reproduces the previous nullable signature and delegates to the current method, unwrapping the argument with `.Value`. The forwarded argument is guarded with `Argument.AssertNotNull` so a `null` argument produces a clear `ArgumentNullException` rather than an `InvalidOperationException` from `.Value`. This also covers extensible enums (which are read back from the last contract as structs), matched by fully-qualified type name. + +**Example:** + +Previous contract: + +```csharp +public string GetData(string data, FileFormatType? value = default, bool? flag = default); +``` + +Current generation makes `value` non-nullable and required: + +```csharp +public string GetData(string data, FileFormatType value, bool? flag = default); +``` + +**Generated Compatibility Overload:** + +```csharp +[EditorBrowsable(EditorBrowsableState.Never)] +public string GetData(string data, FileFormatType? value = default, bool? flag = default) +{ + Argument.AssertNotNull(value, nameof(value)); + return this.GetData(data: data, value: value.Value, flag: flag); +} +``` + +**Key Points:** + +- Only applies to value types (including extensible enums); reference-type nullability (`string?` vs `string`) cannot form distinct overloads and is not handled. +- Only the `T?` → `T` direction (removing nullability) is handled, not the widening direction. +- When the current parameter is required, the previous optional default is preserved so callers that omitted it still compile; the `AssertNotNull` guard turns a `null` argument into a clear `ArgumentNullException`. +- A `ref`/`out` parameter is not eligible (its value cannot be forwarded through `.Value`), so no overload is generated in that case. +- The overload is hidden via `[EditorBrowsable(EditorBrowsableState.Never)]`. + +#### Scenario: Nullable Optional Parameter Became Required + +**Description:** When a nullable parameter that was optional in the last contract (had a default value) becomes required in the current generation (same type, but no longer has a default), callers that omitted it no longer compile. The generator emits a hidden reduced-arity overload that drops the now-required parameter and delegates to the current method, supplying the parameter's previous default value. + +**Example:** + +Previous contract: + +```csharp +public string GetData(string data, FileFormatType? value = default, bool? flag = default); +``` + +Current generation keeps `value` nullable but makes it required: + +```csharp +public string GetData(string data, FileFormatType? value, bool? flag = default); +``` + +**Generated Compatibility Overload:** + +```csharp +[EditorBrowsable(EditorBrowsableState.Never)] +public string GetData(string data, bool? flag = default) +{ + return this.GetData(data: data, value: default, flag: flag); +} +``` + +**Key Points:** + +- Applies only to a single nullable parameter that changed from optional to required; the parameter's type is otherwise unchanged. +- The dropped parameter's type must differ from every other parameter's type so the reduced-arity overload's call sites stay unambiguous. +- The optionality of the remaining parameters is preserved, and the overload is hidden via `[EditorBrowsable(EditorBrowsableState.Never)]`. +- Unlike the nullability-removal scenario, the delegation passes the previous default — there is no `.Value` unwrap and no runtime throw.