Expand back-compat property type support to all model properties

Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/33d000a2-d811-4f6c-a72b-359b57cee1a2

Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>

Author: Copilot

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

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Linq;
 using Microsoft.TypeSpec.Generator.Expressions;
@@ -535,24 +536,13 @@ protected internal override PropertyProvider[] BuildProperties()
                     continue;
                 }
 
-                // Targeted backcompat fix for the case where properties were previously generated as read-only collections
-                if (outputProperty.Type.IsReadWriteList || outputProperty.Type.IsReadWriteDictionary)
+                // Backcompat fix for property types: if a property existed in the last contract
+                // with a compatible but non-identical type, retain the previous type to avoid
+                // breaking consumers of the library.
+                if (TryGetLastContractPropertyTypeOverride(outputProperty, out var lastContractPropertyType))
                 {
-                    // We compare Arguments by name (not just ElementType) to cover both list element types
-                    // and dictionary key/value types. This ensures we only override the collection wrapper
-                    // (e.g. IReadOnlyList<T> → IList<T>) and not when the element type itself has changed.
-                    // We use AreNamesEqual rather than Equals because the argument types may come from
-                    // different sources (TypeProvider vs compiled assembly) but represent the same logical type.
-                    if (LastContractPropertiesMap.TryGetValue(outputProperty.Name,
-                            out CSharpType? lastContractPropertyType) &&
-                        !outputProperty.Type.Equals(lastContractPropertyType) &&
-                        outputProperty.Type.Arguments.Count == lastContractPropertyType.Arguments.Count &&
-                        outputProperty.Type.Arguments.Zip(lastContractPropertyType.Arguments).All(
-                            pair => pair.First.AreNamesEqual(pair.Second)))
-                    {
-                        outputProperty.Type = lastContractPropertyType.ApplyInputSpecProperty(property);
-                        CodeModelGenerator.Instance.Emitter.Info($"Changed property {Name}.{outputProperty.Name} type to {lastContractPropertyType} to match last contract.");
-                    }
+                    outputProperty.Type = lastContractPropertyType.ApplyInputSpecProperty(property);
+                    CodeModelGenerator.Instance.Emitter.Info($"Changed property {Name}.{outputProperty.Name} type to {lastContractPropertyType} to match last contract.");
                 }
 
                 if (!isDiscriminator)
@@ -1276,6 +1266,89 @@ _ when type.Equals(_additionalPropsUnknownType, ignoreNullable: true) => type,
             };
         }
 
+        /// <summary>
+        /// Determines whether the type of a generated property should be replaced with the type
+        /// of the matching property in the last contract to preserve backward compatibility.
+        /// </summary>
+        /// <remarks>
+        /// The override is applied when the generated and last-contract property types differ but
+        /// are logically compatible. Two categories are supported:
+        /// <list type="bullet">
+        /// <item>
+        /// <description>
+        /// Read-write collection properties (<see cref="CSharpType.IsReadWriteList"/> or
+        /// <see cref="CSharpType.IsReadWriteDictionary"/>) whose element/key/value type names
+        /// match the last contract. This handles cases such as <c>IReadOnlyList&lt;T&gt;</c>
+        /// being regenerated as <c>IList&lt;T&gt;</c>.
+        /// </description>
+        /// </item>
+        /// <item>
+        /// <description>
+        /// All other public properties whose top-level type name (including any generic
+        /// argument names) matches the last contract. This handles cases such as a property
+        /// previously generated as a nullable scalar being regenerated as non-nullable, or a
+        /// property whose type is sourced from a different assembly but represents the same
+        /// logical type.
+        /// </description>
+        /// </item>
+        /// </list>
+        /// </remarks>
+        /// <param name="outputProperty">The property generated from the current input spec.</param>
+        /// <param name="lastContractPropertyType">
+        /// When the method returns <c>true</c>, the type from the last contract that should be
+        /// used to override the generated property type.
+        /// </param>
+        /// <returns>
+        /// <c>true</c> if the generated property type should be replaced with the last contract's
+        /// type; otherwise <c>false</c>.
+        /// </returns>
+        private bool TryGetLastContractPropertyTypeOverride(
+            PropertyProvider outputProperty,
+            [NotNullWhen(true)] out CSharpType? lastContractPropertyType)
+        {
+            lastContractPropertyType = null;
+            if (!LastContractPropertiesMap.TryGetValue(outputProperty.Name, out var candidate))
+            {
+                return false;
+            }
+
+            if (outputProperty.Type.Equals(candidate))
+            {
+                return false;
+            }
+
+            // Read-write collections: allow the wrapper (e.g. IList<T> vs IReadOnlyList<T>) to
+            // change as long as the element/key/value type names still match. We compare
+            // Arguments by name (not just ElementType) so this covers both list element types
+            // and dictionary key/value types. AreNamesEqual is used rather than Equals because
+            // the argument types may come from different sources (TypeProvider vs compiled
+            // assembly) but represent the same logical type.
+            if (outputProperty.Type.IsReadWriteList || outputProperty.Type.IsReadWriteDictionary)
+            {
+                if (outputProperty.Type.Arguments.Count == candidate.Arguments.Count &&
+                    outputProperty.Type.Arguments.Zip(candidate.Arguments).All(
+                        pair => pair.First.AreNamesEqual(pair.Second)))
+                {
+                    lastContractPropertyType = candidate;
+                    return true;
+                }
+
+                return false;
+            }
+
+            // Other properties: require the entire type name (top-level plus any generic
+            // argument names) to match. This ensures we only override when the types are
+            // logically the same (e.g. differ only in nullability) and never when the
+            // underlying type has genuinely changed (e.g. string to int).
+            if (outputProperty.Type.AreNamesEqual(candidate))
+            {
+                lastContractPropertyType = candidate;
+                return true;
+            }
+
+            return false;
+        }
+
         /// <summary>
         /// Determines whether to use object type for AdditionalProperties based on backward compatibility requirements.
         /// Checks if the last contract (previous version) had an AdditionalProperties property of type IDictionary&lt;string, object&gt;.

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs

@@ -1134,6 +1134,92 @@ await MockHelpers.LoadMockGeneratorAsync(
             Assert.IsTrue(moreItemsProperty!.Type.Equals(typeof(IDictionary<string, string>)));
         }
 
+        [Test]
+        public async Task BackCompat_NullableScalarPropertyTypeIsRetained()
+        {
+            // Regression: when a scalar property was previously generated as nullable
+            // but the current spec marks it as non-nullable, the previous nullable type
+            // should be preserved to avoid a source-breaking change.
+            var inputModel = InputFactory.Model(
+                "MockInputModel",
+                properties:
+                [
+                    InputFactory.Property("count", InputPrimitiveType.Int32, isRequired: true),
+                ]);
+
+            await MockHelpers.LoadMockGeneratorAsync(
+                inputModelTypes: [inputModel],
+                lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync());
+
+            var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider;
+            Assert.IsNotNull(modelProvider);
+
+            var countProperty = modelProvider!.Properties.FirstOrDefault(p => p.Name == "Count");
+            Assert.IsNotNull(countProperty);
+            // The current spec says non-nullable int, but the last contract had int? – the
+            // generator should preserve the nullable type for backwards compatibility.
+            Assert.IsTrue(countProperty!.Type.Equals(new CSharpType(typeof(int), isNullable: true)));
+        }
+
+        [Test]
+        public async Task BackCompat_ScalarPropertyTypeNotOverriddenWhenTypeNameDiffers()
+        {
+            // When the top-level property type name differs between the last contract and the
+            // current spec (e.g. string vs int), the generator must not silently replace the
+            // spec-defined type with the last contract's type.
+            var inputModel = InputFactory.Model(
+                "MockInputModel",
+                properties:
+                [
+                    InputFactory.Property("count", InputPrimitiveType.Int32, isRequired: true),
+                ]);
+
+            await MockHelpers.LoadMockGeneratorAsync(
+                inputModelTypes: [inputModel],
+                lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync());
+
+            var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider;
+            Assert.IsNotNull(modelProvider);
+
+            var countProperty = modelProvider!.Properties.FirstOrDefault(p => p.Name == "Count");
+            Assert.IsNotNull(countProperty);
+            // Last contract has `string Count { get; set; }` but the new spec says int – the
+            // generator must not override the new type since the names differ entirely.
+            Assert.IsTrue(countProperty!.Type.Equals(typeof(int)));
+        }
+
+        [Test]
+        public async Task BackCompat_EnumPropertyTypeIsRetainedWhenNullabilityDiffers()
+        {
+            // A scalar (non-collection) enum property whose nullability changed between the
+            // last contract and the current spec should retain the last contract's nullability.
+            var statusEnum = InputFactory.StringEnum(
+                "StatusEnum",
+                [("Active", "Active"), ("Inactive", "Inactive")],
+                isExtensible: true);
+            var inputModel = InputFactory.Model(
+                "MockInputModel",
+                properties:
+                [
+                    InputFactory.Property("status", statusEnum, isRequired: true),
+                ]);
+
+            await MockHelpers.LoadMockGeneratorAsync(
+                inputModelTypes: [inputModel],
+                inputEnumTypes: [statusEnum],
+                lastContractCompilation: async () => await Helpers.GetCompilationFromDirectoryAsync());
+
+            var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider;
+            Assert.IsNotNull(modelProvider);
+
+            var statusProperty = modelProvider!.Properties.FirstOrDefault(p => p.Name == "Status");
+            Assert.IsNotNull(statusProperty);
+            // The last contract had StatusEnum? but the spec marks it required/non-nullable –
+            // the generator should preserve the nullable type to avoid a breaking change.
+            Assert.IsTrue(statusProperty!.Type.IsNullable);
+            Assert.AreEqual("StatusEnum", statusProperty.Type.Name);
+        }
+
         [Test]
         public async Task BackCompat_NonAbstractTypeIsRespected()
         {

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_EnumPropertyTypeIsRetainedWhenNullabilityDiffers/MockInputModel.cs

@@ -0,0 +1,11 @@
+namespace Sample.Models
+{
+    public partial class MockInputModel
+    {
+        public StatusEnum? Status { get; set; }
+    }
+
+    public partial struct StatusEnum
+    {
+    }
+}

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_NullableScalarPropertyTypeIsRetained/MockInputModel.cs

@@ -0,0 +1,7 @@
+namespace Sample.Models
+{
+    public partial class MockInputModel
+    {
+        public int? Count { get; set; }
+    }
+}

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ScalarPropertyTypeNotOverriddenWhenTypeNameDiffers/MockInputModel.cs

@@ -0,0 +1,7 @@
+namespace Sample.Models
+{
+    public partial class MockInputModel
+    {
+        public string Count { get; set; }
+    }
+}

packages/http-client-csharp/generator/docs/backward-compatibility.md

@@ -110,7 +110,15 @@ public static PublicModel1 PublicModel1(
 
 ### Model Properties
 
-The generator attempts to maintain backward compatibility for model property types, particularly for collection types.
+The generator attempts to maintain backward compatibility for all public model property types by preserving the previous property type when it differs from the type that would be generated from the current spec but is still logically compatible.
+
+The following scenarios are supported for any public model property:
+
+- **Collection wrapper change:** A property previously generated as a read-only collection (e.g. `IReadOnlyList<T>` / `IReadOnlyDictionary<TKey, TValue>`) that is now produced as a read-write collection (e.g. `IList<T>` / `IDictionary<TKey, TValue>`), or vice versa. The collection element/key/value type names must still match – the wrapper is preserved but genuine element-type changes are honoured.
+- **Nullability change:** A scalar, enum, or model property that was previously generated as nullable (e.g. `int?`, `StatusEnum?`) and is now produced as non-nullable (or vice versa). The last contract's nullability is preserved when the top-level type name (and any generic argument names) still matches.
+- **Same-name types from different sources:** Properties whose generated type is logically the same as the last contract's type, but sourced from different assemblies (e.g. a `TypeProvider`-produced type vs. a compiled-assembly type). Equality is evaluated by name rather than identity so these are treated as the same type.
+
+The override is **not** applied when the top-level property type names differ (for example a property that changed from `string` to `int`) – such changes are passed through so that the new spec is honoured.
 
 #### Scenario: Collection Property Type Changed
 
@@ -136,10 +144,35 @@ public IList<string> Items { get; set; }
 public IReadOnlyList<string> Items { get; }
 ```
 
+#### Scenario: Scalar/Model Property Nullability Changed
+
+**Description:** When the nullability of a scalar, enum, or model property differs between the last contract and the current spec, the generator preserves the last contract's nullability to avoid a source-breaking change.
+
+**Example:**
+
+Previous version:
+
+```csharp
+public int? Count { get; set; }
+```
+
+Current TypeSpec would generate:
+
+```csharp
+public int Count { get; set; }
+```
+
+**Result:** The generator detects the nullability mismatch and preserves the previous nullable type:
+
+```csharp
+public int? Count { get; set; }
+```
+
 **Implementation Details:**
 
-- The generator compares property types against the `LastContractView`
-- For read-write lists and dictionaries, if the previous type was different, the previous type is retained
+- The generator compares property types against the `LastContractView`.
+- For read-write lists and dictionaries, if the previous type was different but the element/key/value type names match, the previous type is retained.
+- For all other properties, if the previous type's top-level name (including any generic argument names) matches the new type's top-level name, the previous type is retained – this covers nullability changes and same-name types sourced from different assemblies.
 - A diagnostic message is logged: `"Changed property {ModelName}.{PropertyName} type to {LastContractType} to match last contract."`
 
 ### AdditionalProperties Type Preservation