From 5936fe7bbb531d1a34154c2559a77feb5c42b983 Mon Sep 17 00:00:00 2001 From: jolov Date: Mon, 22 Jun 2026 11:36:20 -0700 Subject: [PATCH 1/2] fix(http-client-csharp): override MRW *Core methods when external base declares them Issue #11042: derived models of an external/referenced MRW-generated base (e.g. OpenAI's ResponseItem) generated PersistableModelCreateCore and the other serialization *Core methods as `protected virtual` instead of `protected override`, causing CS0114. PR #11002 routed external models through SystemObjectModelProvider, whose ShouldSkipDerivedSerializationMethodOverrides unconditionally returns true (added in #9862 for hand-authored bases like ResourceData that lack the *Core methods). That forced derived models to hide rather than override the base's *Core methods. Now MrwSerializationTypeDefinition only skips the override when the wrapped framework type does not declare all four generated *Core methods; when it does, derived models correctly emit `protected override`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../MrwSerializationTypeDefinition.cs | 75 ++++++++++- .../SystemObjectModelSerializationTests.cs | 120 ++++++++++++++++++ 2 files changed, 194 insertions(+), 1 deletion(-) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs index 0d02ecba187..993e087b9cd 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs @@ -9,6 +9,7 @@ using System.IO; using System.Linq; using System.Net; +using System.Reflection; using System.Text; using System.Text.Json; using System.Xml; @@ -94,7 +95,7 @@ public MrwSerializationTypeDefinition(InputModelType inputModel, ModelProvider m _additionalBinaryDataProperty = new(GetAdditionalBinaryDataPropertiesProp); _additionalProperties = new(() => [.. _model.Properties.Where(p => p.IsAdditionalProperties)]); _shouldOverrideMethods = _model.BaseModelProvider != null && !_isStruct; - _shouldSkipDerivedSerializationMethodOverrides = _model.BaseModelProvider?.ShouldSkipDerivedSerializationMethodOverrides == true; + _shouldSkipDerivedSerializationMethodOverrides = ShouldSkipDerivedSerializationMethodOverrides(_model.BaseModelProvider); _utf8JsonWriterSnippet = _utf8JsonWriterParameter.As(); _mrwOptionsParameterSnippet = _serializationOptionsParameter.As(); _jsonElementParameterSnippet = _jsonElementDeserializationParam.As(); @@ -157,6 +158,78 @@ private static bool IsModelType(CSharpType type) => ScmCodeModelGenerator.Instance.TypeFactory.CSharpTypeMap.TryGetValue(type, out var baseProvider) && baseProvider is ModelProvider; + /// + /// The generated MRW serialization *Core methods. A base model participates in the + /// override chain only if it declares all of these as overridable members. + /// + private static readonly string[] SerializationCoreMethodNames = + [ + JsonModelWriteCoreMethodName, + JsonModelCreateCoreMethodName, + PersistableModelWriteCoreMethodName, + PersistableModelCreateCoreMethodName, + ]; + + /// + /// Determines whether a derived model should skip overriding the generated serialization + /// *Core methods of its base. + /// + /// + /// External/system base models are represented by a . + /// Such a base only participates in the generated MRW *Core override chain when the + /// wrapped framework/referenced type itself follows the generated serialization pattern (i.e. + /// declares the protected virtual *Core methods). When it does, derived models must + /// override those methods rather than hide them (otherwise the compiler reports CS0114). + /// When it does not (for example a hand-authored base such as ResourceData), derived + /// models re-introduce the methods as virtual. + /// + private static bool ShouldSkipDerivedSerializationMethodOverrides(ModelProvider? baseModelProvider) + { + if (baseModelProvider is null) + { + return false; + } + + if (baseModelProvider is SystemObjectModelProvider systemBase) + { + return !SystemTypeDeclaresSerializationCoreMethods(systemBase.SystemType); + } + + return baseModelProvider.ShouldSkipDerivedSerializationMethodOverrides; + } + + private static bool SystemTypeDeclaresSerializationCoreMethods(CSharpType systemType) + { + if (!systemType.IsFrameworkType) + { + return false; + } + + foreach (var methodName in SerializationCoreMethodNames) + { + if (!DeclaresInstanceMethod(systemType.FrameworkType, methodName)) + { + return false; + } + } + + return true; + } + + private static bool DeclaresInstanceMethod(Type type, string methodName) + { + const BindingFlags flags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly; + for (Type? current = type; current is not null; current = current.BaseType) + { + if (current.GetMethods(flags).Any(m => m.Name == methodName)) + { + return true; + } + } + + return false; + } + protected override ConstructorProvider[] BuildConstructors() { List constructors = new(); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs index 94648eebfeb..b7018ebfa6d 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs @@ -2,7 +2,9 @@ // Licensed under the MIT License. using System; +using System.ClientModel.Primitives; using System.Linq; +using System.Text.Json; using Microsoft.TypeSpec.Generator.ClientModel.Providers; using Microsoft.TypeSpec.Generator.Input; using Microsoft.TypeSpec.Generator.Primitives; @@ -201,5 +203,123 @@ public void JsonModelCreateCore_IsOverride_WhenBaseIsRegularModel() Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override), "JsonModelCreateCore should be 'override' with regular base model"); } + + /// + /// Creates a derived model whose SystemObjectModelProvider base wraps a framework type that + /// itself follows the generated MRW pattern (declares the protected virtual *Core methods). + /// + private static (ModelProvider Model, MrwSerializationTypeDefinition Serialization) CreateDerivedModelWithMrwSystemBase() + { + var baseProp = InputFactory.Property("Name", InputPrimitiveType.String); + var baseInputModel = InputFactory.Model("Resource", properties: [baseProp]); + var derivedProp = InputFactory.Property("Location", InputPrimitiveType.String); + var derivedInputModel = InputFactory.Model("TrackedResource", properties: [derivedProp], baseModel: baseInputModel); + + // The base wraps a framework type that declares the generated MRW *Core methods, so a + // derived model must override them (mirrors deriving from an external MRW-generated type). + var systemType = new CSharpType(typeof(FakeMrwBase)); + var systemBase = new SystemObjectModelProvider(systemType, baseInputModel); + + var generator = MockHelpers.LoadMockGenerator( + inputModels: () => [baseInputModel, derivedInputModel], + createModelCore: (model) => + { + if (model.Name == "Resource") + return systemBase; + return new ModelProvider(model); + }, + createSerializationsCore: (inputType, typeProvider) => + inputType is InputModelType modelType && typeProvider is ModelProvider mp + ? [new MrwSerializationTypeDefinition(modelType, mp)] + : []); + generator.Object.TypeFactory.RootInputModels.Add(derivedInputModel); + generator.Object.TypeFactory.RootOutputModels.Add(derivedInputModel); + + var derived = ScmCodeModelGenerator.Instance.TypeFactory.CreateModel(derivedInputModel) as ModelProvider; + Assert.IsNotNull(derived); + Assert.IsInstanceOf(derived!.BaseModelProvider); + + var serializations = derived.SerializationProviders; + Assert.AreEqual(1, serializations.Count); + return (derived, (MrwSerializationTypeDefinition)serializations[0]); + } + + // ------------------------------------------------------------------- + // When the external/system base itself follows the generated MRW pattern (declares the + // protected virtual *Core methods), the derived model must OVERRIDE them rather than + // re-introduce them as virtual. Regression test for issue #11042 where deriving from an + // external MRW-generated base (e.g. OpenAI's ResponseItem) produced CS0114. + // ------------------------------------------------------------------- + + [Test] + public void PersistableModelCreateCore_IsOverride_WhenSystemBaseDeclaresCoreMethods() + { + var (_, serialization) = CreateDerivedModelWithMrwSystemBase(); + var method = serialization.BuildPersistableModelCreateCoreMethod(); + + Assert.IsNotNull(method); + Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override), + "PersistableModelCreateCore should be 'override' when the system base declares the *Core methods"); + Assert.IsFalse(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Virtual), + "PersistableModelCreateCore should NOT be 'virtual' when the system base declares the *Core methods"); + } + + [Test] + public void JsonModelCreateCore_IsOverride_WhenSystemBaseDeclaresCoreMethods() + { + var (_, serialization) = CreateDerivedModelWithMrwSystemBase(); + var method = serialization.BuildJsonModelCreateCoreMethod(); + + Assert.IsNotNull(method); + Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override), + "JsonModelCreateCore should be 'override' when the system base declares the *Core methods"); + Assert.IsFalse(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Virtual), + "JsonModelCreateCore should NOT be 'virtual' when the system base declares the *Core methods"); + } + + [Test] + public void PersistableModelWriteCore_IsOverride_WhenSystemBaseDeclaresCoreMethods() + { + var (_, serialization) = CreateDerivedModelWithMrwSystemBase(); + var method = serialization.BuildPersistableModelWriteCoreMethod(); + + Assert.IsNotNull(method); + Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override), + "PersistableModelWriteCore should be 'override' when the system base declares the *Core methods"); + Assert.IsFalse(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Virtual), + "PersistableModelWriteCore should NOT be 'virtual' when the system base declares the *Core methods"); + } + + [Test] + public void JsonModelWriteCore_IsOverride_WhenSystemBaseDeclaresCoreMethods() + { + var (_, serialization) = CreateDerivedModelWithMrwSystemBase(); + var method = serialization.BuildJsonModelWriteCoreMethod(); + + Assert.IsNotNull(method); + Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override), + "JsonModelWriteCore should be 'override' when the system base declares the *Core methods"); + } + + /// + /// A stand-in for an external framework type generated by the MRW emitter. It declares the + /// same protected virtual *Core methods that generated models expose, so reflection on + /// it reports that the base participates in the serialization override chain. + /// + private class FakeMrwBase + { + protected virtual void JsonModelWriteCore(Utf8JsonWriter writer, ModelReaderWriterOptions options) + { + } + + protected virtual FakeMrwBase JsonModelCreateCore(ref Utf8JsonReader reader, ModelReaderWriterOptions options) + => this; + + protected virtual BinaryData PersistableModelWriteCore(ModelReaderWriterOptions options) + => BinaryData.FromString(string.Empty); + + protected virtual FakeMrwBase PersistableModelCreateCore(BinaryData data, ModelReaderWriterOptions options) + => this; + } } } From f2295b58e3119b26e09600aaf7aa2478c817542d Mon Sep 17 00:00:00 2001 From: jolov Date: Mon, 22 Jun 2026 11:43:22 -0700 Subject: [PATCH 2/2] refactor: detect MRW base via IJsonModel/IPersistableModel interfaces Replace the reflective *Core method-name probe with a check of whether the external base type implements IJsonModel or IPersistableModel. This keys off the public model-reader-writer contract instead of protected implementation details, and drops the System.Reflection BindingFlags scan. The test's FakeMrwBase now implements IJsonModel to mirror a generated model. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../MrwSerializationTypeDefinition.cs | 48 +++++-------------- .../SystemObjectModelSerializationTests.cs | 20 ++++---- 2 files changed, 25 insertions(+), 43 deletions(-) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs index 993e087b9cd..5ac49c9ce6c 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs @@ -9,7 +9,6 @@ using System.IO; using System.Linq; using System.Net; -using System.Reflection; using System.Text; using System.Text.Json; using System.Xml; @@ -158,18 +157,6 @@ private static bool IsModelType(CSharpType type) => ScmCodeModelGenerator.Instance.TypeFactory.CSharpTypeMap.TryGetValue(type, out var baseProvider) && baseProvider is ModelProvider; - /// - /// The generated MRW serialization *Core methods. A base model participates in the - /// override chain only if it declares all of these as overridable members. - /// - private static readonly string[] SerializationCoreMethodNames = - [ - JsonModelWriteCoreMethodName, - JsonModelCreateCoreMethodName, - PersistableModelWriteCoreMethodName, - PersistableModelCreateCoreMethodName, - ]; - /// /// Determines whether a derived model should skip overriding the generated serialization /// *Core methods of its base. @@ -177,9 +164,10 @@ private static bool IsModelType(CSharpType type) /// /// External/system base models are represented by a . /// Such a base only participates in the generated MRW *Core override chain when the - /// wrapped framework/referenced type itself follows the generated serialization pattern (i.e. - /// declares the protected virtual *Core methods). When it does, derived models must - /// override those methods rather than hide them (otherwise the compiler reports CS0114). + /// wrapped framework/referenced type itself follows the model-reader-writer serialization + /// pattern (i.e. implements or , + /// and therefore exposes the overridable *Core methods). When it does, derived models + /// must override those methods rather than hide them (otherwise the compiler reports CS0114). /// When it does not (for example a hand-authored base such as ResourceData), derived /// models re-introduce the methods as virtual. /// @@ -192,38 +180,28 @@ private static bool ShouldSkipDerivedSerializationMethodOverrides(ModelProvider? if (baseModelProvider is SystemObjectModelProvider systemBase) { - return !SystemTypeDeclaresSerializationCoreMethods(systemBase.SystemType); + return !SystemTypeImplementsModelReaderWriter(systemBase.SystemType); } return baseModelProvider.ShouldSkipDerivedSerializationMethodOverrides; } - private static bool SystemTypeDeclaresSerializationCoreMethods(CSharpType systemType) + private static bool SystemTypeImplementsModelReaderWriter(CSharpType systemType) { if (!systemType.IsFrameworkType) { return false; } - foreach (var methodName in SerializationCoreMethodNames) + foreach (var @interface in systemType.FrameworkType.GetInterfaces()) { - if (!DeclaresInstanceMethod(systemType.FrameworkType, methodName)) + if (@interface.IsGenericType) { - return false; - } - } - - return true; - } - - private static bool DeclaresInstanceMethod(Type type, string methodName) - { - const BindingFlags flags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly; - for (Type? current = type; current is not null; current = current.BaseType) - { - if (current.GetMethods(flags).Any(m => m.Name == methodName)) - { - return true; + var definition = @interface.GetGenericTypeDefinition(); + if (definition == typeof(IJsonModel<>) || definition == typeof(IPersistableModel<>)) + { + return true; + } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs index b7018ebfa6d..924e3d48674 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs @@ -302,24 +302,28 @@ public void JsonModelWriteCore_IsOverride_WhenSystemBaseDeclaresCoreMethods() } /// - /// A stand-in for an external framework type generated by the MRW emitter. It declares the - /// same protected virtual *Core methods that generated models expose, so reflection on - /// it reports that the base participates in the serialization override chain. + /// A stand-in for an external framework type generated by the MRW emitter. It implements + /// (and therefore ) just as a + /// generated model does, so the base is recognized as participating in the serialization + /// override chain. /// - private class FakeMrwBase + private class FakeMrwBase : IJsonModel { - protected virtual void JsonModelWriteCore(Utf8JsonWriter writer, ModelReaderWriterOptions options) + void IJsonModel.Write(Utf8JsonWriter writer, ModelReaderWriterOptions options) { } - protected virtual FakeMrwBase JsonModelCreateCore(ref Utf8JsonReader reader, ModelReaderWriterOptions options) + FakeMrwBase IJsonModel.Create(ref Utf8JsonReader reader, ModelReaderWriterOptions options) => this; - protected virtual BinaryData PersistableModelWriteCore(ModelReaderWriterOptions options) + BinaryData IPersistableModel.Write(ModelReaderWriterOptions options) => BinaryData.FromString(string.Empty); - protected virtual FakeMrwBase PersistableModelCreateCore(BinaryData data, ModelReaderWriterOptions options) + FakeMrwBase IPersistableModel.Create(BinaryData data, ModelReaderWriterOptions options) => this; + + string IPersistableModel.GetFormatFromOptions(ModelReaderWriterOptions options) + => "J"; } } }