fix: making weaver include public fields in base classes in auto generated Read/Write (#1977)

* adding test to check if data is sent

* test for generating writer for inherited class

* adding extension method for FindAllPublicFields

Assets/Mirror/Editor/Weaver/Extensions.cs

@@ -253,5 +253,46 @@ public static bool HasMethodInBaseType(this TypeDefinition td, string methodName
 
             return false;
         }
+
+        /// <summary>
+        /// Finds public fields in type and base type
+        /// </summary>
+        /// <param name="variable"></param>
+        /// <returns></returns>
+        public static IEnumerable<FieldDefinition> FindAllPublicFields(this TypeReference variable)
+        {
+            return FindAllPublicFields(variable.Resolve());
+        }
+
+        /// <summary>
+        /// Finds public fields in type and base type
+        /// </summary>
+        /// <param name="variable"></param>
+        /// <returns></returns>
+        public static IEnumerable<FieldDefinition> FindAllPublicFields(this TypeDefinition typeDefinition)
+        {
+            while (typeDefinition != null)
+            {
+                foreach (FieldDefinition field in typeDefinition.Fields)
+                {
+                    if (field.IsStatic || field.IsPrivate)
+                        continue;
+
+                    if (field.IsNotSerialized)
+                        continue;
+
+                    yield return field;
+                }
+
+                try
+                {
+                    typeDefinition = typeDefinition.BaseType.Resolve();
+                }
+                catch (System.Exception e)
+                {
+                    break;
+                }
+            }
+        }
     }
 }

Assets/Mirror/Editor/Weaver/Readers.cs

@@ -332,7 +332,7 @@ static MethodDefinition GenerateClassOrStructReadFunction(TypeReference variable
             TypeDefinition td = variable.Resolve();
 
             CreateNew(variable, worker, td);
-            DeserializeFields(variable, recursionCount, worker);
+            ReadAllFields(variable, recursionCount, worker);
 
             worker.Append(worker.Create(OpCodes.Ldloc_0));
             worker.Append(worker.Create(OpCodes.Ret));
@@ -373,17 +373,11 @@ static void CreateNew(TypeReference variable, ILProcessor worker, TypeDefinition
             }
         }
 
-        static void DeserializeFields(TypeReference variable, int recursionCount, ILProcessor worker)
+        static void ReadAllFields(TypeReference variable, int recursionCount, ILProcessor worker)
         {
             uint fields = 0;
-            foreach (FieldDefinition field in variable.Resolve().Fields)
+            foreach (FieldDefinition field in variable.FindAllPublicFields())
             {
-                if (field.IsStatic || field.IsPrivate)
-                    continue;
-
-                if (field.IsNotSerialized)
-                    continue;
-
                 // mismatched ldloca/ldloc for struct/class combinations is invalid IL, which causes crash at runtime
                 OpCode opcode = variable.IsValueType ? OpCodes.Ldloca : OpCodes.Ldloc;
                 worker.Append(worker.Create(opcode, 0));
@@ -403,6 +397,7 @@ static void DeserializeFields(TypeReference variable, int recursionCount, ILProc
                 worker.Append(worker.Create(OpCodes.Stfld, fieldRef));
                 fields++;
             }
+
             if (fields == 0)
             {
                 Log.Warning($"{variable} has no public or non-static fields to deserialize");

Assets/Mirror/Editor/Weaver/Writers.cs

@@ -139,15 +139,25 @@ static MethodDefinition GenerateClassOrStructWriterFunction(TypeReference variab
 
             ILProcessor worker = writerFunc.Body.GetILProcessor();
 
+            if (!WriteAllFields(variable, recursionCount, worker))
+                return null;
+
+            worker.Append(worker.Create(OpCodes.Ret));
+            return writerFunc;
+        }
+
+        /// <summary>
+        /// Fiends all fields in 
+        /// </summary>
+        /// <param name="variable"></param>
+        /// <param name="recursionCount"></param>
+        /// <param name="worker"></param>
+        /// <returns>false if fail</returns>
+        static bool WriteAllFields(TypeReference variable, int recursionCount, ILProcessor worker)
+        {
             uint fields = 0;
-            foreach (FieldDefinition field in variable.Resolve().Fields)
+            foreach (FieldDefinition field in variable.FindAllPublicFields())
             {
-                if (field.IsStatic || field.IsPrivate)
-                    continue;
-
-                if (field.IsNotSerialized)
-                    continue;
-
                 MethodReference writeFunc = GetWriteFunc(field.FieldType, recursionCount + 1);
                 if (writeFunc != null)
                 {
@@ -162,20 +172,20 @@ static MethodDefinition GenerateClassOrStructWriterFunction(TypeReference variab
                 else
                 {
                     Weaver.Error($"{field.Name} has unsupported type. Use a type supported by Mirror instead", field);
-                    return null;
+                    return false;
                 }
             }
+
             if (fields == 0)
             {
-                Log.Warning($" {variable} has no no public or non-static fields to serialize");
+                Log.Warning($"{variable} has no no public or non-static fields to serialize");
             }
-            worker.Append(worker.Create(OpCodes.Ret));
-            return writerFunc;
+
+            return true;
         }
 
         static MethodDefinition GenerateArrayWriteFunc(TypeReference variable, int recursionCount)
         {
-
             if (!variable.IsArrayType())
             {
                 Weaver.Error($"{variable.Name} is an unsupported type. Jagged and multidimensional arrays are not supported", variable);

Assets/Mirror/Tests/Editor/FieldsInBaseClasses.cs

@@ -0,0 +1,53 @@
+using System;
+using Mirror.Tests.RemoteAttrributeTest;
+using NUnit.Framework;
+
+namespace Mirror.Tests.GeneratedWriterTests
+{
+    public class BaseData
+    {
+        public bool toggle;
+    }
+    public class SomeOtherData : BaseData
+    {
+        public int usefulNumber;
+    }
+
+    public class DataSenderBehaviour : NetworkBehaviour
+    {
+        public event Action<SomeOtherData> onData;
+        [Command]
+        public void CmdSendData(SomeOtherData otherData)
+        {
+            onData?.Invoke(otherData);
+        }
+    }
+
+    public class FieldsInBaseClasses : RemoteTestBase
+    {
+        [Test]
+        public void WriterShouldIncludeFieldsInBaseClass()
+        {
+            DataSenderBehaviour hostBehaviour = CreateHostObject<DataSenderBehaviour>(true);
+
+            const bool toggle = true;
+            const int usefulNumber = 10;
+
+            int callCount = 0;
+            hostBehaviour.onData += data =>
+            {
+                callCount++;
+                Assert.That(data.usefulNumber, Is.EqualTo(usefulNumber));
+                Assert.That(data.toggle, Is.EqualTo(toggle));
+            };
+            hostBehaviour.CmdSendData(new SomeOtherData
+            {
+                usefulNumber = usefulNumber,
+                toggle = toggle
+            });
+
+            ProcessMessages();
+            Assert.That(callCount, Is.EqualTo(1));
+        }
+    }
+}

Assets/Mirror/Tests/Editor/FieldsInBaseClasses.cs.meta

@@ -0,0 +1,11 @@
+fileFormatVersion: 2
+guid: 518288ffe7c7215458a98466131fc7af
+MonoImporter:
+  externalObjects: {}
+  serializedVersion: 2
+  defaultReferences: []
+  executionOrder: 0
+  icon: {instanceID: 0}
+  userData: 
+  assetBundleName: 
+  assetBundleVariant: 

Assets/Mirror/Tests/Editor/Weaver/.WeaverTests.csproj

@@ -67,6 +67,7 @@
     <Compile Include="GeneratedReaderWriter~\CreatesForClass.cs" />
     <Compile Include="GeneratedReaderWriter~\CreatesForClassFromDifferentAssemblies.cs" />
     <Compile Include="GeneratedReaderWriter~\CreatesForClassFromDifferentAssembliesWithValidConstructor.cs" />
+    <Compile Include="GeneratedReaderWriter~\CreatesForClassInherited.cs" />
     <Compile Include="GeneratedReaderWriter~\CreatesForClassWithValidConstructor.cs" />
     <Compile Include="GeneratedReaderWriter~\CreatesForEnums.cs" />
     <Compile Include="GeneratedReaderWriter~\CreatesForInheritedFromScriptableObject.cs" />

Assets/Mirror/Tests/Editor/Weaver/GeneratedReaderWriter~/CreatesForClassInherited.cs

@@ -0,0 +1,23 @@
+using Mirror;
+
+
+namespace GeneratedReaderWriter.CreatesForClassInherited
+{
+    public class CreatesForClassInherited : NetworkBehaviour
+    {
+        [ClientRpc]
+        public void RpcDoSomething(SomeOtherData data)
+        {
+            // empty
+        }
+    }
+
+    public class BaseData
+    {
+        public bool yes;
+    }
+    public class SomeOtherData : BaseData
+    {
+        public int usefulNumber;
+    }
+}

Assets/Mirror/Tests/Editor/Weaver/WeaverGeneratedReaderWriterTests.cs

@@ -30,6 +30,12 @@ public void CreatesForClass()
             Assert.That(weaverErrors, Is.Empty);
         }
 
+        [Test]
+        public void CreatesForClassInherited()
+        {
+            Assert.That(weaverErrors, Is.Empty);
+        }
+
         [Test]
         public void CreatesForClassWithValidConstructor()
         {