Generate real enums instead of enum-like classes #1198

Levi-Lesches · 2024-06-07T22:45:11Z

Fixes #446. @dcharkes @eernstg @mannprerak2 @liamappelbe from that issue.

For the given enum (with defined values and duplicates):

// No 0 value to ensure we always set a status
typedef enum CanStatus {
  ok = 1,
  socketCreateError = 2,
  example = 1;
} CanStatus;

Here's the current behavior:

// generated FFI bindings: 
abstract class CanStatus {
  static const int ok = 1;
  static const int socketCreateError = 2;
  static const int example = 3;
}

And here's the new behavior:

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();
  }
}

Also, empty enums are now a sealed class instead of just an abstract class.

I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Bump the major version as this will be a breaking change (enum values are no longer ints)
Add a section to the changelog
Document new fields and methods
Run tests

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

google-cla · 2024-06-07T22:45:15Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Levi-Lesches · 2024-06-09T23:49:19Z

As an aside, I decided to change BindingStringType.enumClass to .enum_, but noticed I didn't break anything in the process. Looking a bit further, BindingString.type isn't used anywhere in package:ffigen, and commenting it out doesn't result in any errors whatsoever. Should it be removed, or is it being used somewhere else?

There is a comment saying Meta data, (not used for generation), so I guess it should stay in, I just got curious

Levi-Lesches · 2024-06-10T01:41:55Z

The current version is already 13.0.0-wip, so I didn't bump the version but added my changes to the CHANGELOG instead.

Everything should be done and ready to go, after regenerating all the test enums, all tests are passing again! Ready for a final review.

One note, I had to exclude test/native_objc_test/** in analysis_options.yaml. I originally marked it as "for development" because I'm on Windows, but I've since noticed that all the other objective-C tests are also excluded -- should I keep it excluded or include it again?

dcharkes

LGTM!

Thanks @Levi-Lesches! ❤️

github-actions · 2024-06-10T07:17:16Z

PR Health

Changelog Entry ✔️

Details

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️

Details

File	Coverage
pkgs/ffigen/example/libclang-example/generated_bindings.dart	💔 Not covered
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart	💔 Not covered
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart	💔 Not covered
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/binding_string.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/enum_class.dart	💔 Not covered
pkgs/ffigen/lib/src/code_generator/utils.dart	💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ⚠️

Details

// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/ffi/example/main.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/config_provider/config_spec.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_cli/lib/src/api/builder.dart
pkgs/native_assets_cli/lib/src/api/linker.dart
pkgs/native_assets_cli/test/model/checksum_test.dart

This check can be disabled by tagging the PR with skip-license-check.

Package publish validation ✔️

Details

Package	Version	Status
package:ffi	2.1.2	already published at pub.dev
package:ffigen	13.0.0-wip	WIP (no publish necessary)
package:jni	0.9.3-wip	WIP (no publish necessary)
package:jnigen	0.9.2	already published at pub.dev
package:native_assets_cli	0.6.1-wip	WIP (no publish necessary)
package:objective_c	1.1.0-wip	WIP (no publish necessary)
package:swiftgen	0.0.1-wip	WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

dcharkes · 2024-06-10T07:17:31Z

FYI @liamappelbe (no idea if enums are common in Objective-C code).

dcharkes · 2024-06-10T09:47:53Z

FYI, you might see some merge issues with

Enum integer type via libclang. #1187

Levi-Lesches · 2024-06-10T14:35:30Z

@dcharkes I formatted the two files that were failing the CI, can you re-approve?

coveralls · 2024-06-10T14:48:02Z

coverage: 91.215% (+0.1%) from 91.104%
when pulling 3f226ec on Levi-Lesches:real-enums
into acf7d21 on dart-lang:main.

dcharkes · 2024-06-10T14:51:51Z

cc @mannprerak2 If you'd want to review as well.

dcharkes · 2024-06-10T15:19:02Z

@mannprerak2 Should we merge this one first, or yours? (Who gets to do the merge confict? 😆)

mannprerak2 · 2024-06-10T15:20:12Z

Let's merge this first 👍

LeviLesches-KDAB added 3 commits June 7, 2024 16:25

Ignore native ObjC tests for development

cd4fcc3

Native enums are now Dart enums instead of classes

8c70f75

Formatting

e896612

github-actions bot added the package:ffigen label Jun 7, 2024

Levi-Lesches mentioned this pull request Jun 7, 2024

Generate real enums instead of classes with static members #446

Closed

LeviLesches-KDAB added 2 commits June 9, 2024 21:29

All tests passing + new test for duplicates

4b67448

Updated CHANGELOG

2970798

Removed print statement

f0d7f96

dcharkes approved these changes Jun 10, 2024

View reviewed changes

Formatting

3f226ec

mannprerak2 approved these changes Jun 10, 2024

View reviewed changes

dcharkes merged commit ecb8b7e into dart-lang:main Jun 10, 2024
17 checks passed

Levi-Lesches deleted the real-enums branch June 10, 2024 20:04

This was referenced Jun 10, 2024

Enum names #322

Closed

Ffi enums #1202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate real enums instead of enum-like classes #1198

Generate real enums instead of enum-like classes #1198

Levi-Lesches commented Jun 7, 2024 •

edited

Loading

google-cla bot commented Jun 7, 2024

Levi-Lesches commented Jun 9, 2024 •

edited

Loading

Levi-Lesches commented Jun 10, 2024 •

edited

Loading

dcharkes left a comment

github-actions bot commented Jun 10, 2024

dcharkes commented Jun 10, 2024

dcharkes commented Jun 10, 2024

Levi-Lesches commented Jun 10, 2024

coveralls commented Jun 10, 2024

dcharkes commented Jun 10, 2024

dcharkes commented Jun 10, 2024

mannprerak2 commented Jun 10, 2024

Generate real enums instead of enum-like classes #1198

Generate real enums instead of enum-like classes #1198

Conversation

Levi-Lesches commented Jun 7, 2024 • edited Loading

google-cla bot commented Jun 7, 2024

Levi-Lesches commented Jun 9, 2024 • edited Loading

Levi-Lesches commented Jun 10, 2024 • edited Loading

dcharkes left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2024

PR Health

Changelog Entry ✔️

Coverage ⚠️

License Headers ⚠️

Package publish validation ✔️

dcharkes commented Jun 10, 2024

dcharkes commented Jun 10, 2024

Levi-Lesches commented Jun 10, 2024

coveralls commented Jun 10, 2024

dcharkes commented Jun 10, 2024

dcharkes commented Jun 10, 2024

mannprerak2 commented Jun 10, 2024

Levi-Lesches commented Jun 7, 2024 •

edited

Loading

Levi-Lesches commented Jun 9, 2024 •

edited

Loading

Levi-Lesches commented Jun 10, 2024 •

edited

Loading