search

Kotlin / kotlinx.coroutines

Library support for Kotlin coroutines
Apache License 2.0
12.94k stars 1.84k forks source link

1.9.0-RC: `kotlinx-coroutines-core/concurrent/src/...` contains common (expect) and platform (actual) files #4163

Open nreid260 opened 2 months ago

nreid260 commented 2 months ago

What broke?

Our current build setup globs all of kotlinx-coroutines-core/common/src/... and kotlinx-coroutines-core/concurrent/src/... to pass as -Xcommon-srcs. When we do this with -language-version=2.0, which is stricter about expect/actual declarations, there's a EXPECT_AND_ACTUAL_IN_THE_SAME_MODULE error for LockFreeLinkedListNode.

The problem is that there are some files from kotlinx-coroutines-core/concurrent/src/... that must be -Xcommon-srcs (expects) and some that must not be (actuals). These files shouldn't be mixed together.

Ideally, all expect files would be under a single root, such askotlinx-coroutines-core/common/src/....

PS: What naming convention is Builders.concurrent.kt? It looks like someone wanted Builders.common.kt.

Did I check that setting the version to the latest stable release fixes the problem?

Yes.

globsterg commented 2 months ago

The problem is that there are some files from kotlinx-coroutines-core/concurrent/src/... that must be -Xcommon-srcs (expects) and some that must not be (actuals). These files shouldn't be mixed together.

Could you link to some style guide that mandates this? I could not find this requirement anywhere. When I have deep hierarchies of source sets, I freely mix expect and actual together in the intermediate source sets. Am I wrong to do that?

PS: What naming convention is Builders.concurrent.kt? It looks like someone wanted Builders.common.kt.

Yeah, seems strange. The authors probably wanted $fileName.$sourceSetName.kt, but ./kotlinx-coroutines-core/concurrent/src/MultithreadedDispatchers.common.kt violates this scheme. Would also be interested to see any style guide that explains how these names are chosen (I'm generating Kotlin code and would very much like for it to be idiomatic).

nreid260 commented 2 months ago

An alternative solution is simply to remove the actual keyword from kotlinx-coroutines-core/concurrent/src/internal/LockFreeLinkedList.kt, which is probably what's intended. The code in that is already platform-agnostic. There's no need for expect/actual.

dkhalanskyjb commented 1 month ago

The problem is that there are some files from kotlinx-coroutines-core/concurrent/src/... that must be -Xcommon-srcs (expects) and some that must not be (actuals). These files shouldn't be mixed together.

It's natural to establish some expect declarations for different platforms to fulfill, but also to implement actual definitions for the whole group of platforms.

there's a EXPECT_AND_ACTUAL_IN_THE_SAME_MODULE error for LockFreeLinkedListNode.

Maybe that's some compiler bug. The stated reason for this error is to forbid implementing something in the same module where the expect was declared: https://youtrack.jetbrains.com/issue/KT-55177/Deprecate-declaration-of-expect-and-actual-counterparts-of-same-class-in-one-module . We don't do that: LockFreeLinkedListNode is declared as expect in the common module and then is implemented in the concurrent and the jsAndWasmShared modules separately.

The code in that is already platform-agnostic. There's no need for expect/actual.

JS and Wasm have a separate implementation.