Skip to content

Code generation for ObjC protocols #1175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 40 commits into from
Jun 20, 2024
Merged

Code generation for ObjC protocols #1175

merged 40 commits into from
Jun 20, 2024

Conversation

liamappelbe
Copy link
Contributor

@liamappelbe liamappelbe commented May 24, 2024

Dart ObjC
class interface
interface protocol

Protocols in ObjC are analogous to Dart interfaces, but there are some key differences. The main difference that effects this PR is that they're not real types, whereas Dart interfaces are types.

In Dart you can write a function that accepts an interface as an argument, but in ObjC you instead accept an id<MyProtocol>, where id just means any object (the enforcement of this constraint that the object implements MyProtocol is also much weaker than in Dart). During parsing this means that the clang API doesn't give us a CXType associated with the protocol, just a CXCursor for the declaration.

So in ffigen, ObjCInterface is a BindingType, but ObjCProtocol is a NoLookUpBinding. It isn't a Type, so can't be used in function signatures etc. In the bindings, ObjCProtocol is code genned to a util class that cannot be instantiated, and id<MyProtocol> is code genned identically to id.

For example, in the tests we have a protocol called SecondaryProtocol that looks like:

@protocol SuperProtocol<NSObject>

@required
- (NSString*)instanceMethod:(NSString*)s withDouble:(double)x;

@end

The code this generates is:

/// SuperProtocol
abstract final class SuperProtocol {
  /// Builds an object that implements the SuperProtocol protocol. To implement
  /// multiple protocols, use [addToBuilder] or [objc.ObjCProtocolBuilder] directly.
  static objc.ObjCObjectBase implement(
      {required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {
    final builder = objc.ObjCProtocolBuilder();
    builder.implementMethod(
        SuperProtocol.instanceMethod_withDouble_, instanceMethod_withDouble_);
    return builder.build();
  }

  /// Adds the implementation of the SuperProtocol protocol to an existing
  /// [objc.ObjCProtocolBuilder].
  static void addToBuilder(objc.ObjCProtocolBuilder builder,
      {required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {
    builder.implementMethod(
        SuperProtocol.instanceMethod_withDouble_, instanceMethod_withDouble_);
  }

  /// instanceMethod:withDouble:
  static final instanceMethod_withDouble_ = objc.ObjCProtocolMethod(
    _sel_instanceMethod_withDouble_,
    objc.getProtocolMethodSignature(
      _proto_SuperProtocol,
      _sel_instanceMethod_withDouble_,
      isRequired: true,
      isInstance: true,
    ),
    (Function func) =>
        func is objc.NSString Function(objc.NSString, double),
    (Function func) =>
        ObjCBlock_NSString_ffiVoid_NSString_ffiDouble.fromFunction(
            (ffi.Pointer<ffi.Void> _, objc.NSString arg1, double arg2) =>
                func(arg1, arg2)),
    (Function func) => throw UnsupportedError(
        'instanceMethod:withDouble: cannot be a listener because it has a non-void return type'),
  );
}

late final _proto_SuperProtocol = objc.getProtocol("SuperProtocol");
late final _sel_instanceMethod_withDouble_ =
    objc.registerName("instanceMethod:withDouble:");

For the case where the user just wants to implement a single protocol in its entirety, the bindings have a static implement method that takes a block for each method as named parameters (@optional methods will be nullable, and @required methods will be required params). To implement multiple protocols, the user can create an ObjCProtocolBuilder and pass it to the addToBuilder method for their protocols. Both implement and addToBuilder are based on Block.fromFunction, so don't create listener blocks. To implement listener methods, the user can use ObjCProtocolBuilder.implementMethodAsListener.

Each of the protocol's methods becomes a static ObjCProtocolMethod field that can be passed to ObjCProtocolBuilder to implement that method. You can see how these method fields are used in the implement method.

Main changes:

  • Add ObjCProtocol, ffigen's representation of a protocol, which generates the bindings. These objects are created by parseObjCProtocolDeclaration.
  • Add a objc-protocols config option that works exactly like objc-interfaces, letting the user choose what protocols they want to bind.
  • Added ObjCProtocolMethod to package:objective_c, which contains all the info needed to implement a protocol method. Users don't construct this class, they get it from the ffigen bindings.
  • Added ObjCProtocolBuilder to package:objective_c, which is a thin wrapper around DartProxyBuilder. It lets the user implement methods with functions, then build an object containing those methods.

Other changes:

  • Moved classes related to ObjC methods from objc_interface.dart to objc_methods.dart
  • Added ObjCMethods, which is a mixin that pulls the methods logic out of ObjCInterface so they can be reused in ObjCProtocol.
  • Moved ObjCBlock's canonicalization step into its constructor, so that we can easily create blocks for protocol methods without ending up with duplicate block bindings.
  • makeDartDoc now accepts a nullable doc string and returns '' if it receives null.
  • Move isInstanceType util to ObjCBuiltInFunctions for reuse.

Fixes #1040

Built on #1144

@liamappelbe liamappelbe changed the base branch from main to proto May 24, 2024 03:37
Copy link

github-actions bot commented May 24, 2024

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/lib/src/code_generator.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/binding_string.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/compound.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_block.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_interface.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_methods.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_protocol.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/struct.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/typealias.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/union.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/utils.dart 💔 Not covered
pkgs/ffigen/lib/src/config_provider/config.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/includer.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/compounddecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objc_block_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcinterfacedecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcprotocoldecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/translation_unit_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/type_extractor/extractor.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/utils.dart 💔 Not covered
pkgs/ffigen/lib/src/strings.dart 💔 Not covered
pkgs/ffigen/tool/generate_json_schema.dart 💔 Not covered
pkgs/objective_c/lib/objective_c.dart 💔 Not covered
pkgs/objective_c/lib/src/c_bindings_generated.dart 💔 Not covered
pkgs/objective_c/lib/src/internal.dart 💔 Not covered
pkgs/objective_c/lib/src/objective_c_bindings_generated.dart 💔 Not covered
pkgs/objective_c/lib/src/protocol_builder.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/lib/src/header_parser/clang_bindings/clang_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/libclang-example/generated_bindings.dart
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/config_provider/config_spec.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_enum_int_mimic_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_libclang_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.

Copy link

PR Health

Changelog Entry ❗

Details
Package Changed Files
package:ffigen pkgs/ffigen/lib/src/code_generator.dart
pkgs/ffigen/lib/src/code_generator/binding_string.dart
pkgs/ffigen/lib/src/code_generator/objc_block.dart
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart
pkgs/ffigen/lib/src/code_generator/objc_interface.dart
pkgs/ffigen/lib/src/code_generator/objc_methods.dart
pkgs/ffigen/lib/src/code_generator/objc_protocol.dart
pkgs/ffigen/lib/src/code_generator/utils.dart
pkgs/ffigen/lib/src/config_provider/config.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/lib/src/header_parser/includer.dart
pkgs/ffigen/lib/src/header_parser/parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/compounddecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/enumdecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/function_type_param_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/functiondecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/macro_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/objc_block_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcinterfacedecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcprotocoldecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/unnamed_enumdecl_parser.dart
pkgs/ffigen/lib/src/header_parser/translation_unit_parser.dart
pkgs/ffigen/lib/src/header_parser/type_extractor/extractor.dart
pkgs/ffigen/lib/src/header_parser/utils.dart
pkgs/ffigen/lib/src/strings.dart
package:objective_c pkgs/objective_c/lib/objective_c.dart
pkgs/objective_c/lib/src/internal.dart
pkgs/objective_c/lib/src/protocol_builder.dart

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

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

Coverage ⚠️

Details
File Coverage
pkgs/ffigen/lib/src/code_generator.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/binding_string.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_block.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_interface.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_methods.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_protocol.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/utils.dart 💔 Not covered
pkgs/ffigen/lib/src/config_provider/config.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/includer.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/compounddecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/enumdecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/function_type_param_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/functiondecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/macro_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objc_block_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcinterfacedecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcprotocoldecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/unnamed_enumdecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/translation_unit_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/type_extractor/extractor.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/utils.dart 💔 Not covered
pkgs/ffigen/lib/src/strings.dart 💔 Not covered
pkgs/jni/example/lib/main.dart 💔 Not covered
pkgs/native_assets_builder/lib/native_assets_builder.dart 💔 Not covered
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart 💚 98 %
pkgs/native_assets_builder/lib/src/model/build_dry_run_result.dart 💔 Not covered
pkgs/native_assets_builder/lib/src/model/build_result.dart 💔 Not covered
pkgs/native_assets_builder/lib/src/model/hook_result.dart 💚 82 %
pkgs/native_assets_builder/lib/src/model/link_dry_run_result.dart 💔 Not covered
pkgs/native_assets_builder/lib/src/model/link_result.dart 💔 Not covered
pkgs/native_assets_builder/test_data/no_asset_for_link/hook/link.dart 💔 Not covered
pkgs/native_assets_cli/lib/src/model/hook.dart 💔 0 % ⬇️ NaN %
pkgs/native_assets_cli/lib/src/model/hook_config.dart 💚 97 %
pkgs/native_assets_cli/lib/src/model/hook_output.dart 💚 90 %
pkgs/objective_c/lib/objective_c.dart 💔 Not covered
pkgs/objective_c/lib/src/internal.dart 💔 Not covered
pkgs/objective_c/lib/src/protocol_builder.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/lib/src/header_parser/clang_bindings/clang_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/libclang-example/generated_bindings.dart
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/config_provider/config_spec.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_libclang_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/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.1 already published at pub.dev
package:native_assets_builder 0.7.0 already published at pub.dev
package:native_assets_cli 0.6.0 already published at pub.dev
package:native_toolchain_c 0.4.2 already published at pub.dev
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.

1 similar comment
Copy link

PR Health

Changelog Entry ❗

Details
Package Changed Files
package:ffigen pkgs/ffigen/lib/src/code_generator.dart
pkgs/ffigen/lib/src/code_generator/binding_string.dart
pkgs/ffigen/lib/src/code_generator/objc_block.dart
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart
pkgs/ffigen/lib/src/code_generator/objc_interface.dart
pkgs/ffigen/lib/src/code_generator/objc_methods.dart
pkgs/ffigen/lib/src/code_generator/objc_protocol.dart
pkgs/ffigen/lib/src/code_generator/utils.dart
pkgs/ffigen/lib/src/config_provider/config.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/lib/src/header_parser/includer.dart
pkgs/ffigen/lib/src/header_parser/parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/compounddecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/enumdecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/function_type_param_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/functiondecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/macro_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/objc_block_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcinterfacedecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcprotocoldecl_parser.dart
pkgs/ffigen/lib/src/header_parser/sub_parsers/unnamed_enumdecl_parser.dart
pkgs/ffigen/lib/src/header_parser/translation_unit_parser.dart
pkgs/ffigen/lib/src/header_parser/type_extractor/extractor.dart
pkgs/ffigen/lib/src/header_parser/utils.dart
pkgs/ffigen/lib/src/strings.dart
package:objective_c pkgs/objective_c/lib/objective_c.dart
pkgs/objective_c/lib/src/internal.dart
pkgs/objective_c/lib/src/protocol_builder.dart

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

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

Coverage ⚠️

Details
File Coverage
pkgs/ffigen/lib/src/code_generator.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/binding_string.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_block.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_interface.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_methods.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/objc_protocol.dart 💔 Not covered
pkgs/ffigen/lib/src/code_generator/utils.dart 💔 Not covered
pkgs/ffigen/lib/src/config_provider/config.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/includer.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/compounddecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/enumdecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/function_type_param_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/functiondecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/macro_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objc_block_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcinterfacedecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objcprotocoldecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/unnamed_enumdecl_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/translation_unit_parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/type_extractor/extractor.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/utils.dart 💔 Not covered
pkgs/ffigen/lib/src/strings.dart 💔 Not covered
pkgs/jni/example/lib/main.dart 💔 Not covered
pkgs/native_assets_builder/lib/native_assets_builder.dart 💔 Not covered
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart 💚 98 %
pkgs/native_assets_builder/lib/src/model/build_dry_run_result.dart 💔 Not covered
pkgs/native_assets_builder/lib/src/model/build_result.dart 💔 Not covered
pkgs/native_assets_builder/lib/src/model/hook_result.dart 💚 82 %
pkgs/native_assets_builder/lib/src/model/link_dry_run_result.dart 💔 Not covered
pkgs/native_assets_builder/lib/src/model/link_result.dart 💔 Not covered
pkgs/native_assets_builder/test_data/no_asset_for_link/hook/link.dart 💔 Not covered
pkgs/native_assets_cli/lib/src/model/hook.dart 💔 0 % ⬇️ NaN %
pkgs/native_assets_cli/lib/src/model/hook_config.dart 💚 97 %
pkgs/native_assets_cli/lib/src/model/hook_output.dart 💚 90 %
pkgs/objective_c/lib/objective_c.dart 💔 Not covered
pkgs/objective_c/lib/src/internal.dart 💔 Not covered
pkgs/objective_c/lib/src/protocol_builder.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/lib/src/header_parser/clang_bindings/clang_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/libclang-example/generated_bindings.dart
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/config_provider/config_spec.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_libclang_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/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.1 already published at pub.dev
package:native_assets_builder 0.7.0 already published at pub.dev
package:native_assets_cli 0.6.0 already published at pub.dev
package:native_toolchain_c 0.4.2 already published at pub.dev
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.


@required
- (NSString*)instanceMethod:(NSString*)s withDouble:(double)x;

@end

@protocol MyProtocol<SuperProtocol>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add an integration test for an existing API? For example the one you mentioned? https://developer.apple.com/documentation/foundation/nsurlsessiontaskdelegate/1411626-urlsession

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Brian suggests testing all of them 🚀
#1183

Copy link
Collaborator

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First glance.

- Export some structs from package:objective_c
- Don't codegen variadic methods (which involve __va_list_tag)
- Don't codegen methods in package:objective_c that take or return blocks
- Don't codegen @OverRide annotation for methods
@@ -128,13 +223,13 @@ void main() {
final sel = registerName('voidMethod:');
final signature = getProtocolMethodSignature(proto, sel,
isRequired: false, isInstance: true);
final block = DartVoidMethodBlock.listener((Pointer<Void> p, int x) {
final block = VoidMethodBlock.listener((Pointer<Void> p, int x) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you have a test where a listener is consuming an objective-c instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once I fix #835, yeah.

@brianquinlan
Copy link
Contributor

The long type names needed to create protocol functions isn't very usable.

    final redirect =
        ncb.ObjCBlock_ffiVoid_ffiVoid_NSURLSession_NSURLSessionTask_NSHTTPURLResponse_NSURLRequest_ffiVoidNSURLRequest
            .listener((_, session, task, response, request, completionHandler) {
      completionHandler.call(request);
    });
    ncb.NSURLSessionTaskDelegate.addToBuilder(protoBuilder,
        URLSession_task_willPerformHTTPRedirection_newRequest_completionHandler_:
            redirect);

@aam had the idea of typedefing the to something simpler. How about adding a typedef to the protocol like:

typedef ObjCBlock_ffiVoid_ffiVoid_NSURLSession_NSURLSessionTask_NSHTTPURLResponse_NSURLRequest_ffiVoidNSURLRequest NSURLSessionDataDelegate_URLSession_task_willPerformHTTPRedirection_newRequest_completionHandler_

So you'd write:

    final redirect =
        ncb.NSURLSessionDataDelegate_URLSession_task_willPerformHTTPRedirection_newRequest_completionHandler_
            .listener((_, session, task, response, request, completionHandler) {
      completionHandler.call(request);
    });
    ncb.NSURLSessionTaskDelegate.addToBuilder(protoBuilder,
        URLSession_task_willPerformHTTPRedirection_newRequest_completionHandler_:
            redirect);

That's still not great (it would be nice if typedefs could appear in classes) but at least you know the method that you are implementing.

@liamappelbe
Copy link
Contributor Author

I've implemented option 3 from the "DartProxy's selector arg" doc, to get rid of the extra void* arg at the start of the block signatures. This means that users pass functions (rather than blocks) to implement, addToBuilder, and ObjCProtocolBuilder. Note that implement and addToBuilder assume the user wants ordinary blocks, not listeners, so if they want listener blocks they'll need to use ObjCProtocolBuilder.implementMethodAsListener.

The long type names needed to create protocol functions isn't very usable.

@brianquinlan This should be fixed now. You pass in functions instead of blocks.

@liamappelbe liamappelbe requested a review from dcharkes June 17, 2024 04:25
Copy link
Collaborator

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a code quality POV and from the small amount of tests.

I'll defer to @brianquinlan for the review if this approach works for his use cases.

bool returnsRetained = false;
ObjCInternalGlobal? selObject;
ObjCMsgSendFunc? msgSend;
ObjCBlock? protocolBlock;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be late final? E.g. should be assigned once (and be explicitly assigned null for if it's supposed to be null), and throw if accessed before it's assigned instead of returning null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

kind == ObjCMethodKind.propertyGetter ||
kind == ObjCMethodKind.propertySetter;
bool get isRequired => !isOptional;
bool get isInstance => !isClass;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isClass and isInstance could do with documentation that it is an instance method and class method. (A method is never a class itself.) Renaming to something longer could also work isClassMethod isInstanceMethod (but more verbose).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (originalName != other.originalName) return false;
if (kind != other.kind) return false;
if (isClass != other.isClass) return false;
if (isOptional != other.isOptional) return false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it being optional still confict if it has the same name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's what this is checking. We want to catch cases where the methods only differ by their optional flag and report them to the user as an error. If someone reports that this appears in a legitimate API I can add an exception for it (ie, assume the method is required).

import 'writer.dart';

class ObjCProtocol extends NoLookUpBinding with ObjCMethods {
final superProtos = <ObjCProtocol>[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit here and in other placces: Maybe don't abbreviate protocol to proto, Protocol is only 3 characters longer than proto, and proto can mean protocol buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

} else {
final msg = '${method.originalName} cannot be a listener because '
'it has a non-void return type';
listenerBuilder = "(Function func) => throw UnsupportedError('$msg')";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why generate a method that throws if we know already at code gen time it's not going to work?

Maybe we should not generate a method at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to give an answer about how we have to construct a ObjCProtocolMethod to pass to the ObjCProtocolBuilder. There were more ergonomic solutions (with compile time type checking of the function) where I would generate a different method class per method, but Dart doesn't have nested classes so it would have been pretty messy. But then I realized I could at least split listenable vs non-listenable methods.

So methods that have a return value become ObjCProtocolMethod, and void methods become ObjCProtocolListenableMethod. So we can at least compile time check that you're not trying to implement a non-void method as a listener.

@liamappelbe liamappelbe requested a review from brianquinlan June 18, 2024 00:56
@liamappelbe liamappelbe merged commit 6d71f2d into main Jun 20, 2024
19 checks passed
@liamappelbe liamappelbe deleted the protogen branch June 20, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support implementing ObjC protocols from Dart
4 participants