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

Support definitely non-nullable types #1268

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

Conversation

ZacSweers
Copy link
Collaborator

From https://kotlinlang.org/docs/whatsnew17.html#stable-definitely-non-nullable-types

Open to suggestions on the API for this, went back and forth on an explicit asDefinitelyNonNullable() or copy(isDefinitelyNonNullable: Boolean) overload. Also open to suggestions on whether or not any of the checks are cases we should silently "fix" for them, like setting isDefinitelyNonNullable to false if copying over as nullable.

@ZacSweers ZacSweers requested a review from Egorand June 17, 2022 01:02
Copy link
Collaborator

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

I think modeling this as an intersection type rather than some specialization of a type variable is a better and more future-proof path.

Comment on lines +258 to +259
val typeName = TypeVariableName("T").copy()
assertThat(typeName.toString()).isEqualTo("T & Any")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very surprising behavior

@ZacSweers
Copy link
Collaborator Author

That sounds reasonable to me. API like this maybe?

TypeVariableName(..., intersects = listOf(ANY))

The thing I'm less sure about though is how to indicate that this needs to be emitted.

@JakeWharton
Copy link
Collaborator

I was thinking more like IntersectionTypeName(TypeVariableName("T"), ANY)

@ZacSweers
Copy link
Collaborator Author

Hmmm, is that a breaking change if TypeName is currently sealed to add another?

@JakeWharton
Copy link
Collaborator

It's source incompatible and will cause a runtime exception for when's without an else.

@Egorand
Copy link
Collaborator

Egorand commented Jun 27, 2022

What if we made isDefinitelyNonNullable private and introduced IntersectionTypeName as a function that returns TypeVariableName with isDefinitelyNonNullable set to true? While IntersectionTypeName will support any type as the second argument, we can make it fail at runtime and say that only ANY is currently supported. When intersection types become more common in Kotlin we may be close enough to KotlinPoet 2.0, at which point we can promote IntersectionTypeName to an actual subtype of TypeName.

@ZacSweers
Copy link
Collaborator Author

I think that seems reasonable. In general for 2.0, I think we should take a closer look at how kotlinc models these and consider mirroring it, including making TypeName no longer sealed. There's a few things that don't quite fit the kotlin model that are leftovers from JavaPoet. I'll add them to the 2.0 ideas issue

@Egorand Egorand changed the base branch from master to main July 5, 2023 09:22
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.

3 participants