SymbolExtensions.GetMethodInfo allow parameterless method decoding

[mitchcapper]

Use complex LambdaExpression walking to allow GetMethodInfo (() => myMethod) rather than GetMethodInfo(() => myMethod(null,null,null....))

Quality of life improvement. Right now manual patching is a bit annoying to pass complex method replacements to, especially when you have ref variables as for a method like:

private static bool prePatchSomething(ref string something, ref bool correct, ref Assembly loaded, ref MyClass __result, String info, int howLong)
you need to do something like

string s1=null;
bool b1=false;
Assembly a1=null;
MyClass m1 = null;
SymbolExtensions.GetMethodInfo(() => prePatchSomething(ref s1, ref b1, ref a1, ref m1, null, 0);

With this change:
SymbolExtensions.GetMethodInfo(prePatchSomething);

I believe this should be safe. Might even technically have a bit less overhead but im sure performance difference is negligible either way.

[pardeike]

While this is great, what happens if I want to use it for public methods that are not mine and there are overloads.

For example: Console.WriteLine

[mitchcapper]

You need to fallback to the old options as there is no way to clarify what overloaded method you want to use. In fact the compiler error for passing an overloaded method is non-understandable: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs0201

I figured before this would throw an exception so no ones existing code should break (unless you were counting on that exception but that would be unlikely).

[pardeike]

Since most users of Harmony are inexperienced I fear that this introduces a lot of confusion. I am not sure if this is the right way to do what you want.

At least we need to should not use an overload on the existing method to make it clear that this is not friendly to getting overloaded methods

[mitchcapper]

Yeah I am not sure if there is a way to resolve the exceptionally opaque compiler error message. The alternate option would be to add another method as you mentioned like SymbolExtensions.GetMethodInfoFromNonOverloadedMethod (i'm not good at naming things so happy to take suggestions).

Also, minor detail, but technically right now without this PR passing methods in this way does compile and only fails at runtime when caught by:

Harmony/Harmony/Tools/SymbolExtensions.cs

Line 50 in f366a30

throw new ArgumentException("Invalid Expression. Expression should consist of a Method call only.");

If the user was to pass an overload function to GetMethodInfo right now the existing code would also throw the non-obvious error of CS0201. The only work around to avoiding that is changing GetMethodInfo away from taking just a bare LambdaExpression.

Granted this PR could make that problem slightly worse as it would encourage / document the ability of using GetMethodInfo with just the function being passed.

One potential solution is to add this addition to GetMethodInfo but not document it, there a user would either do:
A) pass a non overload method that would work, even though that format is not shown in the docs.
B) pass an overloaded method and get what they get now.

I also find it interesting that the userbase is fairly inexperienced, I wouldn't say this reflection overloading is the most straightforward programming.

Personally I think you shouldn't nerf an arguably complex library by giving power users someway to call something like this as it is a much cleaner and faster coding path. I implemented this in my own GetMethod wrapper so if it isn't added it is not an issue. If not merged one could atleast document this workaround in the docs (happy do that as well). Either way it should have a disclaimer in the docs specifying this only works for non-overloaded methods otherwise you will get a CS0201 error.

[pardeike]

I agree with your reasoning. Let me reflect a bit on what the optimal approach to this is.

Btw, most users use annotations (including myself) because it’s a much more declarative way. Edge cases can be covered by having a TargetMethod() in the patch class that contains the logic that otherwise would not fit in annotations.

[pardeike]

That’s a really nice way to get methods. I can’t stop thinking about more places to use this.

[pardeike]

Something is wrong. Your changes don't compile for me:

public static void Foo() { }
//...
_ = GetMethodInfo(Foo); // does not compile and gives me CS1503
_ = GetMethodInfo(() => Foo); // does not compile and gives me CS0201

[mitchcapper]

You should be using that second form.
https://dotnetfiddle.net/0Pgn1G should replicate what your test was
Works in .NET 6, works on Roslyn 4.0 by default. Works in 4.72 BUT requires langversion preview (or most likely langversion 10 ).

I am guessing it is due to:
https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-10#lambda-expression-improvements

What I find really comical is I get a different error from you in older lang versions:
Cannot convert lambda expression to type 'System.Linq.Expressions.LambdaExpression' because it is not a delegate type

I have not had a chance to look at if it is possible to do a casting to resolve.

[mitchcapper]

Alright well after a good bit of testing I am not sure it is possible with 7.3. I can get it to work with parameter-less functions with another overload of

public static MethodInfo GetMethodInfo(Expression<Func<Action>> expression) {
	return GetMethodInfo((LambdaExpression)expression);
}

(so you can then do GetMethodInfo(() => myfunc)

But the engine won't even try to guess at basic wrapped template calls so:

public static void Test<T>(Action<T> myFunc) {
}
public static void Foo2(string myArg) { }

Test(Foo2) will fail with cannot infer type.

Now one could make it work if you specify the types for the template parameters but that is just about as much work.

Here is a more complex example from my debugger library (although use it less with CallerMemberName but it is more dynamic than that):

https://dotnetfiddle.net/LQfNJE