intellij-community
intellij-community copied to clipboard
JetBrains
Fix NewJavaToKotlinConverter adds against all non-annotated types assuming them as nullable
Resolves: https://youtrack.jetbrains.com/issue/IDEA-324140/NewJavaToKotlinConverter-adds-against-all-non-annotated-types-assuming-them-as-nullable.
Previous discussion for reference: https://github.com/JetBrains/intellij-community/pull/2507 (I should not have closed that PR so soon...)
Thank you, ~I see that all tests pass~. However, I can't accept this change right now because I'm not sure how it interacts with InferenceProcessing and what it might break. I see that it fixes your original testcase from https://youtrack.jetbrains.com/issue/KTIJ-26107, that's good at least.
I'm planning to investigate the whole nullability inference subsystem of J2K in the near future. If you don't mind, I will keep this PR open for now.
Sure, I'll investigate on my side as well. Thank you so much for reviewing!
Oh, sorry, the tests actually fail. I'm not sure why it worked for me before.
The tests are expected to fail after your change, because you're essentially bypassing the whole nullability inference in the post-processing and forcing all types to be not-null. This is counterproductive because one of the major goals of new J2K was to improve nullability inference in presence of other code. If you force all types to be not null, you just remove this feature.
It may work better for your simple example, but imagine that there is a big project with lots of Java surrounding code. Currently we try to be more clever and infer nullability of types based on the actual code in the project.
Yea that makes sense. Failing tests is a good sign. However, Is there a way we can forceNotNullTypes?
We use ErrorProne NullAway so we assume all unannotated types are non-null by default. But J2K converter assumes it as nullable. We could consider:
- Switching all @Nullable annotations on Android to the JSpecify version
- Adding @NullMarked at the package level for com.uber
- Passing the flags above to kotlinc
We tried @ParametersAreNonnullByDefault but it didn't seem to work as expected. Perhaps, if there is an existing solution, that'll work for us.
Yes, it seems that J2K doesn't understand advanced nullability annotations (JSR-305, etc), including annotated packages. There is a related issue: https://youtrack.jetbrains.com/issue/KTIJ-25162/J2K-conversion-does-not-support-TypeQualifierDefault
Have you tried to invoke "Main menu | Code | Analyze code | Infer nullity" on your code? Maybe IDEA itself will understand the nullability correctly, add the Nullable/NotNull annotations, and then you can run J2K. Or maybe there is some other similar tool to add the annotations.
If that doesn't help, you could of course hack it in the J2K, like you did in this PR, and convert the code in the custom Intellij build.
Ideally, we could make J2K understand JSR-305 stuff and maybe add a user-visible option to force non-null types everywhere, but this should be investigated first and will take some time.
I tried "Main menu | Code | Analyze code | Infer nullity" and it worked. Will investigate more. Thanks!
On a similar note: In the issue you can see that the J2K converter adds ? to all generic types. Is there a way to circumvent that?
I can't answer that right now, but I see that we have a bunch of similar issues: https://youtrack.jetbrains.com/issues?q=project:%20ktij%20-Resolved%20Subsystems:%20%7BIDE.%20J2K%7D%20nullable%20generic I guess it is a problem of new J2K inference, hopefully we will improve it in the future.
Ah! Will look into this one. Really appreciate you answering my questions. Thanks and take care!