Do not insert explicit visibility modifier if it's default

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

@@ -22,7 +22,6 @@ import org.jetbrains.annotations.Nullable;
 import org.jetbrains.kotlin.JetNodeTypes;
 import org.jetbrains.kotlin.kdoc.psi.api.KDoc;
 import org.jetbrains.kotlin.lexer.JetModifierKeywordToken;
-import org.jetbrains.kotlin.lexer.JetTokens;
 import org.jetbrains.kotlin.psi.addRemoveModifier.AddRemoveModifierPackage;
 import org.jetbrains.kotlin.psi.findDocComment.FindDocCommentPackage;
 
@@ -48,7 +47,7 @@ abstract class JetDeclarationImpl extends JetExpressionImpl implements JetDeclar
 
     @Override
     public void addModifier(@NotNull JetModifierKeywordToken modifier) {
-        AddRemoveModifierPackage.addModifier(this, modifier, JetTokens.DEFAULT_VISIBILITY_KEYWORD);
+        AddRemoveModifierPackage.addModifier(this, modifier);
     }
 
     @Override

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

@@ -22,7 +22,6 @@ import com.intellij.psi.stubs.StubElement;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.kotlin.lexer.JetModifierKeywordToken;
-import org.jetbrains.kotlin.lexer.JetTokens;
 import org.jetbrains.kotlin.psi.addRemoveModifier.AddRemoveModifierPackage;
 import org.jetbrains.kotlin.psi.stubs.elements.JetStubElementTypes;
 
@@ -52,7 +51,7 @@ public class JetModifierListOwnerStub<T extends StubElement<?>> extends JetEleme
 
     @Override
     public void addModifier(@NotNull JetModifierKeywordToken modifier) {
-        AddRemoveModifierPackage.addModifier(this, modifier, JetTokens.DEFAULT_VISIBILITY_KEYWORD);
+        AddRemoveModifierPackage.addModifier(this, modifier);
     }
 
     @Override

compiler/frontend/src/org/jetbrains/kotlin/psi/JetPrimaryConstructor.kt

@@ -39,7 +39,7 @@ public class JetPrimaryConstructor : JetConstructor<JetPrimaryConstructor> {
     override fun addModifier(modifier: JetModifierKeywordToken) {
         val modifierList = modifierList
         if (modifierList != null) {
-            addModifier(modifierList, modifier, JetTokens.PUBLIC_KEYWORD)
+            addModifier(modifierList, modifier)
             if (this.modifierList == null) {
                 getConstructorKeyword()?.delete()
             }

compiler/frontend/src/org/jetbrains/kotlin/psi/addRemoveModifier.kt

@@ -33,14 +33,13 @@ private fun createModifierList(text: String, owner: JetModifierListOwner): JetMo
     return owner.addBefore(newModifierList, anchor) as JetModifierList
 }
 
-internal fun addModifier(owner: JetModifierListOwner, modifier: JetModifierKeywordToken, defaultVisibilityModifier: JetModifierKeywordToken) {
+internal fun addModifier(owner: JetModifierListOwner, modifier: JetModifierKeywordToken) {
     val modifierList = owner.modifierList
     if (modifierList == null) {
-        if (modifier == defaultVisibilityModifier) return
         createModifierList(modifier.value, owner)
     }
     else {
-        addModifier(modifierList, modifier, defaultVisibilityModifier)
+        addModifier(modifierList, modifier)
     }
 }
 
@@ -54,7 +53,7 @@ internal fun addAnnotationEntry(owner: JetModifierListOwner, annotationEntry: Je
     }
 }
 
-internal fun addModifier(modifierList: JetModifierList, modifier: JetModifierKeywordToken, defaultVisibilityModifier: JetModifierKeywordToken) {
+internal fun addModifier(modifierList: JetModifierList, modifier: JetModifierKeywordToken) {
     if (modifierList.hasModifier(modifier)) return
 
     val newModifier = JetPsiFactory(modifierList).createModifier(modifier)
@@ -63,15 +62,6 @@ internal fun addModifier(modifierList: JetModifierList, modifier: JetModifierKey
             ?.filterNotNull()
             ?.firstOrNull()
 
-    if (modifier == defaultVisibilityModifier) { // do not insert explicit 'internal' keyword (or 'public' for primary constructor)
-        //TODO: code style option
-        modifierToReplace?.delete()
-        if (modifierList.firstChild == null) {
-            modifierList.delete()
-        }
-        return
-    }
-
     if (modifierToReplace != null) {
         modifierToReplace.replace(newModifier)
     }

compiler/frontend/src/org/jetbrains/kotlin/psi/psiUtil/jetPsiUtil.kt

@@ -25,6 +25,7 @@ import com.intellij.psi.PsiWhiteSpace
 import com.intellij.psi.stubs.StubElement
 import com.intellij.psi.util.PsiTreeUtil
 import org.jetbrains.kotlin.JetNodeTypes
+import org.jetbrains.kotlin.lexer.JetModifierKeywordToken
 import org.jetbrains.kotlin.lexer.JetTokens
 import org.jetbrains.kotlin.name.Name
 import org.jetbrains.kotlin.psi.*
@@ -394,3 +395,10 @@ public fun JetFunctionLiteralArgument.getFunctionLiteralArgumentName(bindingCont
 public fun JetExpression.asAssignment(): JetBinaryExpression? =
         if (JetPsiUtil.isAssignment(this)) this as JetBinaryExpression else null
 
+public fun JetDeclaration.visibilityModifier(): PsiElement? {
+    val modifierList = modifierList ?: return null
+    return JetTokens.VISIBILITY_MODIFIERS.types
+                   .asSequence()
+                   .map { modifierList.getModifier(it as JetModifierKeywordToken) }
+                   .firstOrNull { it != null }
+}

core/descriptors/src/org/jetbrains/kotlin/resolve/OverridingUtil.java

@@ -465,7 +465,7 @@ public class OverridingUtil {
     }
 
     @Nullable
-    private static Visibility findMaxVisibility(@NotNull Collection<? extends CallableMemberDescriptor> descriptors) {
+    public static Visibility findMaxVisibility(@NotNull Collection<? extends CallableMemberDescriptor> descriptors) {
         if (descriptors.isEmpty()) {
             return Visibilities.DEFAULT_VISIBILITY;
         }

idea/idea-core/src/org/jetbrains/kotlin/idea/core/psiModificationUtils.kt

@@ -20,13 +20,19 @@ import com.intellij.psi.PsiElement
 import com.intellij.psi.PsiWhiteSpace
 import com.intellij.psi.impl.source.codeStyle.CodeEditUtil
 import com.intellij.psi.util.PsiTreeUtil
+import org.jetbrains.kotlin.descriptors.CallableMemberDescriptor
+import org.jetbrains.kotlin.descriptors.Visibilities
+import org.jetbrains.kotlin.descriptors.Visibility
+import org.jetbrains.kotlin.idea.caches.resolve.resolveToDescriptor
+import org.jetbrains.kotlin.lexer.JetModifierKeywordToken
+import org.jetbrains.kotlin.lexer.JetTokens
 import org.jetbrains.kotlin.name.Name
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.getFunctionLiteralArgumentName
+import org.jetbrains.kotlin.psi.psiUtil.visibilityModifier
 import org.jetbrains.kotlin.resolve.BindingContext
-import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
+import org.jetbrains.kotlin.resolve.OverridingUtil
 import org.jetbrains.kotlin.resolve.calls.callUtil.getValueArgumentsInParentheses
-import org.jetbrains.kotlin.resolve.calls.model.ArgumentMatch
 
 @Suppress("UNCHECKED_CAST")
 public inline fun <reified T: PsiElement> PsiElement.replaced(newElement: T): T {
@@ -153,4 +159,34 @@ public fun PsiElement.deleteSingle() {
 public fun JetClass.getOrCreateCompanionObject() : JetObjectDeclaration {
     getCompanionObjects().firstOrNull()?.let { return it }
     return addDeclaration(JetPsiFactory(this).createCompanionObject()) as JetObjectDeclaration
+}
+
+//TODO: code style option whether to insert redundant 'public' keyword or not
+public fun JetDeclaration.setVisibility(visibilityModifier: JetModifierKeywordToken) {
+    val defaultVisibilityKeyword = if (hasModifier(JetTokens.OVERRIDE_KEYWORD)) {
+        (resolveToDescriptor() as? CallableMemberDescriptor)
+                ?.overriddenDescriptors
+                ?.let { OverridingUtil.findMaxVisibility(it) }
+                ?.toKeyword()
+    }
+    else {
+        JetTokens.DEFAULT_VISIBILITY_KEYWORD
+    }
+
+    if (visibilityModifier == defaultVisibilityKeyword) {
+        this.visibilityModifier()?.let { removeModifier(it.node.elementType as JetModifierKeywordToken) }
+        return
+    }
+
+    addModifier(visibilityModifier)
+}
+
+private fun Visibility.toKeyword(): JetModifierKeywordToken? {
+    return when (this) {
+        Visibilities.PUBLIC -> JetTokens.PUBLIC_KEYWORD
+        Visibilities.PROTECTED -> JetTokens.PROTECTED_KEYWORD
+        Visibilities.INTERNAL -> JetTokens.INTERNAL_KEYWORD
+        Visibilities.PRIVATE -> JetTokens.PRIVATE_KEYWORD
+        else -> null
+    }
 }

idea/src/org/jetbrains/kotlin/idea/intentions/ChangeVisibilityModifierIntention.kt

@@ -19,14 +19,15 @@ package org.jetbrains.kotlin.idea.intentions
 import com.intellij.codeInsight.intention.HighPriorityAction
 import com.intellij.openapi.editor.Editor
 import com.intellij.openapi.util.TextRange
-import com.intellij.psi.PsiElement
 import org.jetbrains.kotlin.descriptors.*
 import org.jetbrains.kotlin.idea.caches.resolve.analyze
 import org.jetbrains.kotlin.idea.caches.resolve.resolveToDescriptor
+import org.jetbrains.kotlin.idea.core.setVisibility
 import org.jetbrains.kotlin.lexer.JetModifierKeywordToken
 import org.jetbrains.kotlin.lexer.JetTokens
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.startOffset
+import org.jetbrains.kotlin.psi.psiUtil.visibilityModifier
 import org.jetbrains.kotlin.resolve.BindingContext
 
 public open class ChangeVisibilityModifierIntention protected constructor(
@@ -83,21 +84,7 @@ public open class ChangeVisibilityModifierIntention protected constructor(
     }
 
     override fun applyTo(element: JetDeclaration, editor: Editor) {
-        val modifierToChange = element.visibilityModifier()
+        element.setVisibility(modifier)
-        if (modifierToChange != null) {
-            modifierToChange.replace(JetPsiFactory(element).createModifier(modifier))
-        }
-        else {
-            element.addModifier(modifier)
-        }
-    }
-
-    private fun JetDeclaration.visibilityModifier(): PsiElement? {
-        val modifierList = getModifierList() ?: return null
-        return JetTokens.VISIBILITY_MODIFIERS.getTypes()
-                       .asSequence()
-                       .map { modifierList.getModifier(it as JetModifierKeywordToken) }
-                       .firstOrNull { it != null } ?: return null
     }
 
     private fun JetModifierKeywordToken.toVisibility(): Visibility {

idea/src/org/jetbrains/kotlin/idea/quickfix/ChangeVisibilityModifierFix.java

@@ -30,15 +30,16 @@ import org.jetbrains.kotlin.descriptors.Visibility;
 import org.jetbrains.kotlin.diagnostics.Diagnostic;
 import org.jetbrains.kotlin.idea.JetBundle;
 import org.jetbrains.kotlin.idea.caches.resolve.ResolutionUtils;
+import org.jetbrains.kotlin.idea.core.PsiModificationUtilsKt;
 import org.jetbrains.kotlin.idea.refactoring.JetRefactoringUtil;
 import org.jetbrains.kotlin.lexer.JetModifierKeywordToken;
+import org.jetbrains.kotlin.psi.JetDeclaration;
 import org.jetbrains.kotlin.psi.JetFile;
-import org.jetbrains.kotlin.psi.JetModifierListOwner;
 import org.jetbrains.kotlin.psi.JetParameter;
 import org.jetbrains.kotlin.resolve.BindingContext;
 
-public class ChangeVisibilityModifierFix extends JetIntentionAction<JetModifierListOwner> {
+public class ChangeVisibilityModifierFix extends JetIntentionAction<JetDeclaration> {
-    public ChangeVisibilityModifierFix(@NotNull JetModifierListOwner element) {
+    public ChangeVisibilityModifierFix(@NotNull JetDeclaration element) {
         super(element);
     }
 
@@ -64,7 +65,7 @@ public class ChangeVisibilityModifierFix extends JetIntentionAction<JetModifierL
     public void invoke(@NotNull Project project, Editor editor, JetFile file) throws IncorrectOperationException {
         JetModifierKeywordToken modifier = findVisibilityChangeTo(file);
         assert modifier != null;
-        element.addModifier(modifier);
+        PsiModificationUtilsKt.setVisibility(element, modifier);
     }
 
     @Nullable
@@ -109,10 +110,10 @@ public class ChangeVisibilityModifierFix extends JetIntentionAction<JetModifierL
     public static JetSingleIntentionActionFactory createFactory() {
         return new JetSingleIntentionActionFactory() {
             @Override
-            public JetIntentionAction<JetModifierListOwner> createAction(Diagnostic diagnostic) {
+            public JetIntentionAction<JetDeclaration> createAction(Diagnostic diagnostic) {
                 PsiElement element = diagnostic.getPsiElement();
-                if (!(element instanceof JetModifierListOwner)) return null;
+                if (!(element instanceof JetDeclaration)) return null;
-                return new ChangeVisibilityModifierFix((JetModifierListOwner)element);
+                return new ChangeVisibilityModifierFix((JetDeclaration)element);
             }
         };
     }

idea/src/org/jetbrains/kotlin/idea/refactoring/changeSignature/usages/JetCallableDefinitionUsage.java

@@ -33,6 +33,7 @@ import org.jetbrains.kotlin.descriptors.impl.AnonymousFunctionDescriptor;
 import org.jetbrains.kotlin.idea.caches.resolve.JavaResolutionUtils;
 import org.jetbrains.kotlin.idea.caches.resolve.ResolutionUtils;
 import org.jetbrains.kotlin.idea.codeInsight.shorten.ShortenPackage;
+import org.jetbrains.kotlin.idea.core.PsiModificationUtilsKt;
 import org.jetbrains.kotlin.idea.refactoring.JetRefactoringUtil;
 import org.jetbrains.kotlin.idea.refactoring.changeSignature.ChangeSignaturePackage;
 import org.jetbrains.kotlin.idea.refactoring.changeSignature.JetChangeInfo;
@@ -346,10 +347,10 @@ public class JetCallableDefinitionUsage<T extends PsiElement> extends JetUsageIn
         JetModifierKeywordToken newVisibilityToken = JetRefactoringUtil.getVisibilityToken(changeInfo.getNewVisibility());
 
         if (element instanceof JetCallableDeclaration) {
-            ((JetCallableDeclaration)element).addModifier(newVisibilityToken);
+            PsiModificationUtilsKt.setVisibility((JetCallableDeclaration)element, newVisibilityToken);
         }
         else if (element instanceof JetClass) {
-            ((JetClass) element).createPrimaryConstructorIfAbsent().addModifier(newVisibilityToken);
+            PsiModificationUtilsKt.setVisibility(((JetClass) element).createPrimaryConstructorIfAbsent(), newVisibilityToken);
         }
         else throw new AssertionError("Invalid element: " + PsiUtilPackage.getElementTextWithContext(element));
     }

idea/testData/intentions/changeVisibility/protected/caretAfter.kt.after

@@ -0,0 +1,3 @@
+class C {
+    protected<caret> fun foo(){}
+}

idea/testData/intentions/changeVisibility/protected/overrideVisibilityToDefault.kt.after

@@ -3,5 +3,5 @@ interface I {
 }
 
 abstract class C : I {
-    <caret>protected override fun foo() {}
+    <caret>override fun foo() {}
 }

idea/testData/intentions/changeVisibility/public/caretAfter.kt.after

@@ -1,3 +0,0 @@
-class C {
-    public<caret> fun foo(){}
-}

idea/testData/intentions/changeVisibility/public/forOverride2.kt

@@ -0,0 +1,7 @@
+interface I {
+    protected fun foo()
+}
+
+abstract class C : I {
+    <caret>override fun foo() {}
+}

idea/testData/intentions/changeVisibility/public/forOverride2.kt.after

@@ -0,0 +1,7 @@
+interface I {
+    protected fun foo()
+}
+
+abstract class C : I {
+    <caret>public override fun foo() {}
+}

idea/testData/intentions/changeVisibility/public/primaryConstructor.kt.after

@@ -1,2 +1,2 @@
 // INTENTION_TEXT: Make public
-class C <caret>public constructor(val v: Int)
+class C <caret>(val v: Int)

idea/testData/intentions/changeVisibility/public/simple.kt.after

@@ -1,3 +1,3 @@
 class C {
-    <caret>public fun foo(){}
+    <caret>fun foo(){}
 }

idea/testData/quickfix/modifiers/visibilityModifer1.kt.after

@@ -4,5 +4,5 @@ open class A {
 }
 
 class B : A() {
-    <caret>protected override fun run() {}
+    <caret>override fun run() {}
 }

idea/testData/quickfix/modifiers/visibilityModifer2.kt.after

@@ -4,5 +4,5 @@ open class A {
 }
 
 class B : A() {
-    <caret>protected override fun run() {}
+    <caret>override fun run() {}
 }

idea/testData/quickfix/modifiers/visibilityModiferOverrideJavaRuntime.kt.after

@@ -1,6 +1,6 @@
 // "Change visibility modifier" "true"
 abstract class C : ClassLoader() {
-    <caret>protected override fun findClass(var1: String): Class<*> {
+    <caret>override fun findClass(var1: String): Class<*> {
         throw ClassNotFoundException(var1)
     }
 }

idea/tests/org/jetbrains/kotlin/idea/intentions/IntentionTestGenerated.java

@@ -2264,6 +2264,12 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
                 JetTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/intentions/changeVisibility/protected"), Pattern.compile("^([\\w\\-_]+)\\.kt$"), true);
             }
 
+            @TestMetadata("caretAfter.kt")
+            public void testCaretAfter() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/protected/caretAfter.kt");
+                doTest(fileName);
+            }
+
             @TestMetadata("noModifier.kt")
             public void testNoModifier() throws Exception {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/protected/noModifier.kt");
@@ -2321,18 +2327,18 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
                 doTest(fileName);
             }
 
-            @TestMetadata("caretAfter.kt")
-            public void testCaretAfter() throws Exception {
-                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/public/caretAfter.kt");
-                doTest(fileName);
-            }
-
             @TestMetadata("forOverride.kt")
             public void testForOverride() throws Exception {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/public/forOverride.kt");
                 doTest(fileName);
             }
 
+            @TestMetadata("forOverride2.kt")
+            public void testForOverride2() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/public/forOverride2.kt");
+                doTest(fileName);
+            }
+
             @TestMetadata("primaryConstructor.kt")
             public void testPrimaryConstructor() throws Exception {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/public/primaryConstructor.kt");