Skip to content

Don't generate ObjC bindings for deprecated APIs #1338

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

Closed
liamappelbe opened this issue Jul 15, 2024 · 5 comments · Fixed by #1403
Closed

Don't generate ObjC bindings for deprecated APIs #1338

liamappelbe opened this issue Jul 15, 2024 · 5 comments · Fixed by #1403
Labels
lang-objective_c Related to Objective C support package:ffigen
Milestone

Comments

@liamappelbe
Copy link
Contributor

liamappelbe commented Jul 15, 2024

Apple APIs have deprecated annotations. If these are detectable using clang, we should not generate bindings for them.

If users need them, we can add a config option to generate them.

@liamappelbe
Copy link
Contributor Author

Did some investigation today. I can get information about whether an API is deprecated using clang_getCursorPlatformAvailability. This returns a bunch of different availability info, but the important one for our needs is CXPlatformAvailability.Deprecated. Here's the result for NSString.stringWithCString::

Availability {
  Platform: ios
  Introduced: 2.0.-1
  Deprecated: 2.0.-1
  Obsoleted: -1.-1.-1
  Unavailable: 0
  Message: Use +stringWithCString:encoding: instead
}
Availability {
  Platform: macos
  Introduced: 10.0.-1
  Deprecated: 10.4.-1
  Obsoleted: -1.-1.-1
  Unavailable: 0
  Message: Use +stringWithCString:encoding: instead
}
Availability {
  Platform: tvos
  Introduced: 9.0.-1
  Deprecated: 9.0.-1
  Obsoleted: -1.-1.-1
  Unavailable: 0
  Message: Use +stringWithCString:encoding: instead
}
Availability {
  Platform: watchos
  Introduced: 2.0.-1
  Deprecated: 2.0.-1
  Obsoleted: -1.-1.-1
  Unavailable: 0
  Message: Use +stringWithCString:encoding: instead
}

So all the deprecation info is there, but it's not quite as simple as just checking a flag that says whether the API is deprecated. We need to compare the deprecation version to some target version. There are a few options for where to get the target version, but I think the best approach is to just make it a config option.

The answer is also going to depend on whether they're targeting ios or macos, so we should allow them to specify minimum target versions for both platforms and keep the method if it'd undeprecated on either.

There's also the question of what to do if the target version isn't specified. I think it makes sense to keep the current behavior (generate all APIs) if the version isn't specified. But if the user is only interested in iOS, so leaves the macOS version unspecified, we should generate/skip the API based purely on the iOS availability info.

This availability info can also be used for #300, using the Introduced field.

Another option is clang_getCursorAvailability, which checks availability based purely on the target platform and version, but since we don't currently require the user to specify whether they're targeting iOS or macOS, I prefer clang_getCursorPlatformAvailability. clang_getCursorAvailability also wouldn't help with #300.

@dcharkes
Copy link
Collaborator

target version

We have two pieces of information:

  • Flutter apps, and Flutter plugins have a target OS version. But since these are the root package, and these can import some arbitrary FFIgenned code from another package, the arbitrary FFIgenned code cannot depend on the target OS version of the root package.
    • If we change package:objective_c to use native assets, we can use target iOS version and target MacOS version to create a dylib or data asset that gives us access to the targetted minimum OS version at runtime to do the checks for [ffigen] ObjC version compatibility runtime check #300. But maybe you want to use the actual OS version that the code is running on, instead of the target OS version (the lowest version the app should be installable on and still working.)
  • Dart and Flutter support ranges of OSes: https://docs.flutter.dev/reference/supported-platforms and https://dart.dev/get-dart#system-requirements.
    • We could possibly use this as the maximum and minimum cutoff for supported versions of what we generate in FFIgen.

There's also the question of what to do if the target version isn't specified

Not only is the question what to do if the target version isn't specified, but also what the version should be that users publishing FFIgenned packages should specify.

Maybe we should go for a range (not just a single target version) based on https://docs.flutter.dev/reference/supported-platforms and https://dart.dev/get-dart#system-requirements.

  • MacOS 12 - 14
  • iOS 12 - 17

And then generate the method if it is available in at least one of those versions.

If we provide a range as guidance to users publishing FFIgen-ed packages, we might as well default to it in the FFIgen config.

Some more open questions:

  • Should we generate a doc-string or annotations on the methods to signal to users what are the valid OS versions?
  • Should we still generate deprecated methods but mark them @Deprecated with the message?
    • If we go for a range, we probably want to generate @Deprecated if it's deprecated in at least one of the versions. Users can always ignore the deprecation.
  • We should not generate any methods that are obsolete in all versions.
  • The methods that are obsolete in some versions should have a runtime check. Dart doesn't have the host OS version, but package:device_info_plus has it, so we probably need to call the same native code as they do in that package. (Or as written above, we could use the root-package target OS version to ensure the code deployed will work on the lowest OS target.)

@dcharkes
Copy link
Collaborator

@HosseinYousefi Is there any metadata on Java/Kotlin APIs on which Android version they require? Should we have something similar in JNIgen?

@HosseinYousefi
Copy link
Member

@HosseinYousefi Is there any metadata on Java/Kotlin APIs on which Android version they require? Should we have something similar in JNIgen?

Yes. I think there is an issue for it already.

@liamappelbe
Copy link
Contributor Author

Flutter apps, and Flutter plugins have a target OS version. But since these are the root package, and these can import some arbitrary FFIgenned code from another package, the arbitrary FFIgenned code cannot depend on the target OS version of the root package.

I think it's reasonable for ffigenned package authors to decide what minimum OS version they're targeting. It would be nice to have a general mechanism for this in the package's pubspec. But in most cases I'd expect the flutter plugin to be the thing that is doing the ffigenning anyway, or at least be owned by the same team (like how the webview_flutter plugin is backed by iOS and Android specific plugins).

But maybe you want to use the actual OS version that the code is running on, instead of the target OS version (the lowest version the app should be installable on and still working.)

Yeah, the runtime checks I'm talking about in #300 should be based on the runtime OS version. The decision this bug is talking about (whether to include the method at all) should be based on the versions that the package author wants to target.

Should we still generate deprecated methods but mark them @Deprecated with the message?

We could maybe add this as a configurable option, but I don't think we should do it by default. Brian was seeing compile errors due to things pulled in as transitive dependencies of deprecated methods.

Maybe we should go for a range (not just a single target version)

I was using "target version" as a shorthand for "minimum target version". We could also add a maximum target version, or use some version range syntax, and compare that to CXPlatformAvailability.Introduced. Not sure how useful that would be at codegen time though. I was mainly thinking of using CXPlatformAvailability.Introduced's value at runtime for the check in #300.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-objective_c Related to Objective C support package:ffigen
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants