[kotlin] K2 J2K: Move RemoveRedundantVisibilityModifierProcessing to JKTree

[jocelynluizzi13]

trafficstars

Move RemoveRedundantVisibilityModifierProcessing to the JKTree stage.

To do this, added some small checks to modifiers.kt whenever possible. For constructors, made changes to the printing phase to avoid printing the constructor itself when it's unnecessary. For overrides, used symbol checking in a separate conversion file to find the visibility of the overridden method.

This still has two failures—there's a nullability problem with enum constructors, and it fails when there is nested inheritance and we're overriding a method not present in the in-between class.

Before I invest more time in fixing these errors, I wanted to post this to get feedback on the implementation

@abelkov @darthorimar @ermattt

[abelkov]

Not sure what is the problem with nullability, but you can try to fix it like this. Since an enum constructor is private, search for all enum constants, look at the mapping from arguments to parameters. If a parameter has unknown nullability, but all actual arguments are not-null (like String constants are), update the nullability of the parameter to be not-null as well.

This logic can actually be extended to all private methods and added to org.jetbrains.kotlin.nj2k.conversions.NullabilityConversion.

[jocelynluizzi13]

NullabilityConversion

I don't think that will work in this case, because the enum constructor is Internal not private, and the parameter is never used in the class. Specifically, plugins/kotlin/j2k/shared/tests/testData/newJ2k/enum/oneLinePerEntry.kt is failing because both name and s are appearing as nullable strings. I can't figure out what happened to change it to nullable in this case.

Otherwise, the rest of the suggested changes have been made and IGNORE_K2 flags have been removed

[abelkov]

the enum constructor is Internal not private

You're thinking about the enum itself. But I mean the enum constructor, which must be private.

In such code, the constructor is private, and we can easily analyze all its three invocations:

internal enum class BigEnum(name: String?) {
    ENUM_ONE("SOME_NAME_TO_PUT_HERE"),
    ENUM_TWO("SOME_NAME_TO_PUT_HERE"),
    ENUM_THREE("SOME_NAME_TO_PUT_HERE"),
}

[abelkov]

Also, there are still failing tests, please take a look.

org.jetbrains.kotlin.nj2k.TextNewJavaToKotlinCopyPasteConversionTestGenerated#testComments org.jetbrains.kotlin.nj2k.NewJavaToKotlinCopyPasteConversionTestGenerated#testComments

[jocelynluizzi13]

This still doesn't work for copypaste conversions (hence leaving the k1 postprocessing in place) but it does fix many k2 tests so putting up the partial PR for merging now

[abelkov]

I also recommend manually testing this change on some real files, because a bug here will lead to red code.

[jocelynluizzi13]

Made some changes based on comments. the org.jetbrains.kotlin.nj2k.PsiUtilsKt#visibility function needed to be updated so that it read text strings if there were no children of modifierList. In Kotlin-specific cases, this is the only way to access the individual modifiers.

Also tested on some live files and ran through the test cases.

[abelkov]

Due to my recent optimization, the compiler errors are checked only in testdata that already has "// ERROR" directives. I now realize that this is a mistake, because we can accidentally introduce some new errors in new or modified testdata. I will revert this soon. After you run the test suite, you may find some new errors in testdata.

[jocelynluizzi13]

Made the requested changes and added back the ERROR flag