SomeRanDev / reflaxe.CSharp

A remake of the Haxe/C# target written entirely within Haxe using Reflaxe.
MIT License
35 stars 2 forks source link

Optional arguments and C# #5

Open jeremyfa opened 1 year ago

jeremyfa commented 1 year ago

Let's assume we have this Haxe method that has one optional argument followed with a required one:

function foo(optInt:Int = 4, reqString:String) {
    // ...
}

In Haxe this is valid. (I believe @Simn was thinking about maybe removing that feature in favor of overload at some point).

Anyway, the naive C# output would be:

public virtual void foo(int optInt = 4, string reqString) {
    // ...
}

But that is not valid in C# because optional args need to be after the required ones.

In current C# target, it would handle that situation by making every argument non-optional in the C# method, but wrap every haxe optional arg into haxe.lang.Null. The above example gives that in current C# target:

public virtual void foo(global::haxe.lang.Null<int> optInt, string reqString) {
    unchecked {
        int optInt1 = ( ( ! (optInt.hasValue) ) ? (4) : ((optInt).@value) );
    }
    #line default
}

Then when you call with haxe foo("hello");, it would become foo(default(global::haxe.lang.Null<int>), "hello") in output to match the C# method.

This isn't ideal because wrapping with haxe.lang.Null primitive types means creating garbage memory even if it is definitely not needed: an int with a default value should never be null anyway.

In the new Reflaxe/C# target I'd like to explore another approach: solving that case with C# overload. The above method with optional arg could generate this C# code instead:

public virtual void foo(string reqString) {
    foo(4, reqString);
}

public virtual void foo(int optInt, string reqString) {
    // ...
}

Then no need to do anything special when calling as one or the other signature should be resolved depending on the arguments provided.

Will need to confirm that later, but it might make it easier to deal with reflection as well (I know there are also issues with dynamic calls and optional args in current C# target).

SomeRanDev commented 1 year ago

HOOOLY MOLY, I didn't even know this was possible. Hmmm, I'd need to investigate this more, but it's definitely something I'd like to implement within Reflaxe so it can be fixed for everything, not just this target (could make it an option to auto-generate the functions before passing to the compiler, and configure whether the new functions use the same name, or generate new ones). Give me a couple days to try that!

SomeRanDev commented 1 year ago

Alterantively, we should consider: removing the default argument from the C# output (make non of the arguments default), then whenever the function is called, we just inject the default argument (4) at the front? It would be a little challenging, but once again something I would implement on Reflaxe's side that would work seamlessly.

Simn commented 1 year ago

I once had a similar idea when we were working on a C target. I still have the test case for that: https://gist.github.com/Simn/f2718bc8037af3c146bc

Basically, when you can be sure about the nullability of your arguments, you can call the _hx_known_ version which has the actual implementation, and otherwise you call the generic one which sets up default values and then calls the first one.

Note that you can only do call-site replacement for methods which are definitely known at compile-time, so mostly final and static ones:

class Parent {
    public function new() {}

    public function test(a = 0) {
        trace(a);
    }
}

class Child extends Parent {
    override function test(a = 1) {
        trace(a);
    }
}

function main() {
    var c:Parent = new Child();
    c.test(); // should trace 1
}
jeremyfa commented 1 year ago

@RobertBorghese careful with moving that to Reflaxe. It could be an interesting option but I’m afraid it would also make it more difficult to get clear info on optional args when needed (like for Reflection). Maybe it would be better to provide a helper provided by reflaxe that you can explicitly call to get a list of functions from one given as argument, as it would be more flexible than having reflaxe do that beforehand.

@Simn yes, changing on-site calls seems tricky for default values, that is why I’m interested in taking advantage of c# overload in cases where it’s possible, as it should work well with non-final and non-static methods too. Will take a look at your C example

Simn commented 1 year ago

I just remembered another reason I wanted to do it like that:

function test(a = 0, ?b = 0) {
    $type(a); // Int
    $type(b); // Null<Int>
}

function main() {
    test();
}

This means that a can be properly typed as Int inside the real function body and only the wrapper-function needs to treat it as Null<Int> in order to determine the default value. So for known calls, no boxing is necessary at all.

flashultra commented 1 year ago

Maybe a naive approach, but what if we set default values for all variables after the first optional argument? ٍSomething like this ( in C#):

public virtual void foo(int optInt = 4, string reqString = null ) {
    // ...
}
Simn commented 1 year ago

Yes I meant to imply that because it works well with my approach. That way you have one wrapper function which deals with all the default value stuff (which is also the one that is called from reflection and such) and one "clean" function.

Aidan63 commented 1 year ago

I've been working on overhauling hxcpp's functions to be strongly typed and optional arguments makes it all a bit of a nightmare. Haxe functions allow for implicit conversions of functions when swapping out arguments with Dynamic, switching nullability on arguments, and signatures changing when round tripping throught Dynamic.

What I'm updating hxcpp to have is a callable class which represents a function you can call.

template<class TReturn, class... TArgs>
struct Callable<TReturn(TArgs...)> : public hx::Object
{
    virtual TReturn _hx_run(TArgs... args) = 0;
};

All local functions and closures are created as sub classes of this callable, each member and static class function also has a internal function which returns a sub class of this callable for invoking that function. Because default values aren't part of the function type and are not inserted at call site TFun doesn't contain enough information to be able to tell if a given ?Null<Int> argument is that way because the user explicitly typed that (i.e. final f = (x : Int = null) -> {}) or its an integer with a default value (i.e. final f = (x : Int = 7) -> {}).

This means we have to be pessimistic for this Callable and you don't really have any other option but to require boxing even if the user types out a strongly typed closure.

e.g.

final f = (x : Int = 7) -> trace(x)

becomes

struct _hx_closure0 : ::hx::Callable<void(::Dynamic)> {
    // actual implementation
};

In situations where the user swaps and arguments with Dynamic, I generate a wrapper functions to handle that.

final c0 : Int->Void = x -> trace(x);
final c1 : Dynamic->Void = c0;

The C++ callable object has an implicit conversion to different callable templated types, this generates a callable which simply holds another callable and forwards the arguments. This works as hxcpp's Dynamic has implicit conversions of ints and other types so it works with little effort required.

Callable(const Callable<TOtherReturn(TOtherArgs...)>& inCallable)
{
    struct AdapterCallable final : public Callable<TReturn(TArgs...)>
    {
        Callable<TOtherReturn(TOtherArgs...)> wrapped;

        AdapterCallable(Callable<TOtherReturn(TOtherArgs...)> _wrapped) : wrapped(_wrapped) {}

        TReturn _hx_run(TArgs... args) override
        {
            return wrapped(args...);
        }
    };

    super::mPtr = new AdapterCallable(inCallable);
}

You can make an optimisation on class member and static functions when calling them directly if they have optional arguments. For a given class function with optional arguments the C++ generates a signature like this.

class MyClass {
    static function MultiplyBy(arg0 : Int, arg1 : Int = 2) { return arg0 * arg1 }
}
int MultiplyBy(int arg0, ::hx::Null<int> __o_arg1) {
    int arg1 = __o_arg1.Default(2);
    return arg0 * arg1;
}

Despite ::hx::Null implying boxing, this type lives on the stack and simply holds a value along with a boolean to say if its default or not. This means direct invocations of that function avoid boxing.

MyClass.MultiplyBy(7); // No Boxing

final f = MyClass.MultiplyBy;

f(7); // boxing

Hopefully all this rambling is useful and isn't too irrelevant being mostly about C++. C# generics are not templates so there might be a better approach and already having a GC might allow for a better C# implementation. You may also want better interop with Action / Func as you don't have to worry about the GC causing issues. But, a direct 1:1 C# version of my described approach might be something like.

readonly ref struct Arg<T>
{
    private readonly bool hasValue;

    private readonly T value;

    public Arg()
    {
        hasValue = false;
        value    = default;
    }

    public Arg(T value)
    {
        this.value    = value;
        this.hasValue = true;
    }

    public T ValueOr(T defaultValue)
    {
        return hasValue ? value : defaultValue;
    }
}

class __MyClassMultiplyBy : Function<int, int, int?>
{
    public override int Run(int arg0, int? arg1)
    {
        return MyClass.MultiplyBy(arg0, arg1.HasValue ? new Arg<int>(arg1.Value) : new Arg<int>());
    }
}

class MyClass
{
    public static int MultiplyBy(int arg0, Arg<int> __o_arg1)
    {
        var arg1 = __o_arg1.ValueOr(12);

        return arg0 * arg1;
    }

    public static Function<int, int, int?> MultiplyBy_dyn()
    {
        return new __MyClassMyFunc();
    }
}

void Main()
{
    MyClass.MultiplyBy(7, new Arg<int>());
    MyClass.MultiplyBy(12, new Arg<int>(24));

    Function<int, int, int?> f = MyClass.MultiplyBy_dyn();

    f.Run(12, 7);
}

You may need some custom types for implicit conversions or have the generator call explicit conversions. For the generic function approach with adapters to work in C# you'll also need to make sure the type constraints are setup properly for implicit conversions.

jeremyfa commented 1 year ago

Thanks for all the feedback, will experiment while keeping in mind all the things that have been said here!

SomeRanDev commented 1 year ago

Added a couple functions in Reflaxe to help. This provides all the possible variations of arguments. Checked to make sure it works, but not tested thoroughly.

// Find all argument variations for optional params
// frontOptionalsOnly - only gen variations for optional arguments before required arguments
// preventRepeats - remove repeated type combos
ClassFuncData.findAllArgumentVariations(frontOptionalsOnly: Bool = false, preventRepeats: Bool = false): Array<{ args: Array<ClassFuncArg>, padExprs: Array<TypedExpr> }>;

---

In Reflaxe/C++, I'm trying my "inject defaults at call expression" idea. To prevent the previously discussed issue, it manually checks if there is a "conflicting default" and reverts to the "passing null and having it reassigned in the function body" as a final resort. Just wanted to point out I also added functions for those in case they might help:

ClassFuncData.replacePadNullsWithDefaults(passedArgs: Array<TypedExpr>): Array<TypedExpr>;

ClassFuncArg.isFrontOptional(): Bool;
ClassFuncArg.hasConflicingDefaultValue(): Bool;
jeremyfa commented 1 year ago

Did implement a first iteration of optional args via C# overloads:

From this Haxe Code:

    function foo(optInt:Int = 4, reqString:String) {

    }

    function foo2(optInt:Int = 4, reqString:String, optBool:Bool = false) {
        return false;
    }

We get this:

        public void foo(int optInt, String reqString) {

        }

        public void foo(String reqString) {
            foo(4, reqString);
        }

        public bool foo2(int optInt, String reqString, bool optBool = false) {
            return false;
        }

        public bool foo2(String reqString, bool optBool = false) {
            return foo2(4, reqString, optBool);
        }

This definitely doesn't cover all uses cases, but that still could be complementary to the other solutions provided by Aidan and Simon.

We'll probably need to have a signature that boxes nullables anyway, to cover the situations where the function type is unknown at compile time.

jeremyfa commented 1 year ago

(Thanks to @RobertBorghese for the helpers added in reflaxe for that)

ShaharMS commented 1 year ago

want to point out somth - if this is implemented, overloading should be opted for - much more comfortable for haxe interop with other c# code, and much more understandable