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

Variance for function output types is ignored #1933

Open
DanySK opened this issue Jun 23, 2024 · 7 comments
Open

Variance for function output types is ignored #1933

DanySK opened this issue Jun 23, 2024 · 7 comments

Comments

@DanySK
Copy link

DanySK commented Jun 23, 2024

Describe the bug

Generating this function is impossible:

fun <T> foo(): Array<out T> = TODO()

Note that this prevents an entire family of functions from being generated

To Reproduce

Run this:

fun main(args: Array<String>) {
    val funspec = FunSpec.builder("foo")
        .addTypeVariable(TypeVariableName("T"))
        .returns(
            ClassName.bestGuess("kotlin.Array")
                .parameterizedBy(TypeVariableName("T", variance = KModifier.OUT))
        )
        .addStatement("TODO()")
        .build()
    println(
        FileSpec.builder("foo.bar", "Baz")
        .addFunction(funspec)
        .build()
        .toString()
    )
}

Expected:

package foo.bar

import kotlin.Array

public fun <T> foo(): Array<out T> {
  TODO()
}

actual:

package foo.bar

import kotlin.Array

public fun <T> foo(): Array<T> {
  TODO()
}

Expected behavior
See above

Additional context
Stumbled on this while building a code generator that wraps fun Array<out T>?.orEmpty(): Array<out T>.
I can only generate: fun <reified T, ID : Any> Field<ID, Array<T>?>.orEmpty(): Array<T>, but I need fun <reified T, ID : Any> Field<ID, Array<T>?>.orEmpty(): Array<out T> for the compilation to succeed.

Alternatively (but it is a workaround) it should be allowed to omit the return type and let the compiler infer (in my case, fun <reified T, ID : Any> Field<ID, Array<T>?>.orEmpty() = ... would work

@DanySK DanySK added the bug label Jun 23, 2024
@DanySK DanySK changed the title Variance for output function types is ignored Variance for function output types is ignored Jun 23, 2024
@Egorand
Copy link
Collaborator

Egorand commented Jun 25, 2024

Here's what you can do:

fun main(args: Array<String>) {
    val funspec = FunSpec.builder("foo")
        .addTypeVariable(TypeVariableName("T"))
        .returns(ARRAY.plusParameter(WildcardTypeName.producerOf(TypeVariableName("T"))))
        .addStatement("TODO()")
        .build()
    println(
        FileSpec.builder("foo.bar", "Baz")
        .addFunction(funspec)
        .build()
        .toString()
    )
}

which also feels slightly more semantically correct, but I guess we do need to be able to correctly print out types parameterized by type variables, since TypeVariableName is TypeName?

@DanySK
Copy link
Author

DanySK commented Jun 25, 2024

Yes, I tried to exemplify a simple case, but what I am doing in practice is detecting the signature of an existing function and creating a new one with the same parameterized return type. So I cannot statically refer to ARRAY or any other type specifically, it can be an arbitrary type that is discovered at runtime (when running the code generation).
I'd like to be able to parameterize these return types with co(ntra)variant type variables and have their variance reflected in the function signature.

@Egorand
Copy link
Collaborator

Egorand commented Jun 25, 2024

Yes, I tried to exemplify a simple case, but what I am doing in practice is detecting the signature of an existing function and creating a new one with the same parameterized return type. So I cannot statically refer to ARRAY or any other type specifically, it can be an arbitrary type that is discovered at runtime (when running the code generation). I'd like to be able to parameterize these return types with co(ntra)variant type variables and have their variance reflected in the function signature.

In that case you can use ClassName.bestGuess() as showcased by your example and parameterize it with WildcardTypeName.producerOf(TypeVariableName("T")).

@DanySK
Copy link
Author

DanySK commented Jun 26, 2024

The workaround works. I can close this issue, but I think that at least this behavior should be documented in parameterizedBy, suggesting to call WildcardTypeName.(producer|consumer)Of(TypeVariableName) for explicitly co(ntra)variant types.

DanySK added a commit to Collektive/collektive that referenced this issue Jun 26, 2024
@Egorand
Copy link
Collaborator

Egorand commented Jun 26, 2024

Agree with keeping it open, I don't see any reason your original code shouldn't work, so that's still a bug.

@JakeWharton
Copy link
Collaborator

I think I'd like to argue this is working as intended. And possibly, instead, that we have an API design problem here. The variance on a type parameter should only exist when we declare a type parameter, not when we use it. When we use variance within a type (regardless of whether it's a type parameter or not) there's a consistent way to do that. And that use of variance in a type is separate from the declaration of the type parameter.

@DanySK
Copy link
Author

DanySK commented Jun 28, 2024

@JakeWharton do I understand correctly that you want to differentiate between the type variable declaration-site variance and use-site variance (projection)?

It makes sense in principle, but in Kotlin variance annotations are only allowed for type parameters of classes and interfaces. Use-site variance is useful when you have an invariant type that needs projection:

data class Invariant<T>(var x: T)

fun <T> covariant(x: T): Invariant<out T> = Invariant(x)
fun <T> contravariant(x: T): Invariant<in T> = Invariant(x)

fun main() {
    val covariant = covariant(1)
    val contravariant = contravariant(1)
    val x: Int = covariant.x // OK
    contravariant.x = 2 // OK
    covariant.x = 1 // Assignment type mismatch: actual type is 'kotlin.Int', but 'kotlin.Nothing' was expected.
    val y: Int = contravariant.x // Initializer type mismatch: expected 'kotlin.Int', actual 'CapturedType(in kotlin.Int)'.
    println(covariant(1))
}

Note that you can always re-declare variance at use site (it's a compiler warning).

DanySK added a commit to Collektive/collektive that referenced this issue Jul 2, 2024
…library types (#273)

* build: add code-generator module

* feat: support runtime alignement check for an arbitrary field count

* feat: add shortcut for getting to the set of neighbors from a field

* porcaio

* stash this commit

* feat: add combinator

* some minor

* some minor

* chore: updates

* feat: add primitives code generator

* feat: support the generation for property members

* feat: generate a file for each specified type

* feat: provide a dedicate function for code generation

* test: provide minimal test for validating the generated code

* refactor: use KClass instead hard-coded string for collektive classes

* refactor: move Utils into codgen folder

* docs: improve kdoc of Field

* chore(deps): move dependency into catalog

* build: sort dependencies

* fix: do not generate field-based method for internal member

* fix: include also the receiver in the checkAligned method

* refactor: move into buildSrc the code generation

* feat: implement field primitives code generator inside buildSrc

* feat: introduce stdlib project

* build: remove hard-coded Field class and use source set pointing to dsl project

* docs: fix documentation comment

* build: enable compiler plugin

* build: enable the foojay resolver in buildSrc

* build: enable the multi jvm test in buildSrc

* feat(stdlib): add a header to the generated sources

* style(codegen): improve readability

* refactor(codegen): generate code with Java 8 and improve the namespacing

* fixup: work around the limitations with recurring generic types

* feat: simplify the type resolution

* fix: exclude `associate`(`By`)`To`

* chore: ignore .kotlin

* fix: exclude `associateWithTo`

* fix: exclude `joinTo`

* fix: exclude `filterIndexedTo`

* fix: exclude `filterNotTo`

* fix: exclude `filterTo`

* fix: exclude `binarySearch`

* fix: exclude `map` and `mapTo`

* fix: exclude `mapNotNullTo`

* fix: exclude `mapIndexedTo`

* fix: exclude `groupByTo`

* fix: exclude `flatMapTo`

* fix: exclude `mapIndexedNotNullTo`

* fix(codegen): add star projection support

* fix(codegen): retain `inline` and `infix`

* feat(codegen): add support for `reified` type parameters in method signatures

* fix(codegen): add support for `crossinline` function types

* fix: solved error No type arguments expected for class *Array

* feat(stdlib): split the generated code into multiple files based on the receiver type

* feat(stdlib): generate type arguments for 0-ary functions

* feat(stdlib): improve wildcard detection and rename

* feat(stdlib): skip filterIsInstanceTo

* feat(stdlib): skip filterNotNullTo

* feat(stdlib): detect nullables in jvmnames

* feat(stdlib): exclude generation for functions affected by square/kotlinpoet#1933

* feat(stdlib): make the generated code compile

* fix(codegen): work around square/kotlinpoet#1933

* feat(codegen)!: avoid type variable names as package segments

* refactor: code refactoring in appropriate utils objects

---------

Co-authored-by: Danilo Pianini <[email protected]>
Co-authored-by: Danilo Pianini <[email protected]>
Co-authored-by: Angela Cortecchia <[email protected]>
nicolasfara pushed a commit to Collektive/collektive that referenced this issue Jul 2, 2024
## [10.0.0](9.2.4...10.0.0) (2024-07-02)

### ⚠ BREAKING CHANGES

* generate library code for field operations on kotlin standard library types (#273)

### Features

* generate library code for field operations on kotlin standard library types ([#273](#273)) ([31545f2](31545f2)), closes [square/kotlinpoet#1933](square/kotlinpoet#1933) [square/kotlinpoet#1933](square/kotlinpoet#1933)

### Dependency updates

* **deps:** update alchemist to v34.0.10 ([b666dc1](b666dc1))
* **deps:** update alchemist to v34.0.11 ([174eace](174eace))
* **deps:** update alchemist to v34.0.13 ([6890653](6890653))
* **deps:** update alchemist to v34.0.8 ([ad9b277](ad9b277))
* **deps:** update alchemist to v34.0.9 ([871197f](871197f))
* **deps:** update dependency com.squareup:kotlinpoet to v1.17.0 ([8b27c9f](8b27c9f))
* **deps:** update dependency gradle to v8.8 ([d29b256](d29b256))
* **deps:** update dependency io.kotest.multiplatform to v5.9.1 ([930081c](930081c))
* **deps:** update dependency it.unibo.alchemist:alchemist-api to v34.0.7 ([f183563](f183563))
* **deps:** update node.js to 20.14 ([ecba005](ecba005))
* **deps:** update node.js to 20.15 ([dc8d650](dc8d650))
* **deps:** update plugin com.gradle.develocity to v3.17.5 ([ffe79c7](ffe79c7))
* **deps:** update plugin gitsemver to v3.1.7 ([bad3765](bad3765))
* **deps:** update plugin kotlin-qa to v0.62.1 ([f43dc25](f43dc25))
* **deps:** update plugin kotlin-qa to v0.62.2 ([870a894](870a894))
* **deps:** update plugin kotlin-qa to v0.62.3 ([1eceb1f](1eceb1f))
* **deps:** update plugin org.danilopianini.gradle-pre-commit-git-hooks to v2.0.7 ([adb4c57](adb4c57))
* **deps:** update plugin publishoncentral to v5.1.2 ([cccde5b](cccde5b))
* **deps:** update plugin publishoncentral to v5.1.3 ([9f3a73a](9f3a73a))
* **deps:** update plugin tasktree to v4 ([1ac746d](1ac746d))

### Build and continuous integration

* **deps:** update actions/checkout action to v4.1.7 ([7068f95](7068f95))
* **deps:** update danysk/build-check-deploy-gradle-action action to v2.4.21 ([4ad0f89](4ad0f89))
* **deps:** update danysk/build-check-deploy-gradle-action action to v2.4.22 ([a3a46f3](a3a46f3))
* **deps:** update danysk/build-check-deploy-gradle-action action to v2.4.23 ([9aa3c31](9aa3c31))
* **deps:** update danysk/build-check-deploy-gradle-action action to v2.4.24 ([8489a81](8489a81))
* **deps:** update danysk/build-check-deploy-gradle-action action to v3 ([a3fd0a7](a3fd0a7))
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

3 participants