Implement sealed inline classes feature #4903

ilmirus · 2022-07-20T11:29:47Z

KEEP: Kotlin/KEEP#312

sfs

I'm still going through the commits, but here's a first batch of comments.

compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java

compiler/ir/backend.js/src/org/jetbrains/kotlin/ir/backend/js/lower/VarargLowering.kt

...er/ir/backend.jvm/lower/src/org/jetbrains/kotlin/backend/jvm/lower/JvmInlineClassLowering.kt

sfs · 2022-08-03T16:53:51Z

...er/ir/backend.jvm/lower/src/org/jetbrains/kotlin/backend/jvm/lower/JvmInlineClassLowering.kt

-                +irDelegatingConstructorCall(context.irBuiltIns.anyClass.owner.constructors.single())
+                +irDelegatingConstructorCall(valueClass.superTypes.single {
+                    it.classOrNull?.owner?.kind == ClassKind.CLASS
+                }.classOrNull!!.constructors.single().owner)


I'm confused. This code (and the example in the test) seems to imply that we generate wrapper classes for inline subclasses of sealed inline classes, while the specification explicitly states that these classes are not generated. Why are they needed and when are they used?

Yeah. Sorry. The problem here is that design has changed middle-way and first commits became obsolete. To avoid the confusion, I should have squashed the commits. I did it now and split it into several relevant parts.

...er/ir/backend.jvm/lower/src/org/jetbrains/kotlin/backend/jvm/lower/JvmInlineClassLowering.kt

sfs · 2022-08-05T12:34:29Z

compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/InlineClassDeclarationChecker.kt

+                        val sealedKeyword = declaration.modifierList?.getModifier(KtTokens.SEALED_KEYWORD) ?: declaration
+                        trace.report(
+                            Errors.INLINE_CLASS_UNDERLYING_VALUE_IS_SUBCLASS_OF_ANOTHER_UNDERLYING_VALUE.on(sealedKeyword)
+                        )


At least with the design in the KEEP this check seems superfluous, since intermediate sealed classes would never actually be materialized.

Plans here are restrict the check even further and disallow non-final types as underlying types of inline subclasses of sealed inline classes, due to Kotlin/KEEP#318 (comment)

...er/ir/backend.jvm/lower/src/org/jetbrains/kotlin/backend/jvm/lower/JvmInlineClassLowering.kt

sfs · 2022-08-09T11:00:20Z

There's two more comments that I couldn't add because they were from earlier commits:

I don't think the code for type parameters in coerceInlineClasses is entirely correct. The unsafeCoerce intrinsic looks like this:

fun <A, B> unsafeCoerce(x: A): B

while the generated call is

unsafeCoerce<From, UnderlyingType>(argument as UnderlyingType)

I think it's probably for the best to avoid folding any additional casts into this intrinsic in any case, since it really should only be used for boxing/unboxing inline classes. Casts from/to upper bounds can always be added around it.

The workaround for nullable receivers in JvmInlineClassLowering.visitGetField won't work for nullable type parameters, e.g., if we have

value class A(val x: String)

and now, e.g., the debugger is asked to evaluate an expression a?.x_field in a context where a : T and T : A? then the code will fail to produce a non-nullable receiver, since makeNotNull doesn't handle type parameters. It's probably best to add an explicit cast to the inline class type instead.

In fact, this should probably be done when we generate code for checked calls in general, since an IrGetField (IrCall) needs a non-nullable (dispatch) receiver.

udalov

I've only reviewed ~2/3 of the changes. Will continue next week.

compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/InlineClassDeclarationChecker.kt

udalov · 2022-08-10T11:28:32Z

...er/ir/backend.jvm/lower/src/org/jetbrains/kotlin/backend/jvm/lower/JvmInlineClassLowering.kt

+     *
+     * First, we check for noinline children, then for inline children and finally, just run the top's method body.
+     */
+    private fun rewriteOpenAndAbstractMethodsForSealed(info: SealedInlineClassInfo) {


It would be amazing to split this function into several logically independent functions to simplify reading/navigation

Split it a bit.

compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java

udalov · 2022-08-12T22:11:26Z

compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/declarations/IrDeclarations.kt

+    get() = this.isValue && valueClassRepresentation is InlineClassRepresentation
+
+val IrClass.isInline: Boolean
+    get() = (isSingleFieldValueClass && annotations.hasAnnotation(JVM_INLINE_ANNOTATION_FQ_NAME)) || (isValue && modality == Modality.SEALED)


It almost seems like it would be a good idea to add another subclass of ValueClassRepresentation, SealedInlineClassRepresentation, for example to keep the invariant isValue == (valueClassRepresentation != null)

Introduced SealedInlineClassRepresentation, and used it everywhere.

udalov · 2022-08-12T22:15:40Z

compiler/testData/diagnostics/tests/inlineClasses/sealed-inline-class/class-child.kt

+--------------------------+-------------+-------------+-------------+-------------+-------------+
+|inline class              |nope         |nope         |nope         |nope         |nope         |
+--------------------------+-------------+-------------+-------------+-------------+-------------+
+|sealed interface          |yep          |OOS          |yep          |yep          |nope         |


Not really clear what "OOS" means :)

Probably "out of scope"

udalov · 2022-08-12T22:24:39Z

compiler/ir/ir.psi2ir/src/org/jetbrains/kotlin/psi2ir/generators/ClassGenerator.kt

                generateAdditionalMembersForMultiFieldValueClasses(irClass, ktClassOrObject)
            }

            if (DescriptorUtils.isEnumClass(classDescriptor)) {
                generateAdditionalMembersForEnumClass(irClass)
            }

+            if (classDescriptor.isInline && !classDescriptor.annotations.hasAnnotation(JVM_INLINE_ANNOTATION_FQ_NAME)) {


It feels weird that we add an annotation which is not there on the declaration site, to supposedly fix the lack of information of whether the class was value/inline in IR... or something? And because of that, a lot of tests depend on stdlib now. Can we invent some other approach, like adding some new information to IR nodes? Maybe you can explain the problem?

Well, it is by design - the annotation is used by libraries, which use reflection, like mockito, to determine, whether a class is inline of not and to pass underlying value instead of boxed one. So, we have to generate the annotation.

...er/ir/backend.jvm/lower/src/org/jetbrains/kotlin/backend/jvm/lower/JvmInlineClassLowering.kt

ilmirus · 2022-10-20T02:43:26Z

@sfs, @udalov Thanks a lot for your comments. I reorganized the code, so it is hopefully easier to review, since there were a lot of obsolete code, due to design change midway. Sorry I kept you waiting - I was waiting for value classes, which required some refactoring, so, I would have to adopt the changes anyway.

They are always mapped to `Any?`, so, there is no need to serialize inline class representation for sealed inline classes. #KT-27576

#KT-27576

`inline` modifier during PSI2IR instead of during lowering. This is to distinguish inline classes and value classes with one property, but which are not inline. #KT-27576

#KT-27576

black box test. #KT-27576

TODO: generate method for is checks. #KT-27576

#KT-27576: Fixed

ilmirus requested a review from udalov July 20, 2022 11:29

ilmirus force-pushed the rra/ilmir/sealed-inline-classes branch from 91bc4fb to 4a87c9a Compare July 22, 2022 10:41

sfs reviewed Aug 9, 2022

View reviewed changes

udalov reviewed Aug 12, 2022

View reviewed changes

ilmirus force-pushed the rra/ilmir/sealed-inline-classes branch from 4a87c9a to 122be41 Compare October 20, 2022 01:53

ilmirus added 9 commits December 18, 2022 21:02

FE, JVM_IR: Create new representation for sealed inline classes

db85fe8

They are always mapped to `Any?`, so, there is no need to serialize inline class representation for sealed inline classes. #KT-27576

FE: Support sealed inline classes

2cbafcd

#KT-27576

JVM_IR: Generate @JvmInline annotation for inline classes with

149ad50

`inline` modifier during PSI2IR instead of during lowering. This is to distinguish inline classes and value classes with one property, but which are not inline. #KT-27576

JVM IR: Support sealed inline classes

f97fc9d

#KT-27576

JVM IR: Support sealed inline classes in inliner

8e54747

#KT-27576

JVM IR: Generate tests for sealed inline classes as a separate

345814d

black box test. #KT-27576

JVM IR: Support casts to/from sealed inline class

dd99e9f

TODO: generate method for is checks. #KT-27576

JVM IR: Support sealed inline class methods

ab3e9a2

#KT-27576: Fixed

Minor. Unmute tests on Old JVM

126d074

ilmirus force-pushed the rra/ilmir/sealed-inline-classes branch from 122be41 to 126d074 Compare December 18, 2022 22:06

kyay10 mentioned this pull request Feb 27, 2023

Value Class Option [Arrow 2] arrow-kt/arrow#2950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement sealed inline classes feature #4903

Implement sealed inline classes feature #4903

ilmirus commented Jul 20, 2022

sfs left a comment

sfs Aug 3, 2022

ilmirus Oct 20, 2022

sfs Aug 5, 2022

ilmirus Oct 20, 2022

sfs commented Aug 9, 2022

udalov left a comment

udalov Aug 10, 2022

ilmirus Oct 20, 2022

udalov Aug 12, 2022

ilmirus Oct 20, 2022

udalov Aug 12, 2022

udalov Aug 12, 2022

ilmirus Oct 20, 2022

udalov Aug 12, 2022

ilmirus Oct 20, 2022

ilmirus commented Oct 20, 2022

Implement sealed inline classes feature #4903

Are you sure you want to change the base?

Implement sealed inline classes feature #4903

Conversation

ilmirus commented Jul 20, 2022

sfs 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

sfs commented Aug 9, 2022

udalov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilmirus commented Oct 20, 2022