Introduce SubclassOptInRequired to the codebase #4115

dkhalanskyjb · 2024-04-24T09:38:02Z

The initial stage of #3770

Marked:

Job,
Deferred and CompletableDeferred,
Flow, SharedFlow, StateFlow,
CloseableCoroutineDispatcher,
CancellableContinuation,
All of their public implementors.

qwwdfsad · 2024-04-26T10:23:31Z

Postponed until Kotlin 2.0 because this feature doesn't work as intended now

dkhalanskyjb · 2024-05-16T11:56:51Z

No idea why the testCoroutinesTimeoutInheritanceWithNoTimeoutInDerived test could fail as a result of these changes.

dkhalanskyjb · 2024-05-16T12:49:53Z

Possibly a fluke. After a restart, the tests passed.

qwwdfsad

It feels like we have a mix of concepts here.

We have SubclassOptInRequired that is "Hey, we are not sure if we won't add new methods/change the contract. Be wary!" (examples: SharedFlow, maybe Deferred), and there are "These were never meant to be extended, but they technically might be because of the language restrictions or our omission" (Job, CancellableContinuation, ChannelFlow).

It feels off to have both covered with Brittle* with WARNING level.

For the later, I suggest using SubclassOptInRequired(InternalCoroutinesApi::class), for the former -- keep it as is, but maybe rename it to something more straightforward -- ExperimentalForInheritanceCoroutinesApi (or UnstableFor*).

qwwdfsad · 2024-05-17T07:49:52Z

kotlinx-coroutines-core/jvm/src/Executors.kt

@@ -13,6 +13,8 @@ import kotlin.coroutines.*
 * This class is generally used as a bridge between coroutine-based API and
 * asynchronous API that requires an instance of the [Executor].
 */
+@OptIn(ExperimentalSubclassOptIn::class)
+@SubclassOptInRequired(BrittleForInheritanceCoroutinesApi::class)


This is a stable class with no mentions of unstable inheritance in the doc. Should we really mark it?

With CloseableCoroutineDispatcher being its typealias, I see no way around it.

qwwdfsad · 2024-05-17T09:33:11Z

kotlinx-coroutines-core/common/src/flow/Flow.kt

@@ -173,6 +173,8 @@ import kotlin.coroutines.*
 * These implementations ensure that the context preservation property is not violated, and prevent most
 * of the developer mistakes related to concurrency, inconsistent flow dispatchers, and cancellation.
 */
+@OptIn(ExperimentalSubclassOptIn::class)
+@SubclassOptInRequired(BrittleForInheritanceCoroutinesApi::class)


Frankly, taking into account that Flow is the way to program and that it has been here for a long time, I doubt we ever can change that, and this warning might be just a headache to opt-in. I suggest we might give up on that

People who want custom Flow implementation should use AbstractFlow instead, so I think it's okay to discourage inheriting from Flow.

I still argue that Flow is neither brittle nor experimental.

In my mental model, it's either Delicate (because of the invariants that must be preserved) or nothing (because it's quite hard to rationalize it otherwise). WDYT?

In my mental model, it's certainly brittle, because it's a mistake to try to implement it without using AbstractFlow. It's not Delicate because we don't give realistic means of fulfilling the contracts in custom Flow instances that don't inherit from AbstractFlow.

by-delegation to Flow does happen occasionally (https://grep.app/search?q=%20%3A%20Flow%3C%5B%5E%3E%5D%2B%3E%20by&regexp=true), and the following code does emit a warning:

public class MyFlow: Flow<Int> by flowOf(1, 2, 3) { }

So, Flow probably shouldn't be marked as InternalForInheritance, at least, as we may want to make it difficult to opt in to that eventually, but this benign pattern should work without issues.

dkhalanskyjb · 2024-05-22T11:35:21Z

Let's look at it in a case-by-case basis:

Job is not an internal API, so I think it would be surprising to have This is an internal kotlinx.coroutines API when you try to inherit from it. We could extend the description of InternalCoroutinesApi with If you get this issue when trying to inherit, this means that inheriting is specifically banned or something like that, but I think this would overcomplicate the meaning of the annotation.
Deferred and CompletableDeferred inherit from Job, so must be guarded by at least that strict a warning, which means they are also in the "must never be inherited from" category.
Flow, SharedFlow, StateFlow are probably also interfaces that must never be inherited from directly: I don't see what one could gain from implementing Flow and not inheriting from AbstractFlow. If we think SharedFlow and StateFlow are useful things to extend, we probably must also provide the corresponding abstract classes.
CloseableCoroutineDispatcher is the one clear-cut thing in this list: it's reasonable to extend it, but not stable.
CancellableContinuation is in the same basket as Job.

To follow the spirit of your suggestion, I introduced another annotation that strongly states that this shouldn't be inherited from and applied it to Job, CancellableContinuation, and Deferred.

qwwdfsad · 2024-05-27T15:14:59Z

kotlinx-coroutines-core/common/src/Annotations.kt

+@RequiresOptIn(
+    level = RequiresOptIn.Level.WARNING, message =
+    "This is a kotlinx.coroutines API that is not intended to be inherited from, " +
+        "as the library may handle predefined instances of this in a special manner."


I would suggest keeping it consistent: InternalForInheritance (or InternalForImplementation) and adding the corresponding disclaimer to the message: No compatibility guarantees are provided. It is recommended to report your use-case of internal API to kotlinx.coroutines issue tracker, so stable API could be provided instead

qwwdfsad · 2024-05-27T15:17:25Z

kotlinx-coroutines-core/common/src/Annotations.kt

+        "Either new methods may be added in the future, which would break the inheritance, " +
+        "or correctly inheriting from it requires fulfilling contracts that may change in the future."
+)
+public annotation class BrittleForInheritanceCoroutinesApi


We now have three terms in our glossary -- experimental, brittle and unstable.

I suggest sticking with experimental as the most straightforward one -- most of our "Not stable for inheritance" mean "as new methods might be added to this interface in the future"

qwwdfsad · 2024-05-27T15:18:53Z

kotlinx-coroutines-core/common/src/flow/Flow.kt

@@ -173,6 +173,8 @@ import kotlin.coroutines.*
 * These implementations ensure that the context preservation property is not violated, and prevent most
 * of the developer mistakes related to concurrency, inconsistent flow dispatchers, and cancellation.
 */
+@OptIn(ExperimentalSubclassOptIn::class)
+@SubclassOptInRequired(BrittleForInheritanceCoroutinesApi::class)


I still argue that Flow is neither brittle nor experimental.

In my mental model, it's either Delicate (because of the invariants that must be preserved) or nothing (because it's quite hard to rationalize it otherwise). WDYT?

whyoleg

mostly just questions, feel free to ignore, if the questions are too silly :)

kotlinx-coroutines-core/common/src/Annotations.kt

kotlinx-coroutines-core/common/src/CancellableContinuation.kt

kotlinx-coroutines-core/common/src/Job.kt

kotlinx-coroutines-core/common/src/JobSupport.kt

The initial stage of #3770 Marked: * Job, * Deferred and CompletableDeferred, * Flow, SharedFlow, StateFlow, * CloseableCoroutineDispatcher, * CancellableContinuation, * All of their `public` implementors.

Before this change, there was just one annotation: APIs that can't be inherited from. Now, it's more nuanced: some APIs are never planned to be available for inheritance and are marked as such.

qwwdfsad · 2024-05-28T08:43:10Z

kotlinx-coroutines-core/common/src/CompletableDeferred.kt

 */
+@OptIn(ExperimentalSubclassOptIn::class)
+@SubclassOptInRequired(markerClass = InternalForInheritanceCoroutinesApi::class)


This is going to be big, I guess.
https://grep.app/search?q=Deferred%3C%5B%5Cw%5D%2B%3E%20by&regexp=true it's used in the wild and previously we got reports about : Deferred<> by .. not working because of our internal type assertions.

dkhalanskyjb requested a review from qwwdfsad April 24, 2024 09:38

qwwdfsad added the postponed label Apr 26, 2024

qwwdfsad removed the postponed label May 16, 2024

dkhalanskyjb force-pushed the dk-subclassoptin branch from b511268 to 4fb023d Compare May 16, 2024 10:52

qwwdfsad reviewed May 17, 2024

View reviewed changes

dkhalanskyjb force-pushed the dk-subclassoptin branch from 4fb023d to f0f1c2a Compare May 22, 2024 10:44

dkhalanskyjb requested a review from qwwdfsad May 22, 2024 11:36

qwwdfsad requested changes May 27, 2024

View reviewed changes

whyoleg reviewed May 27, 2024

View reviewed changes

dkhalanskyjb added 9 commits May 27, 2024 19:33

Introduce SubclassOptInRequired to the codebase

be8ec69

The initial stage of #3770 Marked: * Job, * Deferred and CompletableDeferred, * Flow, SharedFlow, StateFlow, * CloseableCoroutineDispatcher, * CancellableContinuation, * All of their `public` implementors.

API dump

13c7f99

Opt into OptInRequired

68620e4

Do not mark AbstractFlow as brittle for inheritance

4398f4e

Separate APIs that are unsuitable to inherit from

cd7dd93

Before this change, there was just one annotation: APIs that can't be inherited from. Now, it's more nuanced: some APIs are never planned to be available for inheritance and are marked as such.

API dump

dd84bad

Rename the annotations, mention the issue tracker

b751b6e

Mention that the internal-for-inheritance will be an error

11d15c9

API dump

393a486

dkhalanskyjb force-pushed the dk-subclassoptin branch from cb1aceb to 393a486 Compare May 27, 2024 17:36

qwwdfsad self-requested a review May 28, 2024 08:49

dkhalanskyjb added 2 commits May 28, 2024 11:13

Remove some of the annotations

66b39fc

Restore a warning in the comments

7e6814c

dkhalanskyjb force-pushed the dk-subclassoptin branch 3 times, most recently from bdbd0c7 to 9e65346 Compare May 28, 2024 09:18

dkhalanskyjb force-pushed the dk-subclassoptin branch from 9e65346 to df15f32 Compare May 28, 2024 09:18

fixup

8816694

dkhalanskyjb force-pushed the dk-subclassoptin branch from df15f32 to 8816694 Compare May 28, 2024 09:19

qwwdfsad approved these changes May 28, 2024

View reviewed changes

dkhalanskyjb merged commit 61dd23c into develop May 28, 2024
1 check passed

dkhalanskyjb deleted the dk-subclassoptin branch May 28, 2024 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce SubclassOptInRequired to the codebase #4115

Introduce SubclassOptInRequired to the codebase #4115

dkhalanskyjb commented Apr 24, 2024

qwwdfsad commented Apr 26, 2024 •

edited

Loading

dkhalanskyjb commented May 16, 2024

dkhalanskyjb commented May 16, 2024 •

edited

Loading

qwwdfsad left a comment

qwwdfsad May 17, 2024

dkhalanskyjb May 22, 2024

qwwdfsad May 17, 2024

dkhalanskyjb May 22, 2024

qwwdfsad May 27, 2024

dkhalanskyjb May 27, 2024

dkhalanskyjb May 28, 2024

dkhalanskyjb commented May 22, 2024

qwwdfsad May 27, 2024

qwwdfsad May 27, 2024

qwwdfsad May 27, 2024

whyoleg left a comment

qwwdfsad May 28, 2024

Introduce SubclassOptInRequired to the codebase #4115

Introduce SubclassOptInRequired to the codebase #4115

Conversation

dkhalanskyjb commented Apr 24, 2024

qwwdfsad commented Apr 26, 2024 • edited Loading

dkhalanskyjb commented May 16, 2024

dkhalanskyjb commented May 16, 2024 • edited Loading

qwwdfsad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkhalanskyjb commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyoleg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwwdfsad commented Apr 26, 2024 •

edited

Loading

dkhalanskyjb commented May 16, 2024 •

edited

Loading