From 4b75a1c2e28f068af330b2551a2d1b820870bd74 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Mon, 13 Dec 2021 20:26:20 +0300 Subject: [PATCH 1/7] Draft KEEP for #283 --- proposals/enum-entries.md | 168 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 proposals/enum-entries.md diff --git a/proposals/enum-entries.md b/proposals/enum-entries.md new file mode 100644 index 000000000..665998ad8 --- /dev/null +++ b/proposals/enum-entries.md @@ -0,0 +1,168 @@ +# Decommission `Enum.values()` and replace it with `Enum.entries` + +* **Type**: Design proposal +* **Author**: Vsevolod Tolstopytov +* **Status**: Accepted +* **Prototype**: Being discussed +* **Discussion**: [KEEP-283](https://github.com/Kotlin/KEEP/issues/283) +* **Related issues**: [KT-48872](https://youtrack.jetbrains.com/issue/KT-48872) + +This proposal describes a rationale and a path to migrate from function `values()` provided by each enum type to a collection-based and more predictable abstraction. + +## Motivation + +Every Java and Kotlin enum type provides a [`values`](https://kotlinlang.org/spec/declarations.html#enum-class-declaration) method +that returns an array of all enum entries. + +Arrays are mutable by default, meaning that each call to `values()` always has to allocate a new instance of the array. +The API shape does not indicate that fact, often [leading](#examples-of-performance-issues) to hidden performance bugs in both Kotlin and Java. +It is hard for library authors to determine whether an arbitrary call to `values()` may lead to a performance bottleneck as +the profile depends on the "hotness" of the method, not on the enum's characteristics, effectively forcing authors to apply +a workaround or leave it as a potential performance issue. + +Apart from that, most of the API being written leverages Collections API, not arrays, forcing users to manually convert arrays to lists. + +### Acknowledgements of the problem + +* It is considered a ["design bug"](http://mail.openjdk.java.net/pipermail/compiler-dev/2018-July/012242.html) in Java. +* [JDK-8073381 need API to get enums values without creating a new arra...](https://bugs.openjdk.java.net/browse/JDK-8073381). +* Scala 3 [attempted](https://github.com/lampepfl/dotty/issues/6620) to address this issue. +* Graal [specifically optimizes](https://github.com/oracle/graal/issues/574) errorneous pattern `values()[idx]`. +* [Memory-Hogging Enum.values() Method](https://dzone.com/articles/memory-hogging-enumvalues-method) + +### Examples of performance issues + + * [HttpStatus.resolve allocates HttpStatus.values() once per invocation](https://github.com/spring-projects/spring-framework/issues/26842) + * [Kotlin standard library](https://github.com/JetBrains/kotlin/blob/92d200e093c693b3c06e53a39e0b0973b84c7ec5/libraries/stdlib/jvm/src/kotlin/text/CharCategoryJVM.kt#L170) + * [kotlinx.serializarion Enum deserializer]( https://github.com/Kotlin/kotlinx.serialization/issues/1372) + * [MySQL JDBC Remove Enum.values() calls to avoid unnecessary array](https://github.com/Microsoft/mssql-jdbc/pull/1065) + +## Proposal + +The proposal that addresses all of the above is as follows: + +* Decommission of `Enum.values()` with the IDE assistance without deprecation. +* Introduce property `entries: List` that returns an unmodifiable list of all enum entries. +* For already compiled Kotlin enums and Java enums, a special mapping class that contains a pre-allocated list of entries is generated. + +### Decommission of `Enum.values()` + +Enums have been existing since the very beginning of Kotlin, and since Java 1.5, released in 2004, meaning that the deprecation +of `values` using Kotlin's standard [deprecation cycle](https://kotlinlang.org/docs/kotlin-evolution.html#incompatible-changes) +will create unnecessary disturbance in the ecosystem and will render a lot of already existing educational materials outdated. + +To avoid that, `values()` will be softly decommissioned via IDE assistance: + +* `values` will be de-prioritized and eventually removed from IDE auto-completion. +* Soft warning with an intention to replace call to `values()` with call to `entries` will be introduced. +* All the corresponding materials, such as Kotlin guides, J2K and tutorials will be adjusted to use `entries` API. +* Eventually, we are going to re-visit this decision and decide on the further deprecation of `values` API. + +### Naming alternatives + +Various languages use different notations to refer to enumeration elements, making the naming choice especially hard. + +We have considered the following alternatives: + +* `values` property. While being the most familiar one, it carries the burdens of the existing `values()` API: + * It is easy to misspel it with `values()` and fall into the same trap. + * Introduction of `values` property adds an unfixable method reference ambiguity for all the callers of `E::values`. +* Java language [refers](https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.9.1) to enum elements as `constants`. + * The risks of introducing widely-used API named `constants` with the potential clashes with [Kotlin constant evaluation](https://youtrack.jetbrains.com/issue/KT-14652) outweighs the benefits of the straightforward name. +* `valuesList` and `entryList` suffer the same disease -- excessive verbosity of the name. +* `components` is already reserved in Kotlin for positional deconstructors. +* Kotlin specification and documentation already refer to enum elements as entries, making it our final choice. + +### Translation strategy for Kotlin enums + +For source-compiled enums, a new static property with a JVM signature `public static List getEntries()` is added, +returning a pre-allocated immutable list of enum entries. + +For the sake of compatibility with bytecode pre-processors and reflection usages that modify a backing array `$VALUES` field, +the generated bytecode for `values()` is left intact, and the resulting list of entries reads the underlying `$VALUES` lazily, +leaving the chance for existing bytecode preprocessors to change the array without dealing with list allocation. + +#### Implementation note + +A special type, `EnumEntriesList` is introduced to the standard library. The type has the only constructor +accepting the functional type with a signature `() -> Array`. The corresponding lambda is generated +using the standard `invokedynamic` with `LambdaMetafactory`. + +The final decompiled class for the enum +```kotlin +enum class MyEnum { + A; +} +``` + +has the following form (all irrelevant parts omitted): +```java +enum MyEnum extends Enum { + private static final synthetic MyEnum[] $VALUES + private static final synthetic List $ENTRIES; + + { + A = new MyEnum("A", 0); + $VALUES = $values(); + Supplier supplier = #invokedynamic ..args.. $IVALUES; + $ENTRIES = new EnumEntriesList(supplier); + } + + public static MyEnum[] values() { + return $VALUES.clone(); + } + + public static List getEntries() { + return $ENTRIES; + } + + private synthetic static MyEnum[] $values() { + return new MyEnum[] { A }; + } + + private synthetic static MyEnum[] $IVALUES() { + return $VALUES; + } +} +``` + +### Translation strategy for compiled Kotlin enums and Java enums + +For already compiled Kotlin enums and Java enums, a separate synthetic mapping classfile will be introduced +for each callsite of `Enum.entries`. The mapping class acts as a storage for a lazily-initialized and pre-allocated list of +enum entries. + +## Risks and assumptions + +The proposal has two main risks: + +The potential source-compatibility issue for `entries` property in the companion objects within a Enum. +After the implementation of the proposed change as is, the following existing code will change its behaviour: + +```kotlin +enum class MyEnum { + A; + + companion object { + val entries: Any? = ... + } +} + +MyEnum.entries // <- member has a higher priority than a companion member +``` + +To mitigate that, a separate compiler-assistant deprecation cycle is introduced, along with the preservation of +the resolve to `entries` from companion object. +Taking into account the additional deprecation cycle and non-trivial setup for the problem to reproduce, we do not +consider it a serious threat to compatibility. + +The second risk is the education disturbance and a new name for developers to get familiar with -- `entries`, opposed to already well-known `values`. + +## Collateral changes + +In addition to an existing [`enumValues`](https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/enum-values.html) function in the standard library, +`enumEntries` function that returns `entries` list is added. + +## Timeline + +The feature is going to be available as experimental starting from Kotlin 1.7.0 and as stable starting from Kotlin 1.8.0. From cda13ce98f4b8c1c6721d8d86a54967aecc6ee8e Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Tue, 14 Dec 2021 18:34:48 +0300 Subject: [PATCH 2/7] Update KEEP --- proposals/enum-entries.md | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/proposals/enum-entries.md b/proposals/enum-entries.md index 665998ad8..df9e6ac7f 100644 --- a/proposals/enum-entries.md +++ b/proposals/enum-entries.md @@ -3,7 +3,7 @@ * **Type**: Design proposal * **Author**: Vsevolod Tolstopytov * **Status**: Accepted -* **Prototype**: Being discussed +* **Prototype**: In progress * **Discussion**: [KEEP-283](https://github.com/Kotlin/KEEP/issues/283) * **Related issues**: [KT-48872](https://youtrack.jetbrains.com/issue/KT-48872) @@ -42,7 +42,7 @@ Apart from that, most of the API being written leverages Collections API, not ar The proposal that addresses all of the above is as follows: * Decommission of `Enum.values()` with the IDE assistance without deprecation. -* Introduce property `entries: List` that returns an unmodifiable list of all enum entries. +* Introduce property `entries: EnumEntries` that returns an unmodifiable list of all enum entries. * For already compiled Kotlin enums and Java enums, a special mapping class that contains a pre-allocated list of entries is generated. ### Decommission of `Enum.values()` @@ -58,6 +58,15 @@ To avoid that, `values()` will be softly decommissioned via IDE assistance: * All the corresponding materials, such as Kotlin guides, J2K and tutorials will be adjusted to use `entries` API. * Eventually, we are going to re-visit this decision and decide on the further deprecation of `values` API. +### `EnumEntries` type + +Effectively, `entries` represents an immutable list of enum entries and can be represented as type `List`. +To have an ability to further extend Enum's API in a non-intrusive manner that does not involve code-generation +for each enum, we expose a direct subtype of `List` named `EnumEntries`. + +All potential extensions, such as `valueOfOrNull(String)`, `hasValue(String)` can be implemented on the standard library +level as extensions of `EnumEntries`. + ### Naming alternatives Various languages use different notations to refer to enumeration elements, making the naming choice especially hard. @@ -104,7 +113,7 @@ enum MyEnum extends Enum { { A = new MyEnum("A", 0); $VALUES = $values(); - Supplier supplier = #invokedynamic ..args.. $IVALUES; + Supplier supplier = #invokedynamic ..args.. values; $ENTRIES = new EnumEntriesList(supplier); } @@ -119,10 +128,6 @@ enum MyEnum extends Enum { private synthetic static MyEnum[] $values() { return new MyEnum[] { A }; } - - private synthetic static MyEnum[] $IVALUES() { - return $VALUES; - } } ``` @@ -162,6 +167,7 @@ The second risk is the education disturbance and a new name for developers to ge In addition to an existing [`enumValues`](https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/enum-values.html) function in the standard library, `enumEntries` function that returns `entries` list is added. +`enumValues` is deprecated for the removal. ## Timeline From 34bb9857c350a1f3bb868e4277cf333b91618bc5 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Tue, 12 Jul 2022 18:04:40 +0200 Subject: [PATCH 3/7] Update naming --- proposals/enum-entries.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/proposals/enum-entries.md b/proposals/enum-entries.md index df9e6ac7f..c37e607bc 100644 --- a/proposals/enum-entries.md +++ b/proposals/enum-entries.md @@ -84,7 +84,7 @@ We have considered the following alternatives: ### Translation strategy for Kotlin enums -For source-compiled enums, a new static property with a JVM signature `public static List getEntries()` is added, +For source-compiled enums, a new static property with a JVM signature `public static EnumEntries getEntries()` is added, returning a pre-allocated immutable list of enum entries. For the sake of compatibility with bytecode pre-processors and reflection usages that modify a backing array `$VALUES` field, @@ -108,7 +108,7 @@ has the following form (all irrelevant parts omitted): ```java enum MyEnum extends Enum { private static final synthetic MyEnum[] $VALUES - private static final synthetic List $ENTRIES; + private static final synthetic EnumEntries $ENTRIES; { A = new MyEnum("A", 0); @@ -121,7 +121,7 @@ enum MyEnum extends Enum { return $VALUES.clone(); } - public static List getEntries() { + public static EnumEntries getEntries() { return $ENTRIES; } From 721bc2739e5652149077486cdb9fcf32b8c37755 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Fri, 22 Jul 2022 16:47:06 +0200 Subject: [PATCH 4/7] Update timeline --- proposals/enum-entries.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/proposals/enum-entries.md b/proposals/enum-entries.md index c37e607bc..90a12d2ee 100644 --- a/proposals/enum-entries.md +++ b/proposals/enum-entries.md @@ -3,7 +3,7 @@ * **Type**: Design proposal * **Author**: Vsevolod Tolstopytov * **Status**: Accepted -* **Prototype**: In progress +* **Prototype**: Implemented * **Discussion**: [KEEP-283](https://github.com/Kotlin/KEEP/issues/283) * **Related issues**: [KT-48872](https://youtrack.jetbrains.com/issue/KT-48872) @@ -171,4 +171,4 @@ In addition to an existing [`enumValues`](https://kotlinlang.org/api/latest/jvm/ ## Timeline -The feature is going to be available as experimental starting from Kotlin 1.7.0 and as stable starting from Kotlin 1.8.0. +The feature is going to be available as experimental starting from Kotlin 1.8.0 and as stable starting from Kotlin 1.9.0. From edc34e4be2042c3e130fa29088356d4e979fac05 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Mon, 1 Aug 2022 11:25:18 +0300 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: ilya-g --- proposals/enum-entries.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/proposals/enum-entries.md b/proposals/enum-entries.md index 90a12d2ee..45a9a7afa 100644 --- a/proposals/enum-entries.md +++ b/proposals/enum-entries.md @@ -141,7 +141,7 @@ enum entries. The proposal has two main risks: -The potential source-compatibility issue for `entries` property in the companion objects within a Enum. +The first is the potential source-compatibility issue for `entries` property in the companion object within an Enum. After the implementation of the proposed change as is, the following existing code will change its behaviour: ```kotlin @@ -161,7 +161,7 @@ the resolve to `entries` from companion object. Taking into account the additional deprecation cycle and non-trivial setup for the problem to reproduce, we do not consider it a serious threat to compatibility. -The second risk is the education disturbance and a new name for developers to get familiar with -- `entries`, opposed to already well-known `values`. +The second risk is the education disturbance and a new name for developers to get familiar with — `entries`, opposed to already well-known `values`. ## Collateral changes From adc02b7fb13529e6ee99d591fc928ba47b9a81f4 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Mon, 1 Aug 2022 16:34:42 +0200 Subject: [PATCH 6/7] ~stylistic comments --- proposals/enum-entries.md | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/proposals/enum-entries.md b/proposals/enum-entries.md index 45a9a7afa..0aeb256f4 100644 --- a/proposals/enum-entries.md +++ b/proposals/enum-entries.md @@ -62,10 +62,17 @@ To avoid that, `values()` will be softly decommissioned via IDE assistance: Effectively, `entries` represents an immutable list of enum entries and can be represented as type `List`. To have an ability to further extend Enum's API in a non-intrusive manner that does not involve code-generation -for each enum, we expose a direct subtype of `List` named `EnumEntries`. +for each enum, we expose a direct subtype of `List` named `EnumEntries`: + +``` +sealed interface EnumEntries> : List +``` + +We deliberately limit any extensions of this type to the standard library to have a future-proof way to +extend it in the future in a backwards-compatible manner. +All future potential extensions, such as `valueOfOrNull(String)`, `hasValue(String)` can be implemented on the standard library +level as members or extensions of `EnumEntries`. -All potential extensions, such as `valueOfOrNull(String)`, `hasValue(String)` can be implemented on the standard library -level as extensions of `EnumEntries`. ### Naming alternatives @@ -114,7 +121,7 @@ enum MyEnum extends Enum { A = new MyEnum("A", 0); $VALUES = $values(); Supplier supplier = #invokedynamic ..args.. values; - $ENTRIES = new EnumEntriesList(supplier); + $ENTRIES = EnumEntries.Kt.enumEntries(supplier); // internal factory from standard library } public static MyEnum[] values() { @@ -156,12 +163,13 @@ enum class MyEnum { MyEnum.entries // <- member has a higher priority than a companion member ``` -To mitigate that, a separate compiler-assistant deprecation cycle is introduced, along with the preservation of -the resolve to `entries` from companion object. +To mitigate that, a separate compiler-assistant deprecation cycle is introduced, that will keep companion's `entries` +priority higher than auto-generated memeber for the duration of the deprecation cycle. Taking into account the additional deprecation cycle and non-trivial setup for the problem to reproduce, we do not consider it a serious threat to compatibility. The second risk is the education disturbance and a new name for developers to get familiar with — `entries`, opposed to already well-known `values`. +It is mitigated by indefinitely long [soft decommision](#decommission-of-enumvalues) instead of a regular deprecation cycle. ## Collateral changes @@ -172,3 +180,4 @@ In addition to an existing [`enumValues`](https://kotlinlang.org/api/latest/jvm/ ## Timeline The feature is going to be available as experimental starting from Kotlin 1.8.0 and as stable starting from Kotlin 1.9.0. +The corresponding language feature can be enabled with the `-XXLanguage:+EnumEntries` compiler argument in Kotlin 1.8.0. From 76cbd441d2825508ba4760abc347387a731a19a5 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Mon, 1 Aug 2022 16:36:53 +0200 Subject: [PATCH 7/7] ~adjust code generation strategy to reflect the latest changes in the compiler --- proposals/enum-entries.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/proposals/enum-entries.md b/proposals/enum-entries.md index 0aeb256f4..04e45c675 100644 --- a/proposals/enum-entries.md +++ b/proposals/enum-entries.md @@ -120,7 +120,7 @@ enum MyEnum extends Enum { { A = new MyEnum("A", 0); $VALUES = $values(); - Supplier supplier = #invokedynamic ..args.. values; + Supplier supplier = #invokedynamic ..args.. $entries; $ENTRIES = EnumEntries.Kt.enumEntries(supplier); // internal factory from standard library } @@ -132,6 +132,10 @@ enum MyEnum extends Enum { return $ENTRIES; } + private synthetic static MyEnum[] $entries() { + return $VALUES; + } + private synthetic static MyEnum[] $values() { return new MyEnum[] { A }; }