From b33fe857f4b8f7d6fbfbdfacb9677f0c88371e59 Mon Sep 17 00:00:00 2001 From: Jorge Rangel <102122018+jorgerangel-msft@users.noreply.github.com> Date: Mon, 21 Oct 2024 16:04:57 -0500 Subject: [PATCH] [http-client-csharp] Fix: remove setters for props in readonly structs (#4809) Removes the invalid setters from auto-implemented properties in read-only structs. fixes: https://github.com/microsoft/typespec/issues/4351 --- .../src/Providers/PropertyProvider.cs | 9 +++- .../test/InputLibraryVisitorTests.cs | 2 +- .../test/Providers/PropertyProviderTests.cs | 52 ++++++++++++++----- .../test/TestHelpers/TestTypeProvider.cs | 9 ++++ .../TypeProviderWriter_WriteModelAsStruct.cs | 4 +- 5 files changed, 58 insertions(+), 18 deletions(-) diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/src/Providers/PropertyProvider.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/src/Providers/PropertyProvider.cs index 1b756af18a..43868bf8e3 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/src/Providers/PropertyProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/src/Providers/PropertyProvider.cs @@ -77,6 +77,8 @@ private PropertyProvider(InputModelProperty inputProperty, CSharpType propertyTy { propertyType = propertyType.WithNullable(true); } + + EnclosingType = enclosingType; var serializationFormat = CodeModelPlugin.Instance.TypeFactory.GetSerializationFormat(inputProperty.Type); var propHasSetter = PropertyHasSetter(propertyType, inputProperty); MethodSignatureModifiers setterModifier = propHasSetter ? MethodSignatureModifiers.Public : MethodSignatureModifiers.None; @@ -89,7 +91,6 @@ private PropertyProvider(InputModelProperty inputProperty, CSharpType propertyTy XmlDocSummary = PropertyDescriptionBuilder.BuildPropertyDescription(inputProperty, propertyType, serializationFormat, Description); XmlDocs = GetXmlDocs(); WireInfo = new PropertyWireInformation(inputProperty); - EnclosingType = enclosingType; IsDiscriminator = inputProperty.IsDiscriminator; InitializeParameter(Name, FormattableStringHelpers.FromString(inputProperty.Description) ?? FormattableStringHelpers.Empty, Type); @@ -141,7 +142,6 @@ private XmlDocProvider GetXmlDocs() /// /// Returns true if the property has a setter. /// - /// The of the property. protected virtual bool PropertyHasSetter(CSharpType type, InputModelProperty inputProperty) { if (inputProperty.IsDiscriminator) @@ -164,6 +164,11 @@ protected virtual bool PropertyHasSetter(CSharpType type, InputModelProperty inp return false; } + if (EnclosingType.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Struct | TypeSignatureModifiers.ReadOnly)) + { + return false; + } + if (type.IsCollection && !type.IsReadOnlyMemory) { return type.IsNullable; diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/InputLibraryVisitorTests.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/InputLibraryVisitorTests.cs index 727b5385f1..0322505bbe 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/InputLibraryVisitorTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/InputLibraryVisitorTests.cs @@ -40,7 +40,7 @@ public void PreVisitsProperties() _mockVisitor.Object.Visit(_mockPlugin.Object.OutputLibrary); _mockVisitor.Protected().Verify("Visit", Times.Once(), inputModel, ItExpr.Is(m => m.Name == new ModelProvider(inputModel).Name)); - _mockVisitor.Protected().Verify("Visit", Times.Once(), inputModelProperty, ItExpr.Is(m => m.Name == new PropertyProvider(inputModelProperty, new TestTypeProvider()).Name)); + _mockVisitor.Protected().Verify("Visit", Times.Once(), inputModelProperty, ItExpr.Is(m => m.Name == new PropertyProvider(inputModelProperty, TestTypeProvider.Empty).Name)); } [Test] diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/PropertyProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/PropertyProviderTests.cs index c85b1ab711..c81cb0519e 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/PropertyProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Providers/PropertyProviderTests.cs @@ -86,10 +86,10 @@ public void CollectionProperty(CSharpType coreType, InputModelProperty collectio } [TestCaseSource(nameof(BodyHasSetterTestCases))] - public void BodyHasSetterValidation(string name, InputModelType inputModel, bool expectedHasSetter) + public void BodyHasSetterValidation(string name, InputModelType inputModel, bool expectedHasSetter, TypeSignatureModifiers? typeSignatureModifiers = null) { var collectionProperty = inputModel.Properties.Single(); - var property = new PropertyProvider(collectionProperty, new TestTypeProvider()); + var property = new PropertyProvider(collectionProperty, new TestTypeProvider(typeSignatureModifiers)); Assert.AreEqual(expectedHasSetter, property.Body.HasSetter); } @@ -123,47 +123,73 @@ private static IEnumerable BodyHasSetterTestCases() yield return new TestCaseData( "readOnlyString", InputFactory.Model("TestModel", properties: [InputFactory.Property("readOnlyString", InputPrimitiveType.String, isRequired: true, isReadOnly: true)]), - false); + false, + null); yield return new TestCaseData( "readOnlyStringOnInputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("readOnlyString", InputPrimitiveType.Int32, isRequired: true, isReadOnly: true)]), - false); + false, + null); yield return new TestCaseData( "intOnInputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("intProperty", InputPrimitiveType.Int32, isRequired: true)]), - true); + true, + null); yield return new TestCaseData( "readOnlyCollectionOnOutputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output, properties: [InputFactory.Property("readOnlyCollection", new InputNullableType(InputFactory.Array(InputPrimitiveType.String)), isRequired: true, isReadOnly: true)]), - false); + false, + null); yield return new TestCaseData( "readOnlyCollectionOnInputOutputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input | InputModelTypeUsage.Output, properties: [InputFactory.Property("readOnlyCollection", new InputNullableType(InputFactory.Array(InputPrimitiveType.String)), isRequired: true, isReadOnly: true)]), - false); + false, + null); yield return new TestCaseData( "nullableCollectionOnOutputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output, properties: [InputFactory.Property("nullableCollection", new InputNullableType(InputFactory.Array(InputPrimitiveType.String)), isRequired: true, isReadOnly: false)]), - false); + false, + null); yield return new TestCaseData( "nullableCollectionOnInputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("nullableCollection", new InputNullableType(InputFactory.Array(InputPrimitiveType.String)), isRequired: true, isReadOnly: false)]), - true); + true, + null); yield return new TestCaseData( "readOnlyDictionaryOnOutputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output, properties: [InputFactory.Property("readOnlyDictionary", InputFactory.Dictionary(InputPrimitiveType.Int32), isRequired: true)]), - false); + false, + null); yield return new TestCaseData( "readOnlyDictionaryOnInputOutputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output | InputModelTypeUsage.Input, properties: [InputFactory.Property("readOnlyDictionary", InputFactory.Dictionary(InputPrimitiveType.Int32), isRequired: true)]), - false); + false, + null); yield return new TestCaseData( "nullableDictionaryOnOutputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Output, properties: [InputFactory.Property("nullableDictionary", new InputNullableType(InputFactory.Dictionary(InputPrimitiveType.String)), isRequired: true, isReadOnly: false)]), - false); + false, + null); yield return new TestCaseData( "nullableDictionaryOnInputModel", InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("nullableDictionary", new InputNullableType(InputFactory.Dictionary(InputPrimitiveType.String)), isRequired: true, isReadOnly: false)]), - true); + true, + null); + yield return new TestCaseData( + "nonReadOnlyStringPropOnStruct", + InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("nonReadOnlyString", InputPrimitiveType.String)], modelAsStruct: true), + true, + TypeSignatureModifiers.Struct); + yield return new TestCaseData( + "requiredReadOnlyStringPropOnStruct", + InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("readOnlyString", InputPrimitiveType.String, isReadOnly: true, isRequired: true)], modelAsStruct: true), + false, + TypeSignatureModifiers.Struct); + yield return new TestCaseData( + "propInReadOnlyStruct", + InputFactory.Model("TestModel", usage: InputModelTypeUsage.Input, properties: [InputFactory.Property("nonReadOnlyString", InputPrimitiveType.String)], modelAsStruct: true), + false, + TypeSignatureModifiers.Struct | TypeSignatureModifiers.ReadOnly); } } } diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/TestHelpers/TestTypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/TestHelpers/TestTypeProvider.cs index 27aaeb0c24..8c7bcd2dfb 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/TestHelpers/TestTypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/TestHelpers/TestTypeProvider.cs @@ -1,16 +1,25 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using Microsoft.Generator.CSharp.Primitives; using Microsoft.Generator.CSharp.Providers; namespace Microsoft.Generator.CSharp.Tests { internal class TestTypeProvider : TypeProvider { + private readonly TypeSignatureModifiers? _declarationModifiers; protected override string BuildRelativeFilePath() => $"{Name}.cs"; protected override string BuildName() => "TestName"; public static readonly TypeProvider Empty = new TestTypeProvider(); + + internal TestTypeProvider(TypeSignatureModifiers? declarationModifiers = null) + { + _declarationModifiers = declarationModifiers; + } + + protected override TypeSignatureModifiers GetDeclarationModifiers() => _declarationModifiers ?? base.GetDeclarationModifiers(); } } diff --git a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Writers/TestData/TypeProviderWriterTests/TypeProviderWriter_WriteModelAsStruct.cs b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Writers/TestData/TypeProviderWriterTests/TypeProviderWriter_WriteModelAsStruct.cs index 5f69bbe6b6..2e0823e11e 100644 --- a/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Writers/TestData/TypeProviderWriterTests/TypeProviderWriter_WriteModelAsStruct.cs +++ b/packages/http-client-csharp/generator/Microsoft.Generator.CSharp/test/Writers/TestData/TypeProviderWriterTests/TypeProviderWriter_WriteModelAsStruct.cs @@ -34,9 +34,9 @@ internal TestModel(string requiredString, int requiredInt, global::System.Collec } /// Description for requiredString. - public string RequiredString { get; set; } + public string RequiredString { get; } /// Description for requiredInt. - public int RequiredInt { get; set; } + public int RequiredInt { get; } } }