From ddbe4be8ae543e243748e3289946977a08648a4e Mon Sep 17 00:00:00 2001 From: James Courtney Date: Fri, 31 May 2024 00:13:29 -0700 Subject: [PATCH] Fix required properties (#439) * Fix required properties * Fix tests * Update dotnet.yml --- .github/workflows/dotnet.yml | 6 +- src/Directory.Build.props | 4 +- .../SchemaModel/PropertyFieldModel.cs | 10 +++- src/FlatSharp.Compiler/SetterKind.cs | 7 +-- src/FlatSharp/FlatSharp.csproj | 2 +- .../DeserializeClassDefinition.cs | 17 ++---- src/FlatSharp/TypeModel/StructTypeModel.cs | 11 ++-- src/FlatSharp/TypeModel/TableTypeModel.cs | 2 +- src/Tests/CompileTests/CSharp8/CSharp8.csproj | 3 + .../Required/Required.cs | 58 +++++++++++++++++++ .../Required/Required.fbs | 16 ++++- 11 files changed, 102 insertions(+), 34 deletions(-) diff --git a/.github/workflows/dotnet.yml b/.github/workflows/dotnet.yml index b3ddd7e2..6ca5c11c 100644 --- a/.github/workflows/dotnet.yml +++ b/.github/workflows/dotnet.yml @@ -12,11 +12,11 @@ jobs: strategy: matrix: include: - - os: windows-latest + - os: windows-2022 rid: win-x64 - - os: ubuntu-latest + - os: ubuntu-22.04 rid: linux-x64 - - os: macos-latest + - os: macos-12 rid: osx-x64 runs-on: ${{ matrix.os }} env: diff --git a/src/Directory.Build.props b/src/Directory.Build.props index d7df55b9..1311a35c 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -18,8 +18,8 @@ - 7.6.0 - 7.6.0 + 7.7.0 + 7.7.0 $(Version) James Courtney FlatSharp is a fast, idiomatic implementation of the FlatBuffer binary format. diff --git a/src/FlatSharp.Compiler/SchemaModel/PropertyFieldModel.cs b/src/FlatSharp.Compiler/SchemaModel/PropertyFieldModel.cs index b655fab7..febba6d6 100644 --- a/src/FlatSharp.Compiler/SchemaModel/PropertyFieldModel.cs +++ b/src/FlatSharp.Compiler/SchemaModel/PropertyFieldModel.cs @@ -132,14 +132,15 @@ public void WriteCode(CodeWriter writer) this.Attributes.EmitAsMetadata(writer); - string setter = this.Attributes.SetterKind switch + SetterKind setterKind = this.Attributes.SetterKind ?? SetterKind.Public; + + string setter = setterKind switch { SetterKind.PublicInit => "init;", SetterKind.Protected => "protected set;", SetterKind.ProtectedInternal => "protected internal set;", SetterKind.ProtectedInit => "protected init;", SetterKind.ProtectedInternalInit => "protected internal init;", - SetterKind.Private => "private set;", SetterKind.None => string.Empty, SetterKind.Public or _ => "set;", }; @@ -153,8 +154,11 @@ public void WriteCode(CodeWriter writer) } string property = $"{access} virtual {typeName} {this.FieldName} {{ get; {setter} }}"; - if (this.Field.Required == true) + + bool isPublicSetter = setterKind == SetterKind.Public || setterKind == SetterKind.PublicInit; + if (this.Field.Required == true && isPublicSetter) { + // Required keyword is tricky with visibility and setters. writer.BeginPreprocessorIf(CSharpHelpers.Net7PreprocessorVariable, $"required {property}") .Else(property) .Flush(); diff --git a/src/FlatSharp.Compiler/SetterKind.cs b/src/FlatSharp.Compiler/SetterKind.cs index 2004ba6d..d2804ef5 100644 --- a/src/FlatSharp.Compiler/SetterKind.cs +++ b/src/FlatSharp.Compiler/SetterKind.cs @@ -51,13 +51,8 @@ public enum SetterKind /// ProtectedInternalInit = 5, - /// - /// A private setter. - /// - Private = 6, - /// /// No setter. /// - None = 7, + None = 6, } diff --git a/src/FlatSharp/FlatSharp.csproj b/src/FlatSharp/FlatSharp.csproj index fd96349f..2fbbe6bd 100644 --- a/src/FlatSharp/FlatSharp.csproj +++ b/src/FlatSharp/FlatSharp.csproj @@ -1,7 +1,7 @@  true - netstandard2.0;netstandard2.1;net6.0;net7.0;net8.0 + net6.0;net7.0;net8.0 FlatSharp FlatSharp FlatSharp is an idiomatic C# implementation of the FlatBuffer serialization format. Use attributes to declare your data contracts! diff --git a/src/FlatSharp/Serialization/DeserializeClassDefinition.cs b/src/FlatSharp/Serialization/DeserializeClassDefinition.cs index 3b1583d6..8864beab 100644 --- a/src/FlatSharp/Serialization/DeserializeClassDefinition.cs +++ b/src/FlatSharp/Serialization/DeserializeClassDefinition.cs @@ -45,7 +45,7 @@ internal class DeserializeClassDefinition protected readonly string vtableAccessor; protected readonly string remainingDepthAccessor; - private DeserializeClassDefinition( + public DeserializeClassDefinition( string className, MethodInfo? onDeserializeMethod, ITypeModel typeModel, @@ -101,16 +101,6 @@ private DeserializeClassDefinition( } } - public static DeserializeClassDefinition Create( - string className, - MethodInfo? onDeserializeMethod, - ITypeModel typeModel, - int maxVTableIndex, - FlatBufferSerializerOptions options) - { - return new DeserializeClassDefinition(className, onDeserializeMethod, typeModel, maxVTableIndex, options); - } - public bool HasEmbeddedBufferReference => !this.options.GreedyDeserialize; public string ClassName { get; } @@ -222,8 +212,11 @@ private void AddPropertyDefinitions(ItemMemberModel itemModel) } string required = string.Empty; - if (itemModel.IsRequired) + + // Net 6.0 doesn't define this, so leave open the opportunity for users to add their own polyfills. + if (itemModel.PropertyInfo.GetCustomAttributes().Any(x => x.GetType().FullName == "System.Runtime.CompilerServices.RequiredMemberAttribute")) { + FlatSharpInternal.Assert(itemModel.IsRequired, "Expecting the item to be required"); required = " required"; } diff --git a/src/FlatSharp/TypeModel/StructTypeModel.cs b/src/FlatSharp/TypeModel/StructTypeModel.cs index ca345ee2..05a466db 100644 --- a/src/FlatSharp/TypeModel/StructTypeModel.cs +++ b/src/FlatSharp/TypeModel/StructTypeModel.cs @@ -32,9 +32,11 @@ public class StructTypeModel : RuntimeTypeModel private ConstructorInfo? preferredConstructor; private MethodInfo? onDeserializeMethod; private readonly Guid guid = Guid.NewGuid(); + private readonly FlatBufferStructAttribute? attribute; internal StructTypeModel(Type clrType, TypeModelContainer container) : base(clrType, container) { + this.attribute = clrType.GetCustomAttribute(); } /// @@ -102,10 +104,12 @@ public override CodeGeneratedMethod CreateGetMaxSizeMethodBody(GetMaxSizeCodeGen public override CodeGeneratedMethod CreateParseMethodBody(ParserCodeGenContext context) { + FlatSharpInternal.Assert(this.attribute is not null, "Attribute shouldn't be null"); + // We have to implement two items: The table class and the overall "read" method. // Let's start with the read method. string className = this.GetDeserializedClassName(context.Options.DeserializationOption); - DeserializeClassDefinition classDef = DeserializeClassDefinition.Create( + DeserializeClassDefinition classDef = new( className, this.onDeserializeMethod, this, @@ -191,10 +195,7 @@ public override void Validate() { base.Validate(); - { - FlatBufferStructAttribute? attribute = this.ClrType.GetCustomAttribute(); - FlatSharpInternal.Assert(attribute != null, "Missing attribute."); - } + FlatSharpInternal.Assert(this.attribute is not null, "Missing attribute"); // Reset in case validation is invoked multiple times. this.inlineSize = 0; diff --git a/src/FlatSharp/TypeModel/TableTypeModel.cs b/src/FlatSharp/TypeModel/TableTypeModel.cs index 6c4e30ec..656e2fe0 100644 --- a/src/FlatSharp/TypeModel/TableTypeModel.cs +++ b/src/FlatSharp/TypeModel/TableTypeModel.cs @@ -669,7 +669,7 @@ public override CodeGeneratedMethod CreateParseMethodBody(ParserCodeGenContext c // We have to implement two items: The table class and the overall "read" method. // Let's start with the read method. - var classDef = DeserializeClassDefinition.Create(className, this.onDeserializeMethod, this, this.MaxIndex, context.Options); + var classDef = new DeserializeClassDefinition(className, this.onDeserializeMethod, this, this.MaxIndex, context.Options); // Build up a list of property overrides. foreach (var item in this.IndexToMemberMap.Where(x => !x.Value.IsDeprecated)) diff --git a/src/Tests/CompileTests/CSharp8/CSharp8.csproj b/src/Tests/CompileTests/CSharp8/CSharp8.csproj index 4f7370f6..c4d1f720 100644 --- a/src/Tests/CompileTests/CSharp8/CSharp8.csproj +++ b/src/Tests/CompileTests/CSharp8/CSharp8.csproj @@ -23,6 +23,9 @@ + + + diff --git a/src/Tests/FlatSharpEndToEndTests/Required/Required.cs b/src/Tests/FlatSharpEndToEndTests/Required/Required.cs index cb81032a..3c61f655 100644 --- a/src/Tests/FlatSharpEndToEndTests/Required/Required.cs +++ b/src/Tests/FlatSharpEndToEndTests/Required/Required.cs @@ -128,5 +128,63 @@ void ParseAndUse(string fieldName) // Finally, make sure something doesn't throw: ParseAndUse("__"); } + + [TestMethod] + [DataRow(nameof(RequiredTable_Setters.Pub), true)] + [DataRow(nameof(RequiredTable_Setters.PubInit), true)] + [DataRow(nameof(RequiredTable_Setters.Prot), false)] + [DataRow(nameof(RequiredTable_Setters.ProtectedInit), false)] + [DataRow(nameof(RequiredTable_Setters.ProtectedInternal), false)] + [DataRow(nameof(RequiredTable_Setters.ProtectedInternalInit), false)] + [DataRow(nameof(RequiredTable_Setters.None), false)] + public void Only_Public_Setters_Are_CSharp_Required(string propertyName, bool expectRequired) + { + PropertyInfo property = typeof(RequiredTable_Setters).GetProperty(propertyName); + +#if NET7_0_OR_GREATER + // Make sure the property is flagged as being required. + Assert.AreEqual( + expectRequired, + typeof(RequiredTable_Setters).GetProperty(propertyName).GetCustomAttributes().Any(x => x.GetType().FullName == "System.Runtime.CompilerServices.RequiredMemberAttribute")); +#endif + + // Now make sure that FlatSharp still throws the error for required property missing even if the C# property is not required. + RequiredTable_Setters table; + if (propertyName == nameof(RequiredTable_Setters.None)) + { + table = new(false); + } + else + { + table = new(true); + + // Set to null. + property.SetMethod.Invoke(table, new object[] { null }); + } + + // Serialize and expect error. + Assert.ThrowsException(() => RequiredTable_Setters.Serializer.Write(new byte[1024], table)); + } +} + +public partial class RequiredTable_Setters +{ +#if NET7_0_OR_GREATER + [SetsRequiredMembers] +#endif + public RequiredTable_Setters(bool setNone) + { + this.Pub = "a"; + this.PubInit = "b"; + this.Prot = "c"; + this.ProtectedInit = "d"; + this.ProtectedInternal = "e"; + this.ProtectedInternalInit = "f"; + + if (setNone) + { + this.None = "g"; + } + } } diff --git a/src/Tests/FlatSharpEndToEndTests/Required/Required.fbs b/src/Tests/FlatSharpEndToEndTests/Required/Required.fbs index dbb27248..92bd1c73 100644 --- a/src/Tests/FlatSharpEndToEndTests/Required/Required.fbs +++ b/src/Tests/FlatSharpEndToEndTests/Required/Required.fbs @@ -1,6 +1,8 @@  attribute "fs_serializer"; attribute "fs_valueStruct"; +attribute "fs_setter"; +attribute "fs_defaultCtor"; namespace FlatSharpEndToEndTests.Required; @@ -24,6 +26,7 @@ table RequiredTable (fs_serializer) F : ValueStruct (required); } + table NonRequiredTable (fs_serializer) { A : string; @@ -32,4 +35,15 @@ table NonRequiredTable (fs_serializer) D : [ ubyte ]; E : RefStruct; F : ValueStruct; -} \ No newline at end of file +} + +table RequiredTable_Setters (fs_serializer, fs_defaultCtor:"None") +{ + pub : string (required, fs_setter:"Public"); + pub_init : string (required, fs_setter:"PublicInit"); + prot : string (required, fs_setter:"Protected"); + protected_internal : string (required, fs_setter:"ProtectedInternal"); + protected_init : string (required, fs_setter:"ProtectedInit"); + protected_internal_init : string (required, fs_setter:"ProtectedInternalInit"); + none : string (required, fs_setter:"None"); +}