search

openrewrite / rewrite-kotlin

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

PSI to FIR association issue: incorrect types on selects of dot qualified type references. #524

Open traceyyoshima opened 6 months ago

traceyyoshima commented 6 months ago

PSI to FIR associations are not guaranteed since the FIR elements may have a null or fake source element. Each part of a fully qualified type reference like foo.bar.TypeName will be visited through KtSimpleNameExpression. The PsiElementAssociation class will attempt to associate a FIR element to every part of the FQN. PsiElementAssociation#primary will traverse the PSI parent fields until a match is found.

The issue is variances between how to associate the PSI to the FIR due to differences in name references and aliased types. I've checked the PSI utilities and extension functions and found no utilities to identify when a KtSimpleNameExpression is resolved to a type. There are many parts to generating the FIR (posterity: relevant FIR resolution QualifiedNameResolution and FirCallResolver), and resolving types. So, associating types to the PSI correctly without using the existing compiler code will require much reverse engineering.

To link the PSI to the IR, the Kotlin compiler team recommends using Fir2IrConverter.createIrModuleFragment. The conversion to the IR returns an actualized result that contains caches that link the FIR to the IR, and enable the conversion of constants to IR constant expressions. Unfortunately, the caches do not store locally scoped variables, which must be done manually or retrieved from the cache while the variable still exists. Declarations relevant to the local scope MUST exist in the declaration caches to retrieve the cache or create a local variable. So, parsing the PSI elements in order is required, which has yet to happen.

In this case, the existence of foo.bar.TypeName in each target of a J.FieldAccess should not affect recipes unless an author has updated types without using ChangeType.

Based on my analysis, we have two options if we find a case significantly affecting type attribution.

  1. Revise the parser visitor to parse in order and use existing visitors and utilities to link the PSI to the IR to attribute types.
    • Ideal; the most reliable and correct type of attribution.
    • Relevant sources: psi2ir package in Kotlin compiler and PsiSourceManager.
  2. Do not traverse the PSI parents.
knutwannheden commented 6 months ago

In Java when we have a reference like java.util.Map.Entry then the first two will have an Unknown type attribution, which I think should be our goal for Kotlin too.

knutwannheden commented 6 months ago

@traceyyoshima Is this one of the compiler internals you are referring to which would be useful to have access to? https://github.com/JetBrains/kotlin/blob/master/analysis/low-level-api-fir/src/org/jetbrains/kotlin/analysis/low/level/api/fir/file/structure/FileStructureElement.kt

kunli2 commented 6 months ago

@traceyyoshima , do you have any unit test examples to demonstrate this issue? or do you think https://github.com/openrewrite/rewrite-kotlin/issues/517 is a case of this?

traceyyoshima commented 6 months ago

In Java when we have a reference like java.util.Map.Entry then the first two will have an Unknown type attribution, which I think should be our goal for Kotlin too.

Agreed, the goal is to achieve similar behavior as Java. In Kotlin, the FQN parts are not type attributed either, it's a result of mis-mapping while traversing the PSI parents.

@traceyyoshima Is this one of the compiler internals you are referring to which would be useful to have access to? https://github.com/JetBrains/kotlin/blob/master/analysis/low-level-api-fir/src/org/jetbrains/kotlin/analysis/low/level/api/fir/file/structure/FileStructureElement.kt

I'm unfamiliar with most of the internal APIs, but that may help as well!

Info on IR: This is where the FIR to IR conversion occurs: https://github.com/JetBrains/kotlin/blob/master/compiler/fir/fir2ir/src/org/jetbrains/kotlin/fir/backend/Fir2IrConverter.kt The Fir2IrComponents class contains caches for classes, type parameters, functions, etc. There is a cache for scoped local scopes, but those are not preserved since they only apply to the applicable scope.

The PsiSourceManager finds PSI elements in IR elements.

IntelliJ uses the PSI configuration, so it MUST have some mechanisms to link types correctly, but I haven't checked on exactly how it happens.

@traceyyoshima , do you have any unit test examples to demonstrate this issue? or do you think https://github.com/openrewrite/rewrite-kotlin/issues/517 is a case of this?

No, this issue will require pragmatism and consideration of edge cases. I didn't want anyone to attempt to solve the issue based on a test case or two. So, I opted to not include an example. I could be mistaken, but I think these are two different cases This is confirmed to be a different case, a dot qualified expressions that follow a : referring to a type. AFAIK, there are different sets of conditions compared to 517. 517 applies to expressions, but this issue is type trees like : foo.bar.TypeName.