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

Fix type of nested inner-class field access #532

Closed
wants to merge 3 commits into from
Closed

Conversation

kunli2
Copy link
Contributor

@kunli2 kunli2 commented Dec 12, 2023

fixes #517

@@ -98,6 +102,54 @@ class KotlinTypeMapping(
return type(type, parent, signature)
}

fun type(psi: PsiElement?, type: Any?, parent: Any?): JavaType? {
Copy link
Contributor

Choose a reason for hiding this comment

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

The new type method seems to blend the functionality of the PsiElementAssociations, KotlinSignatureBuilder, and KotlinTypeMapping.

What do you think about detecting the case in the PsiElementAssocation which is meant to associate the PSI to the FIR? And have a new method in the signature builder that is capable of returning the expected signature. Then the TypeMapping may use the corrected signature to return the correct type?

Note:
If I understand the changes correctly, it will fail for val x = foo.bar.A.B.A.C(). Please test that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced this method to be able to break down a leaf FIR node (like A.B.A in this case) into pieces (like breaking down the atomic nucleus into quarks), this requires PSI's help, so that's why it has a psi as a parameter.
I didn't find another way to break down a leaf FIR node like A.B.A to A, B, and A.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean to apply the same concept, but in a different way that encapsulates the functionality in the applicable classes. I'll take a look tomorrow

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a look at the changes, and there are still issues. The changes cause parameterization to be lost since the signature created by the signature build is based on flattening a dot-qualified expression rather than using the FIR type attribution.

I tested the original suggestion about using the ClassId and FirSession to associate the PSI to the FIR and confirmed it's possible. So, we can detect the mismatch in the PsiElementAssociation and find the right FIR element, then call typeMapping#type(...) on the associated FIR.

To recap ClassId and FirSession:
A FirResolvedQualifier will have a symbol if it is resolved to a type.
The symbol will have access to a ClassId, which enables the detection of outer classes.
Then, ClassId can be used to look up the associated FIR element, which we can use for type attribution.

I think we can close this PR since it still isn't quite right and it'll be safer to use the FirSession instead.

Copy link
Contributor

@traceyyoshima traceyyoshima left a comment

Choose a reason for hiding this comment

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

confirmed the solution does not work for: val x = foo.bar.A.B.A.C()

@kunli2
Copy link
Contributor Author

kunli2 commented Dec 13, 2023

confirmed the solution does not work for: val x = foo.bar.A.B.A.C()

fixed by the 2nd commit, and added the test case as well.

@kunli2
Copy link
Contributor Author

kunli2 commented Dec 14, 2023

@kunli2 kunli2 closed this Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

PSI to FIR association issue: Incorrect on parent classes.
3 participants