Only private constructors for sealed / enum classes #KT-12377 Fixed

Also #KT-8497 Fixed

compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java

@@ -215,6 +215,9 @@ public interface Errors {
 
     DiagnosticFactory0<KtElement> MISSING_CONSTRUCTOR_KEYWORD = DiagnosticFactory0.create(ERROR);
 
+    DiagnosticFactory0<PsiElement> NON_PRIVATE_CONSTRUCTOR_IN_ENUM = DiagnosticFactory0.create(ERROR);
+    DiagnosticFactory0<PsiElement> NON_PRIVATE_CONSTRUCTOR_IN_SEALED = DiagnosticFactory0.create(ERROR);
+
     // Secondary constructors
 
     DiagnosticFactory0<KtConstructorDelegationReferenceExpression> CYCLIC_CONSTRUCTOR_DELEGATION_CALL = DiagnosticFactory0.create(ERROR);

compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java

@@ -511,6 +511,9 @@ public class DefaultErrorMessages {
 
         MAP.put(MISSING_CONSTRUCTOR_KEYWORD, "Use 'constructor' keyword after modifiers of primary constructor");
 
+        MAP.put(NON_PRIVATE_CONSTRUCTOR_IN_ENUM, "Constructor must be private in enum class");
+        MAP.put(NON_PRIVATE_CONSTRUCTOR_IN_SEALED, "Constructor must be private in sealed class");
+
         MAP.put(VARIANCE_ON_TYPE_PARAMETER_NOT_ALLOWED, "Variance annotations are only allowed for type parameters of classes and interfaces");
         MAP.put(BOUND_ON_TYPE_ALIAS_PARAMETER_NOT_ALLOWED, "Bounds are not allowed on type alias parameters");
 

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

@@ -26,6 +26,7 @@ import org.jetbrains.kotlin.diagnostics.Errors
 import org.jetbrains.kotlin.diagnostics.Errors.*
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.*
+import org.jetbrains.kotlin.psi.psiUtil.visibilityModifier
 import org.jetbrains.kotlin.resolve.BindingContext.TYPE
 import org.jetbrains.kotlin.resolve.BindingContext.TYPE_PARAMETER
 import org.jetbrains.kotlin.resolve.DescriptorUtils.classCanHaveAbstractMembers
@@ -146,6 +147,20 @@ class DeclarationsChecker(
         modifiersChecker.checkModifiersForDeclaration(declaration, constructorDescriptor)
         identifierChecker.checkDeclaration(declaration, trace)
         checkVarargParameters(trace, constructorDescriptor)
+        checkConstructorVisibility(constructorDescriptor, declaration)
+    }
+
+    private fun checkConstructorVisibility(constructorDescriptor: ConstructorDescriptor, declaration: KtDeclaration) {
+        val visibilityModifier = declaration.visibilityModifier()
+        if (visibilityModifier != null && visibilityModifier.node?.elementType != KtTokens.PRIVATE_KEYWORD) {
+            val classDescriptor = constructorDescriptor.containingDeclaration
+            if (classDescriptor.kind == ClassKind.ENUM_CLASS) {
+                trace.report(NON_PRIVATE_CONSTRUCTOR_IN_ENUM.on(visibilityModifier));
+            }
+            else if (classDescriptor.modality == Modality.SEALED) {
+                trace.report(NON_PRIVATE_CONSTRUCTOR_IN_SEALED.on(visibilityModifier));
+            }
+        }
     }
 
     private fun checkModifiersAndAnnotationsInPackageDirective(file: KtFile) {

compiler/testData/codegen/box/enum/publicConstructorWithDefault.kt

@@ -1,9 +0,0 @@
-// KT-8438 org.jetbrains.org.objectweb.asm.tree.analysis.AnalyzerException: Error at instruction 4: Cannot pop operand off an empty stack
-
-enum class E public constructor(x: String = "OK") {
-    ENTRY();
-
-    val result = x
-}
-
-fun box(): String = E.ENTRY.result

compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.kt

@@ -0,0 +1,7 @@
+enum class E <!NON_PRIVATE_CONSTRUCTOR_IN_ENUM!>public<!> constructor(val x: Int) {
+    FIRST();
+
+    <!NON_PRIVATE_CONSTRUCTOR_IN_ENUM!>internal<!> constructor(): this(42)
+
+    constructor(y: Int, z: Int): this(y + z)
+}

compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.txt

@@ -0,0 +1,23 @@
+package
+
+public final enum class E : kotlin.Enum<E> {
+    enum entry FIRST
+
+    internal constructor E()
+    public constructor E(/*0*/ x: kotlin.Int)
+    private constructor E(/*0*/ y: kotlin.Int, /*1*/ z: kotlin.Int)
+    public final override /*1*/ /*fake_override*/ val name: kotlin.String
+    public final override /*1*/ /*fake_override*/ val ordinal: kotlin.Int
+    public final val x: kotlin.Int
+    protected final override /*1*/ /*fake_override*/ fun clone(): kotlin.Any
+    public final override /*1*/ /*fake_override*/ fun compareTo(/*0*/ other: E): kotlin.Int
+    public final override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean
+    protected/*protected and package*/ final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun finalize(): kotlin.Unit
+    public final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun getDeclaringClass(): java.lang.Class<E!>!
+    public final override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int
+    public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String
+
+    // Static members
+    public final /*synthesized*/ fun valueOf(/*0*/ value: kotlin.String): E
+    public final /*synthesized*/ fun values(): kotlin.Array<E>
+}

compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.kt

@@ -0,0 +1,7 @@
+sealed class Sealed <!NON_PRIVATE_CONSTRUCTOR_IN_SEALED!>protected<!> constructor(val x: Int) {
+    object FIRST : Sealed()
+
+    <!NON_PRIVATE_CONSTRUCTOR_IN_SEALED!>public<!> constructor(): this(42)
+
+    constructor(y: Int, z: Int): this(y + z)
+}

compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.txt

@@ -0,0 +1,19 @@
+package
+
+public sealed class Sealed {
+    public constructor Sealed()
+    protected constructor Sealed(/*0*/ x: kotlin.Int)
+    private constructor Sealed(/*0*/ y: kotlin.Int, /*1*/ z: kotlin.Int)
+    public final val x: kotlin.Int
+    public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean
+    public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int
+    public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String
+
+    public object FIRST : Sealed {
+        private constructor FIRST()
+        public final override /*1*/ /*fake_override*/ val x: kotlin.Int
+        public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean
+        public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int
+        public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String
+    }
+}

compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java

@@ -6117,6 +6117,12 @@ public class DiagnosticsTestGenerated extends AbstractDiagnosticsTest {
                 doTest(fileName);
             }
 
+            @TestMetadata("NonPrivateConstructor.kt")
+            public void testNonPrivateConstructor() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.kt");
+                doTest(fileName);
+            }
+
             @TestMetadata("openMemberInEnum.kt")
             public void testOpenMemberInEnum() throws Exception {
                 String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/enum/openMemberInEnum.kt");
@@ -15837,6 +15843,12 @@ public class DiagnosticsTestGenerated extends AbstractDiagnosticsTest {
                 doTest(fileName);
             }
 
+            @TestMetadata("NonPrivateConstructor.kt")
+            public void testNonPrivateConstructor() throws Exception {
+                String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.kt");
+                doTest(fileName);
+            }
+
             @TestMetadata("NotFinal.kt")
             public void testNotFinal() throws Exception {
                 String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/sealed/NotFinal.kt");

compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java

@@ -5560,12 +5560,6 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
             doTest(fileName);
         }
 
-        @TestMetadata("publicConstructorWithDefault.kt")
-        public void testPublicConstructorWithDefault() throws Exception {
-            String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/enum/publicConstructorWithDefault.kt");
-            doTest(fileName);
-        }
-
         @TestMetadata("simple.kt")
         public void testSimple() throws Exception {
             String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/enum/simple.kt");

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

@@ -198,18 +198,21 @@ fun KtModifierListOwner.setVisibility(visibilityModifier: KtModifierKeywordToken
     addModifier(visibilityModifier)
 }
 
-fun KtDeclaration.implicitVisibility(): KtModifierKeywordToken? {
+fun KtDeclaration.implicitVisibility(): KtModifierKeywordToken? =
-    val defaultVisibilityKeyword = if (hasModifier(KtTokens.OVERRIDE_KEYWORD)) {
+        if (this is KtConstructor<*>) {
-        (resolveToDescriptor() as? CallableMemberDescriptor)
+            val klass = getContainingClassOrObject()
-                ?.overriddenDescriptors
+            if (klass is KtClass && (klass.isEnum() || klass.isSealed())) KtTokens.PRIVATE_KEYWORD
-                ?.let { OverridingUtil.findMaxVisibility(it) }
+            else KtTokens.DEFAULT_VISIBILITY_KEYWORD
-                ?.toKeywordToken()
+        }
-    }
+        else if (hasModifier(KtTokens.OVERRIDE_KEYWORD)) {
-    else {
+            (resolveToDescriptor() as? CallableMemberDescriptor)
-        KtTokens.DEFAULT_VISIBILITY_KEYWORD
+                    ?.overriddenDescriptors
-    }
+                    ?.let { OverridingUtil.findMaxVisibility(it) }
-    return defaultVisibilityKeyword
+                    ?.toKeywordToken()
-}
+        }
+        else {
+            KtTokens.DEFAULT_VISIBILITY_KEYWORD
+        }
 
 fun KtModifierListOwner.canBePrivate(): Boolean {
     if (modifierList?.hasModifier(KtTokens.ABSTRACT_KEYWORD) ?: false) return false

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

@@ -124,6 +124,8 @@ class QuickFixRegistrar : QuickFixContributor {
         REDUNDANT_MODIFIER_FOR_TARGET.registerFactory(removeModifierFactory)
         WRONG_MODIFIER_CONTAINING_DECLARATION.registerFactory(removeModifierFactory)
         REPEATED_MODIFIER.registerFactory(removeModifierFactory)
+        NON_PRIVATE_CONSTRUCTOR_IN_ENUM.registerFactory(removeModifierFactory)
+        NON_PRIVATE_CONSTRUCTOR_IN_SEALED.registerFactory(removeModifierFactory)
 
         UNRESOLVED_REFERENCE.registerFactory(AutoImportFix)
         UNRESOLVED_REFERENCE.registerFactory(AddTestLibQuickFix)

idea/testData/inspections/redundantVisibilityModifier/inspectionData/expected.xml

@@ -27,4 +27,20 @@
     <problem_class severity="WARNING" attribute_key="NOT_USED_ELEMENT_ATTRIBUTES">Redundant visibility modifier</problem_class>
     <description>Redundant visibility modifier</description>
   </problem>
+  <problem>
+    <file>redundantVisibilityModifier.kt</file>
+    <line>24</line>
+    <module>light_idea_test_case</module>
+    <entry_point TYPE="file" FQNAME="temp:///src/redundantVisibilityModifier.kt" />
+    <problem_class severity="WARNING" attribute_key="NOT_USED_ELEMENT_ATTRIBUTES">Redundant visibility modifier</problem_class>
+    <description>Redundant visibility modifier</description>
+  </problem>
+  <problem>
+    <file>redundantVisibilityModifier.kt</file>
+    <line>29</line>
+    <module>light_idea_test_case</module>
+    <entry_point TYPE="file" FQNAME="temp:///src/redundantVisibilityModifier.kt" />
+    <problem_class severity="WARNING" attribute_key="NOT_USED_ELEMENT_ATTRIBUTES">Redundant visibility modifier</problem_class>
+    <description>Redundant visibility modifier</description>
+  </problem>
 </problems>

idea/testData/inspections/redundantVisibilityModifier/redundantVisibilityModifier.kt

@@ -20,3 +20,13 @@ class E : D() {
     public override fun willBecomePublic() {
     }
 }
+
+enum class F private constructor(val x: Int) {
+    FIRST(42)
+}
+
+sealed class G constructor(val y: Int) {
+    private constructor(): this(42)
+
+    object H : G()
+}