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

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

Open
traceyyoshima opened this issue Dec 11, 2023 · 4 comments
Labels
bug Something isn't working parser-kotlin

Comments

@traceyyoshima
Copy link
Contributor

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.
@traceyyoshima traceyyoshima added bug Something isn't working parser-kotlin labels Dec 11, 2023
@knutwannheden
Copy link
Contributor

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
Copy link
Contributor

@kunli2
Copy link
Contributor

kunli2 commented Dec 12, 2023

@traceyyoshima , do you have any unit test examples to demonstrate this issue?
or do you think #517 is a case of this?

@traceyyoshima
Copy link
Contributor Author

traceyyoshima commented Dec 13, 2023

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 #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.

@timtebeek timtebeek moved this to Backlog in OpenRewrite May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser-kotlin
Projects
Status: Backlog
Development

No branches or pull requests

3 participants