From 5cc08fb314f37b4e178a1743ada730b5b042bbb6 Mon Sep 17 00:00:00 2001 From: pyos Date: Mon, 14 Nov 2022 20:45:18 +0100 Subject: [PATCH] FIR DFA: check for reassignments of LHS when handling `?.`. `x?.y != null` does not imply that `x != null` if e.g. an argument to `y` has reassigned `x` in the meantime. The same is true for `x == y` and `functionWithContract(x, y)`, but those are somewhat harder to implement since there is no easy way to find the last node of a certain argument. ^KT-55096 --- ...CompilerTestFE10TestdataTestGenerated.java | 6 +++ ...irOldFrontendDiagnosticsTestGenerated.java | 6 +++ ...DiagnosticsWithLightTreeTestGenerated.java | 6 +++ .../analysis/cfa/FirReturnsImpliesAnalyzer.kt | 16 +++--- .../fir/resolve/dfa/FirDataFlowAnalyzer.kt | 30 +++++++---- .../jetbrains/kotlin/fir/resolve/dfa/Flow.kt | 2 +- .../kotlin/fir/resolve/dfa/LogicSystem.kt | 1 + .../fir/resolve/dfa/PersistentLogicSystem.kt | 11 ++-- .../variables/reassignedInRhs.fir.kt | 51 +++++++++++++++++++ .../smartCasts/variables/reassignedInRhs.kt | 51 +++++++++++++++++++ .../test/runners/DiagnosticTestGenerated.java | 6 +++ 11 files changed, 164 insertions(+), 22 deletions(-) create mode 100644 compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.fir.kt create mode 100644 compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt diff --git a/analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/diagnostic/compiler/based/DiagnosisCompilerTestFE10TestdataTestGenerated.java b/analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/diagnostic/compiler/based/DiagnosisCompilerTestFE10TestdataTestGenerated.java index 6d563b93e78..ed1e60219ee 100644 --- a/analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/diagnostic/compiler/based/DiagnosisCompilerTestFE10TestdataTestGenerated.java +++ b/analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/diagnostic/compiler/based/DiagnosisCompilerTestFE10TestdataTestGenerated.java @@ -30643,6 +30643,12 @@ public class DiagnosisCompilerTestFE10TestdataTestGenerated extends AbstractDiag runTest("compiler/testData/diagnostics/tests/smartCasts/variables/reassignedDependency.kt"); } + @Test + @TestMetadata("reassignedInRhs.kt") + public void testReassignedInRhs() throws Exception { + runTest("compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt"); + } + @Test @TestMetadata("varAsUse.kt") public void testVarAsUse() throws Exception { diff --git a/compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsTestGenerated.java b/compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsTestGenerated.java index 9014cc39bec..378026728c2 100644 --- a/compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsTestGenerated.java +++ b/compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsTestGenerated.java @@ -30643,6 +30643,12 @@ public class FirOldFrontendDiagnosticsTestGenerated extends AbstractFirDiagnosti runTest("compiler/testData/diagnostics/tests/smartCasts/variables/reassignedDependency.kt"); } + @Test + @TestMetadata("reassignedInRhs.kt") + public void testReassignedInRhs() throws Exception { + runTest("compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt"); + } + @Test @TestMetadata("varAsUse.kt") public void testVarAsUse() throws Exception { diff --git a/compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsWithLightTreeTestGenerated.java b/compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsWithLightTreeTestGenerated.java index b645cebeb11..f9be1529d69 100644 --- a/compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsWithLightTreeTestGenerated.java +++ b/compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsWithLightTreeTestGenerated.java @@ -30643,6 +30643,12 @@ public class FirOldFrontendDiagnosticsWithLightTreeTestGenerated extends Abstrac runTest("compiler/testData/diagnostics/tests/smartCasts/variables/reassignedDependency.kt"); } + @Test + @TestMetadata("reassignedInRhs.kt") + public void testReassignedInRhs() throws Exception { + runTest("compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt"); + } + @Test @TestMetadata("varAsUse.kt") public void testVarAsUse() throws Exception { diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirReturnsImpliesAnalyzer.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirReturnsImpliesAnalyzer.kt index f483c2cee4a..2488707c124 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirReturnsImpliesAnalyzer.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirReturnsImpliesAnalyzer.kt @@ -87,9 +87,7 @@ object FirReturnsImpliesAnalyzer : FirControlFlowChecker() { } } - val flow = dataFlowInfo.flowOnNodes.getValue(node) as PersistentFlow - var typeStatements: TypeStatements = flow.approvedTypeStatements - + var flow = dataFlowInfo.flowOnNodes.getValue(node) as PersistentFlow val operation = effect.value.toOperation() if (operation != null) { if (resultExpression is FirConstExpression<*>) { @@ -100,17 +98,17 @@ object FirReturnsImpliesAnalyzer : FirControlFlowChecker() { val variableStorage = dataFlowInfo.variableStorage as VariableStorageImpl val resultVar = variableStorage.getOrCreateIfReal(flow, resultExpression) if (resultVar != null) { - typeStatements = logicSystem.andForTypeStatements( - typeStatements, - logicSystem.approveOperationStatement(flow, OperationStatement(resultVar, operation)) - ) + val impliedByReturnValue = logicSystem.approveOperationStatement(flow, OperationStatement(resultVar, operation)) + if (impliedByReturnValue.isNotEmpty()) { + flow = logicSystem.forkFlow(flow).also { logicSystem.addTypeStatements(it, impliedByReturnValue) } + } } } } // TODO: if this is not a top-level function, `FirDataFlowAnalyzer` has erased its value parameters // from `dataFlowInfo.variableStorage` for some reason, so its `getLocalVariable` doesn't work. - val knownVariables = typeStatements.keys.associateBy { it.identifier } + val knownVariables = flow.knownVariables.associateBy { it.identifier } val argumentVariables = Array(function.valueParameters.size + 1) { i -> val parameterSymbol = if (i > 0) { function.valueParameters[i - 1].symbol @@ -134,7 +132,7 @@ object FirReturnsImpliesAnalyzer : FirControlFlowChecker() { return !conditionStatements.values.all { requirement -> val originalType = requirement.variable.identifier.symbol.correspondingParameterType ?: return@all true val requiredType = requirement.smartCastedType(typeContext, originalType) - val actualType = typeStatements[requirement.variable].smartCastedType(typeContext, originalType) + val actualType = flow.getTypeStatement(requirement.variable).smartCastedType(typeContext, originalType) actualType.isSubtypeOf(typeContext, requiredType) } } diff --git a/compiler/fir/resolve/src/org/jetbrains/kotlin/fir/resolve/dfa/FirDataFlowAnalyzer.kt b/compiler/fir/resolve/src/org/jetbrains/kotlin/fir/resolve/dfa/FirDataFlowAnalyzer.kt index 10209e0e265..a257d677943 100644 --- a/compiler/fir/resolve/src/org/jetbrains/kotlin/fir/resolve/dfa/FirDataFlowAnalyzer.kt +++ b/compiler/fir/resolve/src/org/jetbrains/kotlin/fir/resolve/dfa/FirDataFlowAnalyzer.kt @@ -515,9 +515,8 @@ abstract class FirDataFlowAnalyzer( } val flow = node.flow - // TODO: should be flow from the left operand, otherwise statements below will affect - // aliases that were established in the right operand and were not true at the time - // the left operand was evaluated. + // TODO: should be `getOrCreateIfRealAndUnchanged(flow from LHS, flow, leftOperand)`, otherwise the statement will + // be added even if the value has changed in the RHS. Currently the only previous node is the RHS. val leftOperandVariable = variableStorage.getOrCreateIfReal(flow, leftOperand) val rightOperandVariable = variableStorage.getOrCreateIfReal(flow, rightOperand) if (leftOperandVariable == null && rightOperandVariable == null) return @@ -787,7 +786,7 @@ abstract class FirDataFlowAnalyzer( if (flowFromPreviousSafeCall != null) { flow.copyAllInformationFrom(flowFromPreviousSafeCall) } - val receiverVariable = variableStorage.getOrCreateIfReal(node.flow, safeCall.receiver) ?: return + val receiverVariable = variableStorage.getOrCreateIfReal(flow, safeCall.receiver) ?: return flow.commitOperationStatement(receiverVariable notEq null) } @@ -796,7 +795,8 @@ abstract class FirDataFlowAnalyzer( val flow = node.mergeIncomingFlow() mergePostponedLambdaExitsNode?.mergeIncomingFlow() - val receiverVariable = variableStorage.getOrCreateIfReal(flow, safeCall.receiver) ?: return + val receiverLastNode = node.firstPreviousNode + val receiverVariable = variableStorage.getOrCreateIfRealAndUnchanged(receiverLastNode.flow, flow, safeCall.receiver) ?: return val expressionVariable = variableStorage.getOrCreate(flow, safeCall) // TODO? if the callee has non-null return type, then safe-call == null => receiver == null // if (x?.toString() == null) { /* x == null */ } @@ -898,6 +898,8 @@ abstract class FirDataFlowAnalyzer( if (conditionalEffects.isEmpty()) return val arguments = qualifiedAccess.orderedArguments(callee) ?: return + // TODO: should be `getOrCreateIfRealAndUnchanged(last flow of argument i, flow, it)` + // ^-- good luck finding that val argumentVariables = Array(arguments.size) { i -> arguments[i]?.let { variableStorage.getOrCreateIfReal(flow, it) } } if (argumentVariables.all { it == null }) return @@ -1059,6 +1061,7 @@ abstract class FirDataFlowAnalyzer( // If the right operand does not terminate, then we know that the value of the entire expression // has to be saturating (true for or, false for and), and it has to be produced by the left operand. if (leftIsBoolean) { + // Not checking for reassignments is safe since RHS did not execute. flow.commitOperationStatement(leftVariable!! eq !isAnd) } } else { @@ -1070,7 +1073,7 @@ abstract class FirDataFlowAnalyzer( val bothEvaluated = operatorVariable eq isAnd // TODO? `bothEvaluated` also implies all implications from RHS. This requires a second level // of implications, which the logic system currently doesn't support. See also safe calls. - flow.addAllConditionally(bothEvaluated, flowFromRight.approvedTypeStatements) + flow.addAllConditionally(bothEvaluated, flowFromRight) if (rightIsBoolean) { flow.addAllConditionally(bothEvaluated, logicSystem.approveOperationStatement(flowFromRight, rightVariable!! eq isAnd)) } @@ -1080,6 +1083,8 @@ abstract class FirDataFlowAnalyzer( flow.addAllConditionally( operatorVariable eq !isAnd, logicSystem.orForTypeStatements( + // Not checking for reassignments is safe since we will only take statements that are also true in RHS + // (so they're true regardless of whether the variable ends up being reassigned or not). logicSystem.approveOperationStatement(flowFromLeft, leftVariable!! eq !isAnd), // TODO: and(approved from right, ...)? FE1.0 doesn't seem to handle that correctly either. // if (x is A || whatever(x as B)) { /* x is (A | B) */ } @@ -1159,7 +1164,8 @@ abstract class FirDataFlowAnalyzer( mergePostponedLambdaExitsNode?.mergeIncomingFlow() val rhs = elvisExpression.rhs - val lhsVariable = variableStorage.getOrCreateIfReal(node.previousFlow, elvisExpression.lhs) ?: return + // No need to check for reassignments - if we can make any statements about LHS, then RHS has not executed. + val lhsVariable = variableStorage.getOrCreateIfReal(flow, elvisExpression.lhs) ?: return if (isLhsNotNull) { flow.commitOperationStatement(lhsVariable notEq null) } else if ( @@ -1284,9 +1290,15 @@ abstract class FirDataFlowAnalyzer( private fun FLOW.addAllConditionally(condition: OperationStatement, statements: TypeStatements) = statements.values.forEach { addImplication(condition implies it) } + private fun FLOW.addAllConditionally(condition: OperationStatement, from: FLOW) = + from.knownVariables.forEach { + // Only add the statement if this variable is not aliasing another in `this` (but it could be aliasing in `from`). + if (unwrapVariable(it) == it) addImplication(condition implies (from.getTypeStatement(it) ?: return@forEach)) + } + private fun FLOW.commitOperationStatement(statement: OperationStatement) = addAllStatements(logicSystem.approveOperationStatement(this, statement, removeApprovedOrImpossible = true)) - private val CFGNode<*>.previousFlow: FLOW - get() = firstPreviousNode.flow + private fun VariableStorageImpl.getOrCreateIfRealAndUnchanged(originalFlow: FLOW, currentFlow: FLOW, fir: FirElement) = + getOrCreateIfReal(originalFlow, fir)?.takeIf { !it.isReal() || logicSystem.isSameValueIn(originalFlow, currentFlow, it) } } diff --git a/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/Flow.kt b/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/Flow.kt index 1b634d107c7..abca452c359 100644 --- a/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/Flow.kt +++ b/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/Flow.kt @@ -6,7 +6,7 @@ package org.jetbrains.kotlin.fir.resolve.dfa abstract class Flow { - abstract val approvedTypeStatements: TypeStatements + abstract val knownVariables: Set abstract fun unwrapVariable(variable: RealVariable): RealVariable abstract fun getTypeStatement(variable: RealVariable): TypeStatement? } diff --git a/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/LogicSystem.kt b/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/LogicSystem.kt index c064232476f..53cd11856c5 100644 --- a/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/LogicSystem.kt +++ b/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/LogicSystem.kt @@ -23,6 +23,7 @@ abstract class LogicSystem(protected val context: ConeInferenceCont abstract fun recordNewAssignment(flow: FLOW, variable: RealVariable, index: Int) abstract fun removeAllAboutVariable(flow: FLOW, variable: RealVariable) abstract fun copyAllInformation(from: FLOW, to: FLOW) + abstract fun isSameValueIn(a: FLOW, b: FLOW, variable: RealVariable): Boolean abstract fun translateVariableFromConditionInStatements( flow: FLOW, diff --git a/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/PersistentLogicSystem.kt b/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/PersistentLogicSystem.kt index c0fad285b99..bc9e67a9502 100644 --- a/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/PersistentLogicSystem.kt +++ b/compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/PersistentLogicSystem.kt @@ -22,7 +22,7 @@ typealias PersistentImplications = PersistentMap + get() = approvedTypeStatements.keys + directAliasMap.keys + override fun unwrapVariable(variable: RealVariable): RealVariable = directAliasMap[variable] ?: variable @@ -126,8 +129,7 @@ abstract class PersistentLogicSystem(context: ConeInferenceContext) : LogicSyste } val approvedTypeStatements = result.approvedTypeStatements.builder() - val allVariablesWithStatements = flows.flatMapTo(mutableSetOf()) { it.approvedTypeStatements.keys + it.directAliasMap.keys } - allVariablesWithStatements.forEach computeStatement@{ variable -> + flows.flatMapTo(mutableSetOf()) { it.knownVariables }.forEach computeStatement@{ variable -> val statement = if (variable in result.directAliasMap) { return@computeStatement // statements about alias == statements about aliased variable } else if (!allExecute) { @@ -321,6 +323,9 @@ abstract class PersistentLogicSystem(context: ConeInferenceContext) : LogicSyste removeAllAboutVariable(flow, variable) flow.assignmentIndex = flow.assignmentIndex.put(variable, index) } + + override fun isSameValueIn(a: PersistentFlow, b: PersistentFlow, variable: RealVariable): Boolean = + a.assignmentIndex[variable] == b.assignmentIndex[variable] } private fun lowestCommonFlow(left: PersistentFlow, right: PersistentFlow): PersistentFlow { diff --git a/compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.fir.kt b/compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.fir.kt new file mode 100644 index 00000000000..33537dff485 --- /dev/null +++ b/compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.fir.kt @@ -0,0 +1,51 @@ +// ISSUE: KT-55096 +// SKIP_TXT + +import kotlin.contracts.* + +class C(val x: Int) + +@OptIn(ExperimentalContracts::class) +fun isNotNullAlsoCall(a: String?, b: () -> Unit): Boolean { + contract { + returns(true) implies (a != null) + callsInPlace(b, InvocationKind.EXACTLY_ONCE) + } + b() + return a != null +} + +fun binaryBooleanExpression() { + var x: String? = "" + if (x is String || (x is String).also { x = null }) { + x.length // bad (x#0 is String, x#1 is Nothing?, this is either) + } +} + +fun unoverriddenEquals(a: Any?) { + val c = C(1) + var b: Any? + b = c + if (b == c.also { b = a }) { + a.x // bad (b#0 is C, b#1 = a) + b.x // bad (b#0 is C, this is b#1) + if (b is C) { // b#1 + a.x // ok (b#1 = a) + b.x // ok + } + } +} + +fun safeCall() { + var x: String? = "" + if (x?.let { x = null; 1 } != null) { + x.length // bad (#2 != null => x#0 != null; but x#1 = null and this is either) + } +} + +fun contractFunction() { + var x: String? = "" + if (isNotNullAlsoCall(x) { x = null }) { + x.length // bad (#2 == true => x#0 != null; but this is x#1 = null) + } +} diff --git a/compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt b/compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt new file mode 100644 index 00000000000..68b66380e24 --- /dev/null +++ b/compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt @@ -0,0 +1,51 @@ +// ISSUE: KT-55096 +// SKIP_TXT + +import kotlin.contracts.* + +class C(val x: Int) + +@OptIn(ExperimentalContracts::class) +fun isNotNullAlsoCall(a: String?, b: () -> Unit): Boolean { + contract { + returns(true) implies (a != null) + callsInPlace(b, InvocationKind.EXACTLY_ONCE) + } + b() + return a != null +} + +fun binaryBooleanExpression() { + var x: String? = "" + if (x is String || (x is String).also { x = null }) { + x.length // bad (x#0 is String, x#1 is Nothing?, this is either) + } +} + +fun unoverriddenEquals(a: Any?) { + val c = C(1) + var b: Any? + b = c + if (b == c.also { b = a }) { + a.x // bad (b#0 is C, b#1 = a) + b.x // bad (b#0 is C, this is b#1) + if (b is C) { // b#1 + a.x // ok (b#1 = a) + b.x // ok + } + } +} + +fun safeCall() { + var x: String? = "" + if (x?.let { x = null; 1 } != null) { + x.length // bad (#2 != null => x#0 != null; but x#1 = null and this is either) + } +} + +fun contractFunction() { + var x: String? = "" + if (isNotNullAlsoCall(x) { x = null }) { + x.length // bad (#2 == true => x#0 != null; but this is x#1 = null) + } +} diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/DiagnosticTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/DiagnosticTestGenerated.java index cfee8825657..caf6d8b22d0 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/DiagnosticTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/DiagnosticTestGenerated.java @@ -30733,6 +30733,12 @@ public class DiagnosticTestGenerated extends AbstractDiagnosticTest { runTest("compiler/testData/diagnostics/tests/smartCasts/variables/reassignedDependency.kt"); } + @Test + @TestMetadata("reassignedInRhs.kt") + public void testReassignedInRhs() throws Exception { + runTest("compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt"); + } + @Test @TestMetadata("varAsUse.kt") public void testVarAsUse() throws Exception {