Generate real enums instead of classes with static members

[Levi-Lesches]

Here's the current behavior:

// No 0 value to ensure we always set a status
typedef enum CanStatus {
  OK = 1,
  SOCKET_CREATE_ERROR = 2,
  EXAMPLE = 1;
} CanStatus;

// generated FFI bindings: 
abstract class CanStatus {
  static const int OK = 1;
  static const int SOCKET_CREATE_ERROR = 2;
  static const int EXAMPLE = 3;
}

I understand that ffigen can't make this an actual enum because CanStatus.OK == CanStauts.EXAMPLE, and you can't override Enum.operator==. But the current approach has another problem:

String getStatusName(CanStatus status) { ... }
void main() => print(getStatusName(CanStatus.OK);  // Error: int is not of type CanStatus

The type gets completely erased and the original name of the enum can't be used! You could use

typedef MyCanStatus = int;

But you can't use the original name, because then CanStatus.OK becomes int.OK, which isn't defined. This approach isn't great either because then Dart thinks status = 30 is okay, when that value isn't even defined.

Instead, this would be preferable:

class CanStatus {
  final int value;
  const CanStatus._(this.value);

  static const OK = CanStatus(1);
  static const SOCKET_CREATE_ERROR = CanStatus(2);
  static const EXAMPLE = CanStatus(3);
}

That way this would be recognized as an "enum-like class" that would enable using the type CanStatus directly and enable exhaustiveness checking for switch statements and expressions, like getStatusName above.

[dcharkes]

Instead, this would be preferable:

class CanStatus {
  final int value;
  const CanStatus._(this.value);

  static const OK = CanStatus(1);
  static const SOCKET_CREATE_ERROR = CanStatus(2);
  static const EXAMPLE = CanStatus(3);
}

That way this would be recognized as an "enum-like class" that would enable using the type CanStatus directly and enable exhaustiveness checking for switch statements and expressions, like getStatusName above.

I like this idea!

As an alternative, we could wait on extension types dart-lang/language#2727. That should give us the same static typing benefits, but without the runtime overhead of actually allocating an object. cc @eernstg.

[Levi-Lesches]

Extension types would be nice in this case and I considered them too but I didn't see anything in that discussion indicating they would support exhaustiveness checking (I don't see why they wouldn't, but it wasn't really discussed there). Given that this is a simple enough rewrite with immediate benefits (and hopefully non-breaking), I would hope it's worth doing if extension types are still a while away.

[dcharkes]

Given that this is a simple enough rewrite with immediate benefits (and hopefully non-breaking)

It's a breaking change, we might have users putting enum values in ints and vice versa... But we're not really shy of doing breaking changes. Worst case we can make it opt-in at first via a config option.

If we create such an enum-like class, I would also advocate for adding an extra List<String> field labels (or String if there are no conflicting int numbers), and adding using it in the toString() so that we don't have to look up the int->const field mapping when debugging.

[eernstg]

Just wondering, if an enum-like class is relevant then why not go all in?:

enum CanStatus {
  ok(1),
  socketCreateError(2),
  example(1);

  final int value;
  const CanStatus(this.value);
}

String getStatusName(CanStatus status) {
  throw 'NotImplemented';
}

void main() {
  print(getStatusName(CanStatus.ok));
}

(OK, I renamed the enum-ish values to use Dart style identifiers, I don't know if that's good style in this context.)

I modeled CanStatus.example to have the value 1 and still be distinct from Canstatus.ok. If they should be identical then we can do this:

enum CanStatus {
  ok(1),
  socketCreateError(2);

  static const example = ok; // Just an alias for an existing value.

  final int value;
  const CanStatus(this.value);
}

[Levi-Lesches]

It's a breaking change

Oh I was wrong in my initial assessment then: because you can't currently use the enum type, this is necessarily a breaking change almost all the time because a simple void print(int status) will need to turn into void print(CanStatus status)

I modeled CanStatus.example to have the value 1 and still be distinct from Canstatus.ok. If they should be identical then we can do this:

Yeah, the original reason enums have been turned down in the past is because two enum values in C can be identical but not in Dart. I was trying to think of a way to solve this with enhanced enums but I didn't realize it was that simple!

[eernstg]

Here is a variant that shows how this would work with extension types:

extension type const CanStatus._(int value) {
  static const ok = CanStatus._(1);
  static const socketCreateError = CanStatus._(2);
  static const example = ok; // With an extension type they must be the same.
}

String getStatusName(CanStatus status) => switch (status) {
    CanStatus.ok => "ok",
    CanStatus.socketCreateError => "socketCreateError",
    /* `CanStatus.example => ...` would be unreachable */
    _ => 'Unexpected value',
  };

void main() {
  print(getStatusName(CanStatus.ok)); // Typed.
  print(getStatusName(30 as CanStatus)); // But we can bypass the constructor.
}

In this case we only have the int at run time, but there is no notion of exhaustiveness, and we can introduce a rogue value (like 30) by means of a type cast.

One consequence of this is that we cannot distinguish the ok value (which is the int 1) and the example value (which is the exact same object at run time).

If the support for enum-like classes is extended to deal with enum-like extension types then we could presumably have a check for covering all the "nicely declared" values in the switch (that is, all the non-rogue values), but we would still have to have a default clause for the potential rogue value.

So this is a new notion of being exhaustive where we treat different subsets of the values that we may encounter at run time in different ways (you could say that we partition the set of possible objects and then handle each partition separately, and the declared values (static const in CanStatus) must be covered; other values are handled via _).

[dcharkes]

Thanks @eernstg !

enum CanStatus {
  ok(1),
  socketCreateError(2),
  example(1);

@eernstg are ok(1) and example(1) equal? They have to be. Because C returns us an int, so FFIgen will only instantiate ok(1). So we have to treat ok(1) and example(1) as equal (losing the information that the dev gave us if they manually instantiated one of the two).

One consequence of this is that we cannot distinguish the ok value (which is the int 1) and the example value (which is the exact same object at run time).

This is what we need. We can't distinguish ok from example when we get a value from C.

potential rogue value

Ah, with the cast we can create rogue values. Hm, that makes extension types less nice than generating a class with a private constructor for exhaustiveness checking. Don't we have extension types with private constructors that resist casts? =)

[eernstg]

Ah, with the cast we can create rogue values. Hm, that makes extension types less nice than generating a class with a private constructor for exhaustiveness checking. Don't we have extension types with private constructors that resist casts? =)

That's a fundamental property of extension types. An extension type may look a lot like a class with one, final instance variable—like a "wrapper object" that contains another object, which is the "wrapped object"—but there is no wrapper object at run time, and hence there's no protection barrier.

In particular, with an actual wrapper class we can check that the value of an instance variable satisfies some constraints (they would typically be checked by the constructor). When the wrapper does not exist at run time, a dynamic type check can only verify that the given object has the type which is the representation type of the claimed extension type, and the dynamic type check has no way to determine whether we did or did not execute an extension type constructor where the wrapped object was vetted.

For instance, if we are checking whether (4 as dynamic) is Prime where Prime is an extension type whose constructors verify that the wrapped object is indeed a prime number then we can't see the difference between the situation where the given int "was wrapped in a Prime wrapper object" because we evaluated new Prime(4) and ran the checks in the Prime constructor, or the given object is just an arbitrary int, and we're now (roguely) claiming that it is a Prime by means of the type cast. The two situations look the same at run time.

So you can introduce some static checks based on extension types, but it's a matter of manual discipline to ensure that we don't use casts in ways that allow us to skip invocations of constructors whose behavior is needed for correctness. Alternatively (as in the case with the enumerated values from C), we may inherently have the potential for rogue values, and then we just have to introduce some code that handles them.

[dcharkes]

That is true, the C code could return rogue values, if we want to support exhaustiveness checking, we should check the int value at the moment we get the value from C code (as a return value or field access) by running it through a check.

[eernstg]

Perhaps you could offer both approaches: (1) An FFI type like a C enum is represented by a Dart int which is accessed statically via an "enum-like extension type", and we could have a specialized lint ensuring that switches on that extension type will mention all the declared values, and that it also has a default case handling rogue values. (2) An FFI type like a C enum is represented by a Dart enum object that has a value instance variable holding the C value, and it is enforced at the time where the value is received from the foreign code that the given value is among the known ones, and the Dart code will receive the corresponding enum object. (Perhaps null can be used to represent "a rogue value received from C", perhaps the situation should cause an exception to be thrown.)

[dcharkes]

I modeled CanStatus.example to have the value 1 and still be distinct from Canstatus.ok. If they should be identical then we can do this:

enum CanStatus {
  ok(1),
  socketCreateError(2);

  static const example = ok; // Just an alias for an existing value.

  final int value;
  const CanStatus(this.value);
}

I'm leaning towards this.

It would however be nice to override toString() to show both "ok" and "example" on CanStatus.ok and CanStatus.example.

And then throw an UnsupportedError if we get an int-value that should not occur with the enum. I believe it to be an error, not an exception, when an int-value not defined in an enum is returned. Either in the C library itself or the generated bindings for example if the bindings are generated for an older version of the C library. It's a programming error, and the code should not recover, the program should be fixed.

If this approach is too strict, we can add a config option later that will generate extension types on int instead (for the enum names that match the filter in the config).

@mannprerak2, @liamappelbe, @Levi-Lesches any thoughts, concerns, ideas?

@Levi-Lesches If the rest is happy with this approach, would you be willing to contribute a PR? (Note the master branch has recently switched to track Dart dev releases, but we should be able to make this change against the stable branch in this repo, see https://github.com/dart-lang/ffigen/wiki/Branches.)

[Levi-Lesches]

LGTM, as this would let me use the CanStatus type and enable exhaustiveness checking. I'd be happy to try a PR sometime in the next week.

[eernstg]

It would however be nice to override toString() to show both "ok" and "example" on CanStatus.ok and CanStatus.example.

I'm not sure I understand, I suppose that wouldn't mean assert(CanStatus.ok.toString() == 'CanStatus.ok, CanStatus.example')? When we have identical(CanStatus.ok, CanStatus.example), we can't have a different return value for toString() depending on whether we're accessing the object which is the value of CanStatus.ok and the value of CanStatus.example using one or another expression.

We can use an extension type to get this effect, except that extension types can't redefine toString. So let's define a different method toString1 and make that method behave differently based on the static type:

enum CanStatus {
  ok(1),
  socketCreateError(2);

  static const example = _CanStatusExample(ok);

  final int value;

  const CanStatus(this.value);

  String toString1() => toString();
}

extension type const _CanStatusExample(CanStatus status) implements CanStatus {
  String toString1() => 'CanStatus.example';
}

// This could be in some other library.

void main() {
  print(CanStatus.ok.toString1()); // 'ok'.
  print(CanStatus.example.toString1()); // 'example'.
  print(identical(CanStatus.ok, CanStatus.example)); // 'true'.

  String doSwitch(CanStatus status) => switch (status) {
    CanStatus.ok => 'ok',
    CanStatus.socketCreateError => 'socketCreateError',
    // OK, this switch is already exhaustive.
  };

  print(doSwitch(CanStatus.example)); // 'ok'.
}

Note that we make _CanStatusExample a private type because it's intended to be the static type of one particular object, and it would be a misunderstanding to obtain a reference to any other object with that type.

We could do something else, however:

enum CanStatus {
  ok(1),
  socketCreateError(2);

  static const example = ok; // Just an alias for an existing value.

  final int value;
  const CanStatus(this.value);

  static final _nameMap = values.asNameMap()
    ..['CanStatus.example'] = CanStatus.example;
  
  static CanStatus? byName(String s) => _nameMap[s];
}

void main() {
  print(CanStatus.byName('CanStatus.example') == CanStatus.ok); // 'true'.
}

[dcharkes]

Sorry for the ambiguity! 🤓

I'm not sure I understand, I suppose that wouldn't mean assert(CanStatus.ok.toString() == 'CanStatus.ok, CanStatus.example')?

No, that is exactly what I want. It has two identifiers, and the toString should reflect that.

we can't have a different return value for toString() depending on whether we're accessing the object which is the value of CanStatus.ok and the value of CanStatus.example using one or another expression.

That is fine, these are aliases of each other, I just want the toString to reflect all aliases instead of just the first one.

[eernstg]

OK, but then it should work just fine with a plain enum (especially if it's generated ;-).

enum CanStatus {
  ok(1),
  socketCreateError(2);

  static const example = ok; // Just an alias for an existing value.

  final int value;
  const CanStatus(this.value);

  @override
  toString() {
    if (this == ok) return 'CanStatus.ok, CanStatus.example';
    return super.toString();
  }
}

void main() => print(CanStatus.example);

[Levi-Lesches]

I'd be happy to try a PR sometime in the next week.

...It's been a long week. I'll start working on this now.

[Levi-Lesches]

Well, that was probably a little simpler than I thought. I spent a few hours on it, here's what I've got.

The simple case:

enum Colors {
  red,
  green,
  blue,
};

enum Colors {
  // ===== Unique members =====
  red(0),
  green(1),
  blue(2);

  // ===== Aliases =====

  // ===== Constructor =====
  final int value;
  const Colors(this.value);

  // ===== Override toString() for aliases =====
  @override
  String toString() {
    return super.toString();
  }
}

The complex case:

typedef enum CanStatus {
  ok = 1,
  socketCreateError = 2,
  example = 1,
} CanStatus;

enum CanStatus {
  // ===== Unique members =====
  ok(1),
  socketCreateError(2);

  // ===== Aliases =====
  static const example = ok;

  // ===== Constructor =====
  final int value;
  const CanStatus(this.value);

  // ===== Override toString() for aliases =====
  @override
  String toString() {
    if (this == ok) return "CanStatus.ok, CanStatus.example";
    return super.toString();
  }
}

@eernstg @dcharkes What do you think? My code is at #1198