From 1ad4f191812e41e42a82fd63cc118b4538538feb Mon Sep 17 00:00:00 2001 From: Ilya Chernikov Date: Mon, 6 Nov 2023 11:51:07 +0100 Subject: [PATCH] Fix default positioning strategy handling that causes flaky tests because the default positioning strategy was dependent on the order of the reported messages. The code led to it was introduced in an attempt to extract common PSI-independent strategy because PSI is leaking into the abstract diagnostic infrastructure. The approach is definitely problematic, but to fix it now, the leaking dependency to the psi-based module is introduced. This should be fixed in the future by introducing better abstractions. Fixes flaky tests touched in the commit. #KT-63002 fixed --- .../raw-fir/raw-fir.common/build.gradle.kts | 1 + .../diagnostics/KtDiagnosticFactoryDsl.kt | 32 +++++++++---------- .../SourceElementPositioningStrategies.kt | 4 +-- ...bstractSourceElementPositioningStrategy.kt | 13 +------- .../annotations/AnnotatedErrorTypeRef.fir.kt | 11 ------- .../annotations/AnnotatedErrorTypeRef.fir.txt | 2 +- .../annotations/AnnotatedErrorTypeRef.kt | 1 + .../tests/generics/whereClauseSyntax.fir.kt | 6 ---- .../tests/generics/whereClauseSyntax.kt | 1 + .../fir/handlers/FirDiagnosticsHandler.kt | 4 +-- 10 files changed, 24 insertions(+), 51 deletions(-) rename compiler/{frontend.common => frontend.common-psi}/src/org/jetbrains/kotlin/diagnostics/KtDiagnosticFactoryDsl.kt (92%) delete mode 100644 compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.fir.kt delete mode 100644 compiler/testData/diagnostics/tests/generics/whereClauseSyntax.fir.kt diff --git a/compiler/fir/raw-fir/raw-fir.common/build.gradle.kts b/compiler/fir/raw-fir/raw-fir.common/build.gradle.kts index 35c8403b6de..3873201e145 100644 --- a/compiler/fir/raw-fir/raw-fir.common/build.gradle.kts +++ b/compiler/fir/raw-fir/raw-fir.common/build.gradle.kts @@ -12,6 +12,7 @@ dependencies { api(project(":compiler:fir:tree")) implementation(kotlinxCollectionsImmutable()) + implementation(project(":compiler:frontend.common-psi")) implementation(project(":compiler:psi")) compileOnly(intellijCore()) diff --git a/compiler/frontend.common/src/org/jetbrains/kotlin/diagnostics/KtDiagnosticFactoryDsl.kt b/compiler/frontend.common-psi/src/org/jetbrains/kotlin/diagnostics/KtDiagnosticFactoryDsl.kt similarity index 92% rename from compiler/frontend.common/src/org/jetbrains/kotlin/diagnostics/KtDiagnosticFactoryDsl.kt rename to compiler/frontend.common-psi/src/org/jetbrains/kotlin/diagnostics/KtDiagnosticFactoryDsl.kt index c4612ac2ee1..1e06ecb141b 100644 --- a/compiler/frontend.common/src/org/jetbrains/kotlin/diagnostics/KtDiagnosticFactoryDsl.kt +++ b/compiler/frontend.common-psi/src/org/jetbrains/kotlin/diagnostics/KtDiagnosticFactoryDsl.kt @@ -1,5 +1,5 @@ /* - * Copyright 2010-2021 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Copyright 2010-2023 JetBrains s.r.o. and Kotlin Programming Language contributors. * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. */ @@ -13,96 +13,96 @@ import kotlin.reflect.KClass import kotlin.reflect.KProperty inline fun warning0( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory0DelegateProvider { return DiagnosticFactory0DelegateProvider(Severity.WARNING, positioningStrategy, P::class) } inline fun warning1( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory1DelegateProvider { return DiagnosticFactory1DelegateProvider(Severity.WARNING, positioningStrategy, P::class) } inline fun warning2( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory2DelegateProvider { return DiagnosticFactory2DelegateProvider(Severity.WARNING, positioningStrategy, P::class) } inline fun warning3( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory3DelegateProvider { return DiagnosticFactory3DelegateProvider(Severity.WARNING, positioningStrategy, P::class) } inline fun warning4( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory4DelegateProvider { return DiagnosticFactory4DelegateProvider(Severity.WARNING, positioningStrategy, P::class) } inline fun error0( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory0DelegateProvider { return DiagnosticFactory0DelegateProvider(Severity.ERROR, positioningStrategy, P::class) } inline fun error1( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory1DelegateProvider { return DiagnosticFactory1DelegateProvider(Severity.ERROR, positioningStrategy, P::class) } inline fun error2( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory2DelegateProvider { return DiagnosticFactory2DelegateProvider(Severity.ERROR, positioningStrategy, P::class) } inline fun error3( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory3DelegateProvider { return DiagnosticFactory3DelegateProvider(Severity.ERROR, positioningStrategy, P::class) } inline fun error4( - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DiagnosticFactory4DelegateProvider { return DiagnosticFactory4DelegateProvider(Severity.ERROR, positioningStrategy, P::class) } inline fun deprecationError0( featureForError: LanguageFeature, - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DeprecationDiagnosticFactory0DelegateProvider { return DeprecationDiagnosticFactory0DelegateProvider(featureForError, positioningStrategy, P::class) } inline fun deprecationError1( featureForError: LanguageFeature, - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DeprecationDiagnosticFactory1DelegateProvider { return DeprecationDiagnosticFactory1DelegateProvider(featureForError, positioningStrategy, P::class) } inline fun deprecationError2( featureForError: LanguageFeature, - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DeprecationDiagnosticFactory2DelegateProvider { return DeprecationDiagnosticFactory2DelegateProvider(featureForError, positioningStrategy, P::class) } inline fun deprecationError3( featureForError: LanguageFeature, - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DeprecationDiagnosticFactory3DelegateProvider { return DeprecationDiagnosticFactory3DelegateProvider(featureForError, positioningStrategy, P::class) } inline fun deprecationError4( featureForError: LanguageFeature, - positioningStrategy: AbstractSourceElementPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT + positioningStrategy: AbstractSourceElementPositioningStrategy = SourceElementPositioningStrategies.DEFAULT ): DeprecationDiagnosticFactory4DelegateProvider { return DeprecationDiagnosticFactory4DelegateProvider(featureForError, positioningStrategy, P::class) } diff --git a/compiler/frontend.common-psi/src/org/jetbrains/kotlin/diagnostics/SourceElementPositioningStrategies.kt b/compiler/frontend.common-psi/src/org/jetbrains/kotlin/diagnostics/SourceElementPositioningStrategies.kt index b40008998b7..26aeb93b1e3 100644 --- a/compiler/frontend.common-psi/src/org/jetbrains/kotlin/diagnostics/SourceElementPositioningStrategies.kt +++ b/compiler/frontend.common-psi/src/org/jetbrains/kotlin/diagnostics/SourceElementPositioningStrategies.kt @@ -9,9 +9,7 @@ object SourceElementPositioningStrategies { val DEFAULT = SourceElementPositioningStrategy( LightTreePositioningStrategies.DEFAULT, PositioningStrategies.DEFAULT - ).also { - AbstractSourceElementPositioningStrategy.setDefault(it) - } + ) val VAL_OR_VAR_NODE = SourceElementPositioningStrategy( LightTreePositioningStrategies.VAL_OR_VAR_NODE, diff --git a/compiler/frontend.common/src/org/jetbrains/kotlin/diagnostics/AbstractSourceElementPositioningStrategy.kt b/compiler/frontend.common/src/org/jetbrains/kotlin/diagnostics/AbstractSourceElementPositioningStrategy.kt index ec146857d09..467f4e9c067 100644 --- a/compiler/frontend.common/src/org/jetbrains/kotlin/diagnostics/AbstractSourceElementPositioningStrategy.kt +++ b/compiler/frontend.common/src/org/jetbrains/kotlin/diagnostics/AbstractSourceElementPositioningStrategy.kt @@ -12,15 +12,4 @@ abstract class AbstractSourceElementPositioningStrategy { abstract fun markDiagnostic(diagnostic: KtDiagnostic): List abstract fun isValid(element: AbstractKtSourceElement): Boolean - - companion object { - - @JvmStatic - fun setDefault(default: AbstractSourceElementPositioningStrategy) { - DEFAULT = default - } - - var DEFAULT: AbstractSourceElementPositioningStrategy = OffsetsOnlyPositioningStrategy() - private set - } -} \ No newline at end of file +} diff --git a/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.fir.kt b/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.fir.kt deleted file mode 100644 index c056695e649..00000000000 --- a/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.fir.kt +++ /dev/null @@ -1,11 +0,0 @@ -// ISSUE: KT-62447, KT-62628 -// FIR_DUMP - -fun main() { - val x: @SinceKotlin("2.0") -} - -@Target(AnnotationTarget.TYPE) -annotation class Anno - -val prop: @Anno Foo? = null diff --git a/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.fir.txt b/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.fir.txt index b521e08321b..c701b41b291 100644 --- a/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.fir.txt +++ b/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.fir.txt @@ -1,4 +1,4 @@ -FILE: AnnotatedErrorTypeRef.fir.kt +FILE: AnnotatedErrorTypeRef.kt public final fun main(): R|kotlin/Unit| { lval x: @R|kotlin/SinceKotlin|(version = String(2.0)) } diff --git a/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.kt b/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.kt index 4eef9e7560a..9a10ed1eadc 100644 --- a/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.kt +++ b/compiler/testData/diagnostics/tests/annotations/AnnotatedErrorTypeRef.kt @@ -1,3 +1,4 @@ +// FIR_IDENTICAL // ISSUE: KT-62447, KT-62628 // FIR_DUMP diff --git a/compiler/testData/diagnostics/tests/generics/whereClauseSyntax.fir.kt b/compiler/testData/diagnostics/tests/generics/whereClauseSyntax.fir.kt deleted file mode 100644 index eee02bd01fb..00000000000 --- a/compiler/testData/diagnostics/tests/generics/whereClauseSyntax.fir.kt +++ /dev/null @@ -1,6 +0,0 @@ -// DIAGNOSTICS: -DEBUG_INFO_MISSING_UNRESOLVED -interface I -fun foo() where E: I {} -fun fooE1() where : I {} -fun fooE2() where E: {} -fun fooE3() where {} diff --git a/compiler/testData/diagnostics/tests/generics/whereClauseSyntax.kt b/compiler/testData/diagnostics/tests/generics/whereClauseSyntax.kt index f3389e6b534..1e23ce44a33 100644 --- a/compiler/testData/diagnostics/tests/generics/whereClauseSyntax.kt +++ b/compiler/testData/diagnostics/tests/generics/whereClauseSyntax.kt @@ -1,3 +1,4 @@ +// FIR_IDENTICAL // DIAGNOSTICS: -DEBUG_INFO_MISSING_UNRESOLVED interface I fun foo() where E: I {} diff --git a/compiler/tests-common-new/tests/org/jetbrains/kotlin/test/frontend/fir/handlers/FirDiagnosticsHandler.kt b/compiler/tests-common-new/tests/org/jetbrains/kotlin/test/frontend/fir/handlers/FirDiagnosticsHandler.kt index 41431a169b1..5a537659de7 100644 --- a/compiler/tests-common-new/tests/org/jetbrains/kotlin/test/frontend/fir/handlers/FirDiagnosticsHandler.kt +++ b/compiler/tests-common-new/tests/org/jetbrains/kotlin/test/frontend/fir/handlers/FirDiagnosticsHandler.kt @@ -449,7 +449,7 @@ private class DebugDiagnosticConsumer( val factory = KtDiagnosticFactory0( name = debugFactory.name, severity = debugFactory.severity, - defaultPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT, + defaultPositioningStrategy = SourceElementPositioningStrategies.DEFAULT, psiType = PsiElement::class ) @@ -490,7 +490,7 @@ private class DebugDiagnosticConsumer( val factory = KtDiagnosticFactory1( name = debugFactory.name, severity = debugFactory.severity, - defaultPositioningStrategy = AbstractSourceElementPositioningStrategy.DEFAULT, + defaultPositioningStrategy = SourceElementPositioningStrategies.DEFAULT, psiType = PsiElement::class )