fix: adding error when Server/Client is used on abstract methods (#1978)

* tests for Server attribute on virtual methods

test for virtual, abstract and override methods

* adding test for client attribute

* adding error when attribute is put on abstract method

* improving error message

* updating error mesages in tests

* updating to use WeaverTypes

* adding check before error

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

@@ -60,6 +60,15 @@ static void ProcessSiteMethod(TypeDefinition td, MethodDefinition md)
                 md.Name.StartsWith(Weaver.InvokeRpcPrefix))
                 return;
 
+            if (md.IsAbstract)
+            {
+                if (ServerClientAttributeProcessor.HasServerClientAttribute(md))
+                {
+                    Weaver.Error("Server or Client Attributes can't be added to abstract method. Server and Client Attributes are not inherited so they need to be applied to the override methods instead.", md);
+                }
+                return;
+            }
+
             if (md.Body != null && md.Body.Instructions != null)
             {
                 // TODO move this to NetworkBehaviourProcessor

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

@@ -8,6 +8,24 @@ namespace Mirror.Weaver
     /// </summary>
     static class ServerClientAttributeProcessor
     {
+        public static bool HasServerClientAttribute(MethodDefinition md)
+        {
+            foreach (CustomAttribute attr in md.CustomAttributes)
+            {
+                switch (attr.Constructor.DeclaringType.ToString())
+                {
+                    case "Mirror.ServerAttribute":
+                    case "Mirror.ServerCallbackAttribute":
+                    case "Mirror.ClientAttribute":
+                    case "Mirror.ClientCallbackAttribute":
+                        return true;
+                    default:
+                        break;
+                }
+            }
+            return false;
+        }
+
         public static void ProcessMethodAttributes(TypeDefinition td, MethodDefinition md)
         {
             foreach (CustomAttribute attr in md.CustomAttributes)

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

@@ -88,8 +88,14 @@
     <Compile Include="WeaverClientRpcTests~\OverrideAbstractClientRpc.cs" />
     <Compile Include="WeaverClientRpcTests~\OverrideVirtualClientRpc.cs" />
     <Compile Include="WeaverClientRpcTests~\VirtualClientRpc.cs" />
+    <Compile Include="WeaverClientServerAttributeTests~\ClientAttributeOnAbstractMethod.cs" />
+    <Compile Include="WeaverClientServerAttributeTests~\ClientAttributeOnOverrideMethod.cs" />
+    <Compile Include="WeaverClientServerAttributeTests~\ClientAttributeOnVirutalMethod.cs" />
     <Compile Include="WeaverClientServerAttributeTests~\NetworkBehaviourClient.cs" />
     <Compile Include="WeaverClientServerAttributeTests~\NetworkBehaviourServer.cs" />
+    <Compile Include="WeaverClientServerAttributeTests~\ServerAttributeOnAbstractMethod.cs" />
+    <Compile Include="WeaverClientServerAttributeTests~\ServerAttributeOnOverrideMethod.cs" />
+    <Compile Include="WeaverClientServerAttributeTests~\ServerAttributeOnVirutalMethod.cs" />
     <Compile Include="WeaverCommandTests~\AbstractCommand.cs" />
     <Compile Include="WeaverCommandTests~\CommandCantBeStatic.cs" />
     <Compile Include="WeaverCommandTests~\CommandThatIgnoresAuthority.cs" />

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

@@ -11,22 +11,69 @@ public class WeaverClientServerAttributeTests : WeaverTestsBuildFromTestName
         [Test]
         public void NetworkBehaviourServer()
         {
-            Assert.That(CompilationFinishedHook.WeaveFailed, Is.False);
             Assert.That(weaverErrors, Is.Empty);
+
             string networkServerGetActive = WeaverTypes.NetworkServerGetActive.ToString();
             CheckAddedCode(networkServerGetActive, "WeaverClientServerAttributeTests.NetworkBehaviourServer.NetworkBehaviourServer", "ServerOnlyMethod");
+        }
+
+        [Test]
+        public void ServerAttributeOnVirutalMethod()
+        {
+            Assert.That(weaverErrors, Is.Empty);
 
+            string networkServerGetActive = WeaverTypes.NetworkServerGetActive.ToString();
+            CheckAddedCode(networkServerGetActive, "WeaverClientServerAttributeTests.ServerAttributeOnVirutalMethod.ServerAttributeOnVirutalMethod", "ServerOnlyMethod");
+        }
+
+        [Test]
+        public void ServerAttributeOnAbstractMethod()
+        {
+            Assert.That(weaverErrors, Contains.Item("Server or Client Attributes can't be added to abstract method. Server and Client Attributes are not inherited so they need to be applied to the override methods instead. (at System.Void WeaverClientServerAttributeTests.ServerAttributeOnAbstractMethod.ServerAttributeOnAbstractMethod::ServerOnlyMethod())"));
+        }
+
+        [Test]
+        public void ServerAttributeOnOverrideMethod()
+        {
+            Assert.That(weaverErrors, Is.Empty);
+
+            string networkServerGetActive = WeaverTypes.NetworkServerGetActive.ToString();
+            CheckAddedCode(networkServerGetActive, "WeaverClientServerAttributeTests.ServerAttributeOnOverrideMethod.ServerAttributeOnOverrideMethod", "ServerOnlyMethod");
         }
 
         [Test]
         public void NetworkBehaviourClient()
         {
-            Assert.That(CompilationFinishedHook.WeaveFailed, Is.False);
             Assert.That(weaverErrors, Is.Empty);
+
             string networkClientGetActive = WeaverTypes.NetworkClientGetActive.ToString();
             CheckAddedCode(networkClientGetActive, "WeaverClientServerAttributeTests.NetworkBehaviourClient.NetworkBehaviourClient", "ClientOnlyMethod");
         }
 
+        [Test]
+        public void ClientAttributeOnVirutalMethod()
+        {
+            Assert.That(weaverErrors, Is.Empty);
+
+            string networkClientGetActive = WeaverTypes.NetworkClientGetActive.ToString();
+            CheckAddedCode(networkClientGetActive, "WeaverClientServerAttributeTests.ClientAttributeOnVirutalMethod.ClientAttributeOnVirutalMethod", "ClientOnlyMethod");
+        }
+
+        [Test]
+        public void ClientAttributeOnAbstractMethod()
+        {
+            Assert.That(weaverErrors, Contains.Item("Server or Client Attributes can't be added to abstract method. Server and Client Attributes are not inherited so they need to be applied to the override methods instead. (at System.Void WeaverClientServerAttributeTests.ClientAttributeOnAbstractMethod.ClientAttributeOnAbstractMethod::ClientOnlyMethod())"));
+        }
+
+        [Test]
+        public void ClientAttributeOnOverrideMethod()
+        {
+            Assert.That(weaverErrors, Is.Empty);
+
+            string networkClientGetActive = WeaverTypes.NetworkClientGetActive.ToString();
+            CheckAddedCode(networkClientGetActive, "WeaverClientServerAttributeTests.ClientAttributeOnOverrideMethod.ClientAttributeOnOverrideMethod", "ClientOnlyMethod");
+        }
+
         /// <summary>
         /// Checks that first Instructions in MethodBody is addedString
         /// </summary>

Assets/Mirror/Tests/Editor/Weaver/WeaverClientServerAttributeTests~/ClientAttributeOnAbstractMethod.cs

@@ -0,0 +1,10 @@
+using Mirror;
+
+namespace WeaverClientServerAttributeTests.ClientAttributeOnAbstractMethod
+{
+    abstract class ClientAttributeOnAbstractMethod : NetworkBehaviour
+    {
+        [Client]
+        protected abstract void ClientOnlyMethod();
+    }
+}

Assets/Mirror/Tests/Editor/Weaver/WeaverClientServerAttributeTests~/ClientAttributeOnOverrideMethod.cs

@@ -0,0 +1,21 @@
+using Mirror;
+
+namespace WeaverClientServerAttributeTests.ClientAttributeOnOverrideMethod
+{
+    class ClientAttributeOnOverrideMethod : BaseClass
+    {
+        [Client]
+        protected override void ClientOnlyMethod()
+        {
+            // test method
+        }
+    }
+
+    class BaseClass : NetworkBehaviour
+    {
+        protected virtual void ClientOnlyMethod()
+        {
+            // test method
+        }
+    }
+}

Assets/Mirror/Tests/Editor/Weaver/WeaverClientServerAttributeTests~/ClientAttributeOnVirutalMethod.cs

@@ -0,0 +1,13 @@
+using Mirror;
+
+namespace WeaverClientServerAttributeTests.ClientAttributeOnVirutalMethod
+{
+    class ClientAttributeOnVirutalMethod : NetworkBehaviour
+    {
+        [Client]
+        protected virtual void ClientOnlyMethod()
+        {
+            // test method
+        }
+    }
+}

Assets/Mirror/Tests/Editor/Weaver/WeaverClientServerAttributeTests~/ServerAttributeOnAbstractMethod.cs

@@ -0,0 +1,10 @@
+using Mirror;
+
+namespace WeaverClientServerAttributeTests.ServerAttributeOnAbstractMethod
+{
+    abstract class ServerAttributeOnAbstractMethod : NetworkBehaviour
+    {
+        [Server]
+        protected abstract void ServerOnlyMethod();
+    }
+}

Assets/Mirror/Tests/Editor/Weaver/WeaverClientServerAttributeTests~/ServerAttributeOnOverrideMethod.cs

@@ -0,0 +1,21 @@
+using Mirror;
+
+namespace WeaverClientServerAttributeTests.ServerAttributeOnOverrideMethod
+{
+    class ServerAttributeOnOverrideMethod : BaseClass
+    {
+        [Server]
+        protected override void ServerOnlyMethod()
+        {
+            // test method
+        }
+    }
+
+    class BaseClass : NetworkBehaviour
+    {
+        protected virtual void ServerOnlyMethod()
+        {
+            // test method
+        }
+    }
+}

Assets/Mirror/Tests/Editor/Weaver/WeaverClientServerAttributeTests~/ServerAttributeOnVirutalMethod.cs

@@ -0,0 +1,13 @@
+using Mirror;
+
+namespace WeaverClientServerAttributeTests.ServerAttributeOnVirutalMethod
+{
+    class ServerAttributeOnVirutalMethod : NetworkBehaviour
+    {
+        [Server]
+        protected virtual void ServerOnlyMethod()
+        {
+            // test method
+        }
+    }
+}