dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
157 stars 44 forks source link

[ffigen] New idea for enums #1735

Open Levi-Lesches opened 6 days ago

Levi-Lesches commented 6 days ago

I'm back and still thinking about enums! A problem we hit last time in #1217 was that generating Dart enums for native enums isn't always the right choice. As a workaround, we offered an opt-out, but what if we can have the best of both worlds? Instead of enums, how about extension types?

extension type const FakeEnum(int value) { 
  static const option1 = FakeEnum(1);
  static const option2 = FakeEnum(2);
  static const option3 = FakeEnum(4);
  static const option4 = FakeEnum(8);

  FakeEnum operator |(FakeEnum other) => FakeEnum(value | other.value);

  bool has(FakeEnum other) => (value & other.value) > 0;
}

void methodWithOptions(FakeEnum flags) {
  print("Has option1: ${flags.has(FakeEnum.option1)}");
  print("Has option2: ${flags.has(FakeEnum.option2)}");
  print("Has option3: ${flags.has(FakeEnum.option3)}");
  print("Has option4: ${flags.has(FakeEnum.option4)}");
}

void main() {
  final flags = FakeEnum.option1 | FakeEnum.option2 | FakeEnum.option3;
  methodWithOptions(flags);  // still type safe
  print("-------------------");
  methodWithOptions(FakeEnum(5));  // can be manually worked around
}

I'm not 100% sure this is inherently helpful, but one pattern I noticed is that packages that opt out of Dart enums lose the custom type names, and their APIs are covered in ints. So even if not everyone needs operator | or bool has(), they can at least use the extension type to refer to their constants and wrap new integers at runtime.

@dcharkes, thoughts?

liamappelbe commented 6 days ago

Sounds neater, but this will mean another breaking change to enums, shortly after the last one. Might need to make it opt-in in the config, but then we'd be supporting 3 different representations for enums.

Levi-Lesches commented 6 days ago

Agreed it's not great timing. I actually had this idea a little bit ago but held onto it for that reason. Still, if this is something people would want I'm happy to give it a shot.

Maybe it could be a good idea to gather some feedback first to see what people would expect from such a feature, and whether there is still benefit to having plain integers even after this (as long as .value is public I don't see one)

dcharkes commented 5 days ago

Taking a helicopter view here, there are two types of "enum"s in native code:

  1. The ones which are actual enums, and
  2. the ones which are used as flags, ints, etc.

Our current approach has a dedicated solution for both of them, but the solution for 2. has int as public API.

Using extension types could improve the API for users using flags. (Most likely they would actually need a operator | on that extension type to combine flags.)

Using extension types for use case 1. does not have a lot of drawbacks indeed.

The current system with a configuration option to select between the two requires more config and rerunning the generator when the config changes, which makes FFIgen less plug-n-play. One of our north star goals is to make it work out of the box as much as we can. 👍

I'd suggest indeed making it a config option. However, we should be aiming the deprecate the existing two approaches then. So print a warning/error on the old two approaches that we will deprecate them after one or two stable releases.

And I'd want the input from some users. 😄 @brianquinlan @stuartmorgan WDYT about using extension types on int for enums?

Levi-Lesches commented 5 days ago

Using extension types for use case 1. does not have a lot of drawbacks indeed.

Unfortunately there is a trade-off. In the case of exhaustive enums, we want to specifically prohibit new instances and allow exhaustive switch cases. In the case of bit flags, we want to allow new instances, which would not allow for exhaustive checks. And as much as I don't love having two options and needing people to pick, I do think having exhaustive switch cases for the enums that warrant it is very valuable.

It's not as clean as one unified solution, but if we're thoughtful about a sensible default and keep the config simple, maybe it'll still be a smooth experience

HosseinYousefi commented 5 days ago

These are two different usecases, enums and flags. So there is no unified solution for both. If we want to simplify config, FFIgen could look at all the values of an enum and if they are powers of two, log and generate it as an extension type.

brianquinlan commented 5 days ago

WDYT about using extension types on int for enums?

Objective-C enums are (AFAIK) always integral types e.g.

typedef NS_ENUM(NSInteger, NSURLSessionWebSocketCloseCode)
{
    NSURLSessionWebSocketCloseCodeInvalid =                             0,
    NSURLSessionWebSocketCloseCodeNormalClosure =                    1000,
    NSURLSessionWebSocketCloseCodeGoingAway =                        1001,
    ...
};

So a NSURLSessionWebSocketCloseCode can be one of the above constants or any other integer.

So an extension type seems wrong because it limits the sets of operations that you can do on the enum when, in native Objective-C, they are just integers.

Levi-Lesches commented 5 days ago

So an extension type seems wrong because it limits the sets of operations that you can do on the enum when, in native Objective-C, they are just integers.

But you can always access the original .value? We can also have the extension type implements int so you really don't lose anything. And of course, you can also make any integer into the extension type at runtime. The only point of the extension type is to just clean up the APIs that use the enum

About the objective C enums, I believe we already had this discussion and it turns out clang doesn't report which type of enum it is in the API we are using, which severely limits how smart we can be by default. I do like the power of two rule though

brianquinlan commented 5 days ago

So an extension type seems wrong because it limits the sets of operations that you can do on the enum when, in native Objective-C, they are just integers.

But you can always access the original .value?

I have never used extension types so I didn't know that you could do this. This seems fine to me then - but the extension type is really just a namespace to attach constants to then, right?

Levi-Lesches commented 5 days ago

Yep, really just that. It'll help keep the APIs clear, dart docs more helpful, code that tries to use them will have the enum name more often, and users can write extension methods for them.

The | and has() methods in my example were really just examples, we don't have to provide them because users will be able to use | and & directly

There is something of a question whether we should use implements int, as that's the difference between users being about to use the extension type as an int directly or having to unwrap it. The reason why it could be good to not include it is that it forces your APIs to be type safe, but I think it's safe to include it, because we're already in control of generation, so this would just give convenience to the user. And of course, methods we generate will take the extension type directly

stuartmorgan commented 4 days ago

There is something of a question whether we should use implements int, as that's the difference between users being about to use the extension type as an int directly or having to unwrap it.

What happens in the other direction; if it implements int can someone pass a raw int directly to an API that takes an enum type?

Levi-Lesches commented 4 days ago

Short answer: no, but you can wrap the value, even at runtime.

Long answer,

extension type MyEnum(int x) implements int { }

void a(MyEnum x);
void b(int x);

void main() {
  final intVal = 1;
  final enumVal = MyEnum(1);

  a(intVal);  // error
  a(enumVal);  // okay
  a(MyEnum(intVal));  // also okay

  b(intVal);  // okay 
  b(enumVal);  // okay
}

This raises an interesting point, that enums should be generated as extension types, and APIs should be left as integers for maximum compatibility. But when you think about it, there's no value from doing that over the current approach where we already generate plain integers.

The whole benefit of switching to extension types would be that users would see a function and actually know which enum type it accepts, be able to click on it in dart docs, and API authors can write extension methods that only apply to some enums and not others. I think that value is worth the breaking change.

stuartmorgan commented 4 days ago

APIs should be left as integers for maximum compatibility

I disagree; not being able to pass raw ints without explicitly "converting" (wrapping) them is a feature IMO, not a bug.

Levi-Lesches commented 4 days ago

(just to clarify, that's my stance too, as well as the other points in my next paragraph)