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

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

@@ -570,6 +570,7 @@ public interface Errors {
     DiagnosticFactory2<KtDeclaration, ClassDescriptor,
             List<Pair<CallableMemberDescriptor, Map<Incompatible, Collection<CallableMemberDescriptor>>>>> HEADER_CLASS_MEMBERS_ARE_NOT_IMPLEMENTED =
             DiagnosticFactory2.create(ERROR, DECLARATION_SIGNATURE);
+    DiagnosticFactory0<KtDeclaration> IMPL_MISSING = DiagnosticFactory0.create(ERROR, DECLARATION_SIGNATURE);
 
     ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 

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");

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<Compatibility, List<MemberDescriptor>>? {
@@ -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 -> {

compiler/testData/diagnostics/tests/multiplatform/headerClass/implDataClass.kt

@@ -25,8 +25,8 @@ header class Baz(w: List<String>) {
 // 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<String>)
+impl data class Baz impl constructor(impl val w: List<String>)

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 <!HEADER_CLASS_MEMBERS_ARE_NOT_IMPLEMENTED!>Foo<!> {
-    fun bar(): String = "bar"
+impl class Foo {
+    <!IMPL_MISSING!>fun bar(): String<!> = "bar"
 }

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

compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/common.kt

@@ -0,0 +1,3 @@
+package test
+
+header fun foo(s: String): Array<CharSequence?>

compiler/testData/multiplatform/simpleNoImplKeywordOnTopLevelFunction/jvm.kt

@@ -0,0 +1,3 @@
+package test
+
+fun foo(s: String): Array<CharSequence?> = arrayOf(s)

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<CharSequence?> = arrayOf(s)
+^

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)

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)
 

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