Skip to content
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

HiddenFromObjC and ShouldRefineInSwift annotations #4818

Merged

Conversation

rickclephas
Copy link
Contributor

@rickclephas rickclephas commented May 9, 2022

Relates to KT-42297.

The Kotlin ObjC/Swift is pretty great, but at the moment there are also some gaps (especially with the Swift interop) like suspend functions and enum classes.
With some boilerplate code the interop for such code can be improved, which can even be automated with a compiler plugin/annotation processor.
However one limitation of such boilerplate code is that it adds unnecessary declarations to your public API.

With the HiddenFromObjC and ShouldRefineInSwift annotations we can hide these unnecessary declarations from the public API.
We also have the meta-annotations HidesFromObjC and RefinesInSwift such that annotation processors can generate ObjC/Swift friendly APIs while automatically hiding the original declaration.

HiddenFromObjC

Functions and properties annotated with the HiddenFromObjC annotation won't be exported to ObjC.
This allows you to create a more ObjC friendly version in your Kotlin code.

With the ObjCName annotation from #4815 you could even use the original name of the function/property for your ObjC friendly version.

ShouldRefineInSwift

The ShouldRefineInSwift annotation adds the swift_private attribute to the declaration.
This results in the declarations being prefixed with __, which make them "invisible" from Swift.
These declarations can still be used in your Swift code to create your Swift friendly API, but won't be shown in e.g. the Xcode autocomplete.

@abelkov abelkov added the Native label May 11, 2022
@rickclephas rickclephas marked this pull request as ready for review May 12, 2022 20:14
@SvyatoslavScherbina
Copy link
Contributor

Hi. Thank you for the PR! We will take a look later, hopefully next week.

Copy link
Contributor Author

@rickclephas rickclephas left a comment

Choose a reason for hiding this comment

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

Once the design/names are finalised we'll still need to update the KDocs of the annotations.

@rickclephas rickclephas changed the title ObjCRefined and SwiftRefined annotations RefinedForObjC and RefinedInSwift annotations May 25, 2022
Copy link
Contributor

@SvyatoslavScherbina SvyatoslavScherbina left a comment

Choose a reason for hiding this comment

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

At this point, the implementation looks fine. Great job!
Status: We are waiting for a review of the stdlib part from the Kotlin Libraries team.
I'm also going to run the CI one more time, to check the latest changes.

@rickclephas rickclephas force-pushed the feature/objc-swift-refined branch 3 times, most recently from 6bc887d to 34748bf Compare July 24, 2022 16:11
@rickclephas rickclephas changed the title RefinedForObjC and RefinedInSwift annotations HiddenFromObjC and RefinedInSwift annotations Aug 14, 2022
@rickclephas rickclephas changed the title HiddenFromObjC and RefinedInSwift annotations HiddenFromObjC and ShouldRefineInSwift annotations Aug 16, 2022
@SvyatoslavScherbina
Copy link
Contributor

@rickclephas could you please run :compiler:tests-for-compiler-generator:generateTests Gradle task and push the resulting changes?

@rickclephas
Copy link
Contributor Author

@SvyatoslavScherbina done 👍🏻

@demiurg906
Copy link
Contributor

@rickclephas Thank you! From the side of FIR part everything is ok.
If @SvyatoslavScherbina also thinks that PR is ready, then I will ask for one more thing: can you please cleanup commit history?

  • squash commits with fixes
  • add prefixes to commit messages to indicate which subsystem is modified ([FE 1.0] for old frontend, [FIR] for FIR frontend, [Native] for native backend, etc)

Copy link
Contributor

@SvyatoslavScherbina SvyatoslavScherbina left a comment

Choose a reason for hiding this comment

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

The code looks good to me (as I said, I have zero experience in FIR, so I didn't check FIR checkers thoroughly, and rely on the @demiurg906 approval here). Still waiting for the CI. Once we get the green builds and the commit history is cleaned up, we will merge the PR.

@rickclephas
Copy link
Contributor Author

Cool! Updated the name and squashed the commits into three new ones.

@rickclephas
Copy link
Contributor Author

@demiurg906 would you like a PR for the FIR checkers (with similar improvements) for @ObjCName (#4815) as well?:

@demiurg906
Copy link
Contributor

@rickclephas Yes, it would be nice

@SvyatoslavScherbina SvyatoslavScherbina merged commit 0a8cefc into JetBrains:master Aug 22, 2022
@SvyatoslavScherbina
Copy link
Contributor

Thank you!

@rickclephas rickclephas deleted the feature/objc-swift-refined branch August 22, 2022 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants