From 0a5a5a2e36b8824019dd64da3fd2a030d1ba670c Mon Sep 17 00:00:00 2001 From: Mikhail Glukhikh Date: Mon, 2 Nov 2015 16:37:43 +0300 Subject: [PATCH] More precise diagnostics is added for smart cast impossible #KT-8810 Fixed --- .../jetbrains/kotlin/diagnostics/Errors.java | 2 +- .../rendering/DefaultErrorMessages.java | 2 +- .../kotlin/resolve/calls/CandidateResolver.kt | 2 +- .../resolve/calls/smartcasts/DataFlowValue.kt | 29 ++++++++++---- .../smartcasts/DataFlowValueFactory.java | 38 ++++++++++--------- .../calls/smartcasts/SmartCastManager.java | 13 +++---- .../types/expressions/DataFlowAnalyzer.java | 2 +- .../tests/smartCasts/fieldInGetter.kt | 5 +++ .../tests/smartCasts/fieldInGetter.txt | 3 ++ .../checkers/JetDiagnosticsTestGenerated.java | 6 +++ .../idea/quickfix/CastExpressionFix.java | 3 +- idea/testData/checker/infos/SmartCasts.kt | 30 ++++++++++++++- 12 files changed, 98 insertions(+), 37 deletions(-) create mode 100644 compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.kt create mode 100644 compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.txt diff --git a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java index 97757b3ac81..ceaad1bf434 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/Errors.java @@ -654,7 +654,7 @@ public interface Errors { DiagnosticFactory1 IMPLICIT_CAST_TO_UNIT_OR_ANY = DiagnosticFactory1.create(WARNING); - DiagnosticFactory2 SMARTCAST_IMPOSSIBLE = DiagnosticFactory2.create(ERROR); + DiagnosticFactory3 SMARTCAST_IMPOSSIBLE = DiagnosticFactory3.create(ERROR); DiagnosticFactory0 USELESS_NULLABLE_CHECK = DiagnosticFactory0.create(WARNING, NULLABLE_TYPE); 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 afd155c0b03..e667e423abb 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/diagnostics/rendering/DefaultErrorMessages.java @@ -477,7 +477,7 @@ public class DefaultErrorMessages { } }, DECLARATION_NAME); MAP.put(SMARTCAST_IMPOSSIBLE, - "Smart cast to ''{0}'' is impossible, because ''{1}'' could have changed since the is-check", RENDER_TYPE, STRING); + "Smart cast to ''{0}'' is impossible, because ''{1}'' is a {2}", RENDER_TYPE, STRING, STRING); MAP.put(MISSING_CONSTRUCTOR_KEYWORD, "Use 'constructor' keyword after modifiers of primary constructor"); diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/CandidateResolver.kt b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/CandidateResolver.kt index 643d25f2855..1eb0c7acccb 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/CandidateResolver.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/CandidateResolver.kt @@ -453,7 +453,7 @@ public class CandidateResolver( val expression = (receiverArgument as? ExpressionReceiver)?.expression val dataFlowValue = DataFlowValueFactory.createDataFlowValue(receiverArgument, this) - val smartCastResult = smartCastManager.checkAndRecordPossibleCast( + val smartCastResult = SmartCastManager.checkAndRecordPossibleCast( dataFlowValue, expectedReceiverParameterType, expression, this, /*recordType =*/ true ) diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/DataFlowValue.kt b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/DataFlowValue.kt index 357f4b20a48..5a31e30d703 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/DataFlowValue.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/DataFlowValue.kt @@ -26,14 +26,29 @@ import org.jetbrains.kotlin.types.KotlinType */ class DataFlowValue(val id: Any?, val type: KotlinType, val kind: DataFlowValue.Kind, val immanentNullability: Nullability) { - enum class Kind(private val str: String) { + enum class Kind(private val str: String, val description: String = str) { + // Local value, or parameter, or private / internal member value without open / custom getter, + // or protected / public member value from the same module without open / custom getter // Smart casts are completely safe STABLE_VALUE("stable"), - // Smart casts are safe but possible changes in loops / closures ahead must be taken into account - PREDICTABLE_VARIABLE("predictable"), + // Member value with open / custom getter // Smart casts are not safe - UNPREDICTABLE_VARIABLE("unpredictable"), - OTHER("other"); + MEMBER_VALUE_WITH_GETTER("custom getter", "member value that has open or custom getter"), + // Protected / public member value from another module + // Smart casts are not safe + ALIEN_PUBLIC_VALUE("alien public", "public API member value declared in different module"), + // Local variable not yet captured by a changing closure + // Smart casts are safe but possible changes in loops / closures ahead must be taken into account + PREDICTABLE_VARIABLE("predictable", "local variable that can be changed since the check in a loop"), + // Local variable already captured by a changing closure + // Smart casts are not safe + UNPREDICTABLE_VARIABLE("unpredictable", "local variable that is captured by a changing closure"), + // Member variable regardless of its visibility + // Smart casts are not safe + MEMBER_VARIABLE("member", "member variable that can be changed from another thread"), + // Some complex expression + // Smart casts are not safe + OTHER("other", "complex expression"); override fun toString() = str @@ -60,12 +75,12 @@ class DataFlowValue(val id: Any?, val type: KotlinType, val kind: DataFlowValue. } override fun toString(): String { - return kind.toString() + (id?.toString()) + " " + immanentNullability + return kind.toString() + " " + id?.toString() + " " + immanentNullability } override fun hashCode(): Int { var result = if (kind.isStable()) 1 else 0 - result = 31 * result + (type?.hashCode() ?: 0) + result = 31 * result + type.hashCode() result = 31 * result + (id?.hashCode() ?: 0) return result } diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/DataFlowValueFactory.java b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/DataFlowValueFactory.java index e02fc5ce1b3..d068166541f 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/DataFlowValueFactory.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/DataFlowValueFactory.java @@ -207,10 +207,7 @@ public class DataFlowValueFactory { return selectorInfo; } return createInfo(Pair.create(receiverInfo.id, selectorInfo.id), - receiverInfo.kind.isStable() && selectorInfo.kind.isStable() - ? STABLE_VALUE - // x.y can never be a local variable - : OTHER); + receiverInfo.kind.isStable() ? selectorInfo.kind : OTHER); } @NotNull @@ -379,16 +376,31 @@ public class DataFlowValueFactory { return true; } + private static Kind propertyKind(@NotNull PropertyDescriptor propertyDescriptor, @Nullable ModuleDescriptor usageModule) { + if (propertyDescriptor.isVar()) return MEMBER_VARIABLE; + if (!isFinal(propertyDescriptor)) return MEMBER_VALUE_WITH_GETTER; + if (!hasDefaultGetter(propertyDescriptor)) return MEMBER_VALUE_WITH_GETTER; + if (!invisibleFromOtherModules(propertyDescriptor)) { + ModuleDescriptor declarationModule = DescriptorUtils.getContainingModule(propertyDescriptor); + if (usageModule == null || !usageModule.equals(declarationModule)) { + return ALIEN_PUBLIC_VALUE; + } + } + return STABLE_VALUE; + } + private static Kind variableKind( @NotNull VariableDescriptor variableDescriptor, @Nullable ModuleDescriptor usageModule, @NotNull BindingContext bindingContext, @NotNull KtElement accessElement ) { - if (isStableValue(variableDescriptor, usageModule)) return STABLE_VALUE; - boolean isLocalVar = variableDescriptor.isVar() && variableDescriptor instanceof LocalVariableDescriptor; - if (!isLocalVar) return OTHER; - if (variableDescriptor instanceof SyntheticFieldDescriptor) return OTHER; + if (variableDescriptor instanceof PropertyDescriptor) { + return propertyKind((PropertyDescriptor) variableDescriptor, usageModule); + } + if (!(variableDescriptor instanceof LocalVariableDescriptor) && !(variableDescriptor instanceof ParameterDescriptor)) return OTHER; + if (!variableDescriptor.isVar()) return STABLE_VALUE; + if (variableDescriptor instanceof SyntheticFieldDescriptor) return MEMBER_VARIABLE; // Local variable classification: PREDICTABLE or UNPREDICTABLE PreliminaryDeclarationVisitor preliminaryVisitor = @@ -430,15 +442,7 @@ public class DataFlowValueFactory { ) { if (variableDescriptor.isVar()) return false; if (variableDescriptor instanceof PropertyDescriptor) { - PropertyDescriptor propertyDescriptor = (PropertyDescriptor) variableDescriptor; - if (!isFinal(propertyDescriptor)) return false; - if (!hasDefaultGetter(propertyDescriptor)) return false; - if (!invisibleFromOtherModules(propertyDescriptor)) { - ModuleDescriptor declarationModule = DescriptorUtils.getContainingModule(propertyDescriptor); - if (usageModule == null || !usageModule.equals(declarationModule)) { - return false; - } - } + return propertyKind((PropertyDescriptor) variableDescriptor, usageModule) == STABLE_VALUE; } return true; } diff --git a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/SmartCastManager.java b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/SmartCastManager.java index 4e6e7ce29c2..fb3683a4811 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/SmartCastManager.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/resolve/calls/smartcasts/SmartCastManager.java @@ -150,10 +150,10 @@ public class SmartCastManager { @NotNull KtExpression expression, @NotNull KotlinType type, @NotNull BindingTrace trace, - boolean canBeCast, + @NotNull DataFlowValue dataFlowValue, boolean recordExpressionType ) { - if (canBeCast) { + if (dataFlowValue.isPredictable()) { trace.record(SMARTCAST, expression, type); if (recordExpressionType) { //TODO @@ -162,12 +162,12 @@ public class SmartCastManager { } } else { - trace.report(SMARTCAST_IMPOSSIBLE.on(expression, type, expression.getText())); + trace.report(SMARTCAST_IMPOSSIBLE.on(expression, type, expression.getText(), dataFlowValue.getKind().getDescription())); } } @Nullable - public SmartCastResult checkAndRecordPossibleCast( + public static SmartCastResult checkAndRecordPossibleCast( @NotNull DataFlowValue dataFlowValue, @NotNull KotlinType expectedType, @Nullable KtExpression expression, @@ -177,7 +177,7 @@ public class SmartCastManager { for (KotlinType possibleType : c.dataFlowInfo.getPossibleTypes(dataFlowValue)) { if (ArgumentTypeResolver.isSubtypeOfForArgumentType(possibleType, expectedType)) { if (expression != null) { - recordCastOrError(expression, possibleType, c.trace, dataFlowValue.isPredictable(), recordExpressionType); + recordCastOrError(expression, possibleType, c.trace, dataFlowValue, recordExpressionType); } return new SmartCastResult(possibleType, dataFlowValue.isPredictable()); } @@ -203,8 +203,7 @@ public class SmartCastManager { if (ArgumentTypeResolver.isSubtypeOfForArgumentType(dataFlowValue.getType(), nullableExpectedType)) { if (!immanentlyNotNull) { if (expression != null) { - recordCastOrError(expression, dataFlowValue.getType(), c.trace, dataFlowValue.isPredictable(), - recordExpressionType); + recordCastOrError(expression, dataFlowValue.getType(), c.trace, dataFlowValue, recordExpressionType); } } diff --git a/compiler/frontend/src/org/jetbrains/kotlin/types/expressions/DataFlowAnalyzer.java b/compiler/frontend/src/org/jetbrains/kotlin/types/expressions/DataFlowAnalyzer.java index 21903c9d783..e3778d1a893 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/types/expressions/DataFlowAnalyzer.java +++ b/compiler/frontend/src/org/jetbrains/kotlin/types/expressions/DataFlowAnalyzer.java @@ -252,7 +252,7 @@ public class DataFlowAnalyzer { ) { DataFlowValue dataFlowValue = DataFlowValueFactory.createDataFlowValue(expression, expressionType, c); - SmartCastResult result = smartCastManager.checkAndRecordPossibleCast(dataFlowValue, c.expectedType, expression, c, false); + SmartCastResult result = SmartCastManager.checkAndRecordPossibleCast(dataFlowValue, c.expectedType, expression, c, false); return result != null ? result.getResultType() : null; } diff --git a/compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.kt b/compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.kt new file mode 100644 index 00000000000..b5255ada5a0 --- /dev/null +++ b/compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.kt @@ -0,0 +1,5 @@ +val x: Int? = 0 + get() { + if (field != null) return field.hashCode() + return null + } diff --git a/compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.txt b/compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.txt new file mode 100644 index 00000000000..4865d30ebcc --- /dev/null +++ b/compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.txt @@ -0,0 +1,3 @@ +package + +public val x: kotlin.Int? = 0 diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/JetDiagnosticsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/JetDiagnosticsTestGenerated.java index 5ac178ced29..0657557f15b 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/JetDiagnosticsTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/JetDiagnosticsTestGenerated.java @@ -14433,6 +14433,12 @@ public class JetDiagnosticsTestGenerated extends AbstractJetDiagnosticsTest { doTest(fileName); } + @TestMetadata("fieldInGetter.kt") + public void testFieldInGetter() throws Exception { + String fileName = JetTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/smartCasts/fieldInGetter.kt"); + doTest(fileName); + } + @TestMetadata("fieldPlus.kt") public void testFieldPlus() throws Exception { String fileName = JetTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/smartCasts/fieldPlus.kt"); diff --git a/idea/src/org/jetbrains/kotlin/idea/quickfix/CastExpressionFix.java b/idea/src/org/jetbrains/kotlin/idea/quickfix/CastExpressionFix.java index 2e43e67b0cd..c47341d2942 100644 --- a/idea/src/org/jetbrains/kotlin/idea/quickfix/CastExpressionFix.java +++ b/idea/src/org/jetbrains/kotlin/idea/quickfix/CastExpressionFix.java @@ -25,6 +25,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.kotlin.diagnostics.Diagnostic; import org.jetbrains.kotlin.diagnostics.DiagnosticWithParameters2; +import org.jetbrains.kotlin.diagnostics.DiagnosticWithParameters3; import org.jetbrains.kotlin.diagnostics.Errors; import org.jetbrains.kotlin.idea.KotlinBundle; import org.jetbrains.kotlin.idea.caches.resolve.ResolutionUtils; @@ -104,7 +105,7 @@ public class CastExpressionFix extends KotlinQuickFixAction { @Nullable @Override public IntentionAction createAction(@NotNull Diagnostic diagnostic) { - DiagnosticWithParameters2 diagnosticWithParameters = + DiagnosticWithParameters3 diagnosticWithParameters = Errors.SMARTCAST_IMPOSSIBLE.cast(diagnostic); return new CastExpressionFix(diagnosticWithParameters.getPsiElement(), diagnosticWithParameters.getA()); } diff --git a/idea/testData/checker/infos/SmartCasts.kt b/idea/testData/checker/infos/SmartCasts.kt index 98c6680ce1e..54812d9a126 100644 --- a/idea/testData/checker/infos/SmartCasts.kt +++ b/idea/testData/checker/infos/SmartCasts.kt @@ -216,11 +216,39 @@ fun f(): String { a = 42 a } - return a + return a } return "" } +class Mutable(var x: String?) { + + val xx: String? + get() = x + + fun foo(): String { + if (x is String) { + return x + } + if (x != null) { + // It would be better to have smart cast impossible also here + return x + } + if (xx is String) { + return xx + } + return "" + } + + fun bar(other: Mutable): String { + var y = other + if (y.x is String) { + return y.x + } + return "" + } +} + fun foo(aa: Any): Int { var a = aa if (a is Int) {