From 79ecc9751b20317149e0b16053174f6ae14a5cd2 Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Tue, 14 Jun 2016 20:08:48 +0300 Subject: [PATCH] Report error on accidentally "overriding" wait/notify Hard-code only Object members because diagnostics are reported for all other classes out of the box in 1.1 #KT-7174 Fixed --- ...FactoryForDuplicateSignatureDiagnostics.kt | 111 +++++++++++++----- .../tests/TraitOverrideObjectMethods.kt | 4 +- .../finalMembersFromBuiltIns/waitNotify.kt | 17 +++ .../finalMembersFromBuiltIns/waitNotify.txt | 19 +++ .../checkers/DiagnosticsTestGenerated.java | 15 +++ 5 files changed, 134 insertions(+), 32 deletions(-) create mode 100644 compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.kt create mode 100644 compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.txt diff --git a/compiler/backend/src/org/jetbrains/kotlin/codegen/state/BuilderFactoryForDuplicateSignatureDiagnostics.kt b/compiler/backend/src/org/jetbrains/kotlin/codegen/state/BuilderFactoryForDuplicateSignatureDiagnostics.kt index a55440b496f..01810ef2120 100644 --- a/compiler/backend/src/org/jetbrains/kotlin/codegen/state/BuilderFactoryForDuplicateSignatureDiagnostics.kt +++ b/compiler/backend/src/org/jetbrains/kotlin/codegen/state/BuilderFactoryForDuplicateSignatureDiagnostics.kt @@ -31,7 +31,6 @@ import org.jetbrains.kotlin.load.java.descriptors.getParentJavaStaticClassScope import org.jetbrains.kotlin.load.kotlin.incremental.components.IncrementalCache import org.jetbrains.kotlin.resolve.BindingContext import org.jetbrains.kotlin.resolve.DescriptorToSourceUtils -import org.jetbrains.kotlin.resolve.descriptorUtil.propertyIfAccessor import org.jetbrains.kotlin.resolve.jvm.diagnostics.* import org.jetbrains.kotlin.resolve.scopes.DescriptorKindFilter import org.jetbrains.kotlin.utils.addIfNotNull @@ -40,7 +39,18 @@ import java.util.* private val EXTERNAL_SOURCES_KINDS = arrayOf( JvmDeclarationOriginKind.DELEGATION_TO_DEFAULT_IMPLS, JvmDeclarationOriginKind.DELEGATION, - JvmDeclarationOriginKind.BRIDGE) + JvmDeclarationOriginKind.BRIDGE +) + +private val PREDEFINED_SIGNATURES = listOf( + "notify()V", + "notifyAll()V", + "wait()V", + "wait(J)V", + "wait(JI)V" +).map { signature -> + RawSignature(signature.substringBefore('('), signature.substring(signature.indexOf('(')), MemberKind.METHOD) +} class BuilderFactoryForDuplicateSignatureDiagnostics( builderFactory: ClassBuilderFactory, @@ -94,7 +104,25 @@ class BuilderFactoryForDuplicateSignatureDiagnostics( classInternalName: String, signatures: MultiMap ) { - reportDiagnosticsTasks.add { reportClashingSignaturesInHierarchy(classOrigin, classInternalName, signatures) } + reportDiagnosticsTasks.add { + reportClashingWithPredefinedSignatures(classOrigin, classInternalName, signatures) + reportClashingSignaturesInHierarchy(classOrigin, classInternalName, signatures) + } + } + + private fun reportClashingWithPredefinedSignatures( + classOrigin: JvmDeclarationOrigin, + classInternalName: String, + signatures: MultiMap + ) { + for (predefinedSignature in PREDEFINED_SIGNATURES) { + if (!signatures.containsKey(predefinedSignature)) continue + + val origins = signatures[predefinedSignature] + JvmDeclarationOrigin.NO_ORIGIN + + val diagnostic = computeDiagnosticToReport(classOrigin, classInternalName, predefinedSignature, origins) ?: continue + diagnostics.report(ErrorsJvm.CONFLICTING_INHERITED_JVM_DECLARATIONS.on(diagnostic.element, diagnostic.data)) + } } private fun reportClashingSignaturesInHierarchy( @@ -114,41 +142,64 @@ class BuilderFactoryForDuplicateSignatureDiagnostics( } } - signatures@ for ((rawSignature, origins) in groupedBySignature.entrySet()) { if (origins.size <= 1) continue - var memberElement: PsiElement? = null - var ownNonFakeCount = 0 - for (origin in origins) { - val member = origin.descriptor as? CallableMemberDescriptor? - if (member != null && member.containingDeclaration == classOrigin.descriptor && member.kind != FAKE_OVERRIDE) { - ownNonFakeCount++ - // If there's more than one real element, the clashing signature is already reported. - // Only clashes between fake overrides are interesting here - if (ownNonFakeCount > 1) continue@signatures + val diagnostic = computeDiagnosticToReport(classOrigin, classInternalName, rawSignature, origins) - if (member.kind != DELEGATION) { - // Delegates don't have declarations in the code - memberElement = origin.element ?: DescriptorToSourceUtils.descriptorToDeclaration(member) - if (memberElement == null && member is PropertyAccessorDescriptor) { - memberElement = DescriptorToSourceUtils.descriptorToDeclaration(member.correspondingProperty) - } + when (diagnostic) { + is ConflictingDeclarationError.AccidentalOverride -> { + diagnostics.report(ErrorsJvm.ACCIDENTAL_OVERRIDE.on(diagnostic.element, diagnostic.data)) + } + is ConflictingDeclarationError.ConflictingInheritedJvmDeclarations -> { + diagnostics.report(ErrorsJvm.CONFLICTING_INHERITED_JVM_DECLARATIONS.on(diagnostic.element, diagnostic.data)) + } + } + } + } + + private sealed class ConflictingDeclarationError(val element: PsiElement, val data: ConflictingJvmDeclarationsData) { + class AccidentalOverride(element: PsiElement, data: ConflictingJvmDeclarationsData) : + ConflictingDeclarationError(element, data) + class ConflictingInheritedJvmDeclarations(element: PsiElement, data: ConflictingJvmDeclarationsData) : + ConflictingDeclarationError(element, data) + } + + private fun computeDiagnosticToReport( + classOrigin: JvmDeclarationOrigin, + classInternalName: String, + rawSignature: RawSignature, + origins: Collection + ): ConflictingDeclarationError? { + var memberElement: PsiElement? = null + var ownNonFakeCount = 0 + for (origin in origins) { + val member = origin.descriptor as? CallableMemberDescriptor? + if (member != null && member.containingDeclaration == classOrigin.descriptor && member.kind != FAKE_OVERRIDE) { + ownNonFakeCount++ + // If there's more than one real element, the clashing signature is already reported. + // Only clashes between fake overrides are interesting here + if (ownNonFakeCount > 1) return null + + if (member.kind != DELEGATION) { + // Delegates don't have declarations in the code + memberElement = origin.element ?: DescriptorToSourceUtils.descriptorToDeclaration(member) + if (memberElement == null && member is PropertyAccessorDescriptor) { + memberElement = DescriptorToSourceUtils.descriptorToDeclaration(member.correspondingProperty) } } } - - val elementToReportOn = memberElement ?: classOrigin.element - if (elementToReportOn == null) return // TODO: it'd be better to report this error without any element at all - - val data = ConflictingJvmDeclarationsData(classInternalName, classOrigin, rawSignature, origins) - if (memberElement != null) { - diagnostics.report(ErrorsJvm.ACCIDENTAL_OVERRIDE.on(elementToReportOn, data)) - } - else { - diagnostics.report(ErrorsJvm.CONFLICTING_INHERITED_JVM_DECLARATIONS.on(elementToReportOn, data)) - } } + + val data = ConflictingJvmDeclarationsData(classInternalName, classOrigin, rawSignature, origins) + if (memberElement != null) { + return ConflictingDeclarationError.AccidentalOverride(memberElement, data) + } + + // TODO: it'd be better to report this error without any element at all + val elementToReportOn = classOrigin.element ?: return null + + return ConflictingDeclarationError.ConflictingInheritedJvmDeclarations(elementToReportOn, data) } private fun groupMembersDescriptorsBySignature(descriptor: ClassDescriptor): MultiMap { diff --git a/compiler/testData/diagnostics/tests/TraitOverrideObjectMethods.kt b/compiler/testData/diagnostics/tests/TraitOverrideObjectMethods.kt index 6f42ea05f9b..8eb44a1b6f2 100644 --- a/compiler/testData/diagnostics/tests/TraitOverrideObjectMethods.kt +++ b/compiler/testData/diagnostics/tests/TraitOverrideObjectMethods.kt @@ -1,5 +1,5 @@ interface MyTrait: Object { override fun toString(): String public override fun finalize() - public override fun wait() -} \ No newline at end of file + public override fun wait() +} diff --git a/compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.kt b/compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.kt new file mode 100644 index 00000000000..f3bf3c15e5c --- /dev/null +++ b/compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.kt @@ -0,0 +1,17 @@ +// !DIAGNOSTICS: -UNUSED_PARAMETER + +// KT-7174 Report error on members with the same signature as non-overridable methods from mapped Java types (like Object.wait/notify) + +class A { + fun notify() {} + fun notifyAll() {} + fun wait() {} + fun wait(l: Long) {} + fun wait(l: Long, i: Int) {} +} + +fun notify() {} +fun notifyAll() {} +fun wait() {} +fun wait(l: Long) {} +fun wait(l: Long, i: Int) {} diff --git a/compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.txt b/compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.txt new file mode 100644 index 00000000000..8edf1de0971 --- /dev/null +++ b/compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.txt @@ -0,0 +1,19 @@ +package + +public fun notify(): kotlin.Unit +public fun notifyAll(): kotlin.Unit +public fun wait(): kotlin.Unit +public fun wait(/*0*/ l: kotlin.Long): kotlin.Unit +public fun wait(/*0*/ l: kotlin.Long, /*1*/ i: kotlin.Int): kotlin.Unit + +public final class A { + public constructor A() + 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 final fun notify(): kotlin.Unit + public final fun notifyAll(): kotlin.Unit + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + public final fun wait(): kotlin.Unit + public final fun wait(/*0*/ l: kotlin.Long): kotlin.Unit + public final fun wait(/*0*/ l: kotlin.Long, /*1*/ i: kotlin.Int): kotlin.Unit +} diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java index a0dc869f8cd..08720c68125 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java @@ -5865,6 +5865,21 @@ public class DiagnosticsTestGenerated extends AbstractDiagnosticsTest { } } + @TestMetadata("compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns") + @TestDataPath("$PROJECT_ROOT") + @RunWith(JUnit3RunnerWithInners.class) + public static class FinalMembersFromBuiltIns extends AbstractDiagnosticsTest { + public void testAllFilesPresentInFinalMembersFromBuiltIns() throws Exception { + KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns"), Pattern.compile("^(.+)\\.kt$"), true); + } + + @TestMetadata("waitNotify.kt") + public void testWaitNotify() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/duplicateJvmSignature/finalMembersFromBuiltIns/waitNotify.kt"); + doTest(fileName); + } + } + @TestMetadata("compiler/testData/diagnostics/tests/duplicateJvmSignature/functionAndProperty") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class)