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

Mark runtime APIs that are unintentionally public with @InternalAPI #205

Merged
merged 2 commits into from
May 4, 2024

Conversation

qurbonzoda
Copy link
Collaborator

@qurbonzoda qurbonzoda commented Apr 28, 2024

A follow-up to #194

build.gradle Show resolved Hide resolved
final fun valueOf(kotlin/String): kotlinx.benchmark/NativeFork // kotlinx.benchmark/NativeFork.valueOf|valueOf#static(kotlin.String){}[0]
final fun values(): kotlin/Array<kotlinx.benchmark/NativeFork> // kotlinx.benchmark/NativeFork.values|values#static(){}[0]
final val entries // kotlinx.benchmark/NativeFork.entries|#static{}entries[0]
final fun <get-entries>(): kotlin.enums/EnumEntries<kotlinx.benchmark/NativeFork> // kotlinx.benchmark/NativeFork.entries.<get-entries>|<get-entries>#static(){}[0]
}
// Targets: [js.jsIr, wasmJs]
final class kotlinx.benchmark/Blackhole : kotlinx.benchmark/CommonBlackhole { // kotlinx.benchmark/Blackhole|null[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not directly related to the change, but as soon as CommonBlackhole is hidden, there will be no info about Blackhole's members in the dump.

There's a request to overcome that problem on BCV side and dump all members inherited from the private API (Kotlin/binary-compatibility-validator#217), but here, I believe, the better solution would be to introduce a source set descending from commonMain and shared between both jsMain and wasmJsMain source sets and actualize the Blackhole there (kotlinx-io use that approach for implementing nodeJs-dependent classes for js and wasmJs: https://github.com/Kotlin/kotlinx-io/blob/c2d52202984f610054f21fd77def42e6f554bb70/build-logic/src/main/kotlin/kotlinx/io/conventions/kotlinx-io-multiplatform.gradle.kts#L84)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing out. It turns out CommonBlackhole can't be properly made internal. Because in Kotlin public classes can't inherit from internal classes. I have created a separate PR for sharing Blackhole implementation btw Js and WasmJs, so that CommonBlackhole could be dropped later: #207

Copy link
Member

@adam-enko adam-enko left a comment

Choose a reason for hiding this comment

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

LGTM (but I'm not a Benchmark user, or familiar with the runtime API, so please take that into account)

…markPluginInternalApi.kt

Co-authored-by: Adam <152864218+adam-enko@users.noreply.github.com>
@qurbonzoda qurbonzoda merged commit deb5002 into master May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants