search

openrewrite / rewrite-kotlin

Work-in-progress implementation of Kotlin language support for OpenRewrite.
Apache License 2.0
47 stars 13 forks source link

Map types in `PsiElementAssociations` #487

Closed knutwannheden closed 11 months ago

knutwannheden commented 11 months ago

The types themselves are not FIR elements and thus not visited by the visitor used to populate the associations map. However, these can be visited separately, so that they later can be retrieved using type().

fixes #473

knutwannheden commented 11 months ago

@traceyyoshima This is basically what I meant in my comment. Only here the element associations would "proactively" add an association for the type, rather than at lookup time try to navigate the trees to find it again.

I've created this as a draft, because I am a bit unsure if this is generic enough with the two parent calls in the type() method and if we cover all cases like this. Also, we should adjust / add tests for this. But with the test from your PR, the output is now what you were expecting to see. I.e. the type parameters don't have the owning type as their type anymore.

I assigned this PR to you, as I think you are in a better position to take this over the finishing line.

traceyyoshima commented 11 months ago

@traceyyoshima This is basically what I meant in https://github.com/openrewrite/rewrite-kotlin/pull/471#issuecomment-1829138412. Only here the element associations would "proactively" add an association for the type, rather than at lookup time try to navigate the trees to find it again.

I've created this as a draft, because I am a bit unsure if this is generic enough with the two parent calls in the type() method and if we cover all cases like this. Also, we should adjust / add tests for this. But with the test from your PR, the output is now what you were expecting to see. I.e. the type parameters don't have the owning type as their type anymore.

I assigned this PR to you, as I think you are in a better position to take this over the finishing line.

Ah, interesting, this might work. There are at least a few changes that'll be necessary, to catch nullability and maybe ConeStarProjections. I'll check if there are any cases that the approach might miss after making a few adjustments.