Kotlin / KEEP

Kotlin Evolution and Enhancement Process
Apache License 2.0
3.3k stars 356 forks source link

Opt-in requirements (aka Experimental API support) #95

Open udalov opened 6 years ago

udalov commented 6 years ago

Discussion of the proposal: https://github.com/Kotlin/KEEP/blob/master/proposals/experimental.md

LouisCAD commented 6 years ago

Looks good! I'm myself developing a few experimental libraries, including one already released for BluetoothGatt on Android relying on kotlinx.coroutines, and this would be really useful.

cy6erGn0m commented 6 years ago

There is "Same module exemption" but what if my library consist of multiple modules? Is there way to avoid propagation requirement except suppressing an error?

robstoll commented 6 years ago

It's a bit late, so dare with me if I overlooked a phrase. I couldn't find how signature usage and body usage differ in propagation. Or lies the only difference in same module exemption?

robstoll commented 6 years ago

Another question to clear up propagation. The proposal does not mention which access levels need propagation. I assume it is public and protected (where protected makes only sense for an open type), right?

udalov commented 6 years ago

@cy6erGn0m

There is "Same module exemption" but what if my library consist of multiple modules? Is there way to avoid propagation requirement except suppressing an error?

Generally, if you're using an experimental API, you must propagate it. Otherwise, your clients will not be aware of the fact that something may break, and may break unexpectedly. From this perspective, it's irrelevant whether two modules (where a module is a single self-contained compilation unit) comprise a library in any sense, or not.

udalov commented 6 years ago

@robstoll

It's a bit late, so dare with me if I overlooked a phrase. I couldn't find how signature usage and body usage differ in propagation. Or lies the only difference in same module exemption?

There are two key differences between body and signature usages, highlighted in the proposal: 1) body usages of compile-time experimental API need not be propagated 2) body usages need not be propagated in the same module

Another question to clear up propagation. The proposal does not mention which access levels need propagation. I assume it is public and protected (where protected makes only sense for an open type), right?

Visibility modifiers have no effect on the propagation requirements. The reason is that even a private declaration that uses something experimental might be called indirectly from client code, via some public declaration:

// Library code

@ShinyNewAPI
fun foo() {}

// If we do not require propagation here...
private fun bar() {
    foo()
}

// ... then it's not required here...
fun baz() {
    bar()
}

// Usage

fun usage() {
    // ... and then we get effectively experimental behavior without any explicit opt-in from the user
    baz()
}
cy6erGn0m commented 6 years ago

@udalov

From this perspective, it's irrelevant whether two modules (where a module is a single self-contained compilation unit) comprise a library in any sense, or not.

But there is a difference because all the usages are inside of a single project (but different modules) so it a user depends on the same version of my modules then there is no risk so I'd like to force exemption to not poison everything. Consider:

kotlinx-corotunes:

So I'd like to use experimental core's functions in io's implementation but avoid poisoning all my io's stable API functions

udalov commented 6 years ago

@cy6erGn0m We cannot guarantee that the user depends on the same version of these two modules though. If the user depends on kotlinx-coroutines-io version 4.2, and gets kotlinx-coroutines-core version 4.3 via a transitive dependency, any usage of a function from kotlinx-coroutines-io may break because the underlying experimental implementation was changed binary incompatibly. Whereas the user doesn't expect any breakage because 1) no opt-in was given to any experimental features 2) all non-experimental API in 4.3 should be binary compatible with 4.2 according to the compatibility guide

udalov commented 6 years ago

I've updated the proposal after an internal discussion:

udalov commented 6 years ago

A few minor updates:

udalov commented 6 years ago

FYI the prototype has landed into Kotlin master and 1.2.30. Although it's a 1.3-only API, it's possible to use it with -language-version 1.3 -Xskip-runtime-version-check (the latter argument is needed because of KT-22777).

Note that because Kotlin 1.3 is not yet released, using -language-version 1.3 results in "pre-release" binaries being generated by the compiler, which are not supposed to be published as libraries, because stable versions of the Kotlin compiler will reject compiling against them in the classpath.

udalov commented 6 years ago

We've had one more look at this proposal internally and decided to greatly simplify it, or otherwise it was getting out of hand pretty fast. The major change is that we now do not intend to make experimental declarations "poisonous" and verify it in the compiler as much as possible. Because of this simplification, the concept of Impact (also known as Scope previously) goes away, along with signature/body usages, same module exemption, etc.

We've also discussed a bit how we are going to use Experimental in the standard library in relation to SinceKotlin, which I summarized in the new section.

The prototype of the new simplified approach is currently being worked on.

udalov commented 5 years ago

The support for Experimental as described in this proposal has been implemented and is available since 1.2.50 (with -language-version 1.3) and since 1.3.

ilya-g commented 5 years ago

Usually declaring an experimental annotation requires to spell a long list of targets where it makes sense:

@Target(
    CLASS,
    ANNOTATION_CLASS,
    PROPERTY,
    FIELD,
    LOCAL_VARIABLE,
    VALUE_PARAMETER,
    CONSTRUCTOR,
    FUNCTION,
    PROPERTY_GETTER,
    PROPERTY_SETTER,
    TYPEALIAS
)

It's easy to forget this incantation and get an experimental marker annotation applicable where it should not have been.

What if we imply these targets by default for the annotations marked with @Experimental, if @Target is not specified?

udalov commented 5 years ago

@ilya-g Sounds like a good idea to me, although a bit too implicit for Kotlin. Please report an issue.

pdvrieze commented 5 years ago

The feature as currently available in 1.3 is quite interesting and clearly solves a problem. However, I feel it is still quite limited in scope. In particular feature effectively introduces custom visibility scopes - see for example how kotlinx.serialization uses it to limit the reflection API - it is not actually experimental, it doesn't work properly on non-JVM targets). Thinking about this I had an idea how this can be extended.

Why not allow for proper user-defined scopes. You would have the ability to define a custom scope as annotation, specify visibility (and possibly warning/error level). Then the annotation can be used on various identifiers to determine a rich scope. The effective visibility would be the intersection between the declared regular visibility modifier and the annotation - if the code is protected the annotation does not widen it, but an inheriting class cannot access it either without the annotation either - if the annotation is internal this makes a symbol annotated as @MyScope protected effectively inaccessible to external modules. This would look like:

@Target(ANNOTATION_CLASS)
@Retention(BINARY)
annotation class CustomScope(val visibility: Visibility = Visibility.PUBLIC, val level: Level = Level.ERROR) {
    enum class Level { WARNING, ERROR }
    enum class Visibility { PUBLIC, INTERNAL, PROTECTED, PRIVATE }
}

@Target(CLASS, PROPERTY, LOCAL_VARIABLE, VALUE_PARAMETER, CONSTRUCTOR, FUNCTION,
        PROPERTY_GETTER, PROPERTY_SETTER, EXPRESSION, FILE, TYPEALIAS)
@Retention(SOURCE)
annotation class UseScope(
    vararg val markerClass: KClass<out Annotation>
)

The semantics of the annotation and the @UseScope annotation are similar to @Experimental, but the application is broadened to explicitly be about scopes rather than working for public APIs but actually being clearer as to what the feature means. It would solve a number of additional use cases beyond allowing for experimental code:

Limitations:

What do you think about this idea?

antanas-arvasevicius commented 5 years ago

Hi, is there a way to use multiple experimental APIs on the same class/method? Currently I'm trying to set couple @UseExperimental annotations but IDE says: "This annotation is not repeatable" e.g.:

    @UseExperimental(ExperimentalCoroutinesApi::class)
    @UseExperimental(KtorExperimentalAPI::class) // <- error here: not repeatable annotation
    private suspend fun connectToSocket() { ... 

Using Kotlin 1.3.10

LouisCAD commented 5 years ago

Can't you pass multiple classes in UseExeprimental annotation constructor?

On Tue, Nov 20, 2018, 1:36 PM Antanas A. notifications@github.com wrote:

Hi, is there a way to use multiple experimental API's on the same class/method? Currently I'm trying to set couple @UseExperimental annotations but IDE says: "This annotation is not repeatable" e.g.:

@UseExperimental(ExperimentalCoroutinesApi::class)
@UseExperimental(KtorExperimentalAPI::class) // <- error here: not repeatable annotation
private suspend fun connectToSocket() { ...

Using Kotlin 1.3.10

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Kotlin/KEEP/issues/95#issuecomment-440258543, or mute the thread https://github.com/notifications/unsubscribe-auth/AGpvBQzDXQXY3pdF5pnlb3Ocs-lovxeUks5uw_djgaJpZM4RNwF7 .

antanas-arvasevicius commented 5 years ago

Lol, sorry. I've missed that its argument is "vararg". IntelliJ not suggesting this.

antanas-arvasevicius commented 5 years ago

Hi, I was thinking about that UseExperimental annotation and I think the better approach would be omit "vararg" and make it repeatable, because: a) Different experimental APIs will have different lifecycles so they do not become stable all at once; After API will become stable to remove UseExperimental(API::class) will be simpler than variant with "varargs" and all source code could be replaced with simple search+replace.

b) Usage of experimental APIs is not predictable, at first I can use ObsoleteCoroutinesAPI and few days later use some KtorInternalAPI. So to add new UseExperimental is more convenient instead of adding more arguments in existing UseExperimental. Also git merge requests will look better.

c) Now I have an option to add some experimental APIs usage on a whole class and on methods so I'm using multiple experimental APIs and not using "varargs". So if I want to use one more experimental API in a method I'll need pass multiple arguments for UseExperimental. Some UseExperimental now have one argument (for class) and some mulitple arguments (for method).

Wouldn't be better to remove "vararg" support from UseExperimental and only make it repeatable? It will look more consistent, less cognitive thinking, easier to implement autosuggestions in IDE, easier to find and remove all UseExperimental() annotations then API becomes stable.

pdvrieze commented 5 years ago

@antanas-arvasevicius One problem with your suggestion is that the JVM 1.6 target does not support repeated annotations. The retention of the annotation should however be at least class (so that the compiler can verify API levels on usage). The current API seems to be the most elegant solution.

antanas-arvasevicius commented 5 years ago

@pdvrieze Android Studio 3+ should support repeatable annotations. See: https://developer.android.com/studio/write/java8-support

udalov commented 5 years ago

@pdvrieze Sorry for the late answer. I'm afraid I didn't completely understand how CustomScope is supposed to be used in your proposal. Could you please provide an example usage of a library code and the client code using it, demonstrating how exactly will it allow the compiler to warn the user about unwanted usages?

pdvrieze commented 5 years ago

@udalov I'll try to provide some examples. First based upon kotlinx.serialization runtime/common/src/main/kotlin/kotlinx/serialization/SerialImplicits.kt

Instead of:

@Experimental
annotation class ImplicitReflectionSerializer

we would generalize this to

@CustomScope(level=Level.WARNING)
annotation class ImplicitReflectionSerializer

However the idea is more extensive. One can define any kind of scope:

@CustomScope(Visibility.PUBLIC) annotation class CustomizationAPI
@CustomScope(Visibility.INTERNAL) annotation class TestOnly
@CustomScope(Visibility.INTERNAL) annotation class FriendOfExampleClass
@CustomScope(Visibility.PROTECTED) annotation class ProtectedScope
@CustomScope(Visibility.PRIVATE) annotation class PrivateScope
@CustomScope(Visibility.PRIVATE) annotation class MyLockedVar

open class ExampleClass {
  @MyLockedVar
  val myLockedVarLock = ReentrantLock()

  @MyLockedVar
  var myLockedVar: SomeObject = someInitializer

  @UseScope(MyLockedVar::class)
  inline fun <R> useLockedVar(action: (SomeObject)->R):R = myLockedVarLock.withLock {
    action(myLockedVar)
  }

  @PrivateScope
  var brittleVar: Any?=null // This would actually be private due to the annotation
  // perhaps brittleVar needs to have a lock before it can be updated, or any other operation where even 
  // class scope is not appropriate

  @UseScope(PrivateScope::class)
  fun doSomethingWithBrittleVar() {
    brittleVar = "Something" // some actually sensitive operation that can break
  }

  @ProtectedScope
  internal fun doSomethingOnlyInternal() = todo() // This is not visible outside the module - or child classes

  fun accidentallyUseBrittleVar() {
    println(brittleVar) // Will not compile due to scope
  }

  @FriendOfExampleClass // Actually has internal visibility
  fun friendOnlyFun() =todo("do something "internal")

  @TestOnly
  fun resetHookOnlyNeededForUnitTests() = todo()

  @CustomizationAPI // API only useful for customization, not regular users
  fun setCustomObjectFactory(factory: CustomObjectFactory) = todo()
}

// In same module
@UsesScope(FriendOfExampleClass::class)
fun friendFun(c: ExampleClass) // do something that calls c.friendOnlyFun

// In a different module (or the same one)
@UseScope(CustomizationAPI::class)
fun ExampleClass.reconfigure() {
  setCustomObjectFactory(myCustomObjectFactory)
}

@TestOnly
class TestExampleClass {
var c: ExampleClass

  @BeforeEach
  fun resetClass() {
    c.resetHookOnlyNeededForUnitTests()
  }
}

What are the benefits:

Considerations:

udalov commented 5 years ago

@pdvrieze Thank you for the elaborate example and the explanation!

Do I understand it correctly that the compiler uses CustomScope's visibility value to infer the actual visibility of the annotated declaration? E.g. is ExampleClass.brittleVar actually private in the bytecode? I'm not sure then if this would be any better than using the ExperimentalAPI simply as it exists today (maybe under a different name -- see below), and requiring the user to specify the visibility manually via public/internal/protected/private modifiers, which is more natural since modifiers are a very basic language construct, unlike annotations.

It seems to me that if the user manually adds private to brittleVar/myLockedVar, internal to friendOnlyFun/resetHookOnlyNeededForUnitTests etc., the remaining code will look exactly the same as today, only differing in the names: Experimental -> CustomScope, UseExperimental -> UseScope. Am I missing something? The only exception would be doSomethingOnlyInternal which has to be marked both protected and internal which is impossible in Kotlin, but since there's no such visibility in the language, this wouldn't work as simply in your proposal either, because this should be supported in the compiler first anyway. In other words, the visibility aspect in your proposal seems pretty detached from the scope aspect to me, and I don't see any reason to mix two independent language features when it's not required.

Regarding this point:

It prevents misuse of @ExperimentalAPI (such as the kotlinx.serialization example shows - the code is not experimental, it is merely non-portable)

I agree that the annotated API is not experimental and thus marking it as Experimental seems unfair. However, since the tool that the feature provides fits for the purpose the API's author wants to achieve, from my perspective this means that the feature is not actually misused and the problem is rather only in the name. If the Experimental annotation was named differently, for example Scoped (inspired by your proposal) or Restricted or something like that, but behaved exactly the same, we wouldn't call it a misuse, right? There's already an issue highlighting this problem: https://youtrack.jetbrains.com/issue/KT-26216

pdvrieze commented 5 years ago

@udalov Indeed brittleVar would be actually private in bytecode. In the case of private it may not be that worthwhile, but for visibilities such as internal it forces all users to be at most internal (but they can still be private) in bytecode. Private is there for completeness. Most of the difference indeed is in the name.

I can see the point about not mixing it, but in some cases you may want to limit a scope to a certain application. Using the annotation scope does not work as it relates to the declaration of the scope, not the use of it.

My point is mainly to rename, but also to broaden it up a bit. Although that technically breaks the annotations for semantics aspect. Btw. protected internal would be mangled as internal, but protected in the bytecode. The issue is quite close to my idea indeed. I'm not sure about misuse as wording is important, but it doesn't misuse semantics.

udalov commented 5 years ago

@pdvrieze Please share your naming suggestions in the comments to KT-26216, we'll come back to them when discussing whether and how to rename Experimental.

Regarding the visibility-altering behavior of an annotation -- I'm pretty sure this would lead to lots of issues in the compiler and tooling, the main reason of which is that you'd need to resolve and type-check all annotations before you can deduce the actual visibility of a declaration. In fact, the compiler even might need visibility of a declaration before resolving annotations right now in some cases, I can't be sure because such basic information was always available lexically, before any resolution happens. I think it would also complicate the ability of people reading the source code: in case the visibility is not evident from the scope annotation's name, it's pretty hard to understand whether the declaration you're looking at is public or private. To be sure, you'll need to check the source of all annotations on that declaration in the worst case. As such, I don't think this would be a good addition to this feature.

However, if you have any other suggestions on how to broaden the feature without introducing this sort of problems, we'll be glad to discuss them -- please share in KT-26216 as well. Thanks!

pdvrieze commented 5 years ago

@udalov I've added a short comment tot he bug.

On the visibility altering behaviour, perhaps I've not been sufficiently clear on how it works. Effectively it works as an upper bound on the visibility. It should work as follows: if a visibility modifier has been specified at the use site (not the custom scope) it will either be used or fail to compile (violates the bound). If no visibility modifier is specified (default public) then there are two options: either derive the visibility from the annotation, or check the visibility of the annotation and throw an error in case the visibility of the scope is not public. In all cases the actual visibility will be recorded in the visibility attributes of the method exactly the same way it is done already.

The more complex case is that of protected internal. This does not exist on Java bytecode level, but might be able to benefit from the same mechanism applied to regular internal. This could/should still be recorded in the extended Kotlin signature.

Key is that all visibility is resolved/determined at compilation of the scope use site and recorded in the usual way - so everything is visible/final in the method/class/.. declaration and does not depend on scope resolution. It might be that explicit visibility is better than default (except for public as is now) from a readability perspective and I don't mind dropping the default system - I still think it can helpful to be able to limit a scope to a visibility on its application (of course the annotation itself can have limited visibility too).

Scope usage thus at first does not need to resolve the scope annotation at all, it merely needs to compare the locally available scope/api with the one(s) declared on the symbol (potentially) being used. Only the currently already available error/warning option needs resolving/loading the annotation.

udalov commented 4 years ago

We've discussed this proposal internally once again and decided to change it in the following way:

pdvrieze commented 4 years ago

The rename seems like a good idea, it also matches other uses of the capability.

IRus commented 4 years ago

Therefore, we will require each user of RequiresOptIn to provide at least one -Xopt-in compiler argument Unless one of these arguments is provided, the compiler will report a warning

Is require mean that compiler should report error instead?

Issue about this behavior KT-37507