Merge pull request #25 from Unity-Technologies/avoid-unnecessary-import

Avoid unnecessarily creating TypeReferences on import

Author: mrvoorhe

Mono.Cecil/Import.cs

@@ -506,6 +506,16 @@ TypeReference ImportType (TypeReference type, ImportGenericContext context)
 			if (type.IsTypeSpecification ())
 				return ImportTypeSpecification (type, context);
 
+			// If a type's scope is the module definition then we don't need to create a new type reference.  We can reuse the existing one, which is likely the TypeDefinition.
+			// Reusing the type avoid creating an unnecessary entry in the type reference table
+			if (type.Scope == module)
+				return type;
+
+			// This case is more contrived, but it's the same as the above.  If the type's scope matches the current modules assembly then we don't need to create a new type reference.
+			// Nor do we need to import the scope because the scope is the current modules assembly.
+			if (type.Scope is AssemblyNameReference asmName && Mixin.Equals (asmName, module.assembly.Name))
+				return type;
+
 			var reference = new TypeReference (
 				type.Namespace,
 				type.Name,
@@ -535,6 +545,8 @@ protected IMetadataScope ImportScope (IMetadataScope scope)
 			case MetadataScopeType.AssemblyNameReference:
 				return ImportReference ((AssemblyNameReference) scope);
 			case MetadataScopeType.ModuleDefinition:
+				// This change to avoid self reference has been superceded by the check in ImportType to avoid creating the new TypeReference in the first place.
+				// However, given that ImportScope is protected people could be relying on this behavior so I'm not going to remove it
 				if (scope == module) return scope;
 				return ImportReference (((ModuleDefinition) scope).Assembly.Name);
 			case MetadataScopeType.ModuleReference:
@@ -809,7 +821,7 @@ static bool Equals<T> (T a, T b) where T : class, IEquatable<T>
 			return a.Equals (b);
 		}
 
-		static bool Equals (AssemblyNameReference a, AssemblyNameReference b)
+		public static bool Equals (AssemblyNameReference a, AssemblyNameReference b)
 		{
 			if (ReferenceEquals (a, b))
 				return true;

Test/Mono.Cecil.Tests/ImportCecilTests.cs

@@ -286,6 +286,51 @@ public void ContextGenericTest ()
 			Assert.AreEqual ("Mono.Cecil.Tests.ImportCecilTests/Generic`1<TS> Mono.Cecil.Tests.ImportCecilTests/Generic`1<System.String>::ComplexGenericMethod<TS>(T,TS)", method.FullName);
 		}
 
+		[Test]
+		public void ImportGenericTypeWithGenericArgumentInSameAssembly ()
+		{
+			using var module = CreateTestModule ();
+			var generic_type = module.ImportReference (typeof (GenericForSameAssemblyTests<>)).Resolve ();
+			
+			var type_definition = new TypeDefinition(string.Empty, "Bar", TypeAttributes.Public, module.ImportReference (typeof (object)));
+			module.Types.Add(type_definition);
+			
+			var generic_inst = new GenericInstanceType (generic_type);
+			generic_inst.GenericArguments.Add (type_definition);
+			
+			var imported = (GenericInstanceType)module.ImportReference (generic_inst);
+
+			Assert.That(imported.GenericArguments[0], Is.EqualTo (type_definition));
+
+			// Shouldn't be able to find this assert if the assert above passes, but just in case also assert a circular reference isn't created.
+			Assert.That(module.AssemblyReferences.Select(r => r.Name), Does.Not.Contain (module.Name));
+		}
+		
+		[Test]
+		public void ImportGenericTypeWithGenericArgumentInSameAssemblyTypeReference ()
+		{
+			using var module = CreateTestModule ();
+			var generic_type = module.ImportReference (typeof (GenericForSameAssemblyTests<>)).Resolve ();
+			
+			var type_definition = new TypeDefinition(string.Empty, "Bar", TypeAttributes.Public, module.ImportReference (typeof (object)));
+			module.Types.Add(type_definition);
+			
+			var type_reference = new TypeReference(type_definition.Namespace, type_definition.Name, module, new AssemblyNameReference(module.Assembly.Name.Name, module.Assembly.Name.Version));
+			
+			var generic_instance = new GenericInstanceType (generic_type);
+			generic_instance.GenericArguments.Add (type_reference);
+			
+			var imported = (GenericInstanceType)module.ImportReference (generic_instance);
+			
+			// By reusing the same type reference we can avoid triggering an ImportReference call which creates a circular reference.
+			Assert.That(imported.GenericArguments[0], Is.EqualTo (type_reference));
+
+			// Shouldn't be able to find this assert if the assert above passes, but just in case also assert a circular reference isn't created.
+			Assert.That(module.AssemblyReferences.Select(r => r.Name), Does.Not.Contain (module.Name));
+		}
+
+		public class GenericForSameAssemblyTests<T>;
+
 		delegate void Emitter (ModuleDefinition module, MethodBody body);
 
 		static TDelegate Compile<TDelegate> (Emitter emitter, [CallerMemberName] string testMethodName = null)
@@ -319,6 +364,11 @@ static SR.Assembly LoadTestModule (ModuleDefinition module)
 			}
 		}
 
+		static ModuleDefinition CreateTestModule ([CallerMemberName] string testMethodName = null)
+		{
+			return CreateModule("ImportCecil_" + testMethodName);
+		}
+
 		static ModuleDefinition CreateTestModule<TDelegate> (string name, Emitter emitter)
 		{
 			var module = CreateModule (name);