Fix privatescope method resolution bug (#916)

When resolving a `GenericInstanceMethod` for a `privatescope` method that has the same signature as other methods, `MetadataResolver.GetMethod` would incorrectly return the first method with the same name.

The fix for this seems to be an optimization opportunity as well.  Although I have to admit it feels a little to easy.  Please make sure I'm not overlooking some reason why this fix is not safe.

A added 2 variations of tests.

`PrivateScope` - These tests were fine and passed as is.  This is because the instruction operands are MethodDefinitions and therefore didn't need to be resolved by `MetadataResolver`

`PrivateScopeGeneric` - This test triggered the bug.

Mono.Cecil/MetadataResolver.cs

@@ -227,6 +227,22 @@ public virtual MethodDefinition Resolve (MethodReference method)
 
 			if (!type.HasMethods)
 				return null;
+
+			// This is here to handle privatescope.  Aka CompilerControlled.
+			// GetMethod cannot correctly resolve a privatescope method when the method name is the same as another method in the type.
+			// because GetMethod operates on a MethodReference which doesn't have access to the MethodAttributes.
+			// privatescope methods are always private.  There should also never be a MethodReference to a privatescope method.
+			// in other words, privatescope methods should always be referenced by their MethodDefinition.
+			// 
+			// privatescope methods aside, if method is ever a MethodDefinition we don't need to go searching all of the methods on the type
+			// we can return the method directly.  This avoids the cost of a linear search.
+			//
+			// So we have this optimization opportunity here.
+			// And we need to handle privatescope methods somehow, because GetMethod can't.
+			//
+			// and 2 birds one stone.  This if check covers the optimization and privatescope handling.
+			if (method is MethodDefinition definition)
+				return definition;
 
 			return GetMethod (type, method);
 		}

Test/Mono.Cecil.Tests/MethodTests.cs

@@ -2,6 +2,7 @@
 using System.Linq;
 
 using Mono.Cecil;
+using Mono.Cecil.Cil;
 using Mono.Cecil.Metadata;
 using Mono.Collections.Generic;
 using NUnit.Framework;
@@ -281,5 +282,93 @@ public void FunctionPointerArgumentOverload ()
 				Assert.AreEqual (overloaded_methods[2], overloaded_method_cdecl_reference.Resolve ()); 
 			});
 		}
+
+		[Test]
+		public void PrivateScope ()
+		{
+			TestIL ("privatescope.il", module => {
+				var foo = module.GetType ("Foo");
+				var call_same_name_methods = foo.GetMethod ("CallSameNameMethods");
+				var call_instructions = call_same_name_methods.Body.Instructions
+					.Where (ins => ins.OpCode.Code == Code.Call)
+					.ToArray ();
+				
+				var first_same_name_index = 2;
+
+				// The first method will be the normal non-privatescope method.
+				var first_call_resolved = ((MethodReference)call_instructions [0].Operand).Resolve ();
+				var expected_first_call_resolved = foo.Methods [first_same_name_index];
+				Assert.IsFalse(first_call_resolved.IsCompilerControlled);
+				Assert.AreEqual(expected_first_call_resolved, first_call_resolved);
+				
+				// This is the first privatescope method.
+				var second_call_resolved = ((MethodReference)call_instructions [1].Operand).Resolve();
+				var expected_second_call_resolved = foo.Methods [first_same_name_index + 1];
+				
+				// Sanity check to make sure the ordering assumptions were correct.
+				Assert.IsTrue(expected_second_call_resolved.IsCompilerControlled, "The expected method should have been compiler controlled.");
+				
+				// The equality failure isn't going to be very helpful since both methods will have the same ToString value,
+				// so before we assert equality, we'll assert that the method is compiler controlled because that is the key difference
+				Assert.IsTrue(second_call_resolved.IsCompilerControlled, "Expected the method reference to resolve to a compiler controlled method");
+				Assert.AreEqual(expected_second_call_resolved, second_call_resolved);
+				
+				// This is the second privatescope method.
+				var third_call_resolved = ((MethodReference)call_instructions [2].Operand).Resolve ();
+				var expected_third_call_resolved = foo.Methods [first_same_name_index + 2];
+				
+				// Sanity check to make sure the ordering assumptions were correct.
+				Assert.IsTrue(expected_third_call_resolved.IsCompilerControlled, "The expected method should have been compiler controlled.");
+				
+				// The equality failure isn't going to be very helpful since both methods will have the same ToString value,
+				// so before we assert equality, we'll assert that the method is compiler controlled because that is the key difference
+				Assert.IsTrue(third_call_resolved.IsCompilerControlled, "Expected the method reference to resolve to a compiler controlled method");
+				Assert.AreEqual(expected_third_call_resolved, third_call_resolved);
+			});
+		}
+
+		[Test]
+		public void PrivateScopeGeneric ()
+		{
+			TestIL ("privatescope.il", module => {
+				var foo = module.GetType ("Foo");
+				var call_same_name_methods = foo.GetMethod ("CallSameNameMethodsGeneric");
+				var call_instructions = call_same_name_methods.Body.Instructions
+					.Where (ins => ins.OpCode.Code == Code.Call)
+					.ToArray ();
+
+				var first_same_name_generic_index = 6;
+
+				// The first method will be the normal non-privatescope method.
+				var first_call_resolved = ((MethodReference)call_instructions [0].Operand).Resolve();
+				var expected_first_call_resolved = foo.Methods [first_same_name_generic_index];
+				Assert.IsFalse(first_call_resolved.IsCompilerControlled);
+				Assert.AreEqual(expected_first_call_resolved, first_call_resolved);
+				
+				// This is the first privatescope method.
+				var second_call_resolved = ((MethodReference)call_instructions [1].Operand).Resolve();
+				var expected_second_call_resolved = foo.Methods [first_same_name_generic_index + 1];
+				
+				// Sanity check to make sure the ordering assumptions were correct.
+				Assert.IsTrue(expected_second_call_resolved.IsCompilerControlled, "The expected method should have been compiler controlled.");
+				
+				// The equality failure isn't going to be very helpful since both methods will have the same ToString value,
+				// so before we assert equality, we'll assert that the method is compiler controlled because that is the key difference
+				Assert.IsTrue (second_call_resolved.IsCompilerControlled, "Expected the method reference to resolve to a compiler controlled method");
+				Assert.AreEqual(expected_second_call_resolved, second_call_resolved);
+
+				// This is the second privatescope method.
+				var third_call_resolved = ((MethodReference)call_instructions [2].Operand).Resolve();
+				var expected_third_call_resolved = foo.Methods [first_same_name_generic_index + 2];
+				
+				// Sanity check to make sure the ordering assumptions were correct.
+				Assert.IsTrue(expected_third_call_resolved.IsCompilerControlled, "The expected method should have been compiler controlled.");
+				
+				// The equality failure isn't going to be very helpful since both methods will have the same ToString value,
+				// so before we assert equality, we'll assert that the method is compiler controlled because that is the key difference
+				Assert.IsTrue(third_call_resolved.IsCompilerControlled, "Expected the method reference to resolve to a compiler controlled method");
+				Assert.AreEqual(expected_third_call_resolved, third_call_resolved);
+			});
+		}
 	}
 }

Test/Resources/il/privatescope.il

@@ -0,0 +1,77 @@
+.assembly extern mscorlib
+{
+  .publickeytoken = (B7 7A 5C 56 19 34 E0 89)
+  .ver 2:0:0:0
+}
+
+.assembly PrivateScope {}
+
+.module PrivateScope.dll
+
+.class private auto ansi Foo {
+
+  .method public specialname rtspecialname instance void .ctor () cil managed
+  {
+    ldarg.0
+    call instance void [mscorlib]System.Object::.ctor ()
+    ret
+  }
+
+  .method public instance void  CallSameNameMethods() cil managed
+  {
+    ldarg.0
+    call      instance void Foo::'SameName'()
+    ldarg.0
+    call      instance void Foo::'SameName$PST0600A3BA'()
+    ldarg.0
+    call      instance void Foo::'SameName$PST0600A3BC'()
+    ret
+  }
+
+ .method public hidebysig
+    instance void 'SameName' () cil managed
+  {
+    ret
+  }
+
+  .method privatescope
+    instance void 'SameName$PST0600A3BA' () cil managed
+  {
+    ret
+  }
+
+  .method privatescope
+    instance void 'SameName$PST0600A3BC' () cil managed
+  {
+    ret
+  }
+
+  .method public instance void  CallSameNameMethodsGeneric() cil managed
+  {
+    ldarg.0
+    call      instance void Foo::'SameNameGeneric'<int32>()
+    ldarg.0
+    call      instance void Foo::'SameNameGeneric$PST0600A3BD'<int32>()
+    ldarg.0
+    call      instance void Foo::'SameNameGeneric$PST0600A3BE'<int32>()
+    ret
+  }
+
+ .method public hidebysig
+    instance void 'SameNameGeneric'<T> () cil managed
+  {
+    ret
+  }
+
+  .method privatescope
+    instance void 'SameNameGeneric$PST0600A3BD'<T> () cil managed
+  {
+    ret
+  }
+
+  .method privatescope
+    instance void 'SameNameGeneric$PST0600A3BE'<T> () cil managed
+  {
+    ret
+  }
+}