From 1a8181bdc4f9f467e2898fa47b2a1b7e0a3fa5bb Mon Sep 17 00:00:00 2001 From: Mikhail Glukhikh Date: Wed, 18 May 2016 17:02:50 +0300 Subject: [PATCH] Only private constructors for sealed / enum classes #KT-12377 Fixed Also #KT-8497 Fixed --- .../jetbrains/kotlin/diagnostics/Errors.java | 3 +++ .../rendering/DefaultErrorMessages.java | 3 +++ .../kotlin/resolve/DeclarationsChecker.kt | 15 +++++++++++ .../box/enum/publicConstructorWithDefault.kt | 9 ------- .../tests/enum/NonPrivateConstructor.kt | 7 +++++ .../tests/enum/NonPrivateConstructor.txt | 23 ++++++++++++++++ .../tests/sealed/NonPrivateConstructor.kt | 7 +++++ .../tests/sealed/NonPrivateConstructor.txt | 19 +++++++++++++ .../checkers/DiagnosticsTestGenerated.java | 12 +++++++++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 ----- .../kotlin/idea/core/psiModificationUtils.kt | 27 ++++++++++--------- .../kotlin/idea/quickfix/QuickFixRegistrar.kt | 2 ++ .../inspectionData/expected.xml | 16 +++++++++++ .../redundantVisibilityModifier.kt | 10 +++++++ 14 files changed, 132 insertions(+), 27 deletions(-) delete mode 100644 compiler/testData/codegen/box/enum/publicConstructorWithDefault.kt create mode 100644 compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.kt create mode 100644 compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.txt create mode 100644 compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.kt create mode 100644 compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.txt diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java index 5e6104ec522..8475dd3f419 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java @@ -215,6 +215,9 @@ public interface Errors { DiagnosticFactory0 MISSING_CONSTRUCTOR_KEYWORD = DiagnosticFactory0.create(ERROR); + DiagnosticFactory0 NON_PRIVATE_CONSTRUCTOR_IN_ENUM = DiagnosticFactory0.create(ERROR); + DiagnosticFactory0 NON_PRIVATE_CONSTRUCTOR_IN_SEALED = DiagnosticFactory0.create(ERROR); + // Secondary constructors DiagnosticFactory0 CYCLIC_CONSTRUCTOR_DELEGATION_CALL = DiagnosticFactory0.create(ERROR); diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java index a05b0413a43..a998c81daf3 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java +++ b/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"); diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/DeclarationsChecker.kt b/compiler/frontend/src/org/jetbrains/kotlin/resolve/DeclarationsChecker.kt index 52dd26f0792..194fcdef296 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/DeclarationsChecker.kt +++ b/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) { diff --git a/compiler/testData/codegen/box/enum/publicConstructorWithDefault.kt b/compiler/testData/codegen/box/enum/publicConstructorWithDefault.kt deleted file mode 100644 index be8a43b12b5..00000000000 --- a/compiler/testData/codegen/box/enum/publicConstructorWithDefault.kt +++ /dev/null @@ -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 diff --git a/compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.kt b/compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.kt new file mode 100644 index 00000000000..547fb3f1723 --- /dev/null +++ b/compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.kt @@ -0,0 +1,7 @@ +enum class E public constructor(val x: Int) { + FIRST(); + + internal constructor(): this(42) + + constructor(y: Int, z: Int): this(y + z) +} diff --git a/compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.txt b/compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.txt new file mode 100644 index 00000000000..b83ab209ebd --- /dev/null +++ b/compiler/testData/diagnostics/tests/enum/NonPrivateConstructor.txt @@ -0,0 +1,23 @@ +package + +public final enum class E : kotlin.Enum { + 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! + 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 +} diff --git a/compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.kt b/compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.kt new file mode 100644 index 00000000000..97fb24a6e2c --- /dev/null +++ b/compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.kt @@ -0,0 +1,7 @@ +sealed class Sealed protected constructor(val x: Int) { + object FIRST : Sealed() + + public constructor(): this(42) + + constructor(y: Int, z: Int): this(y + z) +} diff --git a/compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.txt b/compiler/testData/diagnostics/tests/sealed/NonPrivateConstructor.txt new file mode 100644 index 00000000000..0a27f00ee97 --- /dev/null +++ b/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 + } +} diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java index e675f092499..aafbfad6166 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java +++ b/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"); diff --git a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java index b71bc189b52..b627d4e3933 100644 --- a/compiler/tests/org/jetbrains/kotlin/codegen/BlackBoxCodegenTestGenerated.java +++ b/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"); diff --git a/idea/idea-core/src/org/jetbrains/kotlin/idea/core/psiModificationUtils.kt b/idea/idea-core/src/org/jetbrains/kotlin/idea/core/psiModificationUtils.kt index 51a9cf8cfd6..27e5acd1136 100644 --- a/idea/idea-core/src/org/jetbrains/kotlin/idea/core/psiModificationUtils.kt +++ b/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? { - val defaultVisibilityKeyword = if (hasModifier(KtTokens.OVERRIDE_KEYWORD)) { - (resolveToDescriptor() as? CallableMemberDescriptor) - ?.overriddenDescriptors - ?.let { OverridingUtil.findMaxVisibility(it) } - ?.toKeywordToken() - } - else { - KtTokens.DEFAULT_VISIBILITY_KEYWORD - } - return defaultVisibilityKeyword -} +fun KtDeclaration.implicitVisibility(): KtModifierKeywordToken? = + if (this is KtConstructor<*>) { + val klass = getContainingClassOrObject() + if (klass is KtClass && (klass.isEnum() || klass.isSealed())) KtTokens.PRIVATE_KEYWORD + else KtTokens.DEFAULT_VISIBILITY_KEYWORD + } + else if (hasModifier(KtTokens.OVERRIDE_KEYWORD)) { + (resolveToDescriptor() as? CallableMemberDescriptor) + ?.overriddenDescriptors + ?.let { OverridingUtil.findMaxVisibility(it) } + ?.toKeywordToken() + } + else { + KtTokens.DEFAULT_VISIBILITY_KEYWORD + } fun KtModifierListOwner.canBePrivate(): Boolean { if (modifierList?.hasModifier(KtTokens.ABSTRACT_KEYWORD) ?: false) return false diff --git a/idea/src/org/jetbrains/kotlin/idea/quickfix/QuickFixRegistrar.kt b/idea/src/org/jetbrains/kotlin/idea/quickfix/QuickFixRegistrar.kt index 216c42ae69f..2de775f1105 100644 --- a/idea/src/org/jetbrains/kotlin/idea/quickfix/QuickFixRegistrar.kt +++ b/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) diff --git a/idea/testData/inspections/redundantVisibilityModifier/inspectionData/expected.xml b/idea/testData/inspections/redundantVisibilityModifier/inspectionData/expected.xml index 62cb5ff4b2e..446bcf69b1d 100644 --- a/idea/testData/inspections/redundantVisibilityModifier/inspectionData/expected.xml +++ b/idea/testData/inspections/redundantVisibilityModifier/inspectionData/expected.xml @@ -27,4 +27,20 @@ Redundant visibility modifier Redundant visibility modifier + + redundantVisibilityModifier.kt + 24 + light_idea_test_case + + Redundant visibility modifier + Redundant visibility modifier + + + redundantVisibilityModifier.kt + 29 + light_idea_test_case + + Redundant visibility modifier + Redundant visibility modifier + diff --git a/idea/testData/inspections/redundantVisibilityModifier/redundantVisibilityModifier.kt b/idea/testData/inspections/redundantVisibilityModifier/redundantVisibilityModifier.kt index fd2bb8c5f3a..2b48e98b44a 100644 --- a/idea/testData/inspections/redundantVisibilityModifier/redundantVisibilityModifier.kt +++ b/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() +}