KT-32551 New J2K: Non-canonical modifiers order inspection is not applied during convertion of inner super class (#2806)

New J2K: add SortModifiers inspection to post-processing

#KT-32551 Fixed

idea/src/org/jetbrains/kotlin/idea/inspections/SortModifiersInspection.kt

@@ -5,75 +5,48 @@
 
 package org.jetbrains.kotlin.idea.inspections
 
-import com.intellij.codeInspection.*
+import com.intellij.codeInspection.CleanupLocalInspectionTool
+import com.intellij.openapi.editor.Editor
 import com.intellij.openapi.project.Project
 import com.intellij.openapi.util.TextRange
-import com.intellij.psi.PsiElementVisitor
 import org.jetbrains.kotlin.lexer.KtModifierKeywordToken
 import org.jetbrains.kotlin.lexer.KtTokens
-import org.jetbrains.kotlin.psi.*
+import org.jetbrains.kotlin.psi.KtAnnotation
+import org.jetbrains.kotlin.psi.KtAnnotationEntry
+import org.jetbrains.kotlin.psi.KtModifierList
+import org.jetbrains.kotlin.psi.KtModifierListOwner
 import org.jetbrains.kotlin.psi.addRemoveModifier.sortModifiers
 import org.jetbrains.kotlin.psi.psiUtil.allChildren
 import org.jetbrains.kotlin.psi.psiUtil.endOffset
 import org.jetbrains.kotlin.psi.psiUtil.startOffset
 
-class SortModifiersInspection : AbstractKotlinInspection(), CleanupLocalInspectionTool {
-
-    override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean, session: LocalInspectionToolSession): PsiElementVisitor {
-        return object : KtVisitorVoid() {
-            override fun visitModifierList(list: KtModifierList) {
-                super.visitModifierList(list)
-
-                val modifierElements = list.allChildren.toList()
-                var modifiersBeforeAnnotations = false
-                var seenModifiers = false
-                for (modifierElement in modifierElements) {
-                    if (modifierElement.node.elementType is KtModifierKeywordToken) {
-                        seenModifiers = true
-                    } else if (seenModifiers && (modifierElement is KtAnnotationEntry || modifierElement is KtAnnotation)) {
-                        modifiersBeforeAnnotations = true
-                    }
-                }
-
-                val modifiers = modifierElements.asSequence().mapNotNull { it.node.elementType as? KtModifierKeywordToken }.toList()
-                if (modifiers.isEmpty()) return
-
-                val startElement = modifierElements.firstOrNull { it.node.elementType is KtModifierKeywordToken } ?: return
-                val endElement = modifierElements.lastOrNull { it.node.elementType is KtModifierKeywordToken } ?: return
-                val rangeInElement = TextRange(startElement.startOffset, endElement.endOffset).shiftLeft(list.startOffset)
-
-                val sortedModifiers = sortModifiers(modifiers)
-                if (modifiers == sortedModifiers && !modifiersBeforeAnnotations) return
-
-                val message = if (modifiersBeforeAnnotations)
-                    "Modifiers should follow annotations"
-                else
-                    "Non-canonical modifiers order"
-
-                val descriptor = holder.manager.createProblemDescriptor(
-                    list,
-                    rangeInElement,
-                    message,
-                    ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
-                    isOnTheFly,
-                    SortModifiersFix(sortedModifiers)
-                )
-                holder.registerProblem(descriptor)
-            }
-        }
+class SortModifiersInspection : AbstractApplicabilityBasedInspection<KtModifierList>(
+    KtModifierList::class.java
+), CleanupLocalInspectionTool {
+    override fun isApplicable(element: KtModifierList): Boolean {
+        val modifiers = element.modifierKeywordTokens()
+        if (modifiers.isEmpty()) return false
+        val sortedModifiers = sortModifiers(modifiers)
+        if (modifiers == sortedModifiers && !element.modifiersBeforeAnnotations()) return false
+        return true
     }
-}
 
-private class SortModifiersFix(private val modifiers: List<KtModifierKeywordToken>) : LocalQuickFix {
-    override fun getName() = "Sort modifiers"
+    override fun inspectionHighlightRangeInElement(element: KtModifierList): TextRange? {
+        val modifierElements = element.allChildren.toList()
+        val startElement = modifierElements.firstOrNull { it.node.elementType is KtModifierKeywordToken } ?: return null
+        val endElement = modifierElements.lastOrNull { it.node.elementType is KtModifierKeywordToken } ?: return null
+        return TextRange(startElement.startOffset, endElement.endOffset).shiftLeft(element.startOffset)
+    }
 
-    override fun getFamilyName() = name
+    override fun inspectionText(element: KtModifierList) =
+        if (element.modifiersBeforeAnnotations()) "Modifiers should follow annotations" else "Non-canonical modifiers order"
 
-    override fun applyFix(project: Project, descriptor: ProblemDescriptor) {
-        val list = descriptor.psiElement as? KtModifierList ?: return
-        val owner = list.parent as? KtModifierListOwner ?: return
+    override val defaultFixText = "Sort modifiers"
 
-        val existingModifiers = modifiers.filter { owner.hasModifier(it) }
+    override fun applyTo(element: KtModifierList, project: Project, editor: Editor?) {
+        val owner = element.parent as? KtModifierListOwner ?: return
+        val sortedModifiers = sortModifiers(element.modifierKeywordTokens())
+        val existingModifiers = sortedModifiers.filter { owner.hasModifier(it) }
         existingModifiers.forEach { owner.removeModifier(it) }
         // We add visibility / modality modifiers after all others,
         // because they can be redundant or not depending on others (e.g. override)
@@ -82,5 +55,22 @@ private class SortModifiersFix(private val modifiers: List<KtModifierKeywordToke
             .let { it.second + it.first }
             .forEach { owner.addModifier(it) }
     }
-}
 
+    private fun KtModifierList.modifierKeywordTokens(): List<KtModifierKeywordToken> {
+        return allChildren.mapNotNull { it.node.elementType as? KtModifierKeywordToken }.toList()
+    }
+
+    private fun KtModifierList.modifiersBeforeAnnotations(): Boolean {
+        val modifierElements = this.allChildren.toList()
+        var modifiersBeforeAnnotations = false
+        var seenModifiers = false
+        for (modifierElement in modifierElements) {
+            if (modifierElement.node.elementType is KtModifierKeywordToken) {
+                seenModifiers = true
+            } else if (seenModifiers && (modifierElement is KtAnnotationEntry || modifierElement is KtAnnotation)) {
+                modifiersBeforeAnnotations = true
+            }
+        }
+        return modifiersBeforeAnnotations
+    }
+}

idea/testData/inspectionsLocal/sortModifiers/annotation.kt

@@ -1,3 +1,4 @@
+// PROBLEM: Modifiers should follow annotations
 annotation class Test
 
 <caret>public @Test /* this is a test */ class MyTest

idea/testData/inspectionsLocal/sortModifiers/annotation.kt.after

@@ -1,3 +1,4 @@
+// PROBLEM: Modifiers should follow annotations
 annotation class Test
 
 @Test

idea/testData/inspectionsLocal/sortModifiers/annotationGroup.kt

@@ -1,3 +1,4 @@
+// PROBLEM: Modifiers should follow annotations
 annotation class Inject
 annotation class VisibleForTesting
 

idea/testData/inspectionsLocal/sortModifiers/annotationGroup.kt.after

@@ -1,3 +1,4 @@
+// PROBLEM: Modifiers should follow annotations
 annotation class Inject
 annotation class VisibleForTesting
 

idea/testData/inspectionsLocal/sortModifiers/simple.kt

@@ -1 +1,3 @@
+// PROBLEM: Non-canonical modifiers order
+// FIX: Sort modifiers
 <caret>abstract public class Test

idea/testData/inspectionsLocal/sortModifiers/simple.kt.after

@@ -1 +1,3 @@
+// PROBLEM: Non-canonical modifiers order
+// FIX: Sort modifiers
 public abstract class Test

nj2k/nj2k-services/src/org/jetbrains/kotlin/nj2k/postProcessing/J2kPostProcessor.kt

@@ -220,7 +220,8 @@ private val inspectionLikePostProcessingGroup =
         MayBeConstantInspectionBasedProcessing(),
         RemoveForExpressionLoopParameterTypeProcessing(),
         intentionBasedProcessing(ReplaceMapGetOrDefaultIntention()),
-        inspectionBasedProcessing(ReplaceGuardClauseWithFunctionCallInspection())
+        inspectionBasedProcessing(ReplaceGuardClauseWithFunctionCallInspection()),
+        inspectionBasedProcessing(SortModifiersInspection())
     )
 
 

nj2k/testData/newJ2k/postProcessing/SortModifiers.java

@@ -0,0 +1,4 @@
+public class TestInnerClasses {
+    public class Base {}
+    public class Derived extends Base {}
+}

nj2k/testData/newJ2k/postProcessing/SortModifiers.kt

@@ -0,0 +1,4 @@
+class TestInnerClasses {
+    open inner class Base
+    inner class Derived : Base()
+}

nj2k/tests/org/jetbrains/kotlin/nj2k/NewJavaToKotlinConverterSingleFileTestGenerated.java

@@ -4226,6 +4226,11 @@ public class NewJavaToKotlinConverterSingleFileTestGenerated extends AbstractNew
             runTest("nj2k/testData/newJ2k/postProcessing/RedunduntTypeCastAndProhibitedInline.java");
         }
 
+        @TestMetadata("SortModifiers.java")
+        public void testSortModifiers() throws Exception {
+            runTest("nj2k/testData/newJ2k/postProcessing/SortModifiers.java");
+        }
+
         @TestMetadata("SyntheticExtensionPropertyAccess.java")
         public void testSyntheticExtensionPropertyAccess() throws Exception {
             runTest("nj2k/testData/newJ2k/postProcessing/SyntheticExtensionPropertyAccess.java");