Fixing issues after code review

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

@@ -253,7 +253,7 @@ public class JetClass extends JetTypeParameterListOwnerStub<KotlinClassStub> imp
     }
 
     @Nullable
-    public PsiElement getClassOrTraitKeyword() {
+    public PsiElement getClassOrInterfaceKeyword() {
-        return findChildByType(TokenSet.create(JetTokens.CLASS_KEYWORD, JetTokens.TRAIT_KEYWORD));
+        return findChildByType(TokenSet.create(JetTokens.CLASS_KEYWORD, JetTokens.INTERFACE_KEYWORD));
     }
 }

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

@@ -28,6 +28,7 @@ import com.intellij.util.LocalTimeCounter
 import org.jetbrains.kotlin.analyzer.ModuleInfo
 import org.jetbrains.kotlin.idea.JetFileType
 import org.jetbrains.kotlin.lexer.JetKeywordToken
+import org.jetbrains.kotlin.lexer.JetModifierKeywordToken
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.name.Name
 import org.jetbrains.kotlin.name.renderName
@@ -225,7 +226,7 @@ public class JetPsiFactory(private val project: Project) {
         return createClass("class Foo {\n $decl \n}").getSecondaryConstructors().first()
     }
 
-    public fun createModifierList(modifier: JetKeywordToken): JetModifierList {
+    public fun createModifierList(modifier: JetModifierKeywordToken): JetModifierList {
         return createModifierList(modifier.getValue())
     }
 
@@ -233,6 +234,10 @@ public class JetPsiFactory(private val project: Project) {
         return createProperty(text + " val x").getModifierList()!!
     }
 
+    public fun createModifier(modifier: JetModifierKeywordToken): PsiElement {
+        return createModifierList(modifier.getValue()).getModifier(modifier)!!
+    }
+
     public fun createAnnotationEntry(text: String): JetAnnotationEntry {
         val modifierList = createProperty(text + " val x").getModifierList()
         return modifierList!!.getAnnotationEntries().first()

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

@@ -44,7 +44,7 @@ internal fun addModifier(owner: JetModifierListOwner, modifier: JetModifierKeywo
 }
 
 internal fun addModifier(modifierList: JetModifierList, modifier: JetModifierKeywordToken, defaultVisibilityModifier: JetModifierKeywordToken) {
-    val newModifier = JetPsiFactory(modifierList).createModifierList(modifier).getModifier(modifier)!!
+    val newModifier = JetPsiFactory(modifierList).createModifier(modifier)
     val modifierToReplace = MODIFIERS_TO_REPLACE[modifier]
             ?.map { modifierList.getModifier(it) }
             ?.filterNotNull()

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

@@ -20,10 +20,14 @@ 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.lexer.JetModifierKeywordToken
 import org.jetbrains.kotlin.lexer.JetTokens
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.startOffset
+import org.jetbrains.kotlin.resolve.BindingContext
 
 public open class ChangeVisibilityModifierIntention protected(
         val modifier: JetModifierKeywordToken
@@ -32,19 +36,33 @@ public open class ChangeVisibilityModifierIntention protected(
     override fun applicabilityRange(element: JetDeclaration): TextRange? {
         val modifierList = element.getModifierList()
         if (modifierList?.hasModifier(modifier) ?: false) return null
-        if (modifierList?.hasModifier(JetTokens.OVERRIDE_KEYWORD) ?: false) return null
+
+//        val descriptor = element.resolveToDescriptor() as? DeclarationDescriptorWithVisibility ?: return null
+        val bindingContext = element.analyze()
+        var descriptor = (if (element is JetPrimaryConstructor) //TODO: temporary code
+            ((element.getParent() as JetClass).resolveToDescriptor() as ClassDescriptor).getUnsubstitutedPrimaryConstructor()
+        else
+            bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, element]) as? DeclarationDescriptorWithVisibility ?: return null
+        if (descriptor is ValueParameterDescriptor) {
+            descriptor = bindingContext[BindingContext.VALUE_PARAMETER_AS_PROPERTY, descriptor] ?: return null
+        }
+        val targetVisibility = modifier.toVisibility()
+        if (descriptor.getVisibility() == targetVisibility) return null
+
+        if (modifierList?.hasModifier(JetTokens.OVERRIDE_KEYWORD) ?: false) {
+            val callableDescriptor = descriptor  as? CallableDescriptor ?: return null
+            // cannot make visibility less than (or non-comparable with) any of the supers
+            if (callableDescriptor.getOverriddenDescriptors().any { val c = Visibilities.compare(it.getVisibility(), targetVisibility); c == null || c > 0  }) return null
+        }
 
         setText(defaultText)
 
-        val modifierToChange = modifierToChange(element)
+        val modifierElement = element.visibilityModifier()
-        if (modifierToChange != null) {
+        if (modifierElement != null) {
-            return modifierToChange.getTextRange()
+            return modifierElement.getTextRange()
         }
 
-        val defaultRange = canAddVisibilityModifier(element) ?: return null
+        val defaultRange = noModifierYetApplicabilityRange(element) ?: return null
-
-        val defaultVisibility = if (element is JetPrimaryConstructor) JetTokens.PUBLIC_KEYWORD else JetTokens.INTERNAL_KEYWORD
-        if (modifier == defaultVisibility) return null
 
         if (element is JetPrimaryConstructor && defaultRange.isEmpty()) {
             setText("Make primary constructor ${modifier.getValue()}") // otherwise it may be confusing
@@ -57,30 +75,39 @@ public open class ChangeVisibilityModifierIntention protected(
     }
 
     override fun applyTo(element: JetDeclaration, editor: Editor) {
-        val modifierToChange = modifierToChange(element)
+        val modifierToChange = element.visibilityModifier()
         if (modifierToChange != null) {
-            val newModifier = JetPsiFactory(element).createModifierList(modifier).getModifier(modifier)!!
+            modifierToChange.replace(JetPsiFactory(element).createModifier(modifier))
-            modifierToChange.replace(newModifier)
         }
         else {
             element.addModifier(modifier)
         }
     }
 
-    private fun modifierToChange(element: JetDeclaration): PsiElement? {
+    private fun JetDeclaration.visibilityModifier(): PsiElement? {
-        val modifierList = element.getModifierList() ?: return null
+        val modifierList = getModifierList() ?: return null
         return JetTokens.VISIBILITY_MODIFIERS.getTypes()
                        .asSequence()
                        .map { modifierList.getModifier(it as JetModifierKeywordToken) }
                        .firstOrNull { it != null } ?: return null
     }
 
-    private fun canAddVisibilityModifier(declaration: JetDeclaration): TextRange? {
+    private fun JetModifierKeywordToken.toVisibility(): Visibility {
+        return when (this) {
+            JetTokens.PUBLIC_KEYWORD -> Visibilities.PUBLIC
+            JetTokens.PRIVATE_KEYWORD -> Visibilities.PRIVATE
+            JetTokens.PROTECTED_KEYWORD -> Visibilities.PROTECTED
+            JetTokens.INTERNAL_KEYWORD -> Visibilities.INTERNAL
+            else -> throw IllegalArgumentException("Unknown visibility modifier:$this")
+        }
+    }
+
+    private fun noModifierYetApplicabilityRange(declaration: JetDeclaration): TextRange? {
         if (JetPsiUtil.isLocal(declaration)) return null
         return when (declaration) {
             is JetNamedFunction -> declaration.getFunKeyword()?.getTextRange()
             is JetProperty -> declaration.getValOrVarNode().getTextRange()
-            is JetClass -> declaration.getClassOrTraitKeyword()?.getTextRange()
+            is JetClass -> declaration.getClassOrInterfaceKeyword()?.getTextRange()
             is JetObjectDeclaration -> declaration.getObjectKeyword().getTextRange()
             is JetPrimaryConstructor -> declaration.getValueParameterList()?.let { TextRange.from(it.startOffset, 0) } //TODO: use constructor keyword if exist
             is JetSecondaryConstructor -> declaration.getConstructorKeyword().getTextRange()

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

@@ -114,6 +114,7 @@ public object SuperClassNotInitialized : JetIntentionActionsFactory() {
         }
 
         override fun getText(): String {
+            // TODO: not too long
             return "Add constructor parameters from " +
                    superConstructor.getContainingDeclaration().getName().asString() +
                    superConstructor.getValueParameters().map { DescriptorRenderer.SHORT_NAMES_IN_TYPES.renderType(it.getType()) }.joinToString(", ", "(", ")")
@@ -132,7 +133,9 @@ public object SuperClassNotInitialized : JetIntentionActionsFactory() {
                 val parametersToAdd = ArrayList<JetParameter>()
                 for (parameter in superParameters) {
                     val name = renderer.renderName(parameter.getName())
-                    parameterNames.add(name)
+                    val varargElementType = parameter.getVarargElementType()
+
+                    parameterNames.add(if (varargElementType != null) "*$name" else name)
 
                     val nameString = parameter.getName().asString()
                     val existingParameter = oldParameters.firstOrNull { it.getName() == nameString }
@@ -141,7 +144,10 @@ public object SuperClassNotInitialized : JetIntentionActionsFactory() {
                         if (type.isSubtypeOf(parameter.getType())) continue // use existing parameter
                     }
 
-                    val parameterText = name + ":" + renderer.renderType(parameter.getType())
+                    val parameterText = if (varargElementType != null)
+                        "vararg " + name + ":" + renderer.renderType(varargElementType)
+                    else
+                        name + ":" + renderer.renderType(parameter.getType())
                     parametersToAdd.add(factory.createParameter(parameterText))
                 }
 

idea/testData/intentions/changeVisibility/internal/valParameter.kt

@@ -0,0 +1,4 @@
+// IS_APPLICABLE: false
+class C(<caret>val p: Int) {
+    public fun foo(){}
+}

idea/testData/intentions/changeVisibility/private/noModifierListInterface.kt

@@ -0,0 +1 @@
+<caret>interface I

idea/testData/intentions/changeVisibility/private/noModifierListInterface.kt.after

@@ -0,0 +1 @@
+<caret>private interface I

idea/testData/intentions/changeVisibility/private/noModifierListTrait.kt

@@ -1 +0,0 @@
-<caret>trait I

idea/testData/intentions/changeVisibility/private/noModifierListTrait.kt.after

@@ -1 +0,0 @@
-<caret>private trait I

idea/testData/intentions/changeVisibility/protected/notForInternalOverride.kt

@@ -0,0 +1,8 @@
+// IS_APPLICABLE: false
+interface I {
+    internal fun foo()
+}
+
+abstract class C : I {
+    <caret>override fun foo() {}
+}

idea/testData/intentions/changeVisibility/protected/notForOverride.kt

@@ -0,0 +1,8 @@
+// IS_APPLICABLE: false
+interface I {
+    public fun foo()
+}
+
+abstract class C : I {
+    <caret>public override fun foo() {}
+}

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

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

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

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

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

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

idea/testData/intentions/changeVisibility/public/forOverride.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/notForOverride.kt

@@ -1,8 +0,0 @@
-// IS_APPLICABLE: false
-// ERROR: Cannot weaken access privilege 'public' for 'foo' in 'T'
-trait T {
-    public fun foo()
-}
-abstract class C : T {
-    <caret>protected override fun foo() {}
-}

idea/testData/quickfix/supertypeInitialization/vararg1.kt

@@ -0,0 +1,4 @@
+// "Add constructor parameters from Base(Int, IntArray)" "true"
+open class Base(p1: Int, vararg p2: Int)
+
+class C : Base<caret>

idea/testData/quickfix/supertypeInitialization/vararg1.kt.after

@@ -0,0 +1,4 @@
+// "Add constructor parameters from Base(Int, IntArray)" "true"
+open class Base(p1: Int, vararg p2: Int)
+
+class C(p1: Int, vararg p2: Int) : Base(p1, *p2)

idea/testData/quickfix/supertypeInitialization/vararg2.kt

@@ -0,0 +1,4 @@
+// "Add constructor parameters from Base(Int, IntArray)" "true"
+open class Base(p1: Int, vararg p2: Int)
+
+class C(p2: IntArray) : Base<caret>

idea/testData/quickfix/supertypeInitialization/vararg2.kt.after

@@ -0,0 +1,4 @@
+// "Add constructor parameters from Base(Int, IntArray)" "true"
+open class Base(p1: Int, vararg p2: Int)
+
+class C(p2: IntArray, p1: Int) : Base<caret>(p1, *p2)

idea/testData/quickfix/supertypeInitialization/vararg3.kt

@@ -0,0 +1,4 @@
+// "Add constructor parameters from Base(Int, IntArray)" "true"
+open class Base(p1: Int, vararg p2: Int)
+
+class C(vararg p2: Int) : Base<caret>

idea/testData/quickfix/supertypeInitialization/vararg3.kt.after

@@ -0,0 +1,4 @@
+// "Add constructor parameters from Base(Int, IntArray)" "true"
+open class Base(p1: Int, vararg p2: Int)
+
+class C(vararg p2: Int, p1: Int) : Base<caret>(p1, *p2)

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

@@ -1963,6 +1963,12 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/internal/simple.kt");
                 doTest(fileName);
             }
+
+            @TestMetadata("valParameter.kt")
+            public void testValParameter() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/internal/valParameter.kt");
+                doTest(fileName);
+            }
         }
 
         @TestMetadata("idea/testData/intentions/changeVisibility/private")
@@ -1985,6 +1991,12 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
                 doTest(fileName);
             }
 
+            @TestMetadata("noModifierListInterface.kt")
+            public void testNoModifierListInterface() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/private/noModifierListInterface.kt");
+                doTest(fileName);
+            }
+
             @TestMetadata("noModifierListObject.kt")
             public void testNoModifierListObject() throws Exception {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/private/noModifierListObject.kt");
@@ -2003,12 +2015,6 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
                 doTest(fileName);
             }
 
-            @TestMetadata("noModifierListTrait.kt")
-            public void testNoModifierListTrait() throws Exception {
-                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/private/noModifierListTrait.kt");
-                doTest(fileName);
-            }
-
             @TestMetadata("noModifierListVal.kt")
             public void testNoModifierListVal() throws Exception {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/private/noModifierListVal.kt");
@@ -2066,18 +2072,36 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
                 doTest(fileName);
             }
 
+            @TestMetadata("notForInternalOverride.kt")
+            public void testNotForInternalOverride() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/protected/notForInternalOverride.kt");
+                doTest(fileName);
+            }
+
             @TestMetadata("notForObjectMember.kt")
             public void testNotForObjectMember() throws Exception {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/protected/notForObjectMember.kt");
                 doTest(fileName);
             }
 
+            @TestMetadata("notForOverride.kt")
+            public void testNotForOverride() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/protected/notForOverride.kt");
+                doTest(fileName);
+            }
+
             @TestMetadata("notForTopLevel.kt")
             public void testNotForTopLevel() throws Exception {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/protected/notForTopLevel.kt");
                 doTest(fileName);
             }
 
+            @TestMetadata("overrideVisibilityToDefault.kt")
+            public void testOverrideVisibilityToDefault() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/protected/overrideVisibilityToDefault.kt");
+                doTest(fileName);
+            }
+
             @TestMetadata("simple.kt")
             public void testSimple() throws Exception {
                 String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/protected/simple.kt");
@@ -2105,9 +2129,9 @@ public class IntentionTestGenerated extends AbstractIntentionTest {
                 doTest(fileName);
             }
 
-            @TestMetadata("notForOverride.kt")
+            @TestMetadata("forOverride.kt")
-            public void testNotForOverride() throws Exception {
+            public void testForOverride() throws Exception {
-                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/public/notForOverride.kt");
+                String fileName = JetTestUtils.navigationMetadata("idea/testData/intentions/changeVisibility/public/forOverride.kt");
                 doTest(fileName);
             }
 

idea/tests/org/jetbrains/kotlin/idea/quickfix/QuickFixTestGenerated.java

@@ -4943,6 +4943,24 @@ public class QuickFixTestGenerated extends AbstractQuickFixTest {
             String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/supertypeInitialization/someParametersAlreadyExist.kt");
             doTest(fileName);
         }
+
+        @TestMetadata("vararg1.kt")
+        public void testVararg1() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/supertypeInitialization/vararg1.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("vararg2.kt")
+        public void testVararg2() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/supertypeInitialization/vararg2.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("vararg3.kt")
+        public void testVararg3() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/supertypeInitialization/vararg3.kt");
+            doTest(fileName);
+        }
     }
 
     @TestMetadata("idea/testData/quickfix/suppress")