fix: no longer requires hook to be the first overload in a class (#1913)

* test for sync var hook order

* adding extension method

* allowing hook method to be in any order

* adding second hard to test functions

Assets/Mirror/Editor/Weaver/Extensions.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using Mono.CecilX;
 
 namespace Mirror.Weaver
@@ -181,6 +182,17 @@ public static MethodDefinition GetMethod(this TypeDefinition td, string methodNa
             return null;
         }
 
+        public static List<MethodDefinition> GetMethods(this TypeDefinition td, string methodName)
+        {
+            List<MethodDefinition> methods = new List<MethodDefinition>();
+            foreach (MethodDefinition md in td.Methods)
+            {
+                if (md.Name == methodName)
+                    methods.Add(md);
+            }
+            return methods;
+        }
+
         /// <summary>
         /// 
         /// </summary>

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

@@ -1,4 +1,5 @@
 using System.Collections.Generic;
+using System.Linq;
 using Mono.CecilX;
 using Mono.CecilX.Cil;
 
@@ -29,42 +30,46 @@ public static MethodDefinition GetHookMethod(TypeDefinition td, FieldDefinition
             if (hookFunctionName == null)
                 return null;
 
-            return GetHookMethod(td, syncVar, hookFunctionName);
+            return FindHookMethod(td, syncVar, hookFunctionName);
         }
 
-        static MethodDefinition GetHookMethod(TypeDefinition td, FieldDefinition syncVar, string hookFunctionName)
+        static MethodDefinition FindHookMethod(TypeDefinition td, FieldDefinition syncVar, string hookFunctionName)
         {
-            MethodDefinition m = td.GetMethod(hookFunctionName);
-            if (m != null)
-            {
-                if (m.Parameters.Count == 2)
-                {
-                    if (m.Parameters[0].ParameterType != syncVar.FieldType ||
-                        m.Parameters[1].ParameterType != syncVar.FieldType)
-                    {
-                        Weaver.Error($"Wrong type for Parameter in hook for '{syncVar.Name}', hook name '{hookFunctionName}'. " +
-                            $"Method signature should be {HookParameterMessage(hookFunctionName, syncVar.FieldType)}",
-                             syncVar);
+            List<MethodDefinition> methods = td.GetMethods(hookFunctionName);
 
-                        return null;
-                    }
-                    return m;
-                }
+            List<MethodDefinition> methodsWith2Param = new List<MethodDefinition>(methods.Where(m => m.Parameters.Count == 2));
 
-                Weaver.Error($"Hook had wrong parameters for '{syncVar.Name}', hook name '{hookFunctionName}'. " +
-                  $"Method signature should be {HookParameterMessage(hookFunctionName, syncVar.FieldType)}",
-                  syncVar);
+            if (methodsWith2Param.Count == 0)
+            {
+                Weaver.Error($"Could not find hook for '{syncVar.Name}', hook name '{hookFunctionName}'. " +
+                    $"Method signature should be {HookParameterMessage(hookFunctionName, syncVar.FieldType)}",
+                    syncVar);
 
                 return null;
             }
 
-            Weaver.Error($"Could not find hook for '{syncVar.Name}', hook name '{hookFunctionName}'. " +
-                   $"Method signature should be {HookParameterMessage(hookFunctionName, syncVar.FieldType)}",
+            foreach (MethodDefinition method in methodsWith2Param)
+            {
+                if (MatchesParameters(syncVar, method))
+                {
+                    return method;
+                }
+            }
+
+            Weaver.Error($"Wrong type for Parameter in hook for '{syncVar.Name}', hook name '{hookFunctionName}'. " +
+                     $"Method signature should be {HookParameterMessage(hookFunctionName, syncVar.FieldType)}",
                    syncVar);
 
             return null;
         }
 
+        static bool MatchesParameters(FieldDefinition syncVar, MethodDefinition method)
+        {
+            // matches void onValueChange(T oldValue, T newValue)
+            return method.Parameters[0].ParameterType.FullName == syncVar.FieldType.FullName &&
+                   method.Parameters[1].ParameterType.FullName == syncVar.FieldType.FullName;
+        }
+
         public static MethodDefinition ProcessSyncVarGet(FieldDefinition fd, string originalName, FieldDefinition netFieldId)
         {
             //Create the get method

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

@@ -191,13 +191,14 @@
     <Compile Include="WeaverSyncSetTests~\SyncSetStruct.cs" />
     <Compile Include="WeaverSyncVarHookTests~\FindsHookWithGameObjects.cs" />
     <Compile Include="WeaverSyncVarHookTests~\FindsHookWithNetworkIdentity.cs" />
+    <Compile Include="WeaverSyncVarHookTests~\FindsHookWithOtherOverloadsInOrder.cs" />
+    <Compile Include="WeaverSyncVarHookTests~\FindsHookWithOtherOverloadsInReverseOrder.cs" />
     <Compile Include="WeaverSyncVarHookTests~\FindsPublicHook.cs" />
     <Compile Include="WeaverSyncVarHookTests~\FindsPrivateHook.cs" />
     <Compile Include="WeaverSyncVarHookTests~\ErrorForWrongTypeNewParameter.cs" />
     <Compile Include="WeaverSyncVarHookTests~\ErrorForWrongTypeOldParameter.cs" />
     <Compile Include="WeaverSyncVarHookTests~\ErrorWhenNoHookWithCorrectParametersFound.cs" />
     <Compile Include="WeaverSyncVarHookTests~\ErrorWhenNoHookFound.cs" />
-    <Compile Include="WeaverSyncVarHookTests~\FindsHookWhenOverMethodsWithSameNameExist.cs" />
     <Compile Include="WeaverSyncVarHookTests~\FindsStaticHook.cs" />
     <Compile Include="WeaverSyncVarTests~\SyncVarsCantBeArray.cs" />
     <Compile Include="WeaverSyncVarTests~\SyncVarsDerivedNetworkBehaviour.cs" />

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

@@ -35,7 +35,13 @@ public void FindsHookWithNetworkIdentity()
         }
 
         [Test]
-        public void FindsHookWhenOverMethodsWithSameNameExist()
+        public void FindsHookWithOtherOverloadsInOrder()
+        {
+            Assert.That(weaverErrors, Is.Empty);
+        }
+
+        [Test]
+        public void FindsHookWithOtherOverloadsInReverseOrder()
         {
             Assert.That(weaverErrors, Is.Empty);
         }
@@ -56,7 +62,7 @@ public void ErrorWhenNoHookFound()
         [Test]
         public void ErrorWhenNoHookWithCorrectParametersFound()
         {
-            Assert.That(weaverErrors, Contains.Item($"Hook had wrong parameters for 'health', hook name 'onChangeHealth'. " +
+            Assert.That(weaverErrors, Contains.Item($"Could not find hook for 'health', hook name 'onChangeHealth'. " +
                 $"Method signature should be {OldNewMethodFormat("onChangeHealth", "System.Int32")} " +
                 $"(at System.Int32 WeaverSyncVarHookTests.ErrorWhenNoHookWithCorrectParametersFound.ErrorWhenNoHookWithCorrectParametersFound::health)"));
         }

Assets/Mirror/Tests/Editor/Weaver/WeaverSyncVarHookTests~/FindsHookWithOtherOverloadsInOrder.cs

@@ -0,0 +1,21 @@
+using Mirror;
+using UnityEngine;
+
+namespace WeaverSyncVarHookTests.FindsHookWithOtherOverloadsInOrder
+{
+    class FindsHookWithOtherOverloadsInOrder : NetworkBehaviour
+    {
+        [SyncVar(hook = nameof(onChangeHealth))]
+        int health;
+
+        void onChangeHealth(int oldValue, int newValue)
+        {
+
+        }
+
+        void onChangeHealth(Vector3 anotherValue, bool secondArg)
+        {
+
+        }
+    }
+}

Assets/Mirror/Tests/Editor/Weaver/WeaverSyncVarHookTests~/FindsHookWithOtherOverloadsInReverseOrder.cs

@@ -0,0 +1,21 @@
+using Mirror;
+using UnityEngine;
+
+namespace WeaverSyncVarHookTests.FindsHookWithOtherOverloadsInReverseOrder
+{
+    class FindsHookWithOtherOverloadsInReverseOrder : NetworkBehaviour
+    {
+        [SyncVar(hook = nameof(onChangeHealth))]
+        int health;
+
+        void onChangeHealth(Vector3 anotherValue, bool secondArg)
+        {
+
+        }
+
+        void onChangeHealth(int oldValue, int newValue)
+        {
+
+        }
+    }
+}