From 14886827a21c43e7af91485b803561cada43e2d3 Mon Sep 17 00:00:00 2001 From: Mikhail Glukhikh Date: Fri, 28 Apr 2017 14:45:13 +0300 Subject: [PATCH] Introduce warning REDUNDANT_ELSE_IN_WHEN #KT-17497 Fixed --- .../cfg/ControlFlowInformationProvider.kt | 36 ++++++---- .../org/jetbrains/kotlin/cfg/WhenChecker.kt | 5 -- .../jetbrains/kotlin/diagnostics/Errors.java | 3 +- .../rendering/DefaultErrorMessages.java | 1 + .../enumInterdependence.kt | 2 +- .../diagnostics/tests/when/RedundantElse.kt | 68 ++++++++++++++++++ .../diagnostics/tests/when/RedundantElse.txt | 71 +++++++++++++++++++ .../checkers/DiagnosticsTestGenerated.java | 6 ++ 8 files changed, 172 insertions(+), 20 deletions(-) create mode 100644 compiler/testData/diagnostics/tests/when/RedundantElse.kt create mode 100644 compiler/testData/diagnostics/tests/when/RedundantElse.txt diff --git a/compiler/frontend/src/org/jetbrains/kotlin/cfg/ControlFlowInformationProvider.kt b/compiler/frontend/src/org/jetbrains/kotlin/cfg/ControlFlowInformationProvider.kt index f7711462201..5d3008b02ba 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/cfg/ControlFlowInformationProvider.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/cfg/ControlFlowInformationProvider.kt @@ -57,6 +57,7 @@ import org.jetbrains.kotlin.resolve.descriptorUtil.isEffectivelyExternal import org.jetbrains.kotlin.types.KotlinType import org.jetbrains.kotlin.types.TypeUtils.* import org.jetbrains.kotlin.types.expressions.ExpressionTypingUtils +import org.jetbrains.kotlin.types.isFlexible import org.jetbrains.kotlin.util.OperatorNameConventions import org.jetbrains.kotlin.utils.addToStdlib.safeAs import java.util.* @@ -778,25 +779,34 @@ class ControlFlowInformationProvider private constructor( for (element in instruction.owner.getValueElements(value)) { if (element !is KtWhenExpression) continue - if (element.isUsedAsExpression(trace.bindingContext)) { + val usedAsExpression = element.isUsedAsExpression(trace.bindingContext) + if (usedAsExpression) { checkImplicitCastOnConditionalExpression(element) } - if (element.elseExpression != null) continue - val context = trace.bindingContext - val necessaryCases = WhenChecker.getNecessaryCases(element, context) - if (!necessaryCases.isEmpty()) { - trace.report(NO_ELSE_IN_WHEN.on(element, necessaryCases)) + val missingCases = WhenChecker.getMissingCases(element, context) + + val elseEntry = element.entries.find { it.isElse } + val subjectExpression = element.subjectExpression + if (usedAsExpression && missingCases.isNotEmpty()) { + if (elseEntry != null) continue + trace.report(NO_ELSE_IN_WHEN.on(element, missingCases)) } - else { - val subjectExpression = element.subjectExpression - if (subjectExpression != null) { - val enumClassDescriptor = WhenChecker.getClassDescriptorOfTypeIfEnum(trace.getType(subjectExpression)) + else if (subjectExpression != null) { + val subjectType = trace.getType(subjectExpression) + if (elseEntry != null) { + if (missingCases.isEmpty() && subjectType != null && !subjectType.isFlexible()) { + trace.report(REDUNDANT_ELSE_IN_WHEN.on(elseEntry)) + } + continue + } + if (!usedAsExpression) { + val enumClassDescriptor = WhenChecker.getClassDescriptorOfTypeIfEnum(subjectType) if (enumClassDescriptor != null) { - val missingCases = WhenChecker.getEnumMissingCases(element, context, enumClassDescriptor) - if (!missingCases.isEmpty()) { - trace.report(NON_EXHAUSTIVE_WHEN.on(element, missingCases)) + val enumMissingCases = WhenChecker.getEnumMissingCases(element, context, enumClassDescriptor) + if (!enumMissingCases.isEmpty()) { + trace.report(NON_EXHAUSTIVE_WHEN.on(element, enumMissingCases)) } } } diff --git a/compiler/frontend/src/org/jetbrains/kotlin/cfg/WhenChecker.kt b/compiler/frontend/src/org/jetbrains/kotlin/cfg/WhenChecker.kt index 554e72ed3fd..602083366ec 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/cfg/WhenChecker.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/cfg/WhenChecker.kt @@ -262,11 +262,6 @@ object WhenChecker { WhenOnEnumExhaustivenessChecker, WhenOnSealedExhaustivenessChecker) - @JvmStatic - fun getNecessaryCases(expression: KtWhenExpression, context: BindingContext) = - if (expression.isUsedAsExpression(context)) getMissingCases(expression, context) - else listOf() - @JvmStatic fun isWhenByEnum(expression: KtWhenExpression, context: BindingContext) = getClassDescriptorOfTypeIfEnum(whenSubjectType(expression, context)) != null diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java index 5118884436a..abb11f97f0c 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java @@ -843,6 +843,7 @@ public interface Errors { DiagnosticFactory0 EXPECTED_CONDITION = DiagnosticFactory0.create(ERROR); DiagnosticFactory0 ELSE_MISPLACED_IN_WHEN = DiagnosticFactory0.create(ERROR, ELSE_ENTRY); + DiagnosticFactory0 REDUNDANT_ELSE_IN_WHEN = DiagnosticFactory0.create(WARNING, ELSE_ENTRY); DiagnosticFactory1> NO_ELSE_IN_WHEN = DiagnosticFactory1.create(ERROR, WHEN_EXPRESSION); DiagnosticFactory1> NON_EXHAUSTIVE_WHEN = DiagnosticFactory1.create(WARNING, WHEN_EXPRESSION); DiagnosticFactory0 COMMA_IN_WHEN_CONDITION_WITHOUT_ARGUMENT = DiagnosticFactory0.create(ERROR); @@ -932,7 +933,7 @@ public interface Errors { INVISIBLE_MEMBER, NON_PUBLIC_CALL_FROM_PUBLIC_INLINE, INVISIBLE_REFERENCE, INVISIBLE_SETTER); ImmutableSet> UNUSED_ELEMENT_DIAGNOSTICS = ImmutableSet.of( UNUSED_VARIABLE, UNUSED_PARAMETER, ASSIGNED_BUT_NEVER_ACCESSED_VARIABLE, VARIABLE_WITH_REDUNDANT_INITIALIZER, - UNUSED_LAMBDA_EXPRESSION, USELESS_CAST, UNUSED_VALUE, USELESS_ELVIS, UNNECESSARY_LATEINIT); + UNUSED_LAMBDA_EXPRESSION, USELESS_CAST, UNUSED_VALUE, USELESS_ELVIS, UNNECESSARY_LATEINIT, REDUNDANT_ELSE_IN_WHEN); ImmutableSet> TYPE_INFERENCE_ERRORS = ImmutableSet.of( TYPE_INFERENCE_NO_INFORMATION_FOR_PARAMETER, TYPE_INFERENCE_CONFLICTING_SUBSTITUTIONS, TYPE_INFERENCE_PARAMETER_CONSTRAINT_ERROR, 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 feae4e1170a..d1b7b74fe61 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java @@ -512,6 +512,7 @@ public class DefaultErrorMessages { MAP.put(NULL_FOR_NONNULL_TYPE, "Null can not be a value of a non-null type {0}", RENDER_TYPE); MAP.put(ELSE_MISPLACED_IN_WHEN, "'else' entry must be the last one in a when-expression"); + MAP.put(REDUNDANT_ELSE_IN_WHEN, "'when' is exhaustive so 'else' is redundant here"); MAP.put(COMMA_IN_WHEN_CONDITION_WITHOUT_ARGUMENT, "Deprecated syntax. Use '||' instead of commas in when-condition for 'when' without argument"); MAP.put(DUPLICATE_LABEL_IN_WHEN, "Duplicate label in when"); diff --git a/compiler/testData/diagnostics/tests/controlFlowAnalysis/enumInterdependence.kt b/compiler/testData/diagnostics/tests/controlFlowAnalysis/enumInterdependence.kt index e2ec8f0801f..e7fcec1adb2 100644 --- a/compiler/testData/diagnostics/tests/controlFlowAnalysis/enumInterdependence.kt +++ b/compiler/testData/diagnostics/tests/controlFlowAnalysis/enumInterdependence.kt @@ -30,6 +30,6 @@ enum class MyEnum { val x = when(this) { A -> 1 B -> 2 - else -> 3 + else -> 3 } } diff --git a/compiler/testData/diagnostics/tests/when/RedundantElse.kt b/compiler/testData/diagnostics/tests/when/RedundantElse.kt new file mode 100644 index 00000000000..0ae9720eb72 --- /dev/null +++ b/compiler/testData/diagnostics/tests/when/RedundantElse.kt @@ -0,0 +1,68 @@ +// FILE: MyEnum.java +public enum MyEnum { + SINGLE; + + public static MyEnum getInstance() { + return SINGLE; + } +} + + +// FILE: test.kt + +sealed class X { + class A : X() + class B : X() +} + +fun foo(x: X) = when (x) { + is X.A -> {} + is X.B -> {} + else -> {} +} + +fun bar(x: X?): String = when (x) { + is X.A -> "A" + is X.B -> "B" + null -> "null" + else -> "Unreachable" +} + +fun justUse(x: X) { + when (x) { + is X.A -> {} + is X.B -> {} + // Redundant even in statement position + else -> {} + } +} + +enum class E { + A, B +} + +fun foo(e: E): String = when (e) { + E.A -> "A" + E.B -> "B" + else -> "" +} + +fun bar(e: E?): String = when (e) { + E.A -> "A" + E.B -> "B" + else -> "" // no warning +} + +fun foo(b: Boolean) = when (b) { + true -> 1 + false -> 0 + else -> -1 +} + +fun useJava(): String { + val me = MyEnum.getInstance() + return when (me) { + MyEnum.SINGLE -> "OK" + else -> "FAIL" // no warning + } +} \ No newline at end of file diff --git a/compiler/testData/diagnostics/tests/when/RedundantElse.txt b/compiler/testData/diagnostics/tests/when/RedundantElse.txt new file mode 100644 index 00000000000..263919657b6 --- /dev/null +++ b/compiler/testData/diagnostics/tests/when/RedundantElse.txt @@ -0,0 +1,71 @@ +package + +public fun bar(/*0*/ e: E?): kotlin.String +public fun bar(/*0*/ x: X?): kotlin.String +public fun foo(/*0*/ e: E): kotlin.String +public fun foo(/*0*/ x: X): kotlin.Unit +public fun foo(/*0*/ b: kotlin.Boolean): kotlin.Int +public fun justUse(/*0*/ x: X): kotlin.Unit +public fun useJava(): kotlin.String + +public final enum class E : kotlin.Enum { + enum entry A + + enum entry B + + private constructor E() + public final override /*1*/ /*fake_override*/ val name: kotlin.String + public final override /*1*/ /*fake_override*/ val ordinal: kotlin.Int + protected final override /*1*/ /*fake_override*/ fun clone(): kotlin.Any + public final override /*1*/ /*fake_override*/ fun compareTo(/*0*/ other: E): kotlin.Int + public final override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + protected/*protected and package*/ final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun finalize(): kotlin.Unit + public final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun getDeclaringClass(): java.lang.Class! + public final override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + + // Static members + public final /*synthesized*/ fun valueOf(/*0*/ value: kotlin.String): E + public final /*synthesized*/ fun values(): kotlin.Array +} + +public final enum class MyEnum : kotlin.Enum { + enum entry SINGLE + + public constructor MyEnum() + public final override /*1*/ /*fake_override*/ val name: kotlin.String + public final override /*1*/ /*fake_override*/ val ordinal: kotlin.Int + protected final override /*1*/ /*fake_override*/ fun clone(): kotlin.Any + public final override /*1*/ /*fake_override*/ fun compareTo(/*0*/ other: MyEnum!): kotlin.Int + public final override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + protected/*protected and package*/ final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun finalize(): kotlin.Unit + public final override /*1*/ /*fake_override*/ /*isHiddenForResolutionEverywhereBesideSupercalls*/ fun getDeclaringClass(): java.lang.Class! + public final override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + + // Static members + public open fun getInstance(): MyEnum! + public final /*synthesized*/ fun valueOf(/*0*/ value: kotlin.String): MyEnum + public final /*synthesized*/ fun values(): kotlin.Array +} + +public sealed class X { + private constructor X() + 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 open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + + public final class A : X { + 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 open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + } + + public final class B : X { + public constructor B() + 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 open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + } +} diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java index 207feef67f3..a7044036aae 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java @@ -23422,6 +23422,12 @@ public class DiagnosticsTestGenerated extends AbstractDiagnosticsTest { doTest(fileName); } + @TestMetadata("RedundantElse.kt") + public void testRedundantElse() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/when/RedundantElse.kt"); + doTest(fileName); + } + @TestMetadata("ReservedExhaustiveWhen.kt") public void testReservedExhaustiveWhen() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/when/ReservedExhaustiveWhen.kt");