Try fix entity deserialization component composition

RELEASE-NOTES.md

@@ -43,7 +43,7 @@ END TEMPLATE-->
 
 ### Bugfixes
 
-*None yet*
+* Fixed entity deserialization for components with a data fields that have a AlwaysPushInheritance Attribute
 
 ### Other
 

Robust.Server/GameObjects/EntitySystems/MapLoaderSystem.cs

@@ -545,8 +545,6 @@ private void LoadEntity(EntityUid uid, MappingDataNode data, MetaDataComponent m
             _context.CurrentReadingEntityComponents.EnsureCapacity(componentList.Count);
             foreach (var compData in componentList.Cast<MappingDataNode>())
             {
-                var datanode = compData.Copy();
-                datanode.Remove("type");
                 var value = ((ValueDataNode)compData["type"]).Value;
                 if (!_factory.TryGetRegistration(value, out var reg))
                 {
@@ -555,15 +553,36 @@ private void LoadEntity(EntityUid uid, MappingDataNode data, MetaDataComponent m
                     continue;
                 }
 
+
+                MappingDataNode datanode;
                 var compType = reg.Type;
                 if (prototype?.Components != null && prototype.Components.TryGetValue(value, out var protData))
                 {
-                    datanode =
-                        _serManager.PushCompositionWithGenericNode(
+                    // Previously this method used generic composition pushing. I.e.:
+                    /*
+                        datanode = _serManager.PushCompositionWithGenericNode(
                             compType,
-                            new[] { protData.Mapping }, datanode, _context);
+                            new[] {protData.Mapping},
+                            datanode,
+                            _context);
+                    */
+                    // However, I don't think this is what we want to do here. I.e., we want to ignore things like the
+                    // AlwaysPushInheritanceAttribute. Complex inheritance pushing should have already been done when
+                    // creating the proto data. Now we just want to override the prototype information with the
+                    // serialized data.
+                    //
+                    // If we do ever want to support this, we need to change entity serialization so that it doesn't do
+                    // a simple diff with respect to the prototype data and instead does some kind of inheritance
+                    // subtraction / removal.
+
+                    datanode = _serManager.CombineMappings(compData, protData.Mapping);
+                }
+                else
+                {
+                    datanode = compData.ShallowClone();
                 }
 
+                datanode.Remove("type");
                 _context.CurrentComponent = value;
                 _context.CurrentReadingEntityComponents[value] = (IComponent) _serManager.Read(compType, datanode, _context)!;
                 _context.CurrentComponent = null;

Robust.Shared/Serialization/Manager/ISerializationManager.cs

@@ -3,6 +3,7 @@
 using JetBrains.Annotations;
 using Robust.Shared.Reflection;
 using Robust.Shared.Serialization.Markdown;
+using Robust.Shared.Serialization.Markdown.Mapping;
 using Robust.Shared.Serialization.Markdown.Validation;
 using Robust.Shared.Serialization.TypeSerializers.Interfaces;
 
@@ -440,6 +441,12 @@ public TNode PushCompositionWithGenericNode<TNode>(Type type, TNode[] parents, T
             return (TNode) PushComposition(type, parents, child, context);
         }
 
+        /// <summary>
+        /// Simple <see cref="MappingDataNode"/> inheritance pusher clones data and overrides a parent's values with
+        /// the child's.
+        /// </summary>
+        MappingDataNode CombineMappings(MappingDataNode child, MappingDataNode parent);
+
         #endregion
 
         public bool TryGetVariableType(Type type, string variableName, [NotNullWhen(true)] out Type? variableType);

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

@@ -95,7 +95,7 @@ private PushCompositionDelegate GetOrCreatePushCompositionDelegate(Type type, Da
                         Expression.Convert(parentParam, nodeType)),
                     MappingDataNode => Expression.Call(
                         instanceConst,
-                        nameof(PushInheritanceMapping),
+                        nameof(CombineMappings),
                         Type.EmptyTypes,
                         Expression.Convert(childParam, nodeType),
                         Expression.Convert(parentParam, nodeType)),
@@ -117,32 +117,26 @@ private SequenceDataNode PushInheritanceSequence(SequenceDataNode child, Sequenc
         //todo implement different inheritancebehaviours for yamlfield
         // I have NFI what this comment means.
 
-        var result = new SequenceDataNode(child.Count + parent.Count);
+        var result = child.Copy();
         foreach (var entry in parent)
         {
-            result.Add(entry);
-        }
-        foreach (var entry in child)
-        {
-            result.Add(entry);
+            result.Add(entry.Copy());
         }
 
         return result;
     }
 
-    private MappingDataNode PushInheritanceMapping(MappingDataNode child, MappingDataNode parent)
+    public MappingDataNode CombineMappings(MappingDataNode child, MappingDataNode parent)
     {
         //todo implement different inheritancebehaviours for yamlfield
         // I have NFI what this comment means.
+        // I still don't know what it means, but if it's talking about the always/never push inheritance attributes,
+        // make sure it doesn't break entity serialization.
 
-        var result = new MappingDataNode(child.Count + parent.Count);
+        var result = child.Copy();
         foreach (var (k, v) in parent)
         {
-            result[k] = v;
-        }
-        foreach (var (k, v) in child)
-        {
-            result[k] = v;
+            result.TryAddCopy(k, v);
         }
 
         return result;

Robust.Shared/Serialization/Markdown/Mapping/MappingDataNode.cs

@@ -3,6 +3,7 @@
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
+using System.Runtime.InteropServices;
 using System.Threading;
 using Robust.Shared.Serialization.Markdown.Value;
 using Robust.Shared.Utility;
@@ -272,6 +273,26 @@ public override MappingDataNode Copy()
             return newMapping;
         }
 
+        /// <summary>
+        /// Variant of <see cref="Copy"/> that doesn't clone the keys or values.
+        /// </summary>
+        public MappingDataNode ShallowClone()
+        {
+            var newMapping = new MappingDataNode(_children.Count)
+            {
+                Tag = Tag,
+                Start = Start,
+                End = End
+            };
+
+            foreach (var (key, val) in _list)
+            {
+                newMapping.Add(key, val);
+            }
+
+            return newMapping;
+        }
+
         /// <summary>
         ///     Variant of <see cref="Except(MappingDataNode)"/> that will recursively call except rather than only checking equality.
         /// </summary>
@@ -396,5 +417,21 @@ public bool Remove(KeyValuePair<DataNode, DataNode> item)
 
         public int Count => _children.Count;
         public bool IsReadOnly => false;
+
+
+        public bool TryAdd(DataNode key, DataNode value)
+        {
+            return _children.TryAdd(key, value);
+        }
+
+        public bool TryAddCopy(DataNode key, DataNode value)
+        {
+            ref var entry = ref CollectionsMarshal.GetValueRefOrAddDefault(_children, key, out var exists);
+            if (exists)
+                return false;
+
+            entry = value.Copy();
+            return true;
+        }
     }
 }