[chunhtai]

Add style guide guideline for using enum in public API

Tracking issue: flutter/flutter#193960

Pre-launch Checklist

• I read RFC 000.0001: Taxonomy and followed the file and path naming and metadata standards.
• I read RFC 000.0002: Process and confirmed this proposal meets the threshold for a full RFC.
• I read and agree to the Code of Conduct.
• I signed the CLA.
• I have linked an issue from flutter/flutter with the label design doc.
• All existing and new tests are passing.
• I have enabled "Allow edits from maintainers" on this PR so the bot can automatically assign an RFC number (or I will run dart run bin/assign_rfc_number.dart locally when instructed).

If you need help, consider asking for advice on the #hackers channel on Discord.

[chunhtai]

@LongCatIsLooong

I can't find a good use case for sealed class without more work. let me know you have other ideas in mind

[LongCatIsLooong]

There aren't that many sealed classes in the framework yet if I recall, but there are several classes that could have been made sealed, from the top of my mind:

The Element class where an Element is either a ComponentElement or a RenderObjectElement
The TextSelection class where a selection is either collapsed (TextPosition) or a non-collapsed (uhh we don't have that subclass, but maybe NonCollapsedTextSelection).

[justinmc]

Here's one I wrote... that probably shouldn't have been a sealed class based on the breaking changes addressed in this document. https://github.com/flutter/flutter/blob/55f060daa5c8fa9c7c7aefa579daa2e59fbe15bf/packages/flutter/lib/src/widgets/system_context_menu.dart#L240

[chunhtai]

sealed class can't have a public constructor, so it seems like the compiler will ask for exhaustive match

sealed class MyEnum {
  // this is private by default
  const MyEnum();
  static const MyEnum foo = MyEnum2(0);
  static const MyEnum bar = MyEnum2(1);

  static const List<MyEnum> values = <MyEnum>[foo, bar];
}

class MyEnum2 extends MyEnum {
  const MyEnum2(this.index);
  final int index;
}

[LongCatIsLooong]

There aren't that many sealed classes in the framework yet if I recall, but there are several classes that could have been made sealed, from the top of my mind:

The Element class where an Element is either a ComponentElement or a RenderObjectElement
The TextSelection class where a selection is either collapsed (TextPosition) or a non-collapsed (uhh we don't have that subclass, but maybe NonCollapsedTextSelection).

[LongCatIsLooong]

I think extension types are cut out for this: not subtypeable, no need for hashCode and == boilerplates, has value semantics if the underlying type is a value type. If you rewrite the MyEnum example:

extension type const MyEnum._(int index) { 
  static const MyEnum foo = MyEnum._(0);
}

[loic-sharma]

Another benefit of extension types is that can be used in a const set or as the key in a const map. However, I'd recommend against extension types because they negatively impact the debugging experience: dart-lang/sdk#53461

Demo of debugging experience of extension types...

void main(List<String> arguments) {
  var e1 = E1.foo; // e1 = E1 (E1.foo)
  var e2 = E2.foo; // e2 = 1
  var e3 = E3.foo; // e3 = E3 (E3.foo)
}

enum E1 { foo, bar }

extension type const E2._(int index) {
  static const E2 foo = E2._(0);
  static const E2 bar = E2._(1);

  // Extension types can't declare members with the same name as a member declared by 'Object'.
  // Try specifying a different name for the member.
  // dart(extension_type_declares_member_of_object)
  // @override
  // String toString() => 'E2.$value';
}

@immutable
final class const E3._(final int index) {
  static const E3 foo = E3._(0);
  static const E3 bar = E3._(1);

  static const List<String> _names = ['foo', 'bar'];

  @override
  bool operator ==(Object other) {
    return other is E3 && other.index == index;
  }

  @override
  int get hashCode => index.hashCode;

  @override
  String toString() => 'E3.${_names[index]}';
}

[chunhtai]

extention type can cause some trouble for equality and toString for debugging, for example MyEnum(1) == MyAnotherEnum(1) will be true

[LongCatIsLooong]

Yeah unfortunately you won't be able to override toString. Also I just realized another library can easily implement your extension type (so if you can't turn that extension type into an enum without breaking those implementers)

[LongCatIsLooong]

We're not changing the breaking change policy right? So the wording here may have to change a bit to be put in the style guide.

[chunhtai]

This is not the exact wording. will rephrase when i actually put this in part of styleguide.

I was thinking to have a section in breaking change to talk about what we are not consider breaking change, which will include this and package private. WDYT

[LongCatIsLooong]

I was thinking to have a section in breaking change to talk about what we are not consider breaking change, which will include this and package private

Do you mean adding a new case to a closed enum? If we add a new value to TextDirection almost every app breaks so I doubt we will be able to get away with changes like that even if we have that in our breaking change policy.

[chunhtai]

Do you mean adding a new case to a closed enum

I think we all agree on this, not sure if we need a written rule for this case.

I am thinking adding a clause like: Adding an enum to a open enum class is not a breaking change.

[LongCatIsLooong]

Adding an enum to a open enum class is not a breaking change.

Hmm are we looking to change the breaking change definition in the breaking change policy? I mean today in dart I can't think of a way to introduce a static member to a final class from outside of the class, but I think the dart team is looking to allow introducing static members to an existing namespace (via extension methods), when that's added to the language, adding a new "case" can introduce name conflicts in client code and that should be a breaking change IMO.

[LongCatIsLooong]

Should AppleLifecycleState have been made an "open" enum? I think there are cases developers / plugin authors want to handle that exhaustively, and when a new case is added to AppleLifecycleState they want to be notified when compiling against newer framework versions?

[chunhtai]

I think it should be open. most people care about resume and pause only. IIRC the latest of the enum, detached, is added for very specific use case (for warmup). I think if we have to consider every possible usage. all the enum will need to be closed

[LongCatIsLooong]

That's true but I think in general I would prefer state machine states to be exhaustive, as the state transition is usually an important part of the contract.
What if I introduce a new state (because, say, Apple introduced a new state), and the contract becomes "after pause, you may either enter this new state or resumed", so if a client fails to handle this new case it will fail to update it's own state and gets stuck in a paused state?

[justinmc]

Overall I'm totally on board with this, just some comments on the details. Good call splitting it out.

[justinmc]

Do we have an issue for these?

[chunhtai]

we haven't added one yet for a long time, but mostly due to we stop adding more platform and prioritize mobile. but they are a lot likely to expand if we do so, and I imagine it will break a bunch of people

[loic-sharma]

For pluggable platforms, I plan to deprecate TargetPlatform in favor of a non-exhaustive enum that's open for extension by external code. Design TBD though :)

[justinmc]

Here's one I wrote... that probably shouldn't have been a sealed class based on the breaking changes addressed in this document. https://github.com/flutter/flutter/blob/55f060daa5c8fa9c7c7aefa579daa2e59fbe15bf/packages/flutter/lib/src/widgets/system_context_menu.dart#L240

[loic-sharma]

I would consider rewording this. Perhaps something like: If the set of values will not change....

For example, today iOS text fields have a fixed set of context menu buttons, which we could represent as an enum. However, iOS might add a new button in a future version. This is a case where you should use non-exhaustive enums.

[loic-sharma]

Same comment for the overview above where it states "is closed by nature".

[chunhtai]

For example, today iOS text fields have a fixed set of context menu buttons, which we could represent as an enum. However, iOS might add a new button in a future version. This is a case where you should use non-exhaustive enums.

I think context menu item button type is not closed by nature so the statement still hold.

[chunhtai]

though I think it should be enum because it is likely be handled exhaustively

[loic-sharma]

though I think it should be enum because it is likely be handled exhaustively

I agree all cases should be matched, but enum's unreachable_switch_case lint disallows adding an default case to an exhaustive switch. You cannot "future proof" a switch on an enum. For example:

enum MyEnum { foo, bar }

String stringify(MyEnum e) {
  return switch (e) {
    MyEnum.foo => 'foo',
    MyEnum.bar => 'bar',
    // dart(unreachable_switch_case)
    // This case is covered by the previous cases.
    // Try removing the case clause, or restructuring the preceding patterns.
    _ => 'future proof against future MyEnum values',
  };
}

This forces users of the enum to be broken when new value are added, which is why I'd recommend against enums if new values might be added.

[loic-sharma]

On second thought, what do you think of simplifying this to:

Suggested change

	return '${objectRuntimeType(this, 'MyEnum')}(name: MyEnum.$name)';
	return 'MyEnum.$name';

Benefits:

• The <type>.<name> pattern mirrors Enum.toString()'s behavior better
• objectRuntimeType is overkill if nothing extends MyEnum.

[chunhtai]

I rewrote this part to only focus on enum giving out to the developer to consume, otherwise they should all be enum-like. Let me know what you think @loic-sharma , @LongCatIsLooong , @justinmc

[chunhtai]

This however has risk for developer override class though. If we want to be more strict, they we will likely need to go back to broader set like layout related enum such as MainAxisAlignment.

[LongCatIsLooong]

Would it be possible to combine bullet point 1 and 2 since they seem to be saying the same thing? Basically something like

Use closed enum in public API only if

• clients only produce enum values and can never consume the enum value (i.e., the framework is the sole consumer, so we can't let the client send us something we don't understand).
• you have reasons to believe that the exhaustiveness needs to be part to the API contract (e.g., states of a state machine).

I'm for prefer exposing non-exhaustive enums when in doubt: if we use a final class then later decides enum is more appropriate, making that change can't be source breaking it seems?

[bparrishMines]

For flutter/packages we want to be able to extend the enum-like class with platform specific values. (e.g. ImageFormat can be extended to AndroidImageFormat for Android only formats.)

[bparrishMines]

Here is also a doc that goes into more details about enums in federated plugins: flutter.dev/go/extensible-enums-plugins

[chunhtai]

updated the wording to be a suggestion rather than a requirement

[bparrishMines]

If final is a suggestion then I think we could also

1. Recommend base be used instead in that case.
2. Recommend adding @protected above the constructor when base is used.

[LongCatIsLooong]

Enums are generally simple data classes and I found it a little strange to subtype data classes. Are there benefits for opening up the type for extending, over a final class with a public constructor?

[bparrishMines]

For federated plugins specifically, yes. The main benefit is discoverability and readability. For example, the camera plugin supports 7 distinct platforms and contains an ImageFormat enum. If each platform wanted to add there own ImageFormats, it would be harder for the code writer/reader to know which formats are platform specific. e.g.

Future<void> aMethod(ImageFormat format) async {
  // Works on all platforms.
  await cameraController.takePicture(ImageFormat.jpg);
  await cameraController.takePicture(ImageFormat.png);

  // Only works on iOS.
  await cameraController.takePicture(ImageFormat.heic);

  // Only works on Android.
  await cameraController.takePicture(ImageFormat.nv12);
}

From experience, this with support for 7+ platforms doesn't scale very well.

[chunhtai]

I just realized you can get around this with extension

final class SystemUiOverlayStyle {
  const SystemUiOverlayStyle();
}
// In library B:
extension CustomOverlayStyles on SystemUiOverlayStyle {
  static const SystemUiOverlayStyle brandDark = SystemUiOverlayStyle();
  bool get isBrandDark => identical(this, brandDark);
}
void main() {
  const SystemUiOverlayStyle style = CustomOverlayStyles.brandDark;
  print(style.isBrandDark);
}

[bparrishMines]

I just realized you can get around this with extension

final class SystemUiOverlayStyle {
  const SystemUiOverlayStyle();
}
// In library B:
extension CustomOverlayStyles on SystemUiOverlayStyle {
  static const SystemUiOverlayStyle brandDark = SystemUiOverlayStyle();
  bool get isBrandDark => identical(this, brandDark);
}
void main() {
  const SystemUiOverlayStyle style = CustomOverlayStyles.brandDark;
  print(style.isBrandDark);
}

Actually I think that option is better than extending. The only thing I think you lose out on is

void aMethod(SystemUiOverlayStyle style) {
  // The name 'CustomOverlayStyles' isn't a type and can't be used in an 'is' expression. 
  if (style is CustomOverlayStyles) {
  }
} 

Though I don't think this is a common use case and I think there a benefit to not have to worry about full external implementations.

[bparrishMines]

This seems very easy to mess up since we have to write this by hand. Instead of an int index, can we not just make the String name the value.

[chunhtai]

I vaguely remembered there is a reason we went with int, but I couldn't remember now. cc @loic-sharma, do you remember why?

[LongCatIsLooong]

Dart enums can carry types that are not ints, similarly I think we should not limit what the underlying type is, for instance if I want to define an AutofillHint non-exhaustive enum it makes a lot more sense to use string as the underlying type.

[chunhtai]

I think main benefit is comparison, also gives enum orders which is useful in some cases. For example, _RouteLifecycle in navigator.dart.

I updated to prefer string by default

[bparrishMines]

If MyEnum(...) is public, what prevents external APIs from creating indexes that overlap?

[chunhtai]

correct, we can't do anything about it. I think the best We can do is to write a custom lint for this

[bparrishMines]

I think that's one reason why using a String that matches the field name is more future proof. Collisions with a String is less likely for external APIs and if they do collide, then it's more likely they were representing the same value anyways.

[justinmc]

In the Dash Forum on this RFC, @reidbaker was talking about using an int as an enum, similar to what Android does in some of the APIs it expects to add values to in the future. I think this is effectively the same thing as using static const values on a class as proposed by this RFC.

// A user's code.
void main(List<String> arguments) {
  final MyIntEnum myIntEnum = MyIntEnum.foo();

  final bool isCool = switch (myIntEnum) {
    MyIntEnum.foo => true,
    MyIntEnum.bar => false,
    // Or instead of throwing, I could gracefully handle an unknown future value.
    _ => throw Exception('Uknown value of MyIntEnum!'),
  };

  print('Is myIntEnum cool? $isCool.');
}

// Part of the Flutter SDK for the sake of this example.
class MyIntEnum {
  final int _index;

  const MyIntEnum.foo() : _index = 0;

  const MyIntEnum.bar() : _index = 1;

  const MyIntEnum.baz() : _index = 2;

  // Could add more in the future!
  // It's up to users whether they want to be broken by a new value or not.

  @override
  bool operator ==(Object other) {
    if (identical(this, other)) return true;

    return other is MyIntEnum &&
        other.runtimeType == runtimeType &&
        other._index == _index;
  }

  @override
  int get hashCode => _index;
}