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

[kotlin] K2 J2K: Move VarToValProcessing LocalVarToValInspectionBasedProcessing and fixValToVarDiagnosticBasedProcessing to JKTree #2726

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jocelynluizzi13
Copy link
Contributor

Removing VarToValProcessing LocalVarToValInspectionBasedProcessing and fixValToVarDiagnosticBasedProcessing from the postprocessor to make it more compatible with K2.

To do this, we will add mutability logic to the JKField class member conversion phase. Now, we will return IMMUTABLE for any private vars that do not have writable logic and are not JPA qualified. Note that StubExpressions are allowed to have 1 write, since this would be the initial assignment of the field.

This is consistently correct with when it has val, however it sometimes has var when it should be val. I have run out of ideas for ways to fix these false positives, and adding back the VarToValPostprocessing doesn't work because that consistently overcorrects (and was relying on the diagnostics to later revert back). For now, putting this up as a draft PR.

@abelkov @darthorimar @ermattt

(This is a simplification of #2702 because I deeply messed up the merge. Please see comments there for additional context)

simplify

quickfixes

add test cases

commit

trycatch fixes

delete var to val
@abelkov abelkov self-assigned this Mar 28, 2024
@abelkov abelkov added the kotlin Pull requests for Kotlin IntelliJ plugin label Mar 28, 2024
@abelkov
Copy link
Member

abelkov commented Mar 28, 2024

Sorry, I will probably have to review next week, as I'm trying to get my nullability prototype done!

@abelkov abelkov assigned darthorimar and unassigned abelkov Apr 10, 2024
@darthorimar
Copy link
Member

Hello, and thank you for the PR :)

It looks like that implementing a custom 'find usages' on top of JKTree may not be the best solution in this case for the following reasons:

  • Performance: This approach may necessitate multiple JKTree traversals, which could significantly impact performance.
  • Complexity: 'Find usages' is a rather complex feature. It takes multiple months to properly implement it for the Kotlin Plugin using the IntelliJ IDEA API, and it also involves handling numerous potential corner cases.

Instead, I suggest that you utilize our existing functionality, specifically the mutability inference in Java. Please consider com.intellij.codeInspection.localCanBeFinal.LocalCanBeFinal.

Some logic from the LocalCanBeFinal inspection can be extracted and reused in JKTree. This would allow us to specify a JKLocalVariable mutability (so, val vs var) during its creation. Consequently, during JKTree conversion, we would already be aware of the mutability of all local variables during all conversion phases.

I'll be happy to discuss details and answer your questions :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kotlin Pull requests for Kotlin IntelliJ plugin
Projects
None yet
3 participants