Skip to content

[ffigen] Category overhaul #1690

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 20 commits into from
Nov 6, 2024
Merged

[ffigen] Category overhaul #1690

merged 20 commits into from
Nov 6, 2024

Conversation

liamappelbe
Copy link
Contributor

@liamappelbe liamappelbe commented Nov 1, 2024

Rewrite how categories are handled:

  • Promote categories to an AST object, ObjCCategory
  • Code gen them as extension methods on the interface, instead of just adding their methods to their interface
  • Add objc-categories config option, which uses the same DeclarationFilters infra as everything else
  • Add include-transitive-objc-categories option, which determines whether the categories that extend an included interface are automatically included too. Unlike the other transitive options, this one defaults to true. See this comment.

Details

  • Factored out the method generation code from ObjCInterface into ObjCMethods, so that ObjCCategory can reuse it. Also factored out some of the interface parsing code.
  • Methods that return instancetype are still copied to the interface, like before. This is because the weird inheritance behavior of instancetype can't work as an extension method. See this comment.
    • If the interface is part of package:objective_c, then we can't add a method to it, so we fall back to defining the method as an extension method. The downside of this is that children of that interface will see the base interface as the return type for that method instead of the child interface, but that's a pretty unlikely edge case.
    • Another small issue with this is that these methods still have the old behavior where categories are unfiltered. That is, all methods that return instancetype will be copied from category to interface, regardless of whether the category is included.
  • We also copy all the methods from any anonymous categories to their interface. Anonymous categories in ObjC are quite different to Dart's anonymous extension methods. One difference is they're allowed to have static methods. Another difference is that their implementation must be a part of the interface's implementation (ie they can't be defined after the fact by a different library). For ffigen they pose a problem because there's no way of referring to them in the config filters. So the easiest way to handle them is to just copy all their methods to the interface.

Fixes #271. Fixes #1478.

Copy link

github-actions bot commented Nov 1, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

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

Coverage ⚠️
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_category.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/config_provider/config.dart 💔 Not covered
pkgs/ffigen/lib/src/config_provider/config_impl.dart 💔 Not covered
pkgs/ffigen/lib/src/config_provider/yaml_config.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/parser.dart 💔 Not covered
pkgs/ffigen/lib/src/header_parser/sub_parsers/objccategorydecl_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/utils.dart 💔 Not covered
pkgs/ffigen/lib/src/strings.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/apply_config_filters.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/copy_methods_from_super_type.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/fill_method_dependencies.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/find_transitive_deps.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/list_bindings.dart 💔 Not covered
pkgs/ffigen/lib/src/visitor/visitor.dart 💔 Not covered
pkgs/objective_c/lib/objective_c.dart 💔 Not covered
pkgs/objective_c/lib/src/objective_c_bindings_generated.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 ✔️
// 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
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
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/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_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_sort_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/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_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/objective_c/lib/src/ns_input_stream.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_initializer_declaration.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_variable_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_globals.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_variable.dart
pkgs/swift2objc/test/unit/parse_initializer_param_test.dart

@coveralls
Copy link

coveralls commented Nov 4, 2024

Coverage Status

coverage: 90.945% (+0.07%) from 90.875%
when pulling 7372be9 on category
into 7855c80 on main.

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Nov 5, 2024
@liamappelbe
Copy link
Contributor Author

The package:objective_c CI error is that it thinks the ObjC bindings are out of date. There are spurious diffs between the bindings my laptop generates, and the bindings CI generates. The diffs are caused by differences in the Apple headers between the xcode version on my laptop vs CI (apple decided to move a method from an interface to a category). The fix is for me to update my laptop.

@github-actions github-actions bot removed the type-infra A repository infrastructure change or enhancement label Nov 5, 2024
@liamappelbe liamappelbe changed the title WIP: [ffigen] Category overhaul [ffigen] Category overhaul Nov 5, 2024
@liamappelbe liamappelbe marked this pull request as ready for review November 5, 2024 23:24
@@ -1756,6 +1145,9 @@ enum NSDataCompressionAlgorithm {
};
}

/// NSDataCreation
extension NSDataCreation on NSData {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this what you'd expect?

I see the definition as:

@interface NSData (NSDataCreation)

+ (instancetype)data;
+ (instancetype)dataWithBytes:(nullable const void *)bytes length:(NSUInteger)length;
+ (instancetype)dataWithBytesNoCopy:(void *)bytes length:(NSUInteger)length;
+ (instancetype)dataWithBytesNoCopy:(void *)bytes length:(NSUInteger)length freeWhenDone:(BOOL)b;
+ (nullable instancetype)dataWithContentsOfFile:(NSString *)path options:(NSDataReadingOptions)readOptionsMask error:(NSError **)errorPtr;
+ (nullable instancetype)dataWithContentsOfURL:(NSURL *)url options:(NSDataReadingOptions)readOptionsMask error:(NSError **)errorPtr;
+ (nullable instancetype)dataWithContentsOfFile:(NSString *)path;
+ (nullable instancetype)dataWithContentsOfURL:(NSURL *)url;
- (instancetype)initWithBytes:(nullable const void *)bytes length:(NSUInteger)length;
- (instancetype)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)length;
- (instancetype)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)length freeWhenDone:(BOOL)b;
- (instancetype)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)length deallocator:(nullable void (^)(void *bytes, NSUInteger length))deallocator API_AVAILABLE(macos(10.9), ios(7.0), watchos(2.0), tvos(9.0));
- (nullable instancetype)initWithContentsOfFile:(NSString *)path options:(NSDataReadingOptions)readOptionsMask error:(NSError **)errorPtr;
- (nullable instancetype)initWithContentsOfURL:(NSURL *)url options:(NSDataReadingOptions)readOptionsMask error:(NSError **)errorPtr;
- (nullable instancetype)initWithContentsOfFile:(NSString *)path;
- (nullable instancetype)initWithContentsOfURL:(NSURL *)url;
- (instancetype)initWithData:(NSData *)data;
+ (instancetype)dataWithData:(NSData *)data;

@end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we copy all the methods that return instancetype to the parent class, because instancetype's inheritance rules can't be replicated with Dart extension methods.

@liamappelbe liamappelbe merged commit 80a005d into main Nov 6, 2024
20 checks passed
@liamappelbe liamappelbe deleted the category branch November 6, 2024 22:03
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.

Can't access AVFoundation NSValue extensions Filter categories
4 participants