Fix enum serialization (#4208)

Robust.Client/GameController/GameController.cs

@@ -33,6 +33,7 @@ using Robust.Shared.Map;
 using Robust.Shared.Network;
 using Robust.Shared.Profiling;
 using Robust.Shared.Prototypes;
+using Robust.Shared.Reflection;
 using Robust.Shared.Replays;
 using Robust.Shared.Serialization;
 using Robust.Shared.Serialization.Manager;
@@ -85,6 +86,7 @@ namespace Robust.Client
         [Dependency] private readonly IReplayLoadManager _replayLoader = default!;
         [Dependency] private readonly IReplayPlaybackManager _replayPlayback = default!;
         [Dependency] private readonly IReplayRecordingManagerInternal _replayRecording = default!;
+        [Dependency] private readonly IReflectionManager _reflectionManager = default!;
 
         private IWebViewManagerHook? _webViewHook;
 
@@ -162,6 +164,7 @@ namespace Robust.Client
             // before prototype load.
             ProgramShared.FinishCheckBadFileExtensions(checkBadExtensions);
 
+            _reflectionManager.Initialize();
             _prototypeManager.Initialize();
             _prototypeManager.LoadDefaultPrototypes();
             _prototypeManager.ResolveResults();

Robust.Server/BaseServer.cs

@@ -30,6 +30,7 @@ using Robust.Shared.Map;
 using Robust.Shared.Network;
 using Robust.Shared.Profiling;
 using Robust.Shared.Prototypes;
+using Robust.Shared.Reflection;
 using Robust.Shared.Replays;
 using Robust.Shared.Serialization;
 using Robust.Shared.Serialization.Manager;
@@ -101,6 +102,7 @@ namespace Robust.Server
         [Dependency] private readonly IReplayRecordingManagerInternal _replay = default!;
         [Dependency] private readonly IGamePrototypeLoadManager _protoLoadMan = default!;
         [Dependency] private readonly NetworkResourceManager _netResMan = default!;
+        [Dependency] private readonly IReflectionManager _refMan = default!;
 
         private readonly Stopwatch _uptimeStopwatch = new();
 
@@ -367,6 +369,7 @@ namespace Robust.Server
             _prototype.Initialize();
             _prototype.LoadDefaultPrototypes();
             _prototype.ResolveResults();
+            _refMan.Initialize();
 
             _consoleHost.Initialize();
             _entityManager.Startup();

Robust.Shared/Reflection/IReflectionManager.cs

@@ -119,5 +119,7 @@ namespace Robust.Shared.Reflection
 
         Type? YamlTypeTagLookup(Type baseType, string typeName);
         IEnumerable<Type> FindAllTypes();
+
+        void Initialize();
     }
 }

Robust.Shared/Reflection/ReflectionManager.cs

@@ -4,6 +4,7 @@ using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Reflection;
 using System.Threading;
+using Robust.Shared.IoC;
 using Robust.Shared.Log;
 using Robust.Shared.Serialization;
 using Robust.Shared.Utility;
@@ -13,6 +14,8 @@ namespace Robust.Shared.Reflection
 {
     public abstract class ReflectionManager : IReflectionManager
     {
+        [Dependency] private readonly ILogManager _logMan = default!;
+
         /// <summary>
         /// Enumerable over prefixes that are added to the type provided to <see cref="GetType(string)"/>
         /// if the type can't be found in any assemblies.
@@ -39,6 +42,12 @@ namespace Robust.Shared.Reflection
         private readonly ReaderWriterLockSlim _yamlTypeTagCacheLock = new();
 
         private readonly List<Type> _getAllTypesCache = new();
+        private ISawmill _sawmill = default!;
+
+        public void Initialize()
+        {
+            _sawmill = _logMan.GetSawmill("Reflection");
+        }
 
         /// <inheritdoc />
         public IEnumerable<Type> GetAllChildren<T>(bool inclusive = false)
@@ -232,22 +241,21 @@ namespace Robust.Shared.Reflection
                     var name = fullName.Substring(dotIndex + 1);
                     reference = $"enum.{name}.{@enum}";
 
-                    if (TryParseEnumReference(reference, out var resolvedEnum, false) && resolvedEnum == @enum)
+                    if (_enumCache.TryAdd(reference, @enum))
                     {
-                        // TryParse will have filled in the cache already.
+                        _reverseEnumCache.Add(@enum, reference);
                         return reference;
                     }
                 }
 
                 // If that failed, just use the full name.
                 reference = $"enum.{fullName}.{@enum}";
-                _reverseEnumCache[@enum] = reference;
-                _enumCache[reference] = @enum;
+                _reverseEnumCache.Add(@enum, reference);
+                _enumCache.Add(reference, @enum);
                 return reference;
             }
         }
 
-
         /// <inheritdoc />
         public bool TryParseEnumReference(string reference, [NotNullWhen(true)] out Enum? @enum,
             bool shouldThrow = true)
@@ -258,19 +266,23 @@ namespace Robust.Shared.Reflection
                 return false;
             }
 
-            reference = reference.Substring(5);
-
             using (_enumCacheLock.ReadGuard())
             {
                 if (_enumCache.TryGetValue(reference, out @enum))
                     return true;
             }
 
-            // Doesn't exist, add it.
-            var dotIndex = reference.LastIndexOf('.');
-            var typeName = reference.Substring(0, dotIndex);
+            using var _ = _enumCacheLock.WriteGuard();
+            if (_enumCache.TryGetValue(reference, out @enum))
+                return true;
 
-            var value = reference.Substring(dotIndex + 1);
+            var cropped = reference.Substring(5);
+
+            // Doesn't exist, add it.
+            var dotIndex = cropped.LastIndexOf('.');
+            var typeName = cropped.Substring(0, dotIndex);
+
+            var value = cropped.Substring(dotIndex + 1);
 
             foreach (var assembly in assemblies)
             {
@@ -284,13 +296,12 @@ namespace Robust.Shared.Reflection
                         continue;
                     }
 
-                    using (_enumCacheLock.WriteGuard())
+                    @enum = (Enum)Enum.Parse(type, value);
+                    if (!_reverseEnumCache.TryAdd(@enum, reference))
                     {
-                        @enum = (Enum)Enum.Parse(type, value);
-                        _enumCache[reference] = @enum;
-                        _reverseEnumCache[@enum] = reference;
+                        _sawmill.Warning($"Conflicting enum references encountered. Enum: {@enum}. Existing: {_reverseEnumCache[@enum]}. New: {reference}");
                     }
-
+                    _enumCache.Add(reference, @enum);
                     return true;
                 }
             }

Robust.Shared/Serialization/Manager/SerializationManager.Reading.cs

@@ -278,6 +278,10 @@ namespace Robust.Shared.Serialization.Manager
             }
             else if (actualType.IsEnum)
             {
+                // Does not include cases where the target type is System.Enum.
+                // Those get handled by the generic enum serializer which uses reflection to resolve strings into enums.
+                DebugTools.Assert(actualType != typeof(Enum));
+
                 if (nodeType == typeof(ValueDataNode))
                 {
                     call = Expression.Call(managerConst, nameof(ReadEnumValue), new[] { actualType },

Robust.Shared/Serialization/Manager/SerializationManager.Validation.cs

@@ -66,6 +66,10 @@ public sealed partial class SerializationManager
             }
             else if (key.type.IsEnum)
             {
+                // Does not include cases where the target type is System.Enum.
+                // Those get handled by the generic enum serializer which uses reflection to resolve strings into enums.
+                DebugTools.Assert(key.type != typeof(Enum));
+
                 call = Expression.Call(
                     managerConst,
                     nameof(ValidateEnum),

Robust.Shared/Serialization/Manager/SerializationManager.Writing.cs

@@ -92,13 +92,33 @@ public sealed partial class SerializationManager
             }
             else if (actualType.IsEnum)
             {
-                // Enums implement IConvertible.
-                // Need it for the culture overload.
-                call = Expression.Call(
-                    instanceParam,
-                    nameof(WriteConvertible),
-                    Type.EmptyTypes,
-                    Expression.Convert(objParam, typeof(IConvertible)));
+                // When writing generic enums, we want to use the enum serializer.
+                // Otherwise, we fall back to the default IConvertible behaviour.
+
+                if (baseType != typeof(Enum) ||
+                    !serializationManager._regularSerializerProvider.TryGetTypeSerializer(typeof(ITypeWriter<>),
+                        typeof(Enum), out serializer))
+                {
+                    call = Expression.Call(
+                        instanceParam,
+                        nameof(WriteConvertible),
+                        Type.EmptyTypes,
+                        Expression.Convert(objParam, typeof(IConvertible)));
+                }
+                else
+                {
+                    var serializerConst = Expression.Constant(serializer);
+                    call = Expression.Call(
+                        instanceParam,
+                        nameof(WriteValue),
+                        new []{typeof(Enum)},
+                        serializerConst,
+                        Expression.Convert(objParam, typeof(Enum)),
+                        alwaysWriteParam,
+                        contextParam,
+                        Expression.Constant(notNullableOverride)
+                    );
+                }
             }
             else if (actualType.IsArray)
             {

Robust.UnitTesting/RobustUnitTest.cs

@@ -70,6 +70,7 @@ namespace Robust.UnitTesting
             var configurationManager = deps.Resolve<IConfigurationManagerInternal>();
 
             configurationManager.Initialize(Project == UnitTestProject.Server);
+            deps.Resolve<IReflectionManager>().Initialize();
 
             foreach (var assembly in assemblies)
             {