From a3c266372018f5d6ce760faced470cdae9aa0ad7 Mon Sep 17 00:00:00 2001 From: Michael Voorhees Date: Fri, 2 May 2025 11:53:56 -0400 Subject: [PATCH] Avoid unnecessarily creating TypeReferences on import When importing a TypeSpecification, an element type or generic argument could be a TypeDefinition within the current module. When this happens, a new TypeReference does not need to be created. This can lead to a TypeReference being added to the typeref table for a type that is already in the assembly. This seems to hold together, although I don't think it's ideal. And really my goal is to avoid failing this logic in the ILLink test framework https://github.com/dotnet/runtime/blob/cec44d6dff9de95421f199f65bbc80b8296da1c0/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs#L56 This fix superceeds https://github.com/jbevain/cecil/pull/138. I've left that fix in since that API is exposed publically and others could be depending on it. While I was adjusting this logic, I thought I would apply the same logic to a TypeReference where the scope is the current modules assembly. This case is a bit contrived as it shouldn't happen, but if it did, it would result in the same circular reference problem as https://github.com/jbevain/cecil/pull/138 --- Mono.Cecil/Import.cs | 14 ++++++- Test/Mono.Cecil.Tests/ImportCecilTests.cs | 50 +++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/Mono.Cecil/Import.cs b/Mono.Cecil/Import.cs index e5670b601..da2e11080 100644 --- a/Mono.Cecil/Import.cs +++ b/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 a, T b) where T : class, IEquatable 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; diff --git a/Test/Mono.Cecil.Tests/ImportCecilTests.cs b/Test/Mono.Cecil.Tests/ImportCecilTests.cs index 70100eac1..2082b05b9 100644 --- a/Test/Mono.Cecil.Tests/ImportCecilTests.cs +++ b/Test/Mono.Cecil.Tests/ImportCecilTests.cs @@ -286,6 +286,51 @@ public void ContextGenericTest () Assert.AreEqual ("Mono.Cecil.Tests.ImportCecilTests/Generic`1 Mono.Cecil.Tests.ImportCecilTests/Generic`1::ComplexGenericMethod(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; + delegate void Emitter (ModuleDefinition module, MethodBody body); static TDelegate Compile (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 (string name, Emitter emitter) { var module = CreateModule (name);