From 92d8da621e6d9a7e2f43f9cf4136902cc24b7b6d Mon Sep 17 00:00:00 2001 From: Nikolay Lunyak Date: Wed, 28 Feb 2024 15:43:13 +0200 Subject: [PATCH] [FIR] Merge the two type operator checkers into a single one This way it's easier to reason about where useless casts/is checks come from, because everything is on the same screen. `USELESS_CAST` disappeared from `FirPsiJsOldFrontendDiagnosticsTestGenerated.testDynamicCastTarget` because `LanguageFeature.EnableDfaWarningsInK2` is disabled, and previously it only affected `FirCastOperatorsChecker`, but not `FirUselessTypeOperationCallChecker`, which felt like an unintended mistake. A related issue: KT-50965 --- .../checkers/CommonExpressionCheckers.kt | 1 - .../checkers/FirCastDiagnosticsHelpers.kt | 11 +++-- .../expression/FirCastOperatorsChecker.kt | 16 ++++--- .../FirUselessTypeOperationCallChecker.kt | 47 ------------------- .../dynamicTypes/dynamicCastTarget.fir.kt | 8 ++-- 5 files changed, 22 insertions(+), 61 deletions(-) delete mode 100644 compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirUselessTypeOperationCallChecker.kt diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/CommonExpressionCheckers.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/CommonExpressionCheckers.kt index 95fe39a9aa2..5b483e4a7b3 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/CommonExpressionCheckers.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/CommonExpressionCheckers.kt @@ -154,7 +154,6 @@ object CommonExpressionCheckers : ExpressionCheckers() { override val typeOperatorCallCheckers: Set get() = setOf( - FirUselessTypeOperationCallChecker, FirCastOperatorsChecker ) diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/FirCastDiagnosticsHelpers.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/FirCastDiagnosticsHelpers.kt index 689011a4e69..f0576f71958 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/FirCastDiagnosticsHelpers.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/FirCastDiagnosticsHelpers.kt @@ -182,11 +182,16 @@ fun isUpcast(context: CheckerContext, candidateType: ConeKotlinType, targetType: internal fun isRefinementUseless( context: CheckerContext, - lhsType: ConeSimpleKotlinType, + lhsType: ConeKotlinType, targetType: ConeKotlinType, expression: FirTypeOperatorCall, - arg: FirExpression, ): Boolean { + if (lhsType is ConeErrorType || targetType is ConeErrorType) { + return false + } + + val arg = expression.argument + return when (expression.operation) { FirOperation.AS, FirOperation.SAFE_AS -> { if (arg is FirFunctionCall) { @@ -211,7 +216,7 @@ internal fun isRefinementUseless( } } -private fun isExactTypeCast(context: CheckerContext, lhsType: ConeSimpleKotlinType, targetType: ConeKotlinType): Boolean { +private fun isExactTypeCast(context: CheckerContext, lhsType: ConeKotlinType, targetType: ConeKotlinType): Boolean { if (!AbstractTypeChecker.equalTypes(context.session.typeContext, lhsType, targetType, stubTypesEqualToAnything = false)) return false // See comments at [isUpcast] why we need to check the existence of @ExtensionFunctionType diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirCastOperatorsChecker.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirCastOperatorsChecker.kt index fe231d829be..357e2a6c0af 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirCastOperatorsChecker.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirCastOperatorsChecker.kt @@ -45,10 +45,9 @@ object FirCastOperatorsChecker : FirTypeOperatorCallChecker(MppCheckerKind.Commo } } - // IDE doesn't care this function is referenced as :: and then used polymorphicly - @Suppress("UNUSED_PARAMETER") private fun checkIsApplicability(l: TypeInfo, r: TypeInfo, expression: FirTypeOperatorCall, context: CheckerContext): Applicability { return when { + isRefinementUseless(context, l.directType.upperBoundIfFlexible(), r.directType, expression) -> Applicability.USELESS_IS_CHECK isCastErased(l.directType, r.directType, context) -> Applicability.CAST_ERASED else -> Applicability.APPLICABLE } @@ -61,11 +60,12 @@ object FirCastOperatorsChecker : FirTypeOperatorCallChecker(MppCheckerKind.Commo || l.type.isNullableNothing && !r.type.isNullable return when { + isRefinementUseless(context, l.directType.upperBoundIfFlexible(), r.directType, expression) -> Applicability.USELESS_CAST l.type.isNothing && r.type.isNothingOrNullableNothing -> Applicability.APPLICABLE r.type.isNothing -> Applicability.IMPOSSIBLE isNullableNothingWithNotNull -> when (expression.operation) { // (null as? WhatEver) == null - FirOperation.SAFE_AS -> Applicability.USELESS + FirOperation.SAFE_AS -> Applicability.USELESS_CAST else -> Applicability.IMPOSSIBLE } oneIsNotNull && oneIsFinal && areUnrelated(l, r, context) -> Applicability.IMPOSSIBLE @@ -92,7 +92,8 @@ object FirCastOperatorsChecker : FirTypeOperatorCallChecker(MppCheckerKind.Commo private enum class Applicability { APPLICABLE, IMPOSSIBLE, - USELESS, + USELESS_CAST, + USELESS_IS_CHECK, CAST_ERASED, } @@ -114,9 +115,12 @@ object FirCastOperatorsChecker : FirTypeOperatorCallChecker(MppCheckerKind.Commo Applicability.IMPOSSIBLE -> getImpossibilityDiagnostic(l, r, context)?.let { reportOn(expression.source, it, context) } - Applicability.USELESS -> getUselessnessDiagnostic(context)?.let { + Applicability.USELESS_CAST -> getUselessCastDiagnostic(context)?.let { reportOn(expression.source, it, context) } + Applicability.USELESS_IS_CHECK -> reportOn( + expression.source, FirErrors.USELESS_IS_CHECK, expression.operation == FirOperation.IS, context, + ) Applicability.CAST_ERASED -> when { expression.operation == FirOperation.AS || expression.operation == FirOperation.SAFE_AS -> { reportOn(expression.source, FirErrors.UNCHECKED_CAST, lUserType, rUserType, context) @@ -133,7 +137,7 @@ object FirCastOperatorsChecker : FirTypeOperatorCallChecker(MppCheckerKind.Commo else -> FirErrors.CAST_NEVER_SUCCEEDS } - private fun getUselessnessDiagnostic(context: CheckerContext) = when { + private fun getUselessCastDiagnostic(context: CheckerContext) = when { !context.languageVersionSettings.supportsFeature(LanguageFeature.EnableDfaWarningsInK2) -> null else -> FirErrors.USELESS_CAST } diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirUselessTypeOperationCallChecker.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirUselessTypeOperationCallChecker.kt deleted file mode 100644 index b714992e37d..00000000000 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirUselessTypeOperationCallChecker.kt +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Copyright 2010-2021 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. - */ - -package org.jetbrains.kotlin.fir.analysis.checkers.expression - -import org.jetbrains.kotlin.diagnostics.DiagnosticReporter -import org.jetbrains.kotlin.diagnostics.reportOn -import org.jetbrains.kotlin.fir.analysis.checkers.MppCheckerKind -import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext -import org.jetbrains.kotlin.fir.analysis.checkers.isRefinementUseless -import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors -import org.jetbrains.kotlin.fir.expressions.FirOperation -import org.jetbrains.kotlin.fir.expressions.FirTypeOperatorCall -import org.jetbrains.kotlin.fir.expressions.argument -import org.jetbrains.kotlin.fir.resolve.fullyExpandedType -import org.jetbrains.kotlin.fir.types.ConeErrorType -import org.jetbrains.kotlin.fir.types.coneType -import org.jetbrains.kotlin.fir.types.resolvedType -import org.jetbrains.kotlin.fir.types.upperBoundIfFlexible - -object FirUselessTypeOperationCallChecker : FirTypeOperatorCallChecker(MppCheckerKind.Common) { - override fun check(expression: FirTypeOperatorCall, context: CheckerContext, reporter: DiagnosticReporter) { - if (expression.operation !in FirOperation.TYPES) return - val arg = expression.argument - - val lhsType = arg.resolvedType.upperBoundIfFlexible().fullyExpandedType(context.session) - if (lhsType is ConeErrorType) return - - val targetType = expression.conversionTypeRef.coneType.fullyExpandedType(context.session) - if (targetType is ConeErrorType) return - - if (isRefinementUseless(context, lhsType, targetType, expression, arg)) { - when (expression.operation) { - FirOperation.IS -> reporter.reportOn(expression.source, FirErrors.USELESS_IS_CHECK, true, context) - FirOperation.NOT_IS -> reporter.reportOn(expression.source, FirErrors.USELESS_IS_CHECK, false, context) - FirOperation.AS, FirOperation.SAFE_AS -> { - if (!expression.argFromStubType) { - reporter.reportOn(expression.source, FirErrors.USELESS_CAST, context) - } - } - else -> throw AssertionError("Should not be here: ${expression.operation}") - } - } - } -} diff --git a/compiler/testData/diagnostics/testsWithJsStdLib/dynamicTypes/dynamicCastTarget.fir.kt b/compiler/testData/diagnostics/testsWithJsStdLib/dynamicTypes/dynamicCastTarget.fir.kt index ad2dbb0d610..b7458e5a7dc 100644 --- a/compiler/testData/diagnostics/testsWithJsStdLib/dynamicTypes/dynamicCastTarget.fir.kt +++ b/compiler/testData/diagnostics/testsWithJsStdLib/dynamicTypes/dynamicCastTarget.fir.kt @@ -1,11 +1,11 @@ // !DIAGNOSTICS: -REDUNDANT_NULLABLE fun test(d: Any, dl: Collection) { - d as dynamic - d as dynamic? + d as dynamic + d as dynamic? - d as? dynamic - d as? dynamic? + d as? dynamic + d as? dynamic? d is dynamic d is dynamic?