Fixes after review in creating single action for fixing several problems

idea/idea-analysis/src/org/jetbrains/kotlin/idea/highlighter/JetPsiChecker.kt

@@ -32,6 +32,7 @@ import com.intellij.openapi.util.TextRange
 import com.intellij.psi.MultiRangeReference
 import com.intellij.psi.PsiElement
 import com.intellij.psi.PsiFile
+import com.intellij.util.containers.MultiMap
 import com.intellij.xml.util.XmlStringUtil
 import org.jetbrains.kotlin.descriptors.DeclarationDescriptor
 import org.jetbrains.kotlin.diagnostics.Diagnostic
@@ -40,7 +41,6 @@ import org.jetbrains.kotlin.diagnostics.Severity
 import org.jetbrains.kotlin.diagnostics.rendering.DefaultErrorMessages
 import org.jetbrains.kotlin.idea.actions.internal.KotlinInternalMode
 import org.jetbrains.kotlin.idea.caches.resolve.analyzeFullyAndGetResult
-import org.jetbrains.kotlin.idea.quickfix.JetIntentionActionsFactory
 import org.jetbrains.kotlin.idea.quickfix.QuickFixes
 import org.jetbrains.kotlin.idea.references.mainReference
 import org.jetbrains.kotlin.idea.util.ProjectRootsUtil
@@ -50,6 +50,7 @@ import org.jetbrains.kotlin.psi.KtParameter
 import org.jetbrains.kotlin.psi.KtReferenceExpression
 import org.jetbrains.kotlin.resolve.BindingContext
 import org.jetbrains.kotlin.resolve.diagnostics.Diagnostics
+import org.jetbrains.kotlin.utils.singletonOrEmptyList
 import java.lang.reflect.*
 import java.util.*
 
@@ -80,184 +81,11 @@ public open class JetPsiChecker : Annotator, HighlightRangeExtension {
 
     fun annotateElement(element: PsiElement, holder: AnnotationHolder, diagnostics: Diagnostics) {
         if (ProjectRootsUtil.isInProjectSource(element) || element.getContainingFile() is KtCodeFragment) {
-            ElementAnnotator(element, holder).registerDiagnosticsAnnotations(diagnostics.forElement(element))
+            ElementAnnotator(element, holder, { param -> shouldSuppressUnusedParameter(param) }).registerDiagnosticsAnnotations(diagnostics.forElement(element))
-        }
-    }
-
-    private inner class ElementAnnotator(private val element: PsiElement, private val holder: AnnotationHolder) {
-        fun registerDiagnosticsAnnotations(diagnostics: Collection<Diagnostic>) {
-            diagnostics.groupBy { diagnostic -> diagnostic.factory }.forEach { group -> registerDiagnosticAnnotations(group.getValue()) }
-        }
-
-        private fun registerDiagnosticAnnotations(diagnostics: List<Diagnostic>) {
-            assert(diagnostics.isNotEmpty())
-
-            val validDiagnostics = diagnostics.filter { it.isValid }
-            if (validDiagnostics.isEmpty()) return
-
-            val diagnostic = diagnostics.first()
-            val factory = diagnostic.getFactory()
-
-            assert(diagnostics.all { it.getPsiElement() == element && it.factory == factory })
-
-            val presentationInfo: AnnotationPresentationInfo = when (factory.severity) {
-                Severity.ERROR -> {
-                    when (factory) {
-                        in Errors.UNRESOLVED_REFERENCE_DIAGNOSTICS -> {
-                            val referenceExpression = element as KtReferenceExpression
-                            val reference = referenceExpression.mainReference
-                            if (reference is MultiRangeReference) {
-                                AnnotationPresentationInfo(diagnostic,
-                                        ranges = reference.getRanges().map { it.shiftRight(referenceExpression.getTextOffset()) },
-                                        highlightType = ProblemHighlightType.LIKE_UNKNOWN_SYMBOL)
-                            }
-                            else {
-                                AnnotationPresentationInfo(diagnostic, highlightType = ProblemHighlightType.LIKE_UNKNOWN_SYMBOL)
-                            }
-                        }
-
-                        Errors.ILLEGAL_ESCAPE -> AnnotationPresentationInfo(diagnostic, textAttributes = JetHighlightingColors.INVALID_STRING_ESCAPE)
-
-                        Errors.REDECLARATION -> AnnotationPresentationInfo(
-                                diagnostic, ranges = listOf(diagnostic.getTextRanges().first()), defaultMessage = "")
-
-                        else -> {
-                            AnnotationPresentationInfo(diagnostic,
-                                    highlightType = if (factory == Errors.INVISIBLE_REFERENCE)
-                                        ProblemHighlightType.LIKE_UNKNOWN_SYMBOL
-                                    else
-                                        null)
-                        }
-                    }
-                }
-                Severity.WARNING -> {
-                    if (factory == Errors.UNUSED_PARAMETER && shouldSuppressUnusedParameter(element as KtParameter)) {
-                        return
-                    }
-
-                    AnnotationPresentationInfo(diagnostic,
-                            textAttributes = if (factory == Errors.DEPRECATION) CodeInsightColors.DEPRECATED_ATTRIBUTES else null,
-                            highlightType = if (factory in Errors.UNUSED_ELEMENT_DIAGNOSTICS)
-                                ProblemHighlightType.LIKE_UNUSED_SYMBOL
-                            else
-                                null
-                    )
-                }
-                Severity.INFO -> return // Do nothing
-            }
-
-            setUpAnnotations(diagnostics, presentationInfo)
-        }
-
-        private fun setUpAnnotations(diagnostics: List<Diagnostic>, data: AnnotationPresentationInfo) {
-            for (range in data.ranges) {
-                registerQuickFixes(diagnostics, range, data)
-            }
-        }
-
-        fun registerQuickFixes(diagnostics: List<Diagnostic>, range: TextRange, data: AnnotationPresentationInfo) {
-            val (multiFixes, processedFactories) = createQuickFixesForSameTypeDiagnostics(diagnostics)
-
-            val annotations = diagnostics.map { diagnostic ->
-                val annotation = data.create(range, holder)
-
-                createQuickfixes(diagnostic, processedFactories).forEach { annotation.registerFix(it) }
-
-                // Making warnings suppressable
-                if (diagnostic.getSeverity() == Severity.WARNING) {
-                    annotation.setProblemGroup(KotlinSuppressableWarningProblemGroup(diagnostic.getFactory()))
-
-                    val fixes = annotation.getQuickFixes()
-                    if (fixes == null || (fixes.isEmpty() && multiFixes.isEmpty())) {
-                        // if there are no quick fixes we need to register an EmptyIntentionAction to enable 'suppress' actions
-                        annotation.registerFix(EmptyIntentionAction(diagnostic.getFactory().getName()))
-                    }
-                }
-
-                annotation
-            }
-
-            // Always register all group fixes on the same annotation
-            val firstAnnotation = annotations.minBy { it.message }!!
-            multiFixes.forEach { fix -> firstAnnotation.registerFix(fix) }
-        }
-
-        private fun createQuickFixesForSameTypeDiagnostics(diagnostics: List<Diagnostic>):
-                Pair<Collection<IntentionAction>, Collection<JetIntentionActionsFactory>> {
-            val factory = diagnostics.first().factory
-            val sameProblemsFixesFactories = QuickFixes.getInstance().getActionFactories(factory).filter { it.canFixSeveralSameProblems() }
-
-            val processedFactories = hashSetOf<JetIntentionActionsFactory>()
-            val fixActions = arrayListOf<IntentionAction>()
-
-            for (actionFactory in sameProblemsFixesFactories) {
-                val actions = actionFactory.createActions(diagnostics)
-                if (actions.isNotEmpty()) {
-                    processedFactories.add(actionFactory)
-                    fixActions.addAll(actions)
-                }
-            }
-
-            return Pair(fixActions, processedFactories)
         }
     }
 
     companion object {
-        private fun getMessage(diagnostic: Diagnostic): String {
-            var message = IdeErrorMessages.render(diagnostic)
-            if (KotlinInternalMode.enabled || ApplicationManager.getApplication().isUnitTestMode()) {
-                val factoryName = diagnostic.getFactory().getName()
-                if (message.startsWith("<html>")) {
-                    message = "<html>[$factoryName] ${message.substring("<html>".length())}"
-                }
-                else {
-                    message = "[$factoryName] $message"
-                }
-            }
-            if (!message.startsWith("<html>")) {
-                message = "<html><body>${XmlStringUtil.escapeString(message)}</body></html>"
-            }
-            return message
-        }
-
-        private fun getDefaultMessage(diagnostic: Diagnostic): String {
-            val message = DefaultErrorMessages.render(diagnostic)
-            if (KotlinInternalMode.enabled || ApplicationManager.getApplication().isUnitTestMode()) {
-                return "[${diagnostic.getFactory().getName()}] $message"
-            }
-            return message
-        }
-
-        class AnnotationPresentationInfo(
-                diagnostic: Diagnostic,
-                val severity: Severity = diagnostic.severity,
-                val ranges: List<TextRange> = diagnostic.textRanges,
-                val defaultMessage: String = getDefaultMessage(diagnostic),
-                val tooltip: String = getMessage(diagnostic),
-                val highlightType: ProblemHighlightType? = null,
-                val textAttributes: TextAttributesKey? = null) {
-
-            public fun create(range: TextRange, holder: AnnotationHolder): Annotation {
-                val annotation = when (severity) {
-                    Severity.ERROR -> holder.createErrorAnnotation(range, defaultMessage)
-                    Severity.WARNING -> holder.createWarningAnnotation(range, defaultMessage)
-                    else -> throw IllegalArgumentException("Only ERROR and WARNING diagnostics are supported")
-                }
-
-                annotation.tooltip = tooltip
-
-                if (highlightType != null) {
-                    annotation.highlightType = highlightType
-                }
-
-                if (textAttributes != null) {
-                    annotation.textAttributes = textAttributes
-                }
-
-                return annotation
-            }
-        }
-
         private fun getAfterAnalysisVisitor(holder: AnnotationHolder, bindingContext: BindingContext) = arrayOf(
                 PropertiesHighlightingVisitor(holder, bindingContext),
                 FunctionsHighlightingVisitor(holder, bindingContext),
@@ -265,53 +93,219 @@ public open class JetPsiChecker : Annotator, HighlightRangeExtension {
                 TypeKindHighlightingVisitor(holder, bindingContext)
         )
 
-        public fun createQuickfixes(
+        public fun createQuickFixes(diagnostic: Diagnostic): Collection<IntentionAction> =
-                diagnostic: Diagnostic,
+                createQuickFixes(diagnostic.singletonOrEmptyList())[diagnostic]
-                excludedFactories : Collection<JetIntentionActionsFactory> = emptySet<JetIntentionActionsFactory>()): Collection<IntentionAction> {
+    }
-            val result = arrayListOf<IntentionAction>()
+}
-            val intentionActionsFactories = QuickFixes.getInstance().getActionFactories(diagnostic.getFactory()) - excludedFactories
+
-            for (intentionActionsFactory in intentionActionsFactories.filterNotNull()) {
+private fun createQuickFixes(similarDiagnostics: Collection<Diagnostic>): MultiMap<Diagnostic, IntentionAction> {
-                result.addAll(intentionActionsFactory.createActions(diagnostic))
+    val first = similarDiagnostics.minBy { it.toString() }
+    val factory = similarDiagnostics.first().factory
+
+    val actions = MultiMap<Diagnostic, IntentionAction>()
+
+    val intentionActionsFactories = QuickFixes.getInstance().getActionFactories(factory)
+    for (intentionActionsFactory in intentionActionsFactories.filterNotNull()) {
+        val allProblemsActions = intentionActionsFactory.createActionsForAllProblems(similarDiagnostics)
+        if (!allProblemsActions.isEmpty()) {
+            actions.putValues(first, allProblemsActions)
+        }
+        else {
+            for (diagnostic in similarDiagnostics) {
+                actions.putValues(diagnostic, intentionActionsFactory.createActions(diagnostic))
             }
-            result.addAll(QuickFixes.getInstance().getActions(diagnostic.factory))
+        }
+    }
 
-            result.forEach { check(it.javaClass) }
+    for (diagnostic in similarDiagnostics) {
+        actions.putValues(diagnostic, QuickFixes.getInstance().getActions(diagnostic.getFactory()))
+    }
 
-            return result
+    actions.values().forEach { NoDeclarationDescriptorsChecker.check(it.javaClass) }
+
+    return actions
+}
+
+private object NoDeclarationDescriptorsChecker {
+    private val LOG = Logger.getInstance(NoDeclarationDescriptorsChecker::class.java)
+
+    private val checkedQuickFixClasses = Collections.synchronizedSet(HashSet<Class<*>>())
+
+    fun check(quickFixClass: Class<*>) {
+        if (!checkedQuickFixClasses.add(quickFixClass)) return
+
+        for (field in quickFixClass.declaredFields) {
+            checkType(field.genericType, field)
         }
 
-        private val LOG = Logger.getInstance(JetPsiChecker::class.java)
+        @Suppress("UNNECESSARY_SAFE_CALL") // Wrong UNNECESSARY_SAFE_CALL
+        quickFixClass.superclass?.let { check(it) }
+    }
 
-        private val checkedQuickFixClasses = Collections.synchronizedSet(HashSet<Class<*>>())
+    private fun checkType(type: Type, field: Field) {
-
+        when (type) {
-        private fun check(quickFixClass: Class<*>) {
+            is Class<*> -> {
-            if (!checkedQuickFixClasses.add(quickFixClass)) return
+                if (DeclarationDescriptor::class.java.isAssignableFrom(type)) {
-
+                    LOG.error("QuickFix class ${field.declaringClass.name} contains field ${field.name} that holds DeclarationDescriptor")
-            for (field in quickFixClass.declaredFields) {
+                }
-                checkType(field.genericType, field)
             }
 
-            quickFixClass.superclass?.let { check(it) }
+            is GenericArrayType -> checkType(type.genericComponentType, field)
+
+            is ParameterizedType -> {
+                if (Collection::class.java.isAssignableFrom(type.rawType as Class<*>)) {
+                    type.actualTypeArguments.forEach { checkType(it, field) }
+                }
+            }
+
+            is WildcardType -> type.upperBounds.forEach { checkType(it, field) }
+        }
+    }
+}
+
+private class ElementAnnotator(private val element: PsiElement,
+                               private val holder: AnnotationHolder,
+                               private val shouldSuppressUnusedParameter: (KtParameter) -> Boolean) {
+    fun registerDiagnosticsAnnotations(diagnostics: Collection<Diagnostic>) {
+        diagnostics.groupBy { it.factory }.forEach { group -> registerDiagnosticAnnotations(group.value) }
+    }
+
+    private fun registerDiagnosticAnnotations(diagnostics: List<Diagnostic>) {
+        assert(diagnostics.isNotEmpty())
+
+        val validDiagnostics = diagnostics.filter { it.isValid }
+        if (validDiagnostics.isEmpty()) return
+
+        val diagnostic = diagnostics.first()
+        val factory = diagnostic.getFactory()
+
+        assert(diagnostics.all { it.getPsiElement() == element && it.factory == factory })
+
+        val ranges = diagnostic.textRanges
+
+        val presentationInfo: AnnotationPresentationInfo = when (factory.severity) {
+            Severity.ERROR -> {
+                when (factory) {
+                    in Errors.UNRESOLVED_REFERENCE_DIAGNOSTICS -> {
+                        val referenceExpression = element as KtReferenceExpression
+                        val reference = referenceExpression.mainReference
+                        if (reference is MultiRangeReference) {
+                            AnnotationPresentationInfo(
+                                    ranges = reference.getRanges().map { it.shiftRight(referenceExpression.getTextOffset()) },
+                                    highlightType = ProblemHighlightType.LIKE_UNKNOWN_SYMBOL)
+                        }
+                        else {
+                            AnnotationPresentationInfo(ranges, highlightType = ProblemHighlightType.LIKE_UNKNOWN_SYMBOL)
+                        }
+                    }
+
+                    Errors.ILLEGAL_ESCAPE -> AnnotationPresentationInfo(ranges, textAttributes = JetHighlightingColors.INVALID_STRING_ESCAPE)
+
+                    Errors.REDECLARATION -> AnnotationPresentationInfo(
+                            ranges = listOf(diagnostic.getTextRanges().first()), nonDefaultMessage = "")
+
+                    else -> {
+                        AnnotationPresentationInfo(
+                                ranges,
+                                highlightType = if (factory == Errors.INVISIBLE_REFERENCE)
+                                    ProblemHighlightType.LIKE_UNKNOWN_SYMBOL
+                                else
+                                    null)
+                    }
+                }
+            }
+            Severity.WARNING -> {
+                if (factory == Errors.UNUSED_PARAMETER && shouldSuppressUnusedParameter(element as KtParameter)) {
+                    return
+                }
+
+                AnnotationPresentationInfo(
+                        ranges,
+                        textAttributes = if (factory == Errors.DEPRECATION) CodeInsightColors.DEPRECATED_ATTRIBUTES else null,
+                        highlightType = if (factory in Errors.UNUSED_ELEMENT_DIAGNOSTICS)
+                            ProblemHighlightType.LIKE_UNUSED_SYMBOL
+                        else
+                            null
+                )
+            }
+            Severity.INFO -> return // Do nothing
         }
 
-        private fun checkType(type: Type, field: Field) {
+        setUpAnnotations(diagnostics, presentationInfo)
-            when (type) {
+    }
-                is Class<*> -> {
+
-                    if (DeclarationDescriptor::class.java.isAssignableFrom(type)) {
+    private fun setUpAnnotations(diagnostics: List<Diagnostic>, data: AnnotationPresentationInfo) {
-                        LOG.error("QuickFix class ${field.declaringClass.name} contains field ${field.name} that holds DeclarationDescriptor")
+        val fixesMap = createQuickFixes(diagnostics)
+        for (range in data.ranges) {
+            for (diagnostic in diagnostics) {
+                val annotation = data.create(diagnostic, range, holder)
+                val fixes = fixesMap[diagnostic]
+
+                fixes.forEach { annotation.registerFix(it) }
+
+                if (diagnostic.getSeverity() == Severity.WARNING) {
+                    annotation.setProblemGroup(KotlinSuppressableWarningProblemGroup(diagnostic.getFactory()))
+
+                    if (fixes.isEmpty()) {
+                        // if there are no quick fixes we need to register an EmptyIntentionAction to enable 'suppress' actions
+                        annotation.registerFix(EmptyIntentionAction(diagnostic.getFactory().getName()))
                     }
                 }
-
-                is GenericArrayType -> checkType(type.genericComponentType, field)
-
-                is ParameterizedType -> {
-                    if (Collection::class.java.isAssignableFrom(type.rawType as Class<*>)) {
-                        type.actualTypeArguments.forEach { checkType(it, field) }
-                    }
-                }
-
-                is WildcardType -> type.upperBounds.forEach { checkType(it, field) }
             }
         }
     }
 }
+
+private class AnnotationPresentationInfo(
+        val ranges: List<TextRange>,
+        val nonDefaultMessage: String? = null,
+        val highlightType: ProblemHighlightType? = null,
+        val textAttributes: TextAttributesKey? = null) {
+
+    public fun create(diagnostic: Diagnostic, range: TextRange, holder: AnnotationHolder): Annotation {
+        val defaultMessage = nonDefaultMessage?: getDefaultMessage(diagnostic)
+
+        val annotation = when (diagnostic.severity) {
+            Severity.ERROR -> holder.createErrorAnnotation(range, defaultMessage)
+            Severity.WARNING -> holder.createWarningAnnotation(range, defaultMessage)
+            else -> throw IllegalArgumentException("Only ERROR and WARNING diagnostics are supported")
+        }
+
+        annotation.tooltip = getMessage(diagnostic)
+
+        if (highlightType != null) {
+            annotation.highlightType = highlightType
+        }
+
+        if (textAttributes != null) {
+            annotation.textAttributes = textAttributes
+        }
+
+        return annotation
+    }
+
+    private fun getMessage(diagnostic: Diagnostic): String {
+        var message = IdeErrorMessages.render(diagnostic)
+        if (KotlinInternalMode.enabled || ApplicationManager.getApplication().isUnitTestMode()) {
+            val factoryName = diagnostic.getFactory().getName()
+            if (message.startsWith("<html>")) {
+                message = "<html>[$factoryName] ${message.substring("<html>".length)}"
+            }
+            else {
+                message = "[$factoryName] $message"
+            }
+        }
+        if (!message.startsWith("<html>")) {
+            message = "<html><body>${XmlStringUtil.escapeString(message)}</body></html>"
+        }
+        return message
+    }
+
+    private fun getDefaultMessage(diagnostic: Diagnostic): String {
+        val message = DefaultErrorMessages.render(diagnostic)
+        if (KotlinInternalMode.enabled || ApplicationManager.getApplication().isUnitTestMode()) {
+            return "[${diagnostic.getFactory().getName()}] $message"
+        }
+        return message
+    }
+}

idea/idea-analysis/src/org/jetbrains/kotlin/idea/quickfix/JetIntentionActionsFactory.kt

@@ -19,21 +19,23 @@ package org.jetbrains.kotlin.idea.quickfix
 import com.intellij.codeInsight.intention.IntentionAction
 import org.jetbrains.kotlin.diagnostics.Diagnostic
 import org.jetbrains.kotlin.psi.KtCodeFragment
-import java.util.Collections
+import org.jetbrains.kotlin.utils.singletonOrEmptyList
 
-// TODO: Replace with trait when all subclasses are translated to Kotlin
 public abstract class JetIntentionActionsFactory {
     protected open fun isApplicableForCodeFragment(): Boolean = false
 
     protected abstract fun doCreateActions(diagnostic: Diagnostic): List<IntentionAction>
 
-    public open fun canFixSeveralSameProblems(): Boolean = false
+    protected open fun doCreateActionsForAllProblems(
-    protected open fun doCreateActions(@Suppress("UNUSED_PARAMETER") sameTypeDiagnostics: List<Diagnostic>): List<IntentionAction> =
+            sameTypeDiagnostics: Collection<Diagnostic>): List<IntentionAction> = emptyList()
-            throw NotImplementedError()
 
-    public fun createActions(diagnostic: Diagnostic) = createActions(listOf(diagnostic))
+    public fun createActions(diagnostic: Diagnostic): List<IntentionAction> =
+            createActions(diagnostic.singletonOrEmptyList(), false)
 
-    public fun createActions(sameTypeDiagnostics: List<Diagnostic>): List<IntentionAction> {
+    public fun createActionsForAllProblems(sameTypeDiagnostics: Collection<Diagnostic>): List<IntentionAction> =
+            createActions(sameTypeDiagnostics, true)
+
+    private fun createActions(sameTypeDiagnostics: Collection<Diagnostic>, createForAll: Boolean): List<IntentionAction> {
         if (sameTypeDiagnostics.isEmpty()) return emptyList()
         val first = sameTypeDiagnostics.first()
 
@@ -41,14 +43,12 @@ public abstract class JetIntentionActionsFactory {
             return emptyList()
         }
 
-        if (sameTypeDiagnostics.size > 1) {
+        if (sameTypeDiagnostics.size > 1 && createForAll) {
             assert(sameTypeDiagnostics.all { it.psiElement == first.psiElement && it.factory == first.factory }) {
                 "It's expected to be the list of diagnostics of same type and for same element"
             }
 
-            if (canFixSeveralSameProblems()) {
+            return doCreateActionsForAllProblems(sameTypeDiagnostics)
-                return doCreateActions(sameTypeDiagnostics)
-            }
         }
 
         return sameTypeDiagnostics.flatMapTo(arrayListOf()) { doCreateActions(it) }

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

@@ -106,7 +106,7 @@ public class KotlinCleanupInspection(): LocalInspectionTool(), CleanupLocalInspe
     }
 
     private fun Diagnostic.toCleanupFixes(): Collection<CleanupFix> {
-        return JetPsiChecker.createQuickfixes(this).filterIsInstance<CleanupFix>()
+        return JetPsiChecker.createQuickFixes(this).filterIsInstance<CleanupFix>()
     }
 
     private class Wrapper(val intention: IntentionAction, file: KtFile) : IntentionWrapper(intention, file) {