Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default value for enum parameters #27

Open
sebthom opened this issue Aug 24, 2017 · 13 comments
Open

Default value for enum parameters #27

sebthom opened this issue Aug 24, 2017 · 13 comments

Comments

@sebthom
Copy link

sebthom commented Aug 24, 2017

I was surprised to find that while

enum Schedule {
    Hourly(?minute:Int);
    // ...
}

is possible, this is not:

enum Schedule {
    Hourly(minute:Int=1); // characters 18-19 : Unexpected =
    // ...
}

Right now I have to do additional null-checks on enums with optional parameters like so

var instance = new MyClass(Schedule.Hourly());

function MyClass {
    var schedule:Schedule;

    public function new(var schedule:Schedule){
       // validate schedule
       switch (schedule) {
           case Hourly(minute):
                // check for null and apply a default value if found
                if(minute == null) {
                   minute = 1;
                   schedule = Schedule.Hourly(minute); 
                } else if (minute < 0 || minute > 59)  
                   throw "Hourly.Minute must be >= 0 and < 60";
                }
            // ...
       }
       this.schedule = schedule;
    }
}

instead of simply:

var instance = new MyClass(Schedule.Hourly());

function MyClass {
    var schedule:Schedule;

    public function new(var schedule:Schedule) {
       // validate schedule
       switch (schedule) {
           case Hourly(minute):
                if (minute < 0 || minute > 59)  
                    throw "Hourly.Minute must be >= 0 and < 60";
            // ...
       }
       this.schedule = schedule;
    }
}

Therefore I would request adding default value behaviour for enums as specified here https://haxe.org/manual/types-function-default-values.html

@delahee
Copy link

delahee commented Aug 24, 2017

I like this, every time we strip null possibilities from core language is a big win and lead to leaner code. +1

@nadako
Copy link
Member

nadako commented Aug 24, 2017

Since enum ctors with args are actually functions, maybe we can support this easily?

@RealyUniqueName
Copy link
Member

I'd like to see this implemented too.

@fullofcaffeine
Copy link

fullofcaffeine commented Sep 6, 2017

+1 @sebthom You could use tink_lang to relieve the pain for now: https://haxetink.github.io/tink_lang/#/declaration-sugar/complex-default-arguments.

@Simn
Copy link
Member

Simn commented Apr 17, 2018

I don't think this could be implemented without significant changes to how we handle default values. In our implementation, the default values are not part of the function type, but of the function itself. Enum constructors to not have actual functions, they are only typed as such. This would only work if we did call-site replacements, but we don't.

The only alternative is to respect this in every generator.

@back2dos
Copy link
Member

back2dos commented Sep 1, 2018

Enum constructors to not have actual functions, they are only typed as such.

On all platforms I've checked, they are actual functions. What's more, the type system also treats them as such, e.g. Some is of type Unknown<0> -> haxe.ds.Option<Unknown<0>>, meaning there must be a physical function somewhere and its implementation could treat default values accordingly.

@Simn
Copy link
Member

Simn commented Sep 3, 2018

Look, I'm not making this up to annoy you or avoid implementing this feature. You can look at haxe/macro/Type.hx and verify for yourself that the only place default values show up is TFunc, and the only place that shows up is the TFunction expression. You will also find that EnumField only has a Type, not an expression. Therefore, adding default values to enum fields would require changes to our enum field representation.

@back2dos
Copy link
Member

back2dos commented Sep 3, 2018

Sorry, I misunderstood. We're in agreement after all: if the default values are actually known, there is no problem.

You can look at haxe/macro/Type.hx and verify for yourself that the only place default values show up [emphasis added] is TFunc [...]

Regrettably, upon following your invitation I failed to verify your claim. As it turns out you already introduced a compile time representation of default values. It's not a pretty one, but it is sufficient and could be used for enums just as well. Just saying ...

@Simn
Copy link
Member

Simn commented Sep 3, 2018

I added that because we wanted to have the original syntax for generating documentation. For generating output, we need a properly typed representation and we cannot store that in metadata. I also wouldn't want to rely on such a hack to generate code in the first place. So the only option is change our representation of enum fields.

I'm not saying it can't be done, but I guarantee you it's more involved than you make it seem.

@grepsuzette
Copy link

Enums are now such a proeminent and powerful feature of Haxe (look at all the powerful variants of switch, it really changes the way we code), I definitely would love to see this implemented, one day or another.

@Simn
Copy link
Member

Simn commented Feb 10, 2024

There are two main related problems here:

  1. In order for Type.createEnum to work, default values will have to be considered at run-time. This means that the enum constructor functions need the usual if (inValue == null) inValue = defaultValue, for which they'll need the defaultValue expression. This requires some internal refactoring and updates to all target generators and/or run-time support code.
  2. minute:Int = 1 highlights the desire to have values stored as Int, not Null<Int>. However, for 1. we'll need a nullable value at the time of creation. This can easily be a hidden performance trap because a naive implementation that maps Hourly to Hourly would have to wrap the actual integer value in Null<Int> in order to call the creation function. This seems to make it necessary to have two creation functions: One for when we know that the passed value is indeed Int and one which does the null-check and then calls the former.

This might sound like premature optimization, but I really don't think that an implementation which wraps basic values only to then null-check and unwrap them would be acceptable.

Edit: Just to make this clear, all this bullshit is only required in order to support the dynamic/reflection paths. If we did call-site replacement of default values then both problems wouldn't exist. That's not how Haxe is designed though and it then would indeed be surprising if default values didn't work through reflection nonsense.

@back2dos
Copy link
Member

I really don't think that an implementation which wraps basic values only to then null-check and unwrap them would be acceptable.

  1. Is that not what we always do for optional primitive args though? How is that any different now?
  2. How is forcing the user to do the null checks themselves any better?

Reflection is not really the problem here (if it were I would suggest solving this in Type.createEnum, by accessing some rtti to pad the arg list to the correct length). Just do var x = SomeEnum.SomeConstructor and the default value is lost. Default values are part of the implementation, not the signature. For better or worse function a(x = true) {} and function b(x = false) {} have the same type.

Of course I'm all for a fast path for call site replacement, if possible.

I was also gonna suggest to use native default values on the platforms that support it, but the more I think about it, the more I wonder how well that can even fit with our beloved arg skipping.

@Simn
Copy link
Member

Simn commented Feb 19, 2024

Just do var x = SomeEnum.SomeConstructor and the default value is lost.

You're right, I was somehow thinking that it would turn the type into not having optional arguments, but it doesn't. In that case this is all doomed anyway.

Plus, I don't want to implement something that would change case V(x): V(x); from V(null) to V(default). This is already really strange for normal functions, but with structured data like enums these kind of value changes seem ridiculous...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants