[ffigen] Generate enum getters for struct members

[Levi-Lesches]

trafficstars

Fixes #1407 where enums in structs were still being generated as integers. This works by changing the bindings like so:

final class MyStruct 
  @ffi.Int()
-  external int enumMember;
+  external int _enumMember;
+
+ MyEnum get enumMember => MyEnum.fromValue(_enumMember);
}

And of course, this PR respects any enums set to be generated as integers instead of actual enums by keeping the current behavior.

I wasn't sure exactly what to do about cases like these:

enum Enum1 { ... }

struct Struct1 [
  Enum1* enum1;
}

ffigen still generates Pointer<Int> instead of Pointer<Enum1>, but I'm not sure I see a way to get a Pointer<DartEnum> out of it. Since fromValue is static, you can't even have a generic extension method in dart:ffi or package:ffi. Maybe an extension can be generated by ffigen? Something like:

@ffi.Enum()
enum Enum1 { ... } 

/// Pointer is changed to accept enums marked with `@ffi.Enum()`
extension on Pointer<Enum1> {
  Pointer<Int> get _underlying => Pointer.fromAddress(address);

  Enum1 get value => Enum1.fromValue(_underlying.value);
}

For now, the standard workaround of using Enum1.fromValue manually will work fine, and if we do go down that route, it won't be a breaking change.

---

• [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR.

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.

[github-actions[bot]]

PR Health

Changelog Entry :heavy_check_mark:

Package	Changed Files

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

Coverage :warning:

File	Coverage
pkgs/ffigen/example/libclang-example/generated_bindings.dart	:broken_heart: Not covered
pkgs/ffigen/lib/src/code_generator/compound.dart	:broken_heart: Not covered
pkgs/ffigen/lib/src/code_generator/enum_class.dart	:broken_heart: 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 :warning:

// 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/test/header_parser_tests/expected_bindings/_expected_enum_int_mimic_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/ffigen/example/shared_bindings/generate.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/example/simple/generated_bindings.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.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_decl_type_name_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_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_forward_decl_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_typedef_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/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/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart

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

Package publish validation :heavy_check_mark:

Package	Version	Status
package:ffi	2.1.3	already published at pub.dev
package:ffigen	14.0.0-wip	WIP (no publish necessary)
package:jni	0.11.0-wip	WIP (no publish necessary)
package:jnigen	0.10.1	already published at pub.dev
package:native_assets_cli	0.7.3-wip	WIP (no publish necessary)
package:objective_c	1.2.0-wip	WIP (no publish necessary)
package:swift2objc	0.0.1-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.

[Levi-Lesches]

@dcharkes Noticed this in the CI logs. Seems unrelated but it seems like it's causing the Publish and Health checks to fail.

Validating package:jnigen 0.10.1 (dir=/home/runner/work/native/native/pkgs/jnigen):jnigen
pubspec:
  - version: 0.10.1
changelog:
- Add backticks to code references in doc comments.
dart pub publish --dry-run
  Resolving dependencies...
  Because jnigen depends on jni from path which requires the Flutter SDK, version solving failed.
  
  Flutter users should use `flutter pub` instead of `dart pub`.
Notice: pub publish dry-run failed; add the `publish-ignore-warnings` label to ignore

Similar error for the Coverage check:

Dart test VM: Because every version of objective_c from path depends on flutter from sdk which doesn't exist (the Flutter SDK is not available), objective_c from path is forbidden.
So, because ffigen depends on objective_c from path, version solving failed.

Flutter users should use `flutter pub` instead of `dart pub`.
...
Compage coverage for pkgs/ffigen/lib/src/code_generator/compound.dart: null vs null

I put a new test specifically for the lines I changed in compound.dart, so maybe the dart pub get error is stopping the CI from computing the coverage properly?

[coveralls]

coverage: 91.895% (+0.01%) from 91.884% when pulling e2ca35e3a58888ec40f25a3b8e22573481f23027 on Levi-Lesches:enum-in-struct into 87e8f924f06458ccba11dda7edc26d607c663d60 on dart-lang:main.

[Levi-Lesches]

This prevents using myStruct._enumMember.address in @Native external leaf calls. Maybe we should consider generating enumMemberAsInt instead? It's a bit of a compromise as it pollutes the API of the struct. If you have other ideas, let me know.

Changed -- I was also thinking about this when first implementing it, but I guess it's simpler/safer to keep the struct field public for now.

Yeah that makes sense. We don't allow enums in NativeType type arguments.

If we generated extension types instead of enums and addressed dart-lang/sdk#54944, then we could have Pointer<Enum1>. And then we'd also need to add support in dart:ffi to index into Pointer<Pointer<Enum1>> to get a Pointer<Enum1>. And then we could generate extension methods in FFIgen for those.

Though, at that point, we might as well support external Enum1 enumMember; and do the unwrapping in dart:ffi. 😄 Which would then also address the asInt duplicate field issue. 😄

I think it makes the most sense long-term to natively support enums in dart:ffi itself rather than trying to rely on extension types, since the whole point of wanting an enum is to be able to get exhaustive switches which iirc extension types can't support. That would basically mean moving all the .fromValue/.value logic further into dart:ffi and out of the generated code, but shouldn't make a difference from the user's point of view. But I think we should still keep those two public so users can get their own integers and use .fromValue.

Maybe add at least a test case that exercises the current behavior where users have to manually get the int out of a Pointer<Int> pointerToEnum struct field and call the enum fromValue.

Done -- added some enum logic to test/native_test that involves a struct with an enum, a pointer to an enum, and an array of enums. Then all that logic again for an enum that was set to generate as an integer.

Yep, these are known issues with those checks not understanding the existence of Flutter.

Seems like a simple flutter pub get before running the main workflow would help?

[Levi-Lesches]

Fixed a bug with some enums as-int still using .value, and changed existing abstract class enums to sealed. That is technically breaking so I added it to the Changelog but it seems more correct

[dcharkes]

Oh, I lost track of this PR. I suppose it's ready to merge? 😄

[Levi-Lesches]

Should be! Given how I've missed a few cases in the past I'd appreciate if you could write up some test header files and verify that they generate as expected, but I've been doing that with unit tests and manual testing and it should be good, in both standard and as-int cases

[dcharkes]

@Levi-Lesches The code apparently isn't formatted.

[Levi-Lesches]

Sorry about that, formatted now. Btw, I noticed a small issue with the tests, so I filed https://github.com/dart-lang/native/issues/1477