From 3bc8ca59131d9782a5d7b5baa89d2c5bed7d9dfe Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Tue, 18 Jul 2017 19:11:09 +0300 Subject: [PATCH] Report "declaration should be marked with impl" when possible Also support a quick fix to add 'impl' modifier (KT-18454), although it doesn't work yet on classes because there's no error on them in the IDE #KT-18087 Fixed #KT-18452 Fixed #KT-18454 --- .../jetbrains/kotlin/diagnostics/Errors.java | 1 + .../rendering/DefaultErrorMessages.java | 1 + .../checkers/HeaderImplDeclarationChecker.kt | 23 +++++++++++++------ .../headerClass/implDataClass.kt | 6 ++--- .../headerClass/noImplKeywordOnMember.kt | 5 ++-- .../tests/multiplatform/namedArguments.kt | 2 +- .../common.kt | 3 +++ .../jvm.kt | 3 +++ .../output.txt | 10 ++++++++ ...MultiPlatformIntegrationTestGenerated.java | 6 +++++ .../kotlin/idea/quickfix/QuickFixRegistrar.kt | 1 + .../js/src/main/kotlin/org/junit/junitTest.kt | 2 +- 12 files changed, 48 insertions(+), 15 deletions(-) create mode 100644 compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/common.kt create mode 100644 compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/jvm.kt create mode 100644 compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/output.txt diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java index 27646fe4d4e..a250c145bc4 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java @@ -570,6 +570,7 @@ public interface Errors { DiagnosticFactory2>>>> HEADER_CLASS_MEMBERS_ARE_NOT_IMPLEMENTED = DiagnosticFactory2.create(ERROR, DECLARATION_SIGNATURE); + DiagnosticFactory0 IMPL_MISSING = DiagnosticFactory0.create(ERROR, DECLARATION_SIGNATURE); //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// 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 853d756a130..b0e75570b62 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java @@ -277,6 +277,7 @@ public class DefaultErrorMessages { MAP.put(HEADER_CLASS_MEMBERS_ARE_NOT_IMPLEMENTED, "''impl'' class ''{0}'' has no implementation of ''header'' class members:{1}", NAME, IncompatibleHeaderImplClassScopesRenderer.INSTANCE); + MAP.put(IMPL_MISSING, "Declaration should be marked with 'impl' (suppress with -Xno-check-impl)"); MAP.put(PROJECTION_ON_NON_CLASS_TYPE_ARGUMENT, "Projections are not allowed on type arguments of functions and properties"); MAP.put(SUPERTYPE_NOT_INITIALIZED, "This type has a constructor, and thus must be initialized here"); diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/HeaderImplDeclarationChecker.kt b/compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/HeaderImplDeclarationChecker.kt index 2204a208ebb..f4f7e0213a8 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/HeaderImplDeclarationChecker.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/checkers/HeaderImplDeclarationChecker.kt @@ -63,11 +63,13 @@ object HeaderImplDeclarationChecker : DeclarationChecker { if (descriptor !is MemberDescriptor) return val checkImpl = !languageVersionSettings.getFlag(AnalysisFlag.multiPlatformDoNotCheckImpl) - if (descriptor.isHeader && declaration.hasModifier(KtTokens.HEADER_KEYWORD)) { - checkHeaderDeclarationHasImplementation(declaration, descriptor, diagnosticHolder, descriptor.module, checkImpl) + if (descriptor.isHeader) { + if (declaration.hasModifier(KtTokens.HEADER_KEYWORD)) { + checkHeaderDeclarationHasImplementation(declaration, descriptor, diagnosticHolder, descriptor.module, checkImpl) + } } - else if (checkImpl && descriptor.isImpl && declaration.hasModifier(KtTokens.IMPL_KEYWORD)) { - checkImplementationHasHeaderDeclaration(declaration, descriptor, diagnosticHolder) + else { + checkImplementationHasHeaderDeclaration(declaration, descriptor, diagnosticHolder, checkImpl) } } @@ -123,13 +125,20 @@ object HeaderImplDeclarationChecker : DeclarationChecker { } private fun checkImplementationHasHeaderDeclaration( - reportOn: KtDeclaration, descriptor: MemberDescriptor, diagnosticHolder: DiagnosticSink + reportOn: KtDeclaration, descriptor: MemberDescriptor, diagnosticHolder: DiagnosticSink, checkImpl: Boolean ) { // Using the platform module instead of the common module is sort of fine here because the former always depends on the latter. // However, it would be clearer to find the common module this platform module implements and look for headers there instead. // TODO: use common module here val compatibility = findHeaderForImpl(descriptor, descriptor.module) ?: return + if (!descriptor.isImpl || !reportOn.hasModifier(KtTokens.IMPL_KEYWORD)) { + if (checkImpl && (Compatible in compatibility || Incompatible.NoImpl in compatibility)) { + diagnosticHolder.report(Errors.IMPL_MISSING.on(reportOn)) + } + return + } + // 'firstOrNull' is needed because in diagnostic tests, common sources appear twice, so the same class is duplicated // TODO: replace with 'singleOrNull' as soon as multi-module diagnostic tests are refactored val singleIncompatibility = compatibility.keys.firstOrNull() @@ -173,7 +182,7 @@ object HeaderImplDeclarationChecker : DeclarationChecker { DescriptorToSourceUtils.getSourceFromDescriptor(this) is KtConstructor<*> } else { - isImpl && kind == CallableMemberDescriptor.Kind.DECLARATION + kind == CallableMemberDescriptor.Kind.DECLARATION } private fun findHeaderForImpl(impl: MemberDescriptor, commonModule: ModuleDescriptor): Map>? { @@ -201,7 +210,7 @@ object HeaderImplDeclarationChecker : DeclarationChecker { Substitutor(headerClass.declaredTypeParameters, container.declaredTypeParameters) } else null - areCompatibleCallables(declaration, impl, checkImpl = false, parentSubstitutor = substitutor) + areCompatibleCallables(declaration, impl, checkImpl = true, parentSubstitutor = substitutor) } } is ClassifierDescriptorWithTypeParameters -> { diff --git a/compiler/testData/diagnostics/tests/multiplatform/headerClass/implDataClass.kt b/compiler/testData/diagnostics/tests/multiplatform/headerClass/implDataClass.kt index c69b286c3dc..ed270f519f8 100644 --- a/compiler/testData/diagnostics/tests/multiplatform/headerClass/implDataClass.kt +++ b/compiler/testData/diagnostics/tests/multiplatform/headerClass/implDataClass.kt @@ -25,8 +25,8 @@ header class Baz(w: List) { // MODULE: m2-jvm(m1-common) // FILE: jvm.kt -impl data class Foo(impl val x: Int, impl val y: String) +impl data class Foo impl constructor(impl val x: Int, impl val y: String) -impl data class Bar(val z: Double) +impl data class Bar impl constructor(val z: Double) -impl data class Baz(impl val w: List) +impl data class Baz impl constructor(impl val w: List) diff --git a/compiler/testData/diagnostics/tests/multiplatform/headerClass/noImplKeywordOnMember.kt b/compiler/testData/diagnostics/tests/multiplatform/headerClass/noImplKeywordOnMember.kt index 9045851d132..7309bdf610f 100644 --- a/compiler/testData/diagnostics/tests/multiplatform/headerClass/noImplKeywordOnMember.kt +++ b/compiler/testData/diagnostics/tests/multiplatform/headerClass/noImplKeywordOnMember.kt @@ -9,7 +9,6 @@ header class Foo { // MODULE: m2-jvm(m1-common) // FILE: jvm.kt -// TODO: run HeaderImplDeclarationChecker on non-impl members of impl classes, and report something like "impl expected" on 'bar' instead -impl class Foo { - fun bar(): String = "bar" +impl class Foo { + fun bar(): String = "bar" } diff --git a/compiler/testData/diagnostics/tests/multiplatform/namedArguments.kt b/compiler/testData/diagnostics/tests/multiplatform/namedArguments.kt index 5320224993e..2a968d2dec1 100644 --- a/compiler/testData/diagnostics/tests/multiplatform/namedArguments.kt +++ b/compiler/testData/diagnostics/tests/multiplatform/namedArguments.kt @@ -20,7 +20,7 @@ fun testCommon() { // MODULE: m2-jvm(m1-common) // FILE: jvm.kt -impl class Foo(val aaa: Boolean) { +impl class Foo impl constructor(val aaa: Boolean) { impl constructor(zzz: Int) : this(zzz == 0) impl fun f1(xxx: String) = xxx diff --git a/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/common.kt b/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/common.kt new file mode 100644 index 00000000000..ca0a00ce950 --- /dev/null +++ b/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/common.kt @@ -0,0 +1,3 @@ +package test + +header fun foo(s: String): Array diff --git a/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/jvm.kt b/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/jvm.kt new file mode 100644 index 00000000000..5d3bf12c7d3 --- /dev/null +++ b/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/jvm.kt @@ -0,0 +1,3 @@ +package test + +fun foo(s: String): Array = arrayOf(s) diff --git a/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/output.txt b/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/output.txt new file mode 100644 index 00000000000..af4ba44896f --- /dev/null +++ b/compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/output.txt @@ -0,0 +1,10 @@ +-- Common -- +Exit code: OK +Output: + +-- JVM -- +Exit code: COMPILATION_ERROR +Output: +compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/jvm.kt:3:1: error: declaration should be marked with 'impl' (suppress with -Xno-check-impl) +fun foo(s: String): Array = arrayOf(s) +^ diff --git a/compiler/tests/org/jetbrains/kotlin/multiplatform/MultiPlatformIntegrationTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/multiplatform/MultiPlatformIntegrationTestGenerated.java index 24b5ac8a27d..5ae511958d2 100644 --- a/compiler/tests/org/jetbrains/kotlin/multiplatform/MultiPlatformIntegrationTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/multiplatform/MultiPlatformIntegrationTestGenerated.java @@ -108,6 +108,12 @@ public class MultiPlatformIntegrationTestGenerated extends AbstractMultiPlatform doTest(fileName); } + @TestMetadata("simpleNoImplKeywordOnTopLevelFunction") + public void testSimpleNoImplKeywordOnTopLevelFunction() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/"); + doTest(fileName); + } + @TestMetadata("compiler/testData/multiplatform/classScopes") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class) diff --git a/idea/src/org/jetbrains/kotlin/idea/quickfix/QuickFixRegistrar.kt b/idea/src/org/jetbrains/kotlin/idea/quickfix/QuickFixRegistrar.kt index 3884b9dcea8..537d593778b 100644 --- a/idea/src/org/jetbrains/kotlin/idea/quickfix/QuickFixRegistrar.kt +++ b/idea/src/org/jetbrains/kotlin/idea/quickfix/QuickFixRegistrar.kt @@ -477,6 +477,7 @@ class QuickFixRegistrar : QuickFixContributor { OVERLOADS_WITHOUT_DEFAULT_ARGUMENTS.registerFactory(RemoveAnnotationFix.JvmOverloads) HEADER_WITHOUT_IMPLEMENTATION.registerFactory(CreateHeaderImplementationFix) + IMPL_MISSING.registerFactory(AddModifierFix.createFactory(KtTokens.IMPL_KEYWORD)) CAST_NEVER_SUCCEEDS.registerFactory(ReplacePrimitiveCastWithNumberConversionFix) diff --git a/libraries/kotlin.test/js/src/main/kotlin/org/junit/junitTest.kt b/libraries/kotlin.test/js/src/main/kotlin/org/junit/junitTest.kt index e0df862fa6b..e6d8aa6e468 100644 --- a/libraries/kotlin.test/js/src/main/kotlin/org/junit/junitTest.kt +++ b/libraries/kotlin.test/js/src/main/kotlin/org/junit/junitTest.kt @@ -3,4 +3,4 @@ package org.junit @Deprecated("Use 'Test' from kotlin.test package", replaceWith = ReplaceWith("Test", "kotlin.test.Test"), level = DeprecationLevel.WARNING) -typealias Test = kotlin.test.Test +impl typealias Test = kotlin.test.Test