[ffigen] Ignore deprecated APIs #1403
Conversation
PR HealthChangelog Entry ✔️Details
Changes to files need to be accounted for in their respective changelogs. Coverage
|
| File | Coverage |
|---|---|
| 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/config_types.dart | 💔 Not covered |
| pkgs/ffigen/lib/src/config_provider/spec_utils.dart | 💔 Not covered |
| pkgs/ffigen/lib/src/config_provider/yaml_config.dart | 💔 Not covered |
| pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart | 💔 Not covered |
| pkgs/ffigen/lib/src/header_parser/sub_parsers/api_availability.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/functiondecl_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/utils.dart | 💔 Not covered |
| pkgs/ffigen/lib/src/strings.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 ⚠️
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/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/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/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 ✔️
Details
| 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.0 | 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.
There was a problem hiding this comment.
This looks great!
Some high level comments:
- I think we should consider also the max OS version as input (and possibly target version as well in addition to min and max).
- We should generate availability info (comment? annotation?) if an API is not available in the full range in all OSes. If an API is removed before the max version of an OS, if an API is introduced after the min version of an OS.
- If possible, I'd like to have guards on the actual-current-version-of-the-OS in the code as well. E.g. if an API was removed in iOS 14, but our user is running on iOS 15, it should throw in Dart. We need to check if we can make a system call to get the iOS/MacOS version. If we can't do that easily, maybe we should add a current OS version to the generated bindings class (when not using
ffi-native:). (I don't have an idea yet for theffi-nativecase.)
Let me know what you think.
Happy to do some of these things in follow up PRs. (But it probably makes sense to set up the code in such a way that that all would become easy.)
pkgs/ffigen/README.md
Outdated
| <td> | ||
|
|
||
| ```yaml | ||
| objc-min-target-version: |
There was a problem hiding this comment.
I believe we should also have a max, not only a min.
external-versions:
ios:
min: 12.0.0
max: 17.0.0
macos:
min: 10.14.0
max: 14.0.0
# whatever external versions we need to deal with in the future. Language versions? Other OSes (for non-objc FFIgen)This will ensure we don't get different generated bindings automatically when running on someone's computer that has a newer iOS/MacOS APi than the previous person that generated the bindings.
This means we'll have a clear commit adding new OS APIs
There was a problem hiding this comment.
I'll rename this option to fit this, but I'll leave adding a max option to a follow up PR.
There was a problem hiding this comment.
👍
Please leave an open issue and make an umbrella issue with versions (also the runtime branching). Or make one issue with a bunch of * [ ]s.
| } | ||
| } | ||
|
|
||
| class ObjCTargetVersion { |
There was a problem hiding this comment.
Let's make this and the YAML be the setup that we have explored in go/native-assets-external-versions.
final class ExternalVersions {
Versions getVersion(String key);
Versions get macos => getVersion('macos');
}
final class Versions {
final Version min;
final Version max;
final Version target;
}
// Version can be from pub_semver.Then we can potentially share the structure with the hooks later. (The info is not used at the same time, dev-invoked-time vs build-time. But it's essentially the same type of info.)
There was a problem hiding this comment.
Done.
How should we share the structure? Add a package:native_utils? We could also use this for our goal of sharing logic between ffigen and jnigen.
Also, what does the target version actually do?
There was a problem hiding this comment.
As discussed offline. For now we just duplicate in the various packages. We can merge later and re-export (and do a major version rev if the API is different).
There was a problem hiding this comment.
Also, what does the
targetversion actually do?
I have been asking myself the same question.
- android:minSdkVersion: the minimum API level on which the application is able to run. The default value is "1".
- android:targetSdkVersion: the API level on which the application is designed to run. In some cases, this lets the application use manifest elements or behaviors defined in the target API level, rather than being restricted to using only those defined for the minimum API level.
- android:maxSdkVersion: the maximum API level on which the application is able to run. Important: Read the information about this attribute on this page before using it.
https://developer.android.com/guide/topics/manifest/uses-sdk-element#max
targetSdkVersion is effectively a bug compatibility flag (behavior changes are often gated on targetSdkVersion).
https://groups.google.com/g/android-ndk/c/AjLT0HSfH4Q
Maybe target only makes sense in the context of an app. In other words: The app works optimally from target version, but there's some polyfills or disable functionality on older versions.
I guess for Swift/ObjC methods based on your comments theres only:
introducedIndeprecatedIn- (
removedInbut things never get removed?)
And our min/max provide a range that overlaps or not with introducedIn/deprecatedIn. So maybe target doesn't make sense for FFIgen configs.
| return true; | ||
| } | ||
|
|
||
| if (alwaysDeprecated || alwaysUnavailable) { |
There was a problem hiding this comment.
Technically speaking, if an API deprecated, but still available, shouldn't we generate it but with a @Deprecated?
Or are all these APIs very old and they always have new versions available?
There was a problem hiding this comment.
ApiAvailability is based on the versioning info we get from clang. It doesn't necessarily reflect the versioning info you can get from ObjC annotations. I haven't actually seen an Apple API that's annotated as obsoleted, and I don't know how to write such annotations. So just marking them as deprecated would mean we never omit those APIs.
I think instead it would make sense to add a config option for whether deprecated APIs are removed or just marked as deprecated, but I'll wait for a user to ask for this feature. At the moment, Brian is the only user who has requested anything around this, and he wants the deprecated APIs to be removed from the bindings. In his case there's an entire class that's deprecated, but is being pulled in as a transitive dep by some deprecated methods. This leads to a lot of unnecessary bindings.
There was a problem hiding this comment.
This should go in the PR description as design choice.
| return false; | ||
| } | ||
|
|
||
| for (final (plat, minVer) in [(ios, minVers.ios), (macos, minVers.macos)]) { |
There was a problem hiding this comment.
I think we should try to add an availability comment or annotation if an API doesn't have the same status (available, deprecated, obsolete, unavailable) for the whole range of versions requested.
There was a problem hiding this comment.
Fair enough. I'll add that when I have more range information (eg max target version).
| } | ||
|
|
||
| for (final (plat, minVer) in [(ios, minVers.ios), (macos, minVers.macos)]) { | ||
| // If the user hasn't specified a minimum version for this platform, defer |
There was a problem hiding this comment.
If users don't use the @Native but the DynamicLibrary version. Should that constructor of the bindings maybe take a currentVersions param that either contains the OS version of iOS or MacOS (or maybe better contain a map, so that we also know the OS properly). Then we can generate asserts / throws on APIs that are only available in a part of the range or on one OS.
We need to still figure out where to get this version from, but putting it on the dylib constructor pushes the responsibility to the user.
Maybe you can take a look at how package:device_info_plus gets the current MacOS and iOS version and see if we can do that within the generated code so that we don't have to expose it in the public API.
There was a problem hiding this comment.
Yep, that feature is covered by #300
| class PlatformAvailability { | ||
| VersionTriple? introduced; | ||
| VersionTriple? deprecated; | ||
| VersionTriple? obsoleted; |
There was a problem hiding this comment.
What is obsoleted? Is it the same as deprecated? E.g. you can still call it?
There was a problem hiding this comment.
This class is just a processed version of the CXPlatformAvailability struct. The docs say obsoleted means "no longer available", but I'm not sure how to trigger that from annotations in ObjC code. That's why I have api_availability_test.dart, to cover cases I'm not sure how to trigger in deprecated_test.m.
If we decide to mark deprecated methods with @Deprecated instead of omitting them (maybe behind a config flag), then it I think would make sense to continue omitting APIs that are obsoleted.
There was a problem hiding this comment.
Maybe you can make a suggestion together with @brianquinlan if it makes sense to generated deprecated methods with @Deprecated or to completely not generate them based on what you've seen so far in the ObjC/Swift APIs.
There was a problem hiding this comment.
Ah, you have answered this above. 👍
| @@ -0,0 +1,94 @@ | |||
| #import <Foundation/NSObject.h> | |||
|
|
|||
There was a problem hiding this comment.
I also see the concept "obsolete" in your code. Should we have an obsolete input as well in this file?
There was a problem hiding this comment.
Not sure how to trigger it from ObjC code.
| language: objc | ||
| output: 'deprecated_bindings.dart' | ||
| exclude-all-by-default: true | ||
| objc-min-target-version: |
There was a problem hiding this comment.
Maybe we should have multiple runs on the same objc file, with different ios and macos versions to see that:
- APIs get indeed omitted
- We generate some comments if an API is not available in the whole iOS or MacOS min/max target range. (e.g. also when an API is introduced after the min OS version asked for. Not only when it's removed before the max OS version.)
liamappelbe
changed the title
| If a version is not specified for a particular platform, the API's | ||
| inclusion will be based purely on the platforms that have a specified | ||
| minimum version. | ||
| </td> |
There was a problem hiding this comment.
Maybe explicitly mention which keys are supported: ios and macos.
pkgs/ffigen/README.md
Outdated
| external-versions: | ||
| # See https://docs.flutter.dev/reference/supported-platforms. | ||
| ios: | ||
| min: 2.5.0 |
There was a problem hiding this comment.
Maybe use some real version numbers here? For example the supported ones from Flutter?
pkgs/objective_c/ffigen_objc.yaml
Outdated
| objc-min-target-version: | ||
| # See https://docs.flutter.dev/reference/supported-platforms. | ||
| ios: | ||
| min: 2.5.0 |
There was a problem hiding this comment.
2.5.0? Shouldn't this be the "Supported versions" or "Best effort versions" from the linked URI?
There was a problem hiding this comment.
Oops. Copypasta
| @@ -43,6 +43,9 @@ enums: | |||
| - CXObjCPropertyAttrKind | |||
| - CXTypeNullabilityKind | |||
| - CXTypeLayoutError | |||
| as-int: | |||
| include: | |||
| - .* | |||
There was a problem hiding this comment.
Off-topic: Does the default cause issues for libclang? If yes, can/should we address that? Should this be in a separate PR?
There was a problem hiding this comment.
Some of the enums are flags. For example, we need to | together the values of CXTranslationUnit_Flags in lib/src/header_parser/parser.dart:102. This is the first time the bindings have had to be regenerated since the enum changes.
|
|
||
| class Versions { | ||
| final Version? min; | ||
| final Version? max; |
There was a problem hiding this comment.
Leave TODO that max isn't supported yet.
| class Versions { | ||
| final Version? min; | ||
| final Version? max; | ||
| final Version? target; |
There was a problem hiding this comment.
As in other comments, maybe target only makes sense for apps and not for bindings? So maybe remove it. (I'm also learning as we go here. 🤓)
Adds a
objc-min-target-versionconfig option to ffigen that sets the minimum target version for each ObjC platform. This version is used to check whether a particular API is deprecated or not. If an API is deprecated, we omit it from the bindings.The API availability check is a bit complicated. The logic is in api_availability.dart, and the main entry point is
isApiAvailable. We parse a bunch of availability info from the cursor, then compare that info to theObjCTargetVersionparsed from theobjc-min-target-versionoption:ObjCTargetVersionis empty, treat the API as available regardless of any availability info. This is so that the current default behavior remains unchanged.alwaysDeprecatedandalwaysUnavailableflag. If either is set, omit the API.a. If any platform's min target version is unspecified, ignore this platform and base the check purely on the other platform.
b. If the API has no deprecation annotation for this platform, assume the API is available on this platform.
c. If the API is marked as unavailable on this platform, or the min target version is >= the API's deprecated or obsoleted version, the API is unavailable on this platform. For the purposes of the >= check, a null deprecated/obsoleted version is treated as some distant future version.
d. Otherwise the API is available on this platform.
Why omit the deprecated APIs rather than just annotate them with
@Deprecated, and remove them when they're marked with obsoleted? Well, I haven't actually seen an Apple API that's annotated as obsoleted, and I don't know how to write such annotations. So just marking them as deprecated would mean we never omit those APIs. There are entire classes in the Apple APIs that are deprecated, which generate a lot of unnecessary bindings. If users want this, we can easily add a config option in the future to annotate them instead of omitting them.Fixes #1338
Related issues:
external-versions.*.max#1422 tracks follow up work to add amaxtarget version. It's similar to the logic in this PR, but I'm deferring it because this PR is already complicated enough.