feat: Allow generic NetworkBehaviour<T> subclasses (#3073)

* feat: Allow generic NetworkBehaviour subclasses

It's only generic SyncVars (via attribute) and rpcs/cmds we don't want to deal with and that aren't supported.
Even generic SyncVar<T> works

* Generate IL2CPP compatible base calls

see cf91e1d547

* Make SyncVar field/hook references generic too

Fixes bad IL

* Update Extensions.cs

* Update Assets/Mirror/Editor/Weaver/Extensions.cs

Co-authored-by: vis2k <info@noobtuts.com>

Assets/Mirror/Editor/Weaver/Extensions.cs

@@ -140,6 +140,18 @@ public static MethodReference MakeHostInstanceGeneric(this MethodReference self,
             return module.ImportReference(reference);
         }
 
+        // needed for NetworkBehaviour<T> support
+        // https://github.com/vis2k/Mirror/pull/3073/
+        public static FieldReference MakeHostInstanceGeneric(this FieldReference self)
+        {
+            var declaringType = new GenericInstanceType(self.DeclaringType);
+            foreach (var parameter in self.DeclaringType.GenericParameters)
+            {
+                declaringType.GenericArguments.Add(parameter);
+            }
+            return new FieldReference(self.Name, self.FieldType, declaringType);
+        }
+
         // Given a field of a generic class such as Writer<T>.write,
         // and a generic instance such as ArraySegment`int
         // Creates a reference to the specialized method  ArraySegment`int`.get_Count
@@ -251,5 +263,75 @@ public static AssemblyNameReference FindReference(this ModuleDefinition module,
             }
             return null;
         }
+
+        // Takes generic arguments from child class and applies them to parent reference, if possible
+        // eg makes `Base<T>` in Child<int> : Base<int> have `int` instead of `T`
+        // Originally by James-Frowen under MIT 
+        // https://github.com/MirageNet/Mirage/commit/cf91e1d54796866d2cf87f8e919bb5c681977e45
+        public static TypeReference ApplyGenericParameters(this TypeReference parentReference,
+            TypeReference childReference)
+        {
+            // If the parent is not generic, we got nothing to apply
+            if (!parentReference.IsGenericInstance)
+                return parentReference;
+
+            GenericInstanceType parentGeneric = (GenericInstanceType)parentReference;
+            // make new type so we can replace the args on it
+            // resolve it so we have non-generic instance (eg just instance with <T> instead of <int>)
+            // if we don't cecil will make it double generic (eg INVALID IL)
+            GenericInstanceType generic = new GenericInstanceType(parentReference.Resolve());
+            foreach (TypeReference arg in parentGeneric.GenericArguments)
+                generic.GenericArguments.Add(arg);
+
+            for (int i = 0; i < generic.GenericArguments.Count; i++)
+            {
+                // if arg is not generic
+                // eg List<int> would be int so not generic.
+                // But List<T> would be T so is generic
+                if (!generic.GenericArguments[i].IsGenericParameter)
+                    continue;
+
+                // get the generic name, eg T
+                string name = generic.GenericArguments[i].Name;
+                // find what type T is, eg turn it into `int` if `List<int>`
+                TypeReference arg = FindMatchingGenericArgument(childReference, name);
+
+                // import just to be safe
+                TypeReference imported = parentReference.Module.ImportReference(arg);
+                // set arg on generic, parent ref will be Base<int> instead of just Base<T>
+                generic.GenericArguments[i] = imported;
+            }
+
+            return generic;
+        }
+
+        // Finds the type reference for a generic parameter with the provided name in the child reference
+        // Originally by James-Frowen under MIT 
+        // https://github.com/MirageNet/Mirage/commit/cf91e1d54796866d2cf87f8e919bb5c681977e45
+        static TypeReference FindMatchingGenericArgument(TypeReference childReference, string paramName)
+        {
+            TypeDefinition def = childReference.Resolve();
+            // child class must be generic if we are in this part of the code
+            // eg Child<T> : Base<T>  <--- child must have generic if Base has T
+            // vs Child : Base<int> <--- wont be here if Base has int (we check if T exists before calling this)
+            if (!def.HasGenericParameters)
+                throw new InvalidOperationException(
+                    "Base class had generic parameters, but could not find them in child class");
+
+            // go through parameters in child class, and find the generic that matches the name
+            for (int i = 0; i < def.GenericParameters.Count; i++)
+            {
+                GenericParameter param = def.GenericParameters[i];
+                if (param.Name == paramName)
+                {
+                    GenericInstanceType generic = (GenericInstanceType)childReference;
+                    // return generic arg with same index
+                    return generic.GenericArguments[i];
+                }
+            }
+
+            // this should never happen, if it does it means that this code is bugged
+            throw new InvalidOperationException("Did not find matching generic");
+        }
     }
 }

Assets/Mirror/Editor/Weaver/Processors/NetworkBehaviourProcessor.cs

@@ -68,14 +68,6 @@ public bool Process(ref bool WeavingFailed)
                 return false;
             }
 
-            if (netBehaviourSubclass.HasGenericParameters)
-            {
-                Log.Error($"{netBehaviourSubclass.Name} cannot have generic parameters", netBehaviourSubclass);
-                WeavingFailed = true;
-                // originally Process returned true in every case, except if already processed.
-                // maybe return false here in the future.
-                return true;
-            }
             MarkAsProcessed(netBehaviourSubclass);
 
             // deconstruct tuple and set fields
@@ -437,8 +429,13 @@ void GenerateSerialization(ref bool WeavingFailed)
             worker.Emit(OpCodes.Ldarg_2);
             worker.Emit(OpCodes.Brfalse, initialStateLabel);
 
-            foreach (FieldDefinition syncVar in syncVars)
+            foreach (FieldDefinition syncVarDef in syncVars)
             {
+                FieldReference syncVar = syncVarDef;
+                if (netBehaviourSubclass.HasGenericParameters)
+                {
+                    syncVar = syncVarDef.MakeHostInstanceGeneric();
+                }
                 // Generates a writer call for each sync variable
                 // writer
                 worker.Emit(OpCodes.Ldarg_1);
@@ -481,8 +478,14 @@ void GenerateSerialization(ref bool WeavingFailed)
 
             // start at number of syncvars in parent
             int dirtyBit = syncVarAccessLists.GetSyncVarStart(netBehaviourSubclass.BaseType.FullName);
-            foreach (FieldDefinition syncVar in syncVars)
+            foreach (FieldDefinition syncVarDef in syncVars)
             {
+
+                FieldReference syncVar = syncVarDef;
+                if (netBehaviourSubclass.HasGenericParameters)
+                {
+                    syncVar = syncVarDef.MakeHostInstanceGeneric();
+                }
                 Instruction varLabel = worker.Create(OpCodes.Nop);
 
                 // Generates: if ((base.get_syncVarDirtyBits() & 1uL) != 0uL)
@@ -539,7 +542,15 @@ void DeserializeField(FieldDefinition syncVar, ILProcessor worker, ref bool Weav
 
             // push 'ref T this.field'
             worker.Emit(OpCodes.Ldarg_0);
-            worker.Emit(OpCodes.Ldflda, syncVar);
+            // if the netbehaviour class is generic, we need to make the field reference generic as well for correct IL
+            if (netBehaviourSubclass.HasGenericParameters)
+            {
+                worker.Emit(OpCodes.Ldflda, syncVar.MakeHostInstanceGeneric());
+            }
+            else
+            {
+                worker.Emit(OpCodes.Ldflda, syncVar);
+            }
 
             // hook? then push 'new Action<T,T>(Hook)' onto stack
             MethodDefinition hookMethod = syncVarAttributeProcessor.GetHookMethod(netBehaviourSubclass, syncVar, ref WeavingFailed);
@@ -821,6 +832,14 @@ bool ValidateParameters(MethodReference method, RemoteCallType callType, ref boo
         // validate parameters for a remote function call like Rpc/Cmd
         bool ValidateParameter(MethodReference method, ParameterDefinition param, RemoteCallType callType, bool firstParam, ref bool WeavingFailed)
         {
+            // need to check this before any type lookups since those will fail since generic types don't resolve
+            if (param.ParameterType.IsGenericParameter)
+            {
+                Log.Error($"{method.Name} cannot have generic parameters", method);
+                WeavingFailed = true;
+                return false;
+            }
+
             bool isNetworkConnection = param.ParameterType.Is<NetworkConnection>();
             bool isSenderConnection = IsSenderConnection(param, callType);
 

Assets/Mirror/Editor/Weaver/Processors/SyncObjectProcessor.cs

@@ -16,6 +16,12 @@ public static List<FieldDefinition> FindSyncObjectsFields(Writers writers, Reade
 
             foreach (FieldDefinition fd in td.Fields)
             {
+                if (fd.FieldType.IsGenericParameter)
+                {
+                    // can't call .Resolve on generic ones
+                    continue;
+                }
+
                 if (fd.FieldType.Resolve().IsDerivedFrom<SyncObject>())
                 {
                     if (fd.IsStatic)

Assets/Mirror/Editor/Weaver/Processors/SyncVarAttributeProcessor.cs

@@ -69,17 +69,28 @@ public void GenerateNewActionFromHookMethod(FieldDefinition syncVar, ILProcessor
                 worker.Emit(OpCodes.Ldarg_0);
             }
 
+            MethodReference hookMethodReference;
+            // if the network behaviour class is generic, we need to make the method reference generic for correct IL
+            if (hookMethod.DeclaringType.HasGenericParameters)
+            {
+                hookMethodReference = hookMethod.MakeHostInstanceGeneric(hookMethod.Module, hookMethod.DeclaringType.MakeGenericInstanceType(hookMethod.DeclaringType.GenericParameters.ToArray()));
+            }
+            else
+            {
+                hookMethodReference = hookMethod;
+            }
+
             // we support regular and virtual hook functions.
             if (hookMethod.IsVirtual)
             {
                 // for virtual / overwritten hooks, we need different IL.
                 // this is from simply testing Action = VirtualHook; in C#.
                 worker.Emit(OpCodes.Dup);
-                worker.Emit(OpCodes.Ldvirtftn, hookMethod);
+                worker.Emit(OpCodes.Ldvirtftn, hookMethodReference);
             }
             else
             {
-                worker.Emit(OpCodes.Ldftn, hookMethod);
+                worker.Emit(OpCodes.Ldftn, hookMethodReference);
             }
 
             // call 'new Action<T,T>()' constructor to convert the function to an action
@@ -143,6 +154,29 @@ public MethodDefinition GenerateSyncVarGetter(FieldDefinition fd, string origina
 
             ILProcessor worker = get.Body.GetILProcessor();
 
+            FieldReference fr;
+            if (fd.DeclaringType.HasGenericParameters)
+            {
+                fr = fd.MakeHostInstanceGeneric();
+            }
+            else
+            {
+                fr = fd;
+            }
+
+            FieldReference netIdFieldReference = null;
+            if (netFieldId != null)
+            {
+                if (netFieldId.DeclaringType.HasGenericParameters)
+                {
+                    netIdFieldReference = netFieldId.MakeHostInstanceGeneric();
+                }
+                else
+                {
+                    netIdFieldReference = netFieldId;
+                }
+            }
+
             // [SyncVar] GameObject?
             if (fd.FieldType.Is<UnityEngine.GameObject>())
             {
@@ -150,9 +184,9 @@ public MethodDefinition GenerateSyncVarGetter(FieldDefinition fd, string origina
                 // this.
                 worker.Emit(OpCodes.Ldarg_0);
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldfld, netFieldId);
+                worker.Emit(OpCodes.Ldfld, netIdFieldReference);
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldflda, fd);
+                worker.Emit(OpCodes.Ldflda, fr);
                 worker.Emit(OpCodes.Call, weaverTypes.getSyncVarGameObjectReference);
                 worker.Emit(OpCodes.Ret);
             }
@@ -163,9 +197,9 @@ public MethodDefinition GenerateSyncVarGetter(FieldDefinition fd, string origina
                 // this.
                 worker.Emit(OpCodes.Ldarg_0);
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldfld, netFieldId);
+                worker.Emit(OpCodes.Ldfld, netIdFieldReference);
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldflda, fd);
+                worker.Emit(OpCodes.Ldflda, fr);
                 worker.Emit(OpCodes.Call, weaverTypes.getSyncVarNetworkIdentityReference);
                 worker.Emit(OpCodes.Ret);
             }
@@ -175,9 +209,9 @@ public MethodDefinition GenerateSyncVarGetter(FieldDefinition fd, string origina
                 // this.
                 worker.Emit(OpCodes.Ldarg_0);
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldfld, netFieldId);
+                worker.Emit(OpCodes.Ldfld, netIdFieldReference);
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldflda, fd);
+                worker.Emit(OpCodes.Ldflda, fr);
                 MethodReference getFunc = weaverTypes.getSyncVarNetworkBehaviourReference.MakeGeneric(assembly.MainModule, fd.FieldType);
                 worker.Emit(OpCodes.Call, getFunc);
                 worker.Emit(OpCodes.Ret);
@@ -186,7 +220,7 @@ public MethodDefinition GenerateSyncVarGetter(FieldDefinition fd, string origina
             else
             {
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldfld, fd);
+                worker.Emit(OpCodes.Ldfld, fr);
                 worker.Emit(OpCodes.Ret);
             }
 
@@ -215,6 +249,28 @@ public MethodDefinition GenerateSyncVarSetter(TypeDefinition td, FieldDefinition
                     weaverTypes.Import(typeof(void)));
 
             ILProcessor worker = set.Body.GetILProcessor();
+            FieldReference fr;
+            if (fd.DeclaringType.HasGenericParameters)
+            {
+               fr = fd.MakeHostInstanceGeneric();
+            }
+            else
+            {
+                fr = fd;
+            }
+
+            FieldReference netIdFieldReference = null;
+            if (netFieldId != null)
+            {
+                if (netFieldId.DeclaringType.HasGenericParameters)
+                {
+                    netIdFieldReference = netFieldId.MakeHostInstanceGeneric();
+                }
+                else
+                {
+                  netIdFieldReference = netFieldId;
+                }
+            }
 
             // if (!SyncVarEqual(value, ref playerData))
             Instruction endOfMethod = worker.Create(OpCodes.Nop);
@@ -241,7 +297,7 @@ public MethodDefinition GenerateSyncVarSetter(TypeDefinition td, FieldDefinition
 
             // push 'ref T this.field'
             worker.Emit(OpCodes.Ldarg_0);
-            worker.Emit(OpCodes.Ldflda, fd);
+            worker.Emit(OpCodes.Ldflda, fr);
 
             // push the dirty bit for this SyncVar
             worker.Emit(OpCodes.Ldc_I8, dirtyBit);
@@ -265,14 +321,14 @@ public MethodDefinition GenerateSyncVarSetter(TypeDefinition td, FieldDefinition
             {
                 // GameObject setter needs one more parameter: netId field ref
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldflda, netFieldId);
+                worker.Emit(OpCodes.Ldflda, netIdFieldReference);
                 worker.Emit(OpCodes.Call, weaverTypes.generatedSyncVarSetter_GameObject);
             }
             else if (fd.FieldType.Is<NetworkIdentity>())
             {
                 // NetworkIdentity setter needs one more parameter: netId field ref
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldflda, netFieldId);
+                worker.Emit(OpCodes.Ldflda, netIdFieldReference);
                 worker.Emit(OpCodes.Call, weaverTypes.generatedSyncVarSetter_NetworkIdentity);
             }
             // TODO this only uses the persistent netId for types DERIVED FROM NB.
@@ -283,7 +339,7 @@ public MethodDefinition GenerateSyncVarSetter(TypeDefinition td, FieldDefinition
                 // NetworkIdentity setter needs one more parameter: netId field ref
                 // (actually its a NetworkBehaviourSyncVar type)
                 worker.Emit(OpCodes.Ldarg_0);
-                worker.Emit(OpCodes.Ldflda, netFieldId);
+                worker.Emit(OpCodes.Ldflda, netIdFieldReference);
                 // make generic version of GeneratedSyncVarSetter_NetworkBehaviour<T>
                 MethodReference getFunc = weaverTypes.generatedSyncVarSetter_NetworkBehaviour_T.MakeGeneric(assembly.MainModule, fd.FieldType);
                 worker.Emit(OpCodes.Call, getFunc);
@@ -315,16 +371,18 @@ public void ProcessSyncVar(TypeDefinition td, FieldDefinition fd, Dictionary<Fie
             if (fd.FieldType.IsDerivedFrom<NetworkBehaviour>())
             {
                 netIdField = new FieldDefinition($"___{fd.Name}NetId",
-                   FieldAttributes.Private,
+                   FieldAttributes.Family, // needs to be protected for generic classes, otherwise access isn't allowed
                    weaverTypes.Import<NetworkBehaviour.NetworkBehaviourSyncVar>());
+                netIdField.DeclaringType = td;
 
                 syncVarNetIds[fd] = netIdField;
             }
             else if (fd.FieldType.IsNetworkIdentityField())
             {
                 netIdField = new FieldDefinition($"___{fd.Name}NetId",
-                    FieldAttributes.Private,
+                    FieldAttributes.Family, // needs to be protected for generic classes, otherwise access isn't allowed
                     weaverTypes.Import<uint>());
+                netIdField.DeclaringType = td;
 
                 syncVarNetIds[fd] = netIdField;
             }
@@ -377,6 +435,13 @@ public void ProcessSyncVar(TypeDefinition td, FieldDefinition fd, Dictionary<Fie
                         continue;
                     }
 
+                    if (fd.FieldType.IsGenericParameter)
+                    {
+                        Log.Error($"{fd.Name} has generic type. Generic SyncVars are not supported", fd);
+                        WeavingFailed = true;
+                        continue;
+                    }
+
                     if (fd.FieldType.IsArray)
                     {
                         Log.Error($"{fd.Name} has invalid type. Use SyncLists instead of arrays", fd);

Assets/Mirror/Editor/Weaver/Resolvers.cs

@@ -48,16 +48,21 @@ public static MethodReference TryResolveMethodInParents(TypeReference tr, Assemb
             {
                 return null;
             }
-            foreach (MethodDefinition methodRef in tr.Resolve().Methods)
+            foreach (MethodDefinition methodDef in tr.Resolve().Methods)
             {
-                if (methodRef.Name == name)
+                if (methodDef.Name == name)
                 {
+                    MethodReference methodRef = methodDef;
+                    if (tr.IsGenericInstance)
+                    {
+                        methodRef = methodRef.MakeHostInstanceGeneric(tr.Module, (GenericInstanceType)tr);
+                    }
                     return assembly.MainModule.ImportReference(methodRef);
                 }
             }
 
             // Could not find the method in this class,  try the parent
-            return TryResolveMethodInParents(tr.Resolve().BaseType, assembly, name);
+            return TryResolveMethodInParents(tr.Resolve().BaseType.ApplyGenericParameters(tr), assembly, name);
         }
 
         public static MethodDefinition ResolveDefaultPublicCtor(TypeReference variable)

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

@@ -5,10 +5,17 @@ namespace Mirror.Weaver.Tests
     public class WeaverNetworkBehaviourTests : WeaverTestsBuildFromTestName
     {
         [Test]
-        public void NetworkBehaviourGeneric()
+        public void NetworkBehaviourGenericSyncVar()
         {
-            HasError("NetworkBehaviourGeneric`1 cannot have generic parameters",
-                "WeaverNetworkBehaviourTests.NetworkBehaviourGeneric.NetworkBehaviourGeneric`1");
+            HasError("genericSyncVarNotAllowed has generic type. Generic SyncVars are not supported",
+                "T WeaverNetworkBehaviourTests.NetworkBehaviourGeneric.NetworkBehaviourGeneric`1::genericSyncVarNotAllowed");
+        }
+
+        [Test]
+        public void NetworkBehaviourGenericRpc()
+        {
+            HasError("RpcGeneric cannot have generic parameters",
+                "System.Void WeaverNetworkBehaviourTests.NetworkBehaviourGeneric.NetworkBehaviourGeneric`1::RpcGeneric(T)");
         }
 
         [Test]

Assets/Mirror/Tests/Editor/Weaver/WeaverNetworkBehaviourTests_IsValid/NetworkBehaviourGeneric.cs

@@ -0,0 +1,16 @@
+using Mirror;
+
+namespace WeaverNetworkBehaviourTests.NetworkBehaviourGeneric
+{
+    class NetworkBehaviourGeneric<T> : NetworkBehaviour
+    {
+        public T param;
+        public readonly SyncVar<T> syncVar = new SyncVar<T>(default);
+        public readonly SyncList<T> syncList = new SyncList<T>();
+    }
+
+    class GenericImplInt : NetworkBehaviourGeneric<int>
+    {
+
+    }
+}

Assets/Mirror/Tests/Editor/Weaver/WeaverNetworkBehaviourTests_IsValid/NetworkBehaviourGeneric.cs.meta

@@ -0,0 +1,3 @@
+fileFormatVersion: 2
+guid: b279c2f37eaa4de09c1f15a5b80029b4
+timeCreated: 1643560588

Assets/Mirror/Tests/Editor/Weaver/WeaverNetworkBehaviourTests~/NetworkBehaviourGenericRpc.cs

@@ -4,6 +4,7 @@ namespace WeaverNetworkBehaviourTests.NetworkBehaviourGeneric
 {
     class NetworkBehaviourGeneric<T> : NetworkBehaviour
     {
-        T genericsNotAllowed;
+        [ClientRpc]
+        void RpcGeneric(T param) {}
     }
 }

Assets/Mirror/Tests/Editor/Weaver/WeaverNetworkBehaviourTests~/NetworkBehaviourGenericSyncVar.cs

@@ -0,0 +1,12 @@
+using Mirror;
+
+namespace WeaverNetworkBehaviourTests.NetworkBehaviourGeneric
+{
+    class NetworkBehaviourGeneric<T> : NetworkBehaviour
+    {
+        [SyncVar]
+        T genericSyncVarNotAllowed;
+
+        T genericTypeIsFine;
+    }
+}