pardeike / Harmony

A library for patching, replacing and decorating .NET and Mono methods during runtime
https://www.patreon.com/pardeike
MIT License
5.15k stars 485 forks source link

SymbolExtensions does not resolve overriden method #600

Closed krafs closed 5 months ago

krafs commented 5 months ago

Describe the bug Getting a MethodInfo for an overriden method using SymbolExtensions resolves the base class method, not the child class method.

To Reproduce

  1. Open a terminal in a new empty folder.
  2. dotnet new console --framework net8.0
  3. dotnet add package Lib.Harmony --version 2.3.1.1
  4. Create file called Program.cs and paste the code below
  5. Save the file
  6. dotnet run

Expected behavior I expect the MethodInfo retrieved for the overriden method in Child class using SymbolExtensions to be the same MethodInfo I get when retrieving it using regular reflection.

Code

using HarmonyLib;
using System.Reflection;

MethodInfo parentHello = typeof(Parent).GetMethod("Hello");
MethodInfo childHello = typeof(Child).GetMethod("Hello");
Console.WriteLine(parentHello == childHello);
// Expected: False
// Actual: False

MethodInfo symbolParentHello = SymbolExtensions.GetMethodInfo<Parent>(x => x.Hello());
MethodInfo symbolChildHello = SymbolExtensions.GetMethodInfo<Child>(x => x.Hello());

// Check if MethodInfo for Child.Hello resolved via Reflection is same the one from SymbolExtensions
Console.WriteLine(childHello == symbolChildHello);
// Expected: True
// Actual: False

// Check if MethodInfo for Parent.Hello and Child.Hello resolved via SymbolExtensions are the same
Console.WriteLine(symbolParentHello == symbolChildHello);
// Expected: False
// Actual: True
public class Parent
{
    public virtual void Hello() { }
}

public class Child : Parent
{
    public override void Hello() { }
}

Runtime environment:

I couldn't find any tests on SymbolExtensions, so not sure if this is an expected limitation on the API?

pardeike commented 5 months ago

The reason is that reflections do not work well with inheritance. Its the same reason that does not allow you to call a base method on a child instance if you don't let the compiler do it. That includes casting, ordinary reflection and delegates. It's the reason Harmony has a reverse patch. I extended your code to demonstrate this effect:

public class Parent
{
    public virtual void Hello() { Console.WriteLine("PARENT"); }
}

public class Child : Parent
{
    public override void Hello() { Console.WriteLine("CHILD"); }
}

parentHello.Invoke(new Parent(), null);
parentHello.Invoke(new Child(), null); // should log PARENT but logsCHILD
childHello.Invoke(new Child(), null);

SymbolExtension uses reflection and thus cannot choose which level of overwrite it should get.

krafs commented 5 months ago

Maybe I've misunderstood the use of SymbolExtensions then. Is SymbolExtensions not for getting a MethodInfo on a given type? It doesn't need to determine the level of the overwrite to return, I am telling it to return the one from Child.

This gets me what I expect. MethodInfo hello1 = typeof(Child).GetMethod("Hello");

And this call has all the same amount of information, so I feel like it should be able to give the same result, no? MethodInfo hello2 = SymbolExtensions.GetMethodInfo<Child>(x => x.Hello());

Or is this not the purpose of SymbolExtensions? It's not an alternative to Reflection for getting a MethodInfo?

pardeike commented 5 months ago

The purpose is the same. The technical limitations are not the same. SymbolExtensions basically wrap a super simple concept and there is no way to improve that: < https://github.com/pardeike/Harmony/blob/669729f618e90ecf7fe943f7ed70f8150216dc5a/Harmony/Tools/SymbolExtensions.cs#L47>