From 612baacc25065cec102f84dfc2133628b2c6b3d3 Mon Sep 17 00:00:00 2001 From: Mikhail Zarechenskiy Date: Thu, 5 Apr 2018 16:43:52 +0300 Subject: [PATCH] Make useless elvis diagnostic more consistent for new and old inference Also, remove diagnostics that can be covered by usual USELESS_ELVIS diagnostic --- .../jetbrains/kotlin/diagnostics/Errors.java | 2 - .../rendering/DefaultErrorMessages.java | 2 - .../kotlin/resolve/TargetPlatform.kt | 3 +- .../calls/checkers/UselessElvisCallChecker.kt | 50 +++++++++++++++++++ .../BasicExpressionTypingVisitor.java | 9 ---- .../callableReference/generic/specialCalls.kt | 3 +- .../controlFlowAnalysis/elvisNotProcessed.kt | 10 ++-- .../lambdasInExclExclAndElvis.kt | 2 +- .../tests/nullableTypes/uselessElvis.kt | 9 ++-- .../noWarningOnDoubleElvis.kt | 8 +++ .../noWarningOnDoubleElvis.txt | 5 ++ .../checkers/DiagnosticsTestGenerated.java | 6 +++ .../DiagnosticsUsingJavacTestGenerated.java | 6 +++ 13 files changed, 88 insertions(+), 27 deletions(-) create mode 100644 compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/checkers/UselessElvisCallChecker.kt create mode 100644 compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.kt create mode 100644 compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.txt diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java index dd4b4e1531f..f9d002994d1 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java @@ -856,8 +856,6 @@ public interface Errors { DiagnosticFactory0 NOT_NULL_ASSERTION_ON_CALLABLE_REFERENCE = DiagnosticFactory0.create(WARNING); DiagnosticFactory1 USELESS_ELVIS = DiagnosticFactory1.create(WARNING, PositioningStrategies.USELESS_ELVIS); - DiagnosticFactory0 USELESS_ELVIS_ON_LAMBDA_EXPRESSION = DiagnosticFactory0.create(WARNING); - DiagnosticFactory0 USELESS_ELVIS_ON_CALLABLE_REFERENCE = DiagnosticFactory0.create(WARNING); DiagnosticFactory0 USELESS_ELVIS_RIGHT_IS_NULL = DiagnosticFactory0.create(WARNING, PositioningStrategies.USELESS_ELVIS); 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 6dd3c1346ec..3ad253ecf16 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java @@ -509,8 +509,6 @@ public class DefaultErrorMessages { MAP.put(REPEATED_BOUND, "Type parameter already has this bound"); MAP.put(DYNAMIC_UPPER_BOUND, "Dynamic type can not be used as an upper bound"); MAP.put(USELESS_ELVIS, "Elvis operator (?:) always returns the left operand of non-nullable type {0}", RENDER_TYPE); - MAP.put(USELESS_ELVIS_ON_LAMBDA_EXPRESSION, "Left operand of elvis operator (?:) is a lambda expression"); - MAP.put(USELESS_ELVIS_ON_CALLABLE_REFERENCE, "Left operand of elvis operator (?:) is a callable reference expression"); MAP.put(USELESS_ELVIS_RIGHT_IS_NULL, "Right operand of elvis operator (?:) is useless if it is null"); MAP.put(CONFLICTING_UPPER_BOUNDS, "Upper bounds of {0} have empty intersection", NAME); diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/TargetPlatform.kt b/compiler/frontend/src/org/jetbrains/kotlin/resolve/TargetPlatform.kt index bf033e0c340..c5b1168e5c0 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/TargetPlatform.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/TargetPlatform.kt @@ -94,7 +94,8 @@ private val DEFAULT_CALL_CHECKERS = listOf( CoroutineSuspendCallChecker, BuilderFunctionsCallChecker, DslScopeViolationCallChecker, MissingDependencyClassChecker, CallableReferenceCompatibilityChecker(), LateinitIntrinsicApplicabilityChecker, UnderscoreUsageChecker, AssigningNamedArgumentToVarargChecker(), - PrimitiveNumericComparisonCallChecker, LambdaWithSuspendModifierCallChecker + PrimitiveNumericComparisonCallChecker, LambdaWithSuspendModifierCallChecker, + UselessElvisCallChecker() ) private val DEFAULT_TYPE_CHECKERS = emptyList() private val DEFAULT_CLASSIFIER_USAGE_CHECKERS = listOf( diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/checkers/UselessElvisCallChecker.kt b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/checkers/UselessElvisCallChecker.kt new file mode 100644 index 00000000000..e6e857a7f37 --- /dev/null +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/checkers/UselessElvisCallChecker.kt @@ -0,0 +1,50 @@ +/* + * Copyright 2010-2018 JetBrains s.r.o. 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.resolve.calls.checkers + +import com.intellij.psi.PsiElement +import org.jetbrains.kotlin.diagnostics.Errors +import org.jetbrains.kotlin.diagnostics.reportDiagnosticOnce +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtPsiUtil +import org.jetbrains.kotlin.resolve.calls.inference.model.TypeVariableTypeConstructor +import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall +import org.jetbrains.kotlin.resolve.calls.smartcasts.Nullability +import org.jetbrains.kotlin.types.TypeUtils +import org.jetbrains.kotlin.types.expressions.ControlStructureTypingUtils +import org.jetbrains.kotlin.types.isError +import org.jetbrains.kotlin.types.isNullabilityFlexible +import org.jetbrains.kotlin.types.typeUtil.contains + +class UselessElvisCallChecker : CallChecker { + override fun check(resolvedCall: ResolvedCall<*>, reportOn: PsiElement, context: CallCheckerContext) { + if (resolvedCall.resultingDescriptor.name != ControlStructureTypingUtils.ResolveConstruct.ELVIS.specialFunctionName) return + + val elvisBinaryExpression = resolvedCall.call.callElement as? KtBinaryExpression ?: return + val left = elvisBinaryExpression.left ?: return + val right = elvisBinaryExpression.right ?: return + + val leftType = context.trace.getType(left) ?: return + + // if type contains not fixed `TypeVariable` it means that call wasn't completed, we should wait for its completion first + if (leftType.isError || leftType.contains { it.constructor is TypeVariableTypeConstructor }) return + + if (!TypeUtils.isNullableType(leftType)) { + context.trace.reportDiagnosticOnce(Errors.USELESS_ELVIS.on(elvisBinaryExpression, leftType)) + return + } + + val dataFlowValue = context.dataFlowValueFactory.createDataFlowValue(left, leftType, context.resolutionContext) + if (context.dataFlowInfo.getStableNullability(dataFlowValue) == Nullability.NOT_NULL) { + context.trace.reportDiagnosticOnce(Errors.USELESS_ELVIS.on(elvisBinaryExpression, leftType)) + return + } + + if (KtPsiUtil.isNullConstant(right) && !leftType.isNullabilityFlexible()) { + context.trace.reportDiagnosticOnce(Errors.USELESS_ELVIS_RIGHT_IS_NULL.on(elvisBinaryExpression)) + } + } +} \ No newline at end of file diff --git a/compiler/frontend/src/org/jetbrains/kotlin/types/expressions/BasicExpressionTypingVisitor.java b/compiler/frontend/src/org/jetbrains/kotlin/types/expressions/BasicExpressionTypingVisitor.java index 2b4b4c7286c..a8a49b31c3a 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/types/expressions/BasicExpressionTypingVisitor.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/types/expressions/BasicExpressionTypingVisitor.java @@ -1254,19 +1254,10 @@ public class BasicExpressionTypingVisitor extends ExpressionTypingVisitor { boolean isLeftFunctionLiteral = ArgumentTypeResolver.isFunctionLiteralArgument(left, context); boolean isLeftCallableReference = ArgumentTypeResolver.isCallableReferenceArgument(left, context); if (leftTypeInfo == null && (isLeftFunctionLiteral || isLeftCallableReference)) { - DiagnosticFactory0 diagnosticFactory = - isLeftFunctionLiteral ? USELESS_ELVIS_ON_LAMBDA_EXPRESSION : USELESS_ELVIS_ON_CALLABLE_REFERENCE; - context.trace.report(diagnosticFactory.on(expression.getOperationReference())); return TypeInfoFactoryKt.noTypeInfo(context); } assert leftTypeInfo != null : "Left expression was not processed: " + expression; KotlinType leftType = leftTypeInfo.getType(); - if (isKnownToBeNotNull(left, leftType, context)) { - context.trace.report(USELESS_ELVIS.on(expression, leftType)); - } - else if (KtPsiUtil.isNullConstant(right) && leftType != null && !FlexibleTypesKt.isNullabilityFlexible(leftType)) { - context.trace.report(USELESS_ELVIS_RIGHT_IS_NULL.on(expression)); - } KotlinTypeInfo rightTypeInfo = BindingContextUtils.getRecordedTypeInfo(right, context.trace.getBindingContext()); if (rightTypeInfo == null && ArgumentTypeResolver.isFunctionLiteralOrCallableReference(right, context)) { // the type is computed later in call completer according to the '?:' semantics as a function diff --git a/compiler/testData/diagnostics/tests/callableReference/generic/specialCalls.kt b/compiler/testData/diagnostics/tests/callableReference/generic/specialCalls.kt index 7427ec827bf..aee3d92a0ac 100644 --- a/compiler/testData/diagnostics/tests/callableReference/generic/specialCalls.kt +++ b/compiler/testData/diagnostics/tests/callableReference/generic/specialCalls.kt @@ -1,4 +1,3 @@ -// !WITH_NEW_INFERENCE // !DIAGNOSTICS: -UNUSED_PARAMETER, -UNUSED_VARIABLE fun baz(i: Int) = i @@ -9,7 +8,7 @@ fun nullableFun(): ((Int) -> Int)? = null fun test() { val x1: (Int) -> Int = bar(if (true) ::baz else ::baz) val x2: (Int) -> Int = bar(nullableFun() ?: ::baz) - val x3: (Int) -> Int = bar(::baz ?: ::baz) + val x3: (Int) -> Int = bar(::baz ?: ::baz) val i = 0 val x4: (Int) -> Int = bar(when (i) { diff --git a/compiler/testData/diagnostics/tests/controlFlowAnalysis/elvisNotProcessed.kt b/compiler/testData/diagnostics/tests/controlFlowAnalysis/elvisNotProcessed.kt index 29e48a8d119..e17ee5b0c38 100644 --- a/compiler/testData/diagnostics/tests/controlFlowAnalysis/elvisNotProcessed.kt +++ b/compiler/testData/diagnostics/tests/controlFlowAnalysis/elvisNotProcessed.kt @@ -2,20 +2,20 @@ // See KT-8277 // NI_EXPECTED_FILE -val v = { true } ?: ( { true } ?: null!! ) +val v = { true } ?: ( { true } ?:null!! ) val w = if (true) { { true } } else { - { true } ?: null!! + { true } ?: null!! } val ww = if (true) { - { true } ?: null!! + { true } ?: null!! } else if (true) { - { true } ?: null!! + { true } ?: null!! } else { null!! @@ -34,6 +34,6 @@ val bbb = null ?: ( l() ?: null) val bbbb = ( l() ?: null) ?: ( l() ?: null) fun f(x : Long?): Long { - var a = x ?: (fun() {} ?: fun() {}) + var a = x ?: (fun() {} ?: fun() {}) return a } diff --git a/compiler/testData/diagnostics/tests/controlStructures/lambdasInExclExclAndElvis.kt b/compiler/testData/diagnostics/tests/controlStructures/lambdasInExclExclAndElvis.kt index 9de1bf0c298..b81ee7acdd5 100644 --- a/compiler/testData/diagnostics/tests/controlStructures/lambdasInExclExclAndElvis.kt +++ b/compiler/testData/diagnostics/tests/controlStructures/lambdasInExclExclAndElvis.kt @@ -9,7 +9,7 @@ fun test() { // KT-KT-9070 { } ?: 1 - use({ 2 } ?: 1); + use({ 2 } ?: 1); 1 ?: { } use(1 ?: { }) diff --git a/compiler/testData/diagnostics/tests/nullableTypes/uselessElvis.kt b/compiler/testData/diagnostics/tests/nullableTypes/uselessElvis.kt index 61b8006ba3f..2f57ac79a41 100644 --- a/compiler/testData/diagnostics/tests/nullableTypes/uselessElvis.kt +++ b/compiler/testData/diagnostics/tests/nullableTypes/uselessElvis.kt @@ -1,4 +1,3 @@ -// !WITH_NEW_INFERENCE // !DIAGNOSTICS: -UNUSED_PARAMETER, -SENSELESS_COMPARISON, -DEBUG_INFO_SMARTCAST fun test1(t: Any?): Any { @@ -23,18 +22,18 @@ fun nullable(): T? = null fun dependOn(x: T) = x fun test() { - takeNotNull(notNull() ?: "") + takeNotNull(notNull() ?: "") takeNotNull(nullable() ?: "") val x: String? = null - takeNotNull(dependOn(x) ?: "") - takeNotNull(dependOn(dependOn(x)) ?: "") + takeNotNull(dependOn(x) ?: "") + takeNotNull(dependOn(dependOn(x)) ?: "") takeNotNull(dependOn(dependOn(x as String)) ?: "") if (x != null) { takeNotNull(dependOn(x) ?: "") takeNotNull(dependOn(dependOn(x)) ?: "") - takeNotNull(dependOn(dependOn(x) as? String) ?: "") + takeNotNull(dependOn(dependOn(x) as? String) ?: "") } takeNotNull(bar()!!) diff --git a/compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.kt b/compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.kt new file mode 100644 index 00000000000..e9891546877 --- /dev/null +++ b/compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.kt @@ -0,0 +1,8 @@ +// !DIAGNOSTICS: -UNUSED_PARAMETER + +fun test() { + take(nullable() ?: nullable() ?: "foo") +} + +fun nullable(): T? = TODO() +fun take(x: Any) {} \ No newline at end of file diff --git a/compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.txt b/compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.txt new file mode 100644 index 00000000000..36182707c89 --- /dev/null +++ b/compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.txt @@ -0,0 +1,5 @@ +package + +public fun nullable(): T? +public fun take(/*0*/ x: kotlin.Any): kotlin.Unit +public fun test(): kotlin.Unit diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java index 5b43c80348a..6333822845c 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java @@ -16903,6 +16903,12 @@ public class DiagnosticsTestGenerated extends AbstractDiagnosticsTest { doTest(fileName); } + @TestMetadata("noWarningOnDoubleElvis.kt") + public void testNoWarningOnDoubleElvis() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.kt"); + doTest(fileName); + } + @TestMetadata("notNullAfterSafeCall.kt") public void testNotNullAfterSafeCall() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/notNullAfterSafeCall.kt"); diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/javac/DiagnosticsUsingJavacTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/javac/DiagnosticsUsingJavacTestGenerated.java index 0dbe7d89307..adb30e1f8f0 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/javac/DiagnosticsUsingJavacTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/javac/DiagnosticsUsingJavacTestGenerated.java @@ -16903,6 +16903,12 @@ public class DiagnosticsUsingJavacTestGenerated extends AbstractDiagnosticsUsing doTest(fileName); } + @TestMetadata("noWarningOnDoubleElvis.kt") + public void testNoWarningOnDoubleElvis() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/noWarningOnDoubleElvis.kt"); + doTest(fileName); + } + @TestMetadata("notNullAfterSafeCall.kt") public void testNotNullAfterSafeCall() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/platformTypes/nullabilityWarnings/notNullAfterSafeCall.kt");