azalea/kotlin-fork
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

FIR DFA: check for reassignments of LHS when handling ?..

Browse Source 
`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
This commit is contained in:
pyos
2022-11-14 20:45:18 +01:00
committed by teamcity
parent ae1048f8a9
commit 5cc08fb314
11 changed files with 164 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
analysis/low-level-api-fir/tests/org/jetbrains/kotlin/analysis/low/level/api/fir/diagnostic/compiler/based/DiagnosisCompilerTestFE10TestdataTestGenerated.java
Generated
+6
View File
@@ -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 {
compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsTestGenerated.java
Generated
+6
View File
@@ -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 {
compiler/fir/analysis-tests/tests-gen/org/jetbrains/kotlin/test/runners/FirOldFrontendDiagnosticsWithLightTreeTestGenerated.java
Generated
+6
View File
@@ -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 {
compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirReturnsImpliesAnalyzer.kt
+7 -9
View File
@@ -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)
}
}
compiler/fir/resolve/src/org/jetbrains/kotlin/fir/resolve/dfa/FirDataFlowAnalyzer.kt
+21 -9
View File
@@ -515,9 +515,8 @@ abstract class FirDataFlowAnalyzer<FLOW : Flow>(
}
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<FLOW : Flow>(
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<FLOW : Flow>(
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<FLOW : Flow>(
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<FLOW : Flow>(
// 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<FLOW : Flow>(
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 : Flow>(
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<FLOW : Flow>(
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<FLOW : Flow>(
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) }
}
compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/Flow.kt
+1 -1
View File
@@ -6,7 +6,7 @@
package org.jetbrains.kotlin.fir.resolve.dfa
abstract class Flow {
abstract val approvedTypeStatements: TypeStatements
abstract val knownVariables: Set<RealVariable>
abstract fun unwrapVariable(variable: RealVariable): RealVariable
abstract fun getTypeStatement(variable: RealVariable): TypeStatement?
}
compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/LogicSystem.kt
+1
View File
@@ -23,6 +23,7 @@ abstract class LogicSystem<FLOW : Flow>(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,
compiler/fir/semantics/src/org/jetbrains/kotlin/fir/resolve/dfa/PersistentLogicSystem.kt
+8 -3
View File
@@ -22,7 +22,7 @@ typealias PersistentImplications = PersistentMap<DataFlowVariable, PersistentLis
class PersistentFlow : Flow {
val previousFlow: PersistentFlow?
override var approvedTypeStatements: PersistentApprovedTypeStatements
var approvedTypeStatements: PersistentApprovedTypeStatements
var logicStatements: PersistentImplications
val level: Int
@@ -60,6 +60,9 @@ class PersistentFlow : Flow {
assignmentIndex = persistentMapOf()
}
override val knownVariables: Set<RealVariable>
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 {
compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.fir.kt
Vendored
+51
View File
@@ -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<!UNSAFE_CALL!>.<!>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 (<!USELESS_IS_CHECK!>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<!UNSAFE_CALL!>.<!>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)
}
}
compiler/testData/diagnostics/tests/smartCasts/variables/reassignedInRhs.kt
Vendored
+51
View File
@@ -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<!UNSAFE_CALL!>.<!>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.<!UNRESOLVED_REFERENCE!>x<!> // bad (b#0 is C, b#1 = a)
<!SMARTCAST_IMPOSSIBLE!>b<!>.x // bad (b#0 is C, this is b#1)
if (b is C) { // b#1
a.<!UNRESOLVED_REFERENCE!>x<!> // ok (b#1 = a)
<!SMARTCAST_IMPOSSIBLE!>b<!>.x // ok
}
}
}
fun safeCall() {
var x: String? = ""
if (x?.let { x = null; 1 } != null) {
<!SMARTCAST_IMPOSSIBLE!>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 }) {
<!SMARTCAST_IMPOSSIBLE!>x<!>.length // bad (#2 == true => x#0 != null; but this is x#1 = null)
}
}
compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/DiagnosticTestGenerated.java
Generated
+6
View File
@@ -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 {