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

Add KEEP for sealed inline classes #312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ilmirus
Copy link
Member

@ilmirus ilmirus commented Jul 20, 2022

No description provided.


## Motivation / use cases

The main use-case for sealed inline classes is `Result`. Currently it is decraled as inline class with underlying type `Any?`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: decraled


This is the reason for the following restrictions.

- Noinline children (classes and objects) should have `value` modifier, as shown in the examples. Since the noinline children can be boxed, they cannot have stable identity. In other words, they are identitiless, and we use `value` modifier to mark them.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: identitiless


`<hash>` is computed using usual [inline classes mangling rules](https://github.com/Kotlin/KEEP/blob/master/proposals/inline-classes.md#mangling-rules).

- When we pass nullable sealed inline class, it is mapped to boxed selaed inline class:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: selaed

the compiler generates the following code

```kotlin
val ic = Integet.valueOf(1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: Integet

val ic = Integet.valueOf(1)
```

The following rulues for passing to function and returning from the function apply:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: rulues

fun `toString-impl`(value: Any?): String = "I3${`$value`}"
```

The logic for `toString` applies for all methods, decrared in interfaces: if the method is not overridden in inline child, we do not generate redirect.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: decrared


```kotlin
// In I1
synthetic fun `str-impl`(value: Any?): String = I2.`str-impl`(value)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

synthetic has been used inconsistently in the examples.
See the following lines or the definition of the $value property.


```kotlin
class IC {
val $value: Any?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a Kotlin property?

We should expect on JVM?

private Object $value
public Object get$value()

* **Status**: Experimental
* **Prototype**: TODO

Discussion of this proposal is held in [this issue](TODO).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, create an issue for discussion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


The changes also rely on [inline classes with generic underlying value](https://youtrack.jetbrains.com/issue/KT-32162/Allow-generics-for-inline-classes).

So, to create the `Result` value one will use constructors instead of factory functions. In other words, instead of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a separate design discussion on whether we shall deprecate Result.success and Result.failure in favor or Result.Success and Result.Failure constructors. Please, add it to open issues.

}
```

here, we can use `Number` as the underlying type. However, adding non-`Number` child will change the underlying type,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to replace "we can use Number" with "we could have used Number" to let a reader understand what's the actual design we are going with and what is just a digression on other potential alternatives we have considered in the design.

@udalov udalov mentioned this pull request Aug 8, 2022
```

## Is checks
Consider the following example
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example does not compile because sealed inline class child cannot be distinguished from another one


- If sealed inline class is a child of another sealed inline class, there can be no other inline children, including other sealed inline class, since sealed inline classes are mapped to `Any?`, which is open class, which other classes override and there is no way to distinguish `Any?` from other type.

- Value objects must be a child of sealed inline class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposal doesn't explain what a "value object" is and when it's allowed

```

Note, that `O2` does not override `toString`, so we should call `Any.toString()`
is case of `O2`, since `I2` does not override `toString` as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is case of - typo?

@JvmInline
sealed value class I2 : I1()

sealed object O1: I1() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be value object instead of sealed object here?

@kyay10 kyay10 mentioned this pull request Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants