From 8dbf46720e5b8fb9a0bd06af36e8bd445e772332 Mon Sep 17 00:00:00 2001 From: Paul Pacheco Date: Tue, 29 Sep 2020 09:39:17 -0500 Subject: [PATCH] fix: generic arguments resolution (#2300) * fix: generic arguments lookup The weaver was not able to figure out the synclist type in code like this: ```cs public class SomeList : SyncList { } public class SomeListInt : SomeList { } ``` This code fixes that problem, and greatly reduces the complexity of argument lookup. * linting --- Assets/Mirror/Editor/Weaver/Extensions.cs | 4 +- .../Processors/GenericArgumentResolver.cs | 117 +++++------------- .../Processors/SyncDictionaryProcessor.cs | 19 +-- .../Weaver/Processors/SyncListProcessor.cs | 10 +- .../Weaver/Processors/SyncObjectProcessor.cs | 7 +- Assets/Mirror/Editor/Weaver/Weaver.cs | 4 +- .../Editor/Weaver/WeaverSyncListTests.cs | 3 +- ...stGenericInheritanceWithMultipleGeneric.cs | 5 +- 8 files changed, 52 insertions(+), 117 deletions(-) diff --git a/Assets/Mirror/Editor/Weaver/Extensions.cs b/Assets/Mirror/Editor/Weaver/Extensions.cs index 083d0c5c828..ed4c14a0c21 100644 --- a/Assets/Mirror/Editor/Weaver/Extensions.cs +++ b/Assets/Mirror/Editor/Weaver/Extensions.cs @@ -249,12 +249,12 @@ public static MethodDefinition GetMethodInBaseType(this TypeDefinition td, strin /// /// /// - public static bool HasMethodInBaseType(this TypeDefinition td, string methodName, TypeReference stopAt) + public static bool HasMethodInBaseType(this TypeDefinition td, string methodName, Type stopAt) { TypeDefinition typedef = td; while (typedef != null) { - if (typedef.FullName == stopAt.FullName) + if (typedef.Is(stopAt)) break; foreach (MethodDefinition md in typedef.Methods) diff --git a/Assets/Mirror/Editor/Weaver/Processors/GenericArgumentResolver.cs b/Assets/Mirror/Editor/Weaver/Processors/GenericArgumentResolver.cs index 1336f6555b9..23216b32c02 100644 --- a/Assets/Mirror/Editor/Weaver/Processors/GenericArgumentResolver.cs +++ b/Assets/Mirror/Editor/Weaver/Processors/GenericArgumentResolver.cs @@ -1,107 +1,56 @@ -using System.Collections.Generic; +using System; using Mono.CecilX; namespace Mirror.Weaver { - public class GenericArgumentResolver + public static class GenericArgumentResolver { - readonly Stack stack = new Stack(); - readonly int maxGenericArgument; - - public GenericArgumentResolver(int maxGenericArgument) + static TypeReference[] GetGenericArguments(TypeReference tr, Type baseType, TypeReference[] genericArguments) { - this.maxGenericArgument = maxGenericArgument; - } + if (tr == null) + return null; - public bool GetGenericFromBaseClass(TypeDefinition td, int genericArgument, TypeReference baseType, out TypeReference itemType) - { - itemType = null; - if (GetGenericBaseType(td, baseType, out GenericInstanceType parent)) - { - TypeReference arg = parent.GenericArguments[genericArgument]; - if (arg.IsGenericParameter) - { - itemType = FindParameterInStack(td, genericArgument); - } - else - { - itemType = Weaver.CurrentAssembly.MainModule.ImportReference(arg); - } - } + TypeReference[] resolvedArguments = new TypeReference[0]; - return itemType != null; - } - - TypeReference FindParameterInStack(TypeDefinition td, int genericArgument) - { - while (stack.Count > 0) + if (tr is GenericInstanceType genericInstance) { - TypeReference next = stack.Pop(); + // this type is a generic instance, for example List + // however, the parameter may be generic itself, for example: + // List. If T is a generic parameter, then look it up in + // from the arguments we got. + resolvedArguments = genericInstance.GenericArguments.ToArray(); - if (!(next is GenericInstanceType genericType)) + for (int i = 0; i< resolvedArguments.Length; i++) { - // if type is not GenericInstanceType something has gone wrong - return null; + TypeReference argument = resolvedArguments[i]; + if (argument is GenericParameter genericArgument) + { + resolvedArguments[i] = genericArguments[genericArgument.Position]; + } } + } - if (genericType.GenericArguments.Count < genericArgument) - { - // if less than `genericArgument` then we didnt find generic argument - return null; - } + if (tr.Is(baseType)) + return resolvedArguments; - if (genericType.GenericArguments.Count > maxGenericArgument) - { - // if greater than `genericArgument` it is hard to know which generic arg we want - // See SyncListGenericInheritanceWithMultipleGeneric test - Weaver.Error($"Type {td.Name} has too many generic arguments in base class {next}", td); - return null; - } + if (tr.CanBeResolved()) + return GetGenericArguments(tr.Resolve().BaseType, baseType, resolvedArguments); - TypeReference genericArg = genericType.GenericArguments[genericArgument]; - if (!genericArg.IsGenericParameter) - { - // if not generic, successfully found type - return Weaver.CurrentAssembly.MainModule.ImportReference(genericArg); - } - } - - // nothing left in stack, something went wrong return null; } - bool GetGenericBaseType(TypeDefinition td, TypeReference baseType, out GenericInstanceType found) + /// + /// Find out what the arguments are to a generic base type + /// + /// + /// + /// + public static TypeReference[] GetGenericArguments(TypeDefinition td, Type baseType) { - stack.Clear(); - TypeReference parent = td.BaseType; - found = null; - - while (parent != null) - { - string parentName = parent.FullName; - - // strip generic parameters from class name (if any) - parentName = Extensions.StripGenericParametersFromClassName(parentName); - - if (parentName == baseType.FullName) - { - found = parent as GenericInstanceType; - break; - } - - try - { - stack.Push(parent); - parent = parent.Resolve().BaseType; - } - catch (AssemblyResolutionException) - { - // this can happen for plugins. - break; - } - } + if (td == null) + return null; - return found != null; + return GetGenericArguments(td.BaseType, baseType, new TypeReference[] { }); } } } diff --git a/Assets/Mirror/Editor/Weaver/Processors/SyncDictionaryProcessor.cs b/Assets/Mirror/Editor/Weaver/Processors/SyncDictionaryProcessor.cs index 1a85484fb21..f9f07cc0dda 100644 --- a/Assets/Mirror/Editor/Weaver/Processors/SyncDictionaryProcessor.cs +++ b/Assets/Mirror/Editor/Weaver/Processors/SyncDictionaryProcessor.cs @@ -13,22 +13,11 @@ static class SyncDictionaryProcessor /// The synclist class public static void Process(TypeDefinition td) { - GenericArgumentResolver resolver = new GenericArgumentResolver(2); - TypeReference syncDictionaryType = WeaverTypes.Import(typeof(SyncDictionary<,>)); - - if (resolver.GetGenericFromBaseClass(td, 0, syncDictionaryType, out TypeReference keyType)) - { - SyncObjectProcessor.GenerateSerialization(td, keyType, syncDictionaryType, "SerializeKey", "DeserializeKey"); - } - else - { - Weaver.Error($"Could not find generic arguments for SyncDictionary in {td.Name}", td); - return; - } - - if (resolver.GetGenericFromBaseClass(td, 1, syncDictionaryType, out TypeReference itemType)) + TypeReference []arguments = GenericArgumentResolver.GetGenericArguments(td, typeof(SyncDictionary<,>)); + if (arguments != null) { - SyncObjectProcessor.GenerateSerialization(td, itemType, syncDictionaryType, "SerializeItem", "DeserializeItem"); + SyncObjectProcessor.GenerateSerialization(td, arguments[0], typeof(SyncDictionary<,>), "SerializeKey", "DeserializeKey"); + SyncObjectProcessor.GenerateSerialization(td, arguments[1], typeof(SyncDictionary<,>), "SerializeItem", "DeserializeItem"); } else { diff --git a/Assets/Mirror/Editor/Weaver/Processors/SyncListProcessor.cs b/Assets/Mirror/Editor/Weaver/Processors/SyncListProcessor.cs index 67af00e3886..cebfc8fb8d2 100644 --- a/Assets/Mirror/Editor/Weaver/Processors/SyncListProcessor.cs +++ b/Assets/Mirror/Editor/Weaver/Processors/SyncListProcessor.cs @@ -1,3 +1,4 @@ +using System; using Mono.CecilX; namespace Mirror.Weaver @@ -12,13 +13,12 @@ static class SyncListProcessor /// /// The synclist class /// the base SyncObject td inherits from - public static void Process(TypeDefinition td, TypeReference mirrorBaseType) + public static void Process(TypeDefinition td, Type mirrorBaseType) { - GenericArgumentResolver resolver = new GenericArgumentResolver(1); - - if (resolver.GetGenericFromBaseClass(td, 0, mirrorBaseType, out TypeReference itemType)) + TypeReference [] arguments = GenericArgumentResolver.GetGenericArguments(td, mirrorBaseType); + if (arguments != null) { - SyncObjectProcessor.GenerateSerialization(td, itemType, mirrorBaseType, "SerializeItem", "DeserializeItem"); + SyncObjectProcessor.GenerateSerialization(td, arguments[0], mirrorBaseType, "SerializeItem", "DeserializeItem"); } else { diff --git a/Assets/Mirror/Editor/Weaver/Processors/SyncObjectProcessor.cs b/Assets/Mirror/Editor/Weaver/Processors/SyncObjectProcessor.cs index 8a06536ed16..b0bbcedbd8f 100644 --- a/Assets/Mirror/Editor/Weaver/Processors/SyncObjectProcessor.cs +++ b/Assets/Mirror/Editor/Weaver/Processors/SyncObjectProcessor.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using Mono.CecilX; using Mono.CecilX.Cil; @@ -48,7 +49,7 @@ public static List FindSyncObjectsFields(TypeDefinition td) /// the base SyncObject td inherits from /// The name of the serialize method /// The name of the deserialize method - public static void GenerateSerialization(TypeDefinition td, TypeReference itemType, TypeReference mirrorBaseType, string serializeMethod, string deserializeMethod) + public static void GenerateSerialization(TypeDefinition td, TypeReference itemType, Type mirrorBaseType, string serializeMethod, string deserializeMethod) { Weaver.DLog(td, "SyncObjectProcessor Start item:" + itemType.FullName); @@ -65,7 +66,7 @@ public static void GenerateSerialization(TypeDefinition td, TypeReference itemTy } // serialization of individual element - static bool GenerateSerialization(string methodName, TypeDefinition td, TypeReference itemType, TypeReference mirrorBaseType) + static bool GenerateSerialization(string methodName, TypeDefinition td, TypeReference itemType, Type mirrorBaseType) { Weaver.DLog(td, " GenerateSerialization"); bool existing = td.HasMethodInBaseType(methodName, mirrorBaseType); @@ -109,7 +110,7 @@ static bool GenerateSerialization(string methodName, TypeDefinition td, TypeRefe return true; } - static bool GenerateDeserialization(string methodName, TypeDefinition td, TypeReference itemType, TypeReference mirrorBaseType) + static bool GenerateDeserialization(string methodName, TypeDefinition td, TypeReference itemType, Type mirrorBaseType) { Weaver.DLog(td, " GenerateDeserialization"); bool existing = td.HasMethodInBaseType(methodName, mirrorBaseType); diff --git a/Assets/Mirror/Editor/Weaver/Weaver.cs b/Assets/Mirror/Editor/Weaver/Weaver.cs index 5131d2a9ff4..a695bf92ccc 100644 --- a/Assets/Mirror/Editor/Weaver/Weaver.cs +++ b/Assets/Mirror/Editor/Weaver/Weaver.cs @@ -210,12 +210,12 @@ static bool WeaveSyncObject(TypeDefinition td) { if (td.IsDerivedFrom(typeof(SyncList<>))) { - SyncListProcessor.Process(td, WeaverTypes.Import(typeof(SyncList<>))); + SyncListProcessor.Process(td, typeof(SyncList<>)); modified = true; } else if (td.IsDerivedFrom(typeof(SyncSet<>))) { - SyncListProcessor.Process(td, WeaverTypes.Import(typeof(SyncSet<>))); + SyncListProcessor.Process(td, typeof(SyncSet<>)); modified = true; } else if (td.IsDerivedFrom(typeof(SyncDictionary<,>))) diff --git a/Assets/Mirror/Tests/Editor/Weaver/WeaverSyncListTests.cs b/Assets/Mirror/Tests/Editor/Weaver/WeaverSyncListTests.cs index 60e088d5d4d..32bf5d75210 100644 --- a/Assets/Mirror/Tests/Editor/Weaver/WeaverSyncListTests.cs +++ b/Assets/Mirror/Tests/Editor/Weaver/WeaverSyncListTests.cs @@ -31,8 +31,7 @@ public void SyncListGenericInheritance() [Test] public void SyncListGenericInheritanceWithMultipleGeneric() { - HasError("Could not find generic arguments for SyncList`1 in WeaverSyncListTests.SyncListGenericInheritanceWithMultipleGeneric.SyncListGenericInheritanceWithMultipleGeneric/SomeListInt", - "WeaverSyncListTests.SyncListGenericInheritanceWithMultipleGeneric.SyncListGenericInheritanceWithMultipleGeneric/SomeListInt"); + IsSuccess(); } [Test] diff --git a/Assets/Mirror/Tests/Editor/Weaver/WeaverSyncListTests~/SyncListGenericInheritanceWithMultipleGeneric.cs b/Assets/Mirror/Tests/Editor/Weaver/WeaverSyncListTests~/SyncListGenericInheritanceWithMultipleGeneric.cs index 7fd51550b07..c23619f510a 100644 --- a/Assets/Mirror/Tests/Editor/Weaver/WeaverSyncListTests~/SyncListGenericInheritanceWithMultipleGeneric.cs +++ b/Assets/Mirror/Tests/Editor/Weaver/WeaverSyncListTests~/SyncListGenericInheritanceWithMultipleGeneric.cs @@ -3,11 +3,8 @@ namespace WeaverSyncListTests.SyncListGenericInheritanceWithMultipleGeneric { /* - This test will fail - It is hard to know which generic argument we want from `SomeList` - So instead give a useful error for this edge case + This test should pass */ - class SyncListGenericInheritanceWithMultipleGeneric : NetworkBehaviour { readonly SomeListInt someList = new SomeListInt();