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

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

Open
udalov opened this issue Dec 27, 2017 · 31 comments
Open

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

udalov opened this issue Dec 27, 2017 · 31 comments
Assignees

Comments

@udalov
Copy link
Member

udalov commented Dec 27, 2017

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

@udalov udalov self-assigned this Dec 27, 2017
udalov added a commit that referenced this issue Dec 27, 2017
@LouisCAD
Copy link
Contributor

LouisCAD commented Dec 28, 2017

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
Copy link
Contributor

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
Copy link

robstoll commented Jan 11, 2018

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
Copy link

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
Copy link
Member Author

udalov commented Jan 22, 2018

@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
Copy link
Member Author

udalov commented Jan 22, 2018

@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
Copy link
Contributor

@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:

  • core
  • io

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
Copy link
Member Author

udalov commented Jan 25, 2018

@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
Copy link
Member Author

udalov commented Jan 30, 2018

I've updated the proposal after an internal discussion:

  • we've decided to get rid fo the confusing behavior in the "same module exemption" rule: what is taken into account now is the declaring module of the annotation marker. If the call site is in that module, body usages do not require propagation. If it's in another module, body usages always require propagation. (The reason is that otherwise it was easy to come up with an example with three modules where if the middle module is not recompiled, one could observe runtime errors in the leaf module upon changes to the experimental API, even if no consent to use any experimental API was given in the leaf module.)
  • we've changed Scope.{COMPILE_TIME/RUNTIME} to Impact.{COMPILATION/LINKAGE/RUNTIME} to be able to say more clearly what exactly can the changes to the experimental API break.

udalov added a commit that referenced this issue Feb 7, 2018
To avoid conflict with the Annotation.annotationClass extension from
kotlin-stdlib
@udalov
Copy link
Member Author

udalov commented Feb 7, 2018

A few minor updates:

  • same module exemption is now also supported for non-effectively-public inline functions, since they can only be used in the same module where the experimental API is introduced and thus are guaranteed to be updated accordingly after any breaking change to the experimental API
  • UseExperimental's argument has been renamed from annotationClass to markerClass to avoid clash with an existing extension property annotationClass that returns a class of the annotation

@udalov
Copy link
Member Author

udalov commented Feb 8, 2018

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 added a commit that referenced this issue Apr 24, 2018
Looks like there is a way to make these declarations experimental
themselves after all
@udalov
Copy link
Member Author

udalov commented Apr 24, 2018

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 added a commit to JetBrains/kotlin that referenced this issue Apr 26, 2018
See Kotlin/KEEP#95 (comment)

Drop Experimental.changesMayBreak, Experimental.Impact, the concept of
signature/body usage, same module exemption. Make the majority of tests
single-module because there is now no difference in the checker between
usages from the same module or from another module
udalov added a commit to JetBrains/kotlin that referenced this issue Apr 30, 2018
See Kotlin/KEEP#95 (comment)

Drop Experimental.changesMayBreak, Experimental.Impact, the concept of
signature/body usage, same module exemption. Make the majority of tests
single-module because there is now no difference in the checker between
usages from the same module or from another module
udalov added a commit to JetBrains/kotlin that referenced this issue May 4, 2018
See Kotlin/KEEP#95 (comment)

Drop Experimental.changesMayBreak, Experimental.Impact, the concept of
signature/body usage, same module exemption. Make the majority of tests
single-module because there is now no difference in the checker between
usages from the same module or from another module
udalov added a commit that referenced this issue May 4, 2018
It made little sense in the first version of the proposal, but now
there's no reason to prevent using experimental API in type alias
declaration
udalov added a commit that referenced this issue Jul 13, 2018
Looks like there is a way to make these declarations experimental
themselves after all
udalov added a commit that referenced this issue Jul 13, 2018
It made little sense in the first version of the proposal, but now
there's no reason to prevent using experimental API in type alias
declaration
@ilya-g
Copy link
Member

ilya-g commented Oct 16, 2018

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
Copy link
Member Author

udalov commented Oct 16, 2018

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

@pdvrieze
Copy link

pdvrieze commented Nov 3, 2018

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:

  • It would allow for a library to have different APIs for different use cases: for example one for plugins, one for mere users.
  • It would allow for internal or otherwise restricted visibilities that could also replace the most common uses for package level visibility

Limitations:

  • These custom scopes are use-site opt-ins. Within a module it is not possible to stop code from either having the annotation (becoming part of the interface) or declaring usage - Of course this can be statically analysed and restricted. It should be fairly straightforward to limit propagating use of the visibility beyond the module (if this is desirable).
  • It does not directly express the semantics of experimental
  • It needs a bit more compiler support to work correctly for non-public visibility.

What do you think about this idea?

@antanas-arvasevicius
Copy link

antanas-arvasevicius commented Nov 20, 2018

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
Copy link
Contributor

LouisCAD commented Nov 20, 2018 via email

@antanas-arvasevicius
Copy link

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

@antanas-arvasevicius
Copy link

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
Copy link

@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
Copy link

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

@udalov
Copy link
Member Author

udalov commented Dec 12, 2018

@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
Copy link

@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:

  • Libraries can distinguish between different kinds of public API's for different purposes - be it experimental or some other reason to separate it. For example customization APIs can be easy to use incorrectly and can clutter up the documentation for mere users.
  • It is a superset of @experimentalapi
  • It solves the problem that package protection is used to solve in Java, in particular it allows for a TestOnly scope.
  • In large classes it can even restrict access to private members to a select few (especially audited) functions. One could use this for example to ensure that a certain variable is always accessed through its lock (for example useLockedVar)
  • It prevents misuse of @experimentalapi (such as the kotlinx.serialization example shows - the code is not experimental, it is merely non-portable)
  • Using annotations there is no keyword proliferation.

Considerations:

  • Some possibilities may be more worthwhile than others but there is value in being a generic solution. Is a custom private scope valuable in practice? It might be with more complex classes, but not including private is inconsistent and it is not detrimental.
  • For warning-level scopes the annotation would not actually replace the pre-established scope.
  • @UseScope(...) is a bit verbose, I'm not sure what to do about that.
  • There is no implied usage semantics to the scope, while there are clear semantics on what the compiler will do, it does not imply to the user that it is for experimental API's only. It only requires opt-in.

@udalov
Copy link
Member Author

udalov commented Jan 2, 2019

@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
Copy link

pdvrieze commented Jan 2, 2019

@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
Copy link
Member Author

udalov commented Jan 2, 2019

@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
Copy link

pdvrieze commented Jan 7, 2019

@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 added a commit that referenced this issue Dec 16, 2019
)

The annotations are renamed too: `Experimental` -> `RequiresOptIn`,
`UseExperimental` -> `OptIn`.

This is needed to facilitate usage of this mechanism in other related
areas, such as restricted, internal, obsolete APIs etc. See KT-26216.
udalov added a commit that referenced this issue Dec 16, 2019
@udalov
Copy link
Member Author

udalov commented Dec 16, 2019

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

  • Rename Experimental -> RequiresOptIn, UseExperimental -> OptIn and the whole feature to "opt-in requirements" to enable broader usages.
  • Add custom message to RequiresOptIn to override the default "this API is experimental" compiler diagnostic message.
  • Remove the compiler argument -Xexperimental (but keep -Xuse-experimental, now known as -Xopt-in).

@pdvrieze
Copy link

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

udalov added a commit that referenced this issue Dec 24, 2019
The annotations are renamed too: `Experimental` -> `RequiresOptIn`,
`UseExperimental` -> `OptIn`.

This is needed to facilitate usage of this mechanism in other related
areas, such as restricted, internal, obsolete APIs etc. See KT-26216.

Also, add message to `RequiresOptIn`, see KT-28872.
@udalov udalov changed the title Experimental API support Opt-in requirements (aka Experimental API support) Dec 24, 2019
@IRus
Copy link

IRus commented Apr 25, 2020

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

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

8 participants