BHoM_Engine: Compute.TryRunExtensionMethod does not support null method arguments

Description:

As spotted by @michal-pekacki, currently Compute.TryRunExtensionMethod will fail in case of any method arguments being equal to null, due to the following line: https://github.com/BHoM/BHoM_Engine/blob/f56fb5f93ce94587ce5b403b9a057fdd387dd830/BHoM_Engine/Compute/TryRunExtensionMethod.cs#L155

Naturally, that makes sense because we do not know the type null argument is meant to represent, no way to bind.

Recently this limitation started becoming an issue (null method argument can sometimes be perfectly valid), so I had a think about potential improvements and came up with the following pattern:

public static bool TryRunExtensionMethod<T1>(this T1 obj, string methodName, out object result)
{
    Type[] types = new Type[] { typeof(T1) };
    Func<object[], object> method = ExtensionMethodToRun(methodName, types); // Existing method after minor tweaks
    // Call the method and return the outputs
}

public static bool TryRunExtensionMethod<T1, T2>(this object obj, string methodName, T2 parameter1, out object result)
{
    Type[] types = new Type[] { typeof(T1), typeof(T2) };
    Func<object[], object> method = ExtensionMethodToRun(methodName, types); // Existing method after minor tweaks
    // Call the method and return the outputs
}

public static bool TryRunExtensionMethod<T1, T2, T3>(this object obj, string methodName, T2 parameter1, T3 parameter2, out object result)
{
    Type[] types = new Type[] { typeof(T1), typeof(T2), typeof(T3) };
    Func<object[], object> method = ExtensionMethodToRun(methodName, types); // Existing method after minor tweaks
    // Call the method and return the outputs
}

...repeat until T1-T10

First question: do you see an immediate gap in the above? The objective is to maintain the current functionality while providing necessary type strength that would ensure the correctness of bindings. If not, how afraid would you be about refactoring the current code? It would require raising a long chain of PRs to align all repos that leverage the mechanism, also a breaking change in the UI I'm afraid (although that should be less of an issue, assuming the method is not called from e.g. GH). In general, please shout if you dislike the idea for any reason.

Finally, currently the method has an out parameter, which I could also migrate to Output<T1, T2> if there is such will - happy to hear your opinion on that too. Cheers!

Not really sure of this tbh. Would prefer to keep generics out of the situation for something like this if possible. Also, if the method calling this already knows the type, could you not simply just call the right method then instead of going through the hoops of RunExtensionMethod?

Not saying completly invalid, but think I would like to see a concrete usecase where this is required before being convinced tbh.

One thing we could try to change and test though is to remove the null check on all the inputs, and then slightly modify the "extensinMethodToRun" method to check the input parameters for nulls and check in the input parameter is nullable, and thereby allow for it to be a match. would ofc give an arbitrary anwer for something like:

        public static double Foo(Line line, Point point)
        {
            return 5;
        }

compared to

        public static double Foo(Line line, Vector vector)
        {
            return 6;
        }

if you provide null as the second input, but that is also a case we could catch and raise some error about ambiguous methods found.

Would honestly prefer some option like this to allow nullables over introducing the generic pattern above.

Not really sure of this tbh. Would prefer to keep generics out of the situation for something like this if possible. Also, if the method calling this already knows the type, could you not simply just call the right method then instead of going through the hoops of RunExtensionMethod?

A good example can be Transform: Spatial_Engine does not know about all objects that implement IElement interfaces so we can't do dynamic casting, thus TryRunExtensionMethod: https://github.com/BHoM/BHoM_Engine/blob/f56fb5f93ce94587ce5b403b9a057fdd387dd830/Spatial_Engine/Modify/Transform.cs#L58

On the consumer side we may also not know the exact type of object meant to be transformed, so we delegate the binding to ITransform, see PostProcess in Revit_Toolkit: https://github.com/BHoM/Revit_Toolkit/blob/80f61c04c829f09bd1e6ee86c203bc68f2327e3b/Revit_Core_Engine/Modify/Postprocess.cs#L81

Now, if any method parameter (transform or tolerance in our example) is null, the appropriate extension method will not be found. Maybe it does not make sense to pass a null in this particular case, but imagine SetLocation or any method with optional parameters, where null could be literally desirable - then the whole pattern becomes helpless.

One thing we could try to change and test though is to remove the null check on all the inputs, and then slightly modify the "extensinMethodToRun" method to check the input parameters for nulls and check in the input parameter is nullable, and thereby allow for it to be a match.

Not sure why would we implement another faulty solution (as you already spotted yourself, it won't work correctly if there is more than one method with same number of inputs) just for the sake of avoiding generics. What I want to achieve is to avoid the exact behaviour that you mention (raising errors about ambiguous methods found) and introduce strong typing into the pattern to simply remove the limitation that is starting to block the team.

Had a good offline chat with @IsakNaslundBh, concluding that generics would not help in my case for 2 reasons:

binding to type deduced from a generic argument would not work because then each object would need to be cast to its exact type before passing to TryRunExtensionMethod in order to search the whole method tree - not very practical
raising errors in case of ambiguity should be OK because that would mimic the dynamic casting mechanism 1:1, going beyond that is likely not necessary in any case (went through the whole thought process with @IsakNaslundBh to confirm)

...so to summarise, the solution suggested by @IsakNaslundBh is the best we can get according to our best knowledge 👍 Thanks!

BHoM / BHoM_Engine

BHoM_Engine: Compute.TryRunExtensionMethod does not support null method arguments #3393

Description: