Refactored RemoveModifierFix + some psi API refactoring

#EA-69237 Fixed

compiler/frontend/src/org/jetbrains/kotlin/diagnostics/PositioningStrategies.kt

@@ -21,7 +21,7 @@ import com.intellij.psi.PsiElement
 import com.intellij.psi.PsiNameIdentifierOwner
 import com.intellij.psi.tree.TokenSet
 import org.jetbrains.kotlin.JetNodeTypes
-import org.jetbrains.kotlin.lexer.JetKeywordToken
+import org.jetbrains.kotlin.lexer.JetModifierKeywordToken
 import org.jetbrains.kotlin.lexer.JetTokens
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.endOffset
@@ -238,15 +238,15 @@ public object PositioningStrategies {
     }
 
     platformStatic
-    public fun modifierSetPosition(vararg tokens: JetKeywordToken): PositioningStrategy<JetModifierListOwner> {
+    public fun modifierSetPosition(vararg tokens: JetModifierKeywordToken): PositioningStrategy<JetModifierListOwner> {
         return object : PositioningStrategy<JetModifierListOwner>() {
             override fun mark(element: JetModifierListOwner): List<TextRange> {
                 val modifierList = element.getModifierList().sure { "No modifier list, but modifier has been found by the analyzer" }
 
                 for (token in tokens) {
-                    val node = modifierList.getModifierNode(token)
+                    val element = modifierList.getModifier(token)
-                    if (node != null) {
+                    if (element != null) {
-                        return markNode(node)
+                        return markElement(element)
                     }
                 }
                 throw IllegalStateException("None of the modifiers is found: " + listOf(*tokens))
@@ -265,7 +265,7 @@ public object PositioningStrategies {
             val visibilityTokens = listOf(JetTokens.PRIVATE_KEYWORD, JetTokens.PROTECTED_KEYWORD, JetTokens.PUBLIC_KEYWORD, JetTokens.INTERNAL_KEYWORD)
             val modifierList = element.getModifierList()
 
-            val result = visibilityTokens.map { modifierList?.getModifierNode(it)?.getTextRange() }.filterNotNull()
+            val result = visibilityTokens.map { modifierList?.getModifier(it)?.getTextRange() }.filterNotNull()
             if (!result.isEmpty()) return result
 
             // Try to resolve situation when there's no visibility modifiers written before element
@@ -289,7 +289,7 @@ public object PositioningStrategies {
 
     public val VARIANCE_IN_PROJECTION: PositioningStrategy<JetTypeProjection> = object : PositioningStrategy<JetTypeProjection>() {
         override fun mark(element: JetTypeProjection): List<TextRange> {
-            return markNode(element.getProjectionNode()!!)
+            return markElement(element.getProjectionToken()!!)
         }
     }
 

compiler/frontend/src/org/jetbrains/kotlin/psi/JetModifierList.java

@@ -22,7 +22,6 @@ import com.intellij.psi.stubs.IStubElementType;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.kotlin.lexer.JetModifierKeywordToken;
-import org.jetbrains.kotlin.lexer.JetToken;
 import org.jetbrains.kotlin.psi.psiUtil.PsiUtilPackage;
 import org.jetbrains.kotlin.psi.stubs.KotlinModifierListStub;
 import org.jetbrains.kotlin.psi.stubs.elements.JetStubElementTypes;
@@ -54,30 +53,28 @@ public abstract class JetModifierList extends JetElementImplStub<KotlinModifierL
         return PsiUtilPackage.collectAnnotationEntriesFromStubOrPsi(this);
     }
 
-    public boolean hasModifier(@NotNull JetModifierKeywordToken token) {
+    public boolean hasModifier(@NotNull JetModifierKeywordToken tokenType) {
         KotlinModifierListStub stub = getStub();
         if (stub != null) {
-            return stub.hasModifier(token);
+            return stub.hasModifier(tokenType);
         }
-        return getModifierNode(token) != null;
+        return getModifier(tokenType) != null;
     }
 
     @Nullable
-    public PsiElement getModifier(@NotNull JetModifierKeywordToken token) {
+    public PsiElement getModifier(@NotNull JetModifierKeywordToken tokenType) {
-        return findChildByType(token);
+        return findChildByType(tokenType);
-    }
-
-    @Nullable
-    public ASTNode getModifierNode(@NotNull JetToken token) {
-        ASTNode node = getNode().getFirstChildNode();
-        while (node != null) {
-            if (node.getElementType() == token) return node;
-            node = node.getTreeNext();
-        }
-        return null;
     }
 
     public PsiElement getOwner() {
         return getParentByStub();
     }
+
+    @Override
+    public void deleteChildInternal(@NotNull ASTNode child) {
+        super.deleteChildInternal(child);
+        if (getFirstChild() == null) {
+            delete();
+        }
+    }
 }

compiler/frontend/src/org/jetbrains/kotlin/psi/JetTypeProjection.java

@@ -42,14 +42,14 @@ public class JetTypeProjection extends JetModifierListOwnerStub<KotlinTypeProjec
             return stub.getProjectionKind();
         }
 
-        ASTNode projectionNode = getProjectionNode();
+        PsiElement projectionToken = getProjectionToken();
-        IElementType token = projectionNode != null ? projectionNode.getElementType() : null;
+        IElementType token = projectionToken != null ? projectionToken.getNode().getElementType() : null;
         for (JetProjectionKind projectionKind : JetProjectionKind.values()) {
             if (projectionKind.getToken() == token) {
                 return projectionKind;
             }
         }
-        throw new IllegalStateException(projectionNode.getText());
+        throw new IllegalStateException(projectionToken.getText());
     }
 
     @Override
@@ -63,22 +63,19 @@ public class JetTypeProjection extends JetModifierListOwnerStub<KotlinTypeProjec
     }
 
     @Nullable
-    public ASTNode getProjectionNode() {
+    public PsiElement getProjectionToken() {
         PsiElement star = findChildByType(JetTokens.MUL);
         if (star != null) {
-            return star.getNode();
+            return star;
         }
 
         JetModifierList modifierList = getModifierList();
         if (modifierList != null) {
-            ASTNode node = modifierList.getModifierNode(JetTokens.IN_KEYWORD);
+            PsiElement element = modifierList.getModifier(JetTokens.IN_KEYWORD);
-            if (node != null) {
+            if (element != null) return element;
-                return node;
+
-            }
+            element = modifierList.getModifier(JetTokens.OUT_KEYWORD);
-            node = modifierList.getModifierNode(JetTokens.OUT_KEYWORD);
+            if (element != null) return element;
-            if (node != null) {
-                return node;
-            }
         }
 
         return null;

compiler/frontend/src/org/jetbrains/kotlin/resolve/DeclarationsChecker.java

@@ -20,7 +20,6 @@ import com.google.common.collect.Multimap;
 import com.google.common.collect.Sets;
 import com.intellij.lang.ASTNode;
 import com.intellij.psi.PsiElement;
-import com.intellij.psi.tree.IElementType;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.kotlin.descriptors.*;
@@ -28,14 +27,16 @@ import org.jetbrains.kotlin.diagnostics.Errors;
 import org.jetbrains.kotlin.lexer.JetModifierKeywordToken;
 import org.jetbrains.kotlin.lexer.JetTokens;
 import org.jetbrains.kotlin.psi.*;
-import org.jetbrains.kotlin.psi.psiUtil.PsiUtilPackage;
 import org.jetbrains.kotlin.types.JetType;
 import org.jetbrains.kotlin.types.SubstitutionUtils;
 import org.jetbrains.kotlin.types.TypeConstructor;
 import org.jetbrains.kotlin.types.TypeProjection;
 import org.jetbrains.kotlin.types.checker.JetTypeChecker;
 
-import java.util.*;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Set;
 
 import static org.jetbrains.kotlin.diagnostics.Errors.*;
 import static org.jetbrains.kotlin.resolve.BindingContext.TYPE;
@@ -352,9 +353,8 @@ public class DeclarationsChecker {
         JetPropertyAccessor getter = property.getGetter();
         JetPropertyAccessor setter = property.getSetter();
         JetModifierList modifierList = property.getModifierList();
-        ASTNode abstractNode = modifierList != null ? modifierList.getModifierNode(JetTokens.ABSTRACT_KEYWORD) : null;
 
-        if (abstractNode != null) { //has abstract modifier
+        if (modifierList != null && modifierList.hasModifier(JetTokens.ABSTRACT_KEYWORD)) { //has abstract modifier
             if (!classCanHaveAbstractMembers(classDescriptor)) {
                 String name = property.getName();
                 trace.report(ABSTRACT_PROPERTY_IN_NON_ABSTRACT_CLASS.on(property, name != null ? name : "", classDescriptor));
@@ -486,17 +486,17 @@ public class DeclarationsChecker {
         PropertyGetterDescriptor getterDescriptor = propertyDescriptor.getGetter();
         JetModifierList getterModifierList = getter != null ? getter.getModifierList() : null;
         if (getterModifierList != null && getterDescriptor != null) {
-            Map<JetModifierKeywordToken, ASTNode> nodes = modifiersChecker.getNodesCorrespondingToModifiers(getterModifierList, Sets
+            Map<JetModifierKeywordToken, PsiElement> tokens = modifiersChecker.getTokensCorrespondingToModifiers(getterModifierList, Sets
                     .newHashSet(JetTokens.PUBLIC_KEYWORD, JetTokens.PROTECTED_KEYWORD, JetTokens.PRIVATE_KEYWORD,
                                 JetTokens.INTERNAL_KEYWORD));
             if (getterDescriptor.getVisibility() != propertyDescriptor.getVisibility()) {
-                for (ASTNode node : nodes.values()) {
+                for (PsiElement token : tokens.values()) {
-                    trace.report(Errors.GETTER_VISIBILITY_DIFFERS_FROM_PROPERTY_VISIBILITY.on(node.getPsi()));
+                    trace.report(Errors.GETTER_VISIBILITY_DIFFERS_FROM_PROPERTY_VISIBILITY.on(token));
                 }
             }
             else {
-                for (ASTNode node : nodes.values()) {
+                for (PsiElement token : tokens.values()) {
-                    trace.report(Errors.REDUNDANT_MODIFIER_IN_GETTER.on(node.getPsi()));
+                    trace.report(Errors.REDUNDANT_MODIFIER_IN_GETTER.on(token));
                 }
             }
         }

compiler/frontend/src/org/jetbrains/kotlin/resolve/ModifiersChecker.java

@@ -19,7 +19,6 @@ package org.jetbrains.kotlin.resolve;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
-import com.intellij.lang.ASTNode;
 import com.intellij.psi.PsiElement;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -256,17 +255,17 @@ public class ModifiersChecker {
         }
 
         @NotNull
-        public Map<JetModifierKeywordToken, ASTNode> getNodesCorrespondingToModifiers(
+        public Map<JetModifierKeywordToken, PsiElement> getTokensCorrespondingToModifiers(
                 @NotNull JetModifierList modifierList,
                 @NotNull Collection<JetModifierKeywordToken> possibleModifiers
         ) {
-            Map<JetModifierKeywordToken, ASTNode> nodes = Maps.newHashMap();
+            Map<JetModifierKeywordToken, PsiElement> tokens = Maps.newHashMap();
             for (JetModifierKeywordToken modifier : possibleModifiers) {
                 if (modifierList.hasModifier(modifier)) {
-                    nodes.put(modifier, modifierList.getModifierNode(modifier));
+                    tokens.put(modifier, modifierList.getModifier(modifier));
                 }
             }
-            return nodes;
+            return tokens;
         }
 
 

idea/idea-analysis/src/org/jetbrains/kotlin/idea/JetBundle.properties

@@ -20,10 +20,6 @@ make.class.annotation.class.family=Make Class an Annotation Class
 add.star.projections=Add ''{0}''
 change.to.star.projection=Change type arguments to {0}
 change.to.star.projection.family=Change to Star Projection
-make.element.not.modifier=Make {0} not {1}
-remove.modifier=Remove ''{0}'' modifier
-remove.modifier.family=Remove Modifier
-remove.redundant.modifier=Remove redundant ''{0}'' modifier
 remove.parts.from.property=Remove {0} from property
 remove.parts.from.property.family=Remove Parts from Property
 remove.psi.element.family=Remove Element

idea/src/org/jetbrains/kotlin/idea/quickfix/RemoveModifierFix.kt

@@ -16,16 +16,18 @@
 
 package org.jetbrains.kotlin.idea.quickfix
 
-import com.intellij.extapi.psi.ASTDelegatePsiElement
 import com.intellij.openapi.editor.Editor
 import com.intellij.openapi.project.Project
+import com.intellij.psi.PsiFile
 import org.jetbrains.kotlin.diagnostics.Diagnostic
-import org.jetbrains.kotlin.idea.JetBundle
 import org.jetbrains.kotlin.idea.core.quickfix.QuickFixUtil
-import org.jetbrains.kotlin.lexer.JetKeywordToken
 import org.jetbrains.kotlin.lexer.JetModifierKeywordToken
 import org.jetbrains.kotlin.lexer.JetTokens
-import org.jetbrains.kotlin.psi.*
+import org.jetbrains.kotlin.psi.JetFile
+import org.jetbrains.kotlin.psi.JetModifierListOwner
+import org.jetbrains.kotlin.psi.JetTypeParameter
+import org.jetbrains.kotlin.psi.JetTypeProjection
+import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType
 import org.jetbrains.kotlin.types.Variance
 
 public class RemoveModifierFix(
@@ -34,53 +36,37 @@ public class RemoveModifierFix(
         private val isRedundant: Boolean
 ) : JetIntentionAction<JetModifierListOwner>(element) {
 
-    override fun getFamilyName() = JetBundle.message("remove.modifier.family")
+    private val text = run {
+        val modifierText = modifier.value
+        when {
+            isRedundant ->
+                "Remove redundant '$modifierText' modifier"
+            modifier === JetTokens.ABSTRACT_KEYWORD || modifier === JetTokens.OPEN_KEYWORD ->
+                "Make ${AddModifierFix.getElementName(element)} not $modifierText"
+            else ->
+                "Remove '$modifierText' modifier"
+        }
+    }
 
-    override fun getText() = makeText(element, modifier, isRedundant)
+    override fun getFamilyName() = "Remove modifier"
+
+    override fun getText() = text
+
+    override fun isAvailable(project: Project, editor: Editor?, file: PsiFile)
+            = super.isAvailable(project, editor, file) && element.hasModifier(modifier)
 
     override fun invoke(project: Project, editor: Editor?, file: JetFile) {
         invoke()
     }
 
     public fun invoke() {
+        //TODO: without this copy&replace we get bad formatting on removing last modifier
         val newElement = element.copy() as JetModifierListOwner
-        element.replace(removeModifier(newElement, modifier))
+        newElement.removeModifier(modifier)
+        element.replace(newElement)
     }
 
     companion object {
-        private fun makeText(element: JetModifierListOwner?, modifier: JetKeywordToken, isRedundant: Boolean): String {
-            if (isRedundant) {
-                return JetBundle.message("remove.redundant.modifier", modifier.value)
-            }
-            if (element != null && (modifier === JetTokens.ABSTRACT_KEYWORD || modifier === JetTokens.OPEN_KEYWORD)) {
-                return JetBundle.message("make.element.not.modifier", AddModifierFix.getElementName(element), modifier.value)
-            }
-            return JetBundle.message("remove.modifier", modifier.value)
-        }
-
-        private fun <T : JetModifierListOwner> removeModifier(element: T, modifier: JetModifierKeywordToken): T {
-            val modifierList = element.modifierList!!
-            removeModifierFromList(modifierList, modifier)
-            if (modifierList.firstChild == null) {
-                val whiteSpace = modifierList.nextSibling
-                assert(element is ASTDelegatePsiElement)
-                (element as ASTDelegatePsiElement).deleteChildInternal(modifierList.node)
-                QuickFixUtil.removePossiblyWhiteSpace(element, whiteSpace)
-            }
-            return element
-        }
-
-        private fun removeModifierFromList(modifierList: JetModifierList, modifier: JetModifierKeywordToken): JetModifierList {
-            val modifierNode = modifierList.getModifierNode(modifier)!!
-            val whiteSpace = modifierNode.psi.nextSibling
-            val wsRemoved = QuickFixUtil.removePossiblyWhiteSpace(modifierList, whiteSpace)
-            modifierList.deleteChildInternal(modifierNode)
-            if (!wsRemoved) {
-                QuickFixUtil.removePossiblyWhiteSpace(modifierList, modifierList.lastChild)
-            }
-            return modifierList
-        }
-
         public fun createRemoveModifierFromListOwnerFactory(modifier: JetModifierKeywordToken, isRedundant: Boolean = false): JetSingleIntentionActionFactory {
             return object : JetSingleIntentionActionFactory() {
                 override fun createAction(diagnostic: Diagnostic): JetIntentionAction<JetModifierListOwner>? {
@@ -93,10 +79,9 @@ public class RemoveModifierFix(
         public fun createRemoveModifierFactory(isRedundant: Boolean = false): JetSingleIntentionActionFactory {
             return object : JetSingleIntentionActionFactory() {
                 override fun createAction(diagnostic: Diagnostic): JetIntentionAction<JetModifierListOwner>? {
-                    val modifierListOwner = QuickFixUtil.getParentElementOfType(diagnostic, javaClass<JetModifierListOwner>()) ?: return null
                     val psiElement = diagnostic.psiElement
-                    val elementType = psiElement.node.elementType
+                    val elementType = psiElement.node.elementType as? JetModifierKeywordToken ?: return null
-                    if (elementType !is JetModifierKeywordToken) return null
+                    val modifierListOwner = psiElement.getStrictParentOfType<JetModifierListOwner>() ?: return null
                     return RemoveModifierFix(modifierListOwner, elementType, isRedundant)
                 }
             }
@@ -105,10 +90,8 @@ public class RemoveModifierFix(
         public fun createRemoveProjectionFactory(isRedundant: Boolean): JetSingleIntentionActionFactory {
             return object : JetSingleIntentionActionFactory() {
                 override fun createAction(diagnostic: Diagnostic): JetIntentionAction<JetModifierListOwner>? {
-                    val projection = QuickFixUtil.getParentElementOfType(diagnostic, javaClass<JetTypeProjection>()) ?: return null
+                    val projection = diagnostic.psiElement as JetTypeProjection
-                    val projectionAstNode = projection.projectionNode ?: return null
+                    val elementType = projection.projectionToken?.node?.elementType as? JetModifierKeywordToken ?: return null
-                    val elementType = projectionAstNode.elementType
-                    if (elementType !is JetModifierKeywordToken) return null
                     return RemoveModifierFix(projection, elementType, isRedundant)
                 }
             }
@@ -117,17 +100,13 @@ public class RemoveModifierFix(
         public fun createRemoveVarianceFactory(): JetSingleIntentionActionFactory {
             return object : JetSingleIntentionActionFactory() {
                 override fun createAction(diagnostic: Diagnostic): JetIntentionAction<JetModifierListOwner>? {
-                    val modifierListOwner = QuickFixUtil.getParentElementOfType(diagnostic, javaClass<JetModifierListOwner>()) ?: return null
+                    val psiElement = diagnostic.psiElement as JetTypeParameter
-                    val psiElement = diagnostic.psiElement
+                    val modifier = when (psiElement.variance) {
-                    if (psiElement !is JetTypeParameter) return null
+                        Variance.IN_VARIANCE -> JetTokens.IN_KEYWORD
-                    val variance = psiElement.variance
+                        Variance.OUT_VARIANCE -> JetTokens.OUT_KEYWORD
-                    val modifier: JetModifierKeywordToken
-                    when (variance) {
-                        Variance.IN_VARIANCE -> modifier = JetTokens.IN_KEYWORD
-                        Variance.OUT_VARIANCE -> modifier = JetTokens.OUT_KEYWORD
                         else -> return null
                     }
-                    return RemoveModifierFix(modifierListOwner, modifier, false)
+                    return RemoveModifierFix(psiElement, modifier, isRedundant = false)
                 }
             }
         }