feat(csharp): rework enum serialization to eliminate reflection#13519
feat(csharp): rework enum serialization to eliminate reflection#13519fern-support merged 10 commits intomainfrom
Conversation
- Regular enums now generate a companion serializer class with compile-time switch statements instead of using generic EnumSerializer<T> - String enums now generate a nested serializer class instead of using StringEnumSerializer<T> which relied on Activator.CreateInstance - Remove generic EnumSerializer.Template.cs and StringEnumSerializer.Template.cs from as-is includes - Update template test files to use inline serializers Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
… handle renamed Value property Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
| } No newline at end of file | ||
| } | ||
|
|
||
| internal class DummyEnumSerializer : JsonConverter<DummyEnum> |
There was a problem hiding this comment.
I think we can do better. Let's create two maps enum -> string and string -> enum and just do lookups instead of switch statements.
There was a problem hiding this comment.
Good idea — I'll rework regular enum serializers to use Dictionary<string, TEnum> and Dictionary<TEnum, string> lookups instead of switch statements.
|
|
||
| public static bool operator !=(DummyEnum value1, string value2) => !value1.Value.Equals(value2); | ||
|
|
||
| internal class DummyEnumSerializer : JsonConverter<DummyEnum> |
There was a problem hiding this comment.
Do we need an inline serializer, what was the previous StringEnumSerializer coming short for?
There was a problem hiding this comment.
The previous StringEnumSerializer<T> used Activator.CreateInstance (reflection) to construct the string enum instance from the deserialized string value — that's the core thing FER-4622 asked us to eliminate. The inline serializer replaces that with a direct new MyEnum(stringValue) call, no reflection needed.
For string enums specifically, the serializer is simple (just construct from string / read the Value property), so the inline version is lightweight.
…m serializers Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
|
|
||
| public override void Write(System.Text.Json.Utf8JsonWriter writer, DummyEnum value, JsonSerializerOptions options) | ||
| { | ||
| writer.WriteStringValue(_enumToString.TryGetValue(value, out var stringValue) ? stringValue : null); |
There was a problem hiding this comment.
The Write method writes null for unknown enum values instead of throwing an exception. This is inconsistent with the test template in QueryStringConverterTest.Template.cs:170 which throws an ArgumentOutOfRangeException. Writing null can cause silent data corruption when an invalid enum value is serialized.
// Current (problematic):
writer.WriteStringValue(_enumToString.TryGetValue(value, out var stringValue) ? stringValue : null);
// Should be:
writer.WriteStringValue(_enumToString[value]); // Throws if key not found| writer.WriteStringValue(_enumToString.TryGetValue(value, out var stringValue) ? stringValue : null); | |
| writer.WriteStringValue(_enumToString[value]); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
…8.0 from main) Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Description
Linear ticket: Closes FER-4622
Eliminates all reflection usage from C# enum JSON serialization by generating per-enum serializer classes with compile-time dictionary lookups.
Link to Devin Session
Requested by: @Swimburger
Changes Made
Regular enums (
EnumGenerator.ts/Enum.ts):{EnumName}Serializerclass in the same file_stringToEnum,_enumToString) provide O(1) bidirectional mappingReaduses_stringToEnum.TryGetValue()— returnsdefaultfor unknown valuesWriteuses_enumToString.TryGetValue()— writesnullfor unknown values[JsonConverter(typeof(EnumSerializer<T>))]→[JsonConverter(typeof(TSerializer))]String enums (
StringEnumGenerator.ts):{EnumName}Serializerclass inside the record structRead:new MyEnum(stringValue)— no reflection needed (previously usedActivator.CreateInstance)Write:writer.WriteStringValue(value.{propertyName})— uses the actual property name (may beValue_if a member is named "Value")[JsonConverter(typeof(StringEnumSerializer<T>))]→[JsonConverter(typeof(T.TSerializer))]Removed generic serializer files from as-is includes:
EnumSerializer.Template.csandStringEnumSerializer.Template.csare no longer included in generated outputEnumSerializerTests,StringEnumSerializerTests) no longer included eitherUpdated template test files to use inline serializers instead of generic ones.
Updates Since Last Revision
static readonly Dictionaryfields provide bidirectional mapping;Read/WriteuseTryGetValue().generators/csharp/sdk/versions.yml— bumped SDK version to2.28.0(main claimed2.27.0for gRPC changes).Value_property fix, biome formatting fixes, changelog validation fixes.Testing
exhaustivefixture variants pass (build + tests)undiscriminated-unionsfixture passes (build + tests) — validates theValueproperty rename edge caseenumfixture (bothplain-enumsandforward-compatible-enums) passesHuman Review Checklist
Writebehavior for unknown values: The dictionary-basedWritemethod writesnullfor unmapped enum values (viaTryGetValuereturningfalse). The previous switch-based approach threwArgumentOutOfRangeException. Verify this is the desired behavior for forward compatibility.QueryStringConverterTest.Template.csstill uses switch-based serializer: The test helperTestEnumSerializerin this file uses switch statements rather than dictionaries. This is a test-only helper (not generated code), but is inconsistent with the generated pattern. May want to align for clarity.StringEnumGenerator.ts— serializer created aftervalueProperty: The nested serializer class is intentionally defined aftervalueProperty(line ~227) so that theWritemethod can referencevalueProperty.name. If anyone reorders this code and moves the serializer back beforevalueProperty, the hardcodedvalue.Valuebug will return for enums with a "Value" member. Worth verifying in theundiscriminated-unions/KeyType.csseed output thatvalue.Value_is generated correctly.Enum.ts:writeSerializerClass()— The companion serializer is written via raw string generation rather than the AST builder. Verify the generated dictionary initializers handle edge cases (special chars in wire values,defaultfallback for unknown values on deserialization).Extensions.csStringifystill uses reflection — Determined to be unused in generated code, so left as-is. Confirm this is acceptable.popScope(false)calls inEnum.ts— Used to close dictionary initializer blocks without trailing newline. Verify the generated C# formatting is correct (semicolons placed correctly after closing braces).