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

Decide on Java interop w/ Kotlin top-level functions #1387

Open
alancai98 opened this issue Mar 13, 2024 · 1 comment
Open

Decide on Java interop w/ Kotlin top-level functions #1387

alancai98 opened this issue Mar 13, 2024 · 1 comment

Comments

@alancai98
Copy link
Member

For some context, Kotlin has an internal visibility modifier that becomes public in Java. From the docs:

internal declarations become public in Java. Members of internal classes go through name mangling, to make it harder to accidentally use them from Java and to allow overloading for members with the same signature that don't see each other according to Kotlin rules

So internal Kotlin class and functions/properties within those classes will have mangled names in Java. But top-level internal functions (e.g. from https://github.com/partiql/partiql-lang-kotlin/pull/1382/files#r1522281665) will not have mangled names. These functions are callable from Java using syntax like the following:

PhysicalPlanCompilerAsyncImplKt.getTypes(StaticType.ANY);

IDEs like IntelliJ will give an error for calling the internal Kotlin function from Java but these programs will still compile and run. Moving around or removing these internal functions also adds a lot of noise to our Java API compliance results. We should decide if we want to disallow such top-level declarations. @johnedquinn had brought this up before and had added to our code style guide https://github.com/partiql/partiql-lang-kotlin/blob/main/CODE_STYLE.md#top-level-functions.

If we choose to disallow such public and internal functions, we could make it easier to identify such functions by:

  • adding a custom rule to our ktlint rule set (as brought up by John)
  • adding some GH Action to report on public API changes

Additional Context

  • Java version: 11
  • PartiQL version: all versions
@alancai98
Copy link
Member Author

Conclusion from team discussion:

  • Add ktlint rule to disallow top-level internal values and functions
  • Add ktlint rule to disallow top-level public values and functions UNLESS there is a file:@JvmName annotation
  • Make it explicit in our versioning guidance that Kotlin internal artifacts are NOT part of the public API -- regardless of whether or not they are accessible via Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant