FIR checker: report CAPTURED_VAL_INITIALIZATION

2021-05-11 15:41:56 -07:00
parent d38effcbbe
commit f0f1e2d945
20 changed files with 158 additions and 31 deletions
@@ -2,7 +2,7 @@ fun test1() {
    val x: Int

    fun func() {
-        x = 0
+        <!CAPTURED_VAL_INITIALIZATION!>x<!> = 0
    }

    println(<!UNINITIALIZED_VARIABLE!>x<!>)
@@ -43,4 +43,4 @@ fun test3() {

    println(<!UNINITIALIZED_VARIABLE!>x<!>)
    println(<!UNINITIALIZED_VARIABLE!>x<!>)
-}
+}
@@ -691,6 +691,12 @@ object DIAGNOSTICS_LIST : DiagnosticList() {
        val VAL_REASSIGNMENT_VIA_BACKING_FIELD_ERROR by error<KtExpression> {
            parameter<FirPropertySymbol>("property")
        }
+        val CAPTURED_VAL_INITIALIZATION by error<KtExpression> {
+            parameter<FirPropertySymbol>("property")
+        }
+        val CAPTURED_MEMBER_VAL_INITIALIZATION by error<KtExpression> {
+            parameter<FirPropertySymbol>("property")
+        }
        val WRONG_INVOCATION_KIND by warning<PsiElement> {
            parameter<Symbol>("declaration")
            parameter<EventOccurrencesRange>("requiredRange")
@@ -411,6 +411,8 @@ object FirErrors {
    val VAL_REASSIGNMENT by error1<KtExpression, FirVariableSymbol<*>>()
    val VAL_REASSIGNMENT_VIA_BACKING_FIELD by warning1<KtExpression, FirPropertySymbol>()
    val VAL_REASSIGNMENT_VIA_BACKING_FIELD_ERROR by error1<KtExpression, FirPropertySymbol>()
+    val CAPTURED_VAL_INITIALIZATION by error1<KtExpression, FirPropertySymbol>()
+    val CAPTURED_MEMBER_VAL_INITIALIZATION by error1<KtExpression, FirPropertySymbol>()
    val WRONG_INVOCATION_KIND by warning3<PsiElement, AbstractFirBasedSymbol<*>, EventOccurrencesRange, EventOccurrencesRange>()
    val LEAKED_IN_PLACE_LAMBDA by error1<PsiElement, AbstractFirBasedSymbol<*>>()
    val WRONG_IMPLIES_CONDITION by warning0<PsiElement>()
@@ -7,6 +7,7 @@ package org.jetbrains.kotlin.fir.analysis.cfa

 import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext
 import org.jetbrains.kotlin.fir.analysis.diagnostics.DiagnosticReporter
+import org.jetbrains.kotlin.fir.expressions.FirVariableAssignment
 import org.jetbrains.kotlin.fir.resolve.dfa.cfg.CFGNode
 import org.jetbrains.kotlin.fir.resolve.dfa.cfg.ControlFlowGraph
 import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol
@@ -17,6 +18,7 @@ abstract class AbstractFirPropertyInitializationChecker {
        reporter: DiagnosticReporter,
        data: Map<CFGNode<*>, PathAwarePropertyInitializationInfo>,
        properties: Set<FirPropertySymbol>,
+        capturedWrites: Set<FirVariableAssignment>,
        context: CheckerContext
    )
 }
@@ -11,7 +11,6 @@ import org.jetbrains.kotlin.contracts.description.EventOccurrencesRange
 import org.jetbrains.kotlin.fir.FirElement
 import org.jetbrains.kotlin.fir.declarations.*
 import org.jetbrains.kotlin.fir.expressions.*
-import org.jetbrains.kotlin.fir.references.FirResolvedNamedReference
 import org.jetbrains.kotlin.fir.resolve.dfa.cfg.*
 import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol
 import org.jetbrains.kotlin.fir.util.SetMultimap
@@ -60,21 +59,58 @@ class PropertyInitializationInfo(
        ::EMPTY
 }

-class LocalPropertyCollector private constructor() : ControlFlowGraphVisitorVoid() {
+class LocalPropertyAndCapturedWriteCollector private constructor() : ControlFlowGraphVisitorVoid() {
    companion object {
-        fun collect(graph: ControlFlowGraph): MutableSet<FirPropertySymbol> {
-            val collector = LocalPropertyCollector()
+        fun collect(graph: ControlFlowGraph): Pair<Set<FirPropertySymbol>, Set<FirVariableAssignment>> {
+            val collector = LocalPropertyAndCapturedWriteCollector()
            graph.traverse(TraverseDirection.Forward, collector)
-            return collector.symbols
+            return collector.symbols.keys to collector.capturedWrites
        }
    }

-    private val symbols: MutableSet<FirPropertySymbol> = mutableSetOf()
+    // Mapping from a property symbol to its declaration context
+    // `true` if the (local) property is declared in the currently visited function.
+    // `false` if it is declared in a lambda or a local function (inside the currently visited function).
+    private val symbols: MutableMap<FirPropertySymbol, Boolean> = mutableMapOf()
+
+    private val lambdaOrLocalFunctionStack: MutableList<FirFunction<*>> = mutableListOf()
+    private val capturedWrites: MutableSet<FirVariableAssignment> = mutableSetOf()

    override fun visitNode(node: CFGNode<*>) {}

    override fun visitVariableDeclarationNode(node: VariableDeclarationNode) {
-        symbols += node.fir.symbol
+        symbols[node.fir.symbol] = lambdaOrLocalFunctionStack.lastOrNull() == null
+    }
+
+    override fun visitPostponedLambdaEnterNode(node: PostponedLambdaEnterNode) {
+        lambdaOrLocalFunctionStack.add(node.fir)
+    }
+
+    override fun visitPostponedLambdaExitNode(node: PostponedLambdaExitNode) {
+        lambdaOrLocalFunctionStack.remove(node.fir)
+    }
+
+    override fun visitLocalFunctionDeclarationNode(node: LocalFunctionDeclarationNode, data: Nothing?) {
+        lambdaOrLocalFunctionStack.add(node.fir)
+    }
+
+    override fun visitFunctionExitNode(node: FunctionExitNode) {
+        lambdaOrLocalFunctionStack.remove(node.fir)
+    }
+
+    override fun visitVariableAssignmentNode(node: VariableAssignmentNode) {
+        // Check if this variable assignment is inside a lambda or a local function.
+        if (lambdaOrLocalFunctionStack.isEmpty()) return
+
+        // Check if the assigned variable doesn't belong to any lambda or local function.
+        val symbol = node.fir.referredPropertySymbol ?: return
+        if (symbol !in symbols || symbols[symbol] == false) return
+
+        // If all nested declarations are lambdas that are invoked in-place (according to the contract),
+        // this variable assignment is not a captured write.
+        if (lambdaOrLocalFunctionStack.all { it is FirAnonymousFunction && it.invocationKind.isInPlace }) return
+
+        capturedWrites.add(node.fir)
    }
 }

@@ -110,8 +146,7 @@ class PropertyInitializationInfoCollector(
        data: Collection<Pair<EdgeLabel, PathAwarePropertyInitializationInfo>>
    ): PathAwarePropertyInitializationInfo {
        val dataForNode = visitNode(node, data)
-        val reference = node.fir.lValue as? FirResolvedNamedReference ?: return dataForNode
-        val symbol = reference.resolvedSymbol as? FirPropertySymbol ?: return dataForNode
+        val symbol = node.fir.referredPropertySymbol ?: return dataForNode
        return if (symbol !in localProperties) {
            dataForNode
        } else {
@@ -61,9 +61,9 @@ class FirControlFlowAnalyzer(
    }

    private fun runAssignmentCfaCheckers(graph: ControlFlowGraph, reporter: DiagnosticReporter, context: CheckerContext) {
-        val properties = LocalPropertyCollector.collect(graph)
+        val (properties, capturedWrites) = LocalPropertyAndCapturedWriteCollector.collect(graph)
        if (properties.isEmpty()) return
        val data = PropertyInitializationInfoCollector(properties).getData(graph)
-        variableAssignmentCheckers.forEach { it.analyze(graph, reporter, data, properties, context) }
+        variableAssignmentCheckers.forEach { it.analyze(graph, reporter, data, properties, capturedWrites, context) }
    }
 }
@@ -11,8 +11,11 @@ import org.jetbrains.kotlin.contracts.description.isDefinitelyVisited
 import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext
 import org.jetbrains.kotlin.fir.analysis.diagnostics.DiagnosticReporter
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors
+import org.jetbrains.kotlin.fir.analysis.diagnostics.reportOn
 import org.jetbrains.kotlin.fir.declarations.isLateInit
+import org.jetbrains.kotlin.fir.declarations.referredPropertySymbol
 import org.jetbrains.kotlin.fir.expressions.FirQualifiedAccess
+import org.jetbrains.kotlin.fir.expressions.FirVariableAssignment
 import org.jetbrains.kotlin.fir.references.FirResolvedNamedReference
 import org.jetbrains.kotlin.fir.resolve.dfa.cfg.*
 import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol
@@ -24,6 +27,7 @@ object FirPropertyInitializationAnalyzer : AbstractFirPropertyInitializationChec
        reporter: DiagnosticReporter,
        data: Map<CFGNode<*>, PathAwarePropertyInitializationInfo>,
        properties: Set<FirPropertySymbol>,
+        capturedWrites: Set<FirVariableAssignment>,
        context: CheckerContext
    ) {
        val localData = data.filter {
@@ -33,21 +37,21 @@ object FirPropertyInitializationAnalyzer : AbstractFirPropertyInitializationChec

        val localProperties = properties.filter { it.fir.initializer == null && it.fir.delegate == null }.toSet()

-        val reporterVisitor = PropertyReporter(localData, localProperties, reporter, context)
+        val reporterVisitor = PropertyReporter(localData, localProperties, capturedWrites, reporter, context)
        graph.traverse(TraverseDirection.Forward, reporterVisitor)
    }

    private class PropertyReporter(
        val data: Map<CFGNode<*>, PathAwarePropertyInitializationInfo>,
        val localProperties: Set<FirPropertySymbol>,
+        val capturedWrites: Set<FirVariableAssignment>,
        val reporter: DiagnosticReporter,
        val context: CheckerContext
    ) : ControlFlowGraphVisitorVoid() {
        override fun visitNode(node: CFGNode<*>) {}

        private fun getPropertySymbol(node: CFGNode<*>): FirPropertySymbol? {
-            val reference = (node.fir as? FirQualifiedAccess)?.calleeReference as? FirResolvedNamedReference ?: return null
-            return reference.resolvedSymbol as? FirPropertySymbol
+            return (node.fir as? FirQualifiedAccess)?.referredPropertySymbol
        }

        override fun visitVariableAssignmentNode(node: VariableAssignmentNode) {
@@ -66,10 +70,17 @@ object FirPropertyInitializationAnalyzer : AbstractFirPropertyInitializationChec
            symbol: FirPropertySymbol,
            node: VariableAssignmentNode
        ): Boolean {
+            if (symbol.fir.isVal && node.fir in capturedWrites) {
+                if (symbol.fir.isLocal) {
+                    reporter.reportOn(node.fir.lValue.source, FirErrors.CAPTURED_VAL_INITIALIZATION, symbol, context)
+                } else {
+                    reporter.reportOn(node.fir.lValue.source, FirErrors.CAPTURED_MEMBER_VAL_INITIALIZATION, symbol, context)
+                }
+                return true
+            }
            val kind = info[symbol] ?: EventOccurrencesRange.ZERO
            if (symbol.fir.isVal && kind.canBeRevisited()) {
                node.fir.lValue.source?.let {
-                    // TODO: differentiate CAPTURED_VAL_INITIALIZATION
                    reporter.report(FirErrors.VAL_REASSIGNMENT.on(it, symbol), context)
                    return true
                }
@@ -18,6 +18,7 @@ import org.jetbrains.kotlin.fir.analysis.checkers.getChildren
 import org.jetbrains.kotlin.fir.analysis.diagnostics.DiagnosticReporter
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors
 import org.jetbrains.kotlin.fir.analysis.diagnostics.reportOn
+import org.jetbrains.kotlin.fir.expressions.FirVariableAssignment
 import org.jetbrains.kotlin.fir.references.FirResolvedNamedReference
 import org.jetbrains.kotlin.fir.resolve.dfa.cfg.*
 import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol
@@ -28,6 +29,7 @@ object CanBeValChecker : AbstractFirPropertyInitializationChecker() {
        reporter: DiagnosticReporter,
        data: Map<CFGNode<*>, PathAwarePropertyInitializationInfo>,
        properties: Set<FirPropertySymbol>,
+        capturedWrites: Set<FirVariableAssignment>,
        context: CheckerContext
    ) {
        val unprocessedProperties = mutableSetOf<FirPropertySymbol>()
@@ -36,7 +36,7 @@ object UnusedChecker : FirControlFlowChecker() {
                !it.symbol.classId.isLocal
            } != null
        ) return
-        val properties = LocalPropertyCollector.collect(graph)
+        val (properties, _) = LocalPropertyAndCapturedWriteCollector.collect(graph)
        if (properties.isEmpty()) return

        val data = ValueWritesWithoutReading(context.session, properties).getData(graph)
@@ -58,6 +58,8 @@ import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CANNOT_INFER_PARA
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CANNOT_WEAKEN_ACCESS_PRIVILEGE
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CAN_BE_REPLACED_WITH_OPERATOR_ASSIGNMENT
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CAN_BE_VAL
+import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CAPTURED_MEMBER_VAL_INITIALIZATION
+import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CAPTURED_VAL_INITIALIZATION
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CATCH_PARAMETER_WITH_DEFAULT_VALUE
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CLASS_IN_SUPERTYPE_FOR_ENUM
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors.CLASS_LITERAL_LHS_NOT_A_CLASS
@@ -941,6 +943,16 @@ class FirDefaultErrorMessages : DefaultErrorMessages.Extension {
            map.put(VAL_REASSIGNMENT, "Val cannot be reassigned", VARIABLE_NAME)
            map.put(VAL_REASSIGNMENT_VIA_BACKING_FIELD, "Reassignment of read-only property via backing field is deprecated", VARIABLE_NAME)
            map.put(VAL_REASSIGNMENT_VIA_BACKING_FIELD_ERROR, "Reassignment of read-only property via backing field", VARIABLE_NAME)
+            map.put(
+                CAPTURED_VAL_INITIALIZATION,
+                "Captured values initialization is forbidden due to possible reassignment",
+                VARIABLE_NAME
+            )
+            map.put(
+                CAPTURED_MEMBER_VAL_INITIALIZATION,
+                "Captured member values initialization is forbidden due to possible reassignment",
+                VARIABLE_NAME
+            )
            map.put(
                WRONG_INVOCATION_KIND,
                "{2} wrong invocation kind: given {3} case, but {4} case is possible",
@@ -0,0 +1,11 @@
+/*
+ * Copyright 2010-2021 JetBrains s.r.o. and Kotlin Programming Language contributors.
+ * 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.fir.resolve.dfa.cfg
+
+import org.jetbrains.kotlin.contracts.description.EventOccurrencesRange
+
+val EventOccurrencesRange?.isInPlace: Boolean
+    get() = this != null
@@ -306,9 +306,6 @@ class ControlFlowGraphBuilder {
            else -> false
        }

-    private val EventOccurrencesRange?.isInPlace: Boolean
-        get() = this != null
-
    fun exitAnonymousFunction(anonymousFunction: FirAnonymousFunction): Triple<FunctionExitNode, PostponedLambdaExitNode?, ControlFlowGraph> {
        val symbol = anonymousFunction.symbol
        val exitNode = exitsOfAnonymousFunctions.remove(symbol)!!.also {
@@ -11,10 +11,13 @@ import org.jetbrains.kotlin.fir.declarations.builder.FirRegularClassBuilder
 import org.jetbrains.kotlin.fir.declarations.builder.FirTypeParameterBuilder
 import org.jetbrains.kotlin.fir.declarations.impl.FirFileImpl
 import org.jetbrains.kotlin.fir.declarations.impl.FirRegularClassImpl
+import org.jetbrains.kotlin.fir.expressions.FirQualifiedAccess
+import org.jetbrains.kotlin.fir.expressions.FirVariableAssignment
 import org.jetbrains.kotlin.fir.references.FirResolvedNamedReference
 import org.jetbrains.kotlin.fir.render
 import org.jetbrains.kotlin.fir.symbols.impl.FirAnonymousObjectSymbol
 import org.jetbrains.kotlin.fir.symbols.impl.FirClassSymbol
+import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol
 import org.jetbrains.kotlin.fir.symbols.impl.FirRegularClassSymbol
 import org.jetbrains.kotlin.fir.types.ConeClassLikeType
 import org.jetbrains.kotlin.fir.types.ConeFlexibleType
@@ -216,6 +219,12 @@ val FirProperty.hasBackingField: Boolean
        }
    }

+val FirQualifiedAccess.referredPropertySymbol: FirPropertySymbol?
+    get() {
+        val reference = calleeReference as? FirResolvedNamedReference ?: return null
+        return reference.resolvedSymbol as? FirPropertySymbol
+    }
+
 inline val FirDeclaration.isFromLibrary: Boolean
    get() = origin == FirDeclarationOrigin.Library
 inline val FirDeclaration.isSynthetic: Boolean
@@ -17,14 +17,14 @@ fun foo() {
 fun bar() {
    val x: Int
    exec {
-        x = 13
+        <!CAPTURED_VAL_INITIALIZATION!>x<!> = 13
    }
 }

 fun bar2() {
    val x: Int
    fun foo() {
-        x = 3
+        <!CAPTURED_VAL_INITIALIZATION!>x<!> = 3
    }
    foo()
 }
@@ -60,7 +60,7 @@ class Your {
    val y = if (true) {
        val xx: Int
        exec {
-            xx = 42
+            <!CAPTURED_VAL_INITIALIZATION!>xx<!> = 42
        }
        24
    }
@@ -70,7 +70,7 @@ class Your {
 val z = if (true) {
    val xx: Int
    exec {
-        xx = 24
+        <!CAPTURED_VAL_INITIALIZATION!>xx<!> = 24
    }
    42
 }
@@ -59,7 +59,7 @@ fun gau() {
        object: Any() {
            init {
                // Error!
-                y = ""
+                <!CAPTURED_VAL_INITIALIZATION!>y<!> = ""
            }
        }
    }
@@ -36,7 +36,7 @@ fun branchingIndetermineFlow(a: Any?) {

 fun nonAnonymousLambdas() {
    val x: Int
-    val initializer = { x = 42 }
+    val initializer = { <!CAPTURED_VAL_INITIALIZATION!>x<!> = 42 }
    myRun(initializer)
    <!UNINITIALIZED_VARIABLE!>x<!>.inc()
 }
@@ -55,7 +55,7 @@ fun funWithUnknownInvocations(block: () -> Unit) = block()
 fun nestedIndefiniteAssignment() {
    val x: Int
    // Captured val initialization reported, because we don't know anything about funWithUnknownInvocations
-    funWithUnknownInvocations { myRun { x = 42 } }
+    funWithUnknownInvocations { myRun { <!CAPTURED_VAL_INITIALIZATION!>x<!> = 42 } }
    <!UNINITIALIZED_VARIABLE!>x<!>.inc()
 }

@@ -65,4 +65,4 @@ class InitializationForbiddenInNonInitSection {
    fun setup() {
        myRun { x = 42 }
    }
-}
+}
@@ -46,6 +46,6 @@ fun funWithUnknownInvocations(block: () -> Unit) = block()

 fun nestedIndefiniteAssignment() {
    val x: Int
-    funWithUnknownInvocations { myRun { x = 42 } }
+    funWithUnknownInvocations { myRun { <!CAPTURED_VAL_INITIALIZATION!>x<!> = 42 } }
    <!UNINITIALIZED_VARIABLE!>x<!>.inc()
-}
+}
@@ -1988,6 +1988,20 @@ internal val KT_DIAGNOSTIC_CONVERTER = KtDiagnosticConverterBuilder.buildConvert
            token,
        )
    }
+    add(FirErrors.CAPTURED_VAL_INITIALIZATION) { firDiagnostic ->
+        CapturedValInitializationImpl(
+            firSymbolBuilder.variableLikeBuilder.buildVariableSymbol(firDiagnostic.a.fir),
+            firDiagnostic as FirPsiDiagnostic<*>,
+            token,
+        )
+    }
+    add(FirErrors.CAPTURED_MEMBER_VAL_INITIALIZATION) { firDiagnostic ->
+        CapturedMemberValInitializationImpl(
+            firSymbolBuilder.variableLikeBuilder.buildVariableSymbol(firDiagnostic.a.fir),
+            firDiagnostic as FirPsiDiagnostic<*>,
+            token,
+        )
+    }
    add(FirErrors.WRONG_INVOCATION_KIND) { firDiagnostic ->
        WrongInvocationKindImpl(
            firSymbolBuilder.buildSymbol(firDiagnostic.a.fir as FirDeclaration),
@@ -1395,6 +1395,16 @@ sealed class KtFirDiagnostic<PSI: PsiElement> : KtDiagnosticWithPsi<PSI> {
        abstract val property: KtVariableSymbol
    }

+    abstract class CapturedValInitialization : KtFirDiagnostic<KtExpression>() {
+        override val diagnosticClass get() = CapturedValInitialization::class
+        abstract val property: KtVariableSymbol
+    }
+
+    abstract class CapturedMemberValInitialization : KtFirDiagnostic<KtExpression>() {
+        override val diagnosticClass get() = CapturedMemberValInitialization::class
+        abstract val property: KtVariableSymbol
+    }
+
    abstract class WrongInvocationKind : KtFirDiagnostic<PsiElement>() {
        override val diagnosticClass get() = WrongInvocationKind::class
        abstract val declaration: KtSymbol
@@ -2269,6 +2269,22 @@ internal class ValReassignmentViaBackingFieldErrorImpl(
    override val firDiagnostic: FirPsiDiagnostic<*> by weakRef(firDiagnostic)
 }

+internal class CapturedValInitializationImpl(
+    override val property: KtVariableSymbol,
+    firDiagnostic: FirPsiDiagnostic<*>,
+    override val token: ValidityToken,
+) : KtFirDiagnostic.CapturedValInitialization(), KtAbstractFirDiagnostic<KtExpression> {
+    override val firDiagnostic: FirPsiDiagnostic<*> by weakRef(firDiagnostic)
+}
+
+internal class CapturedMemberValInitializationImpl(
+    override val property: KtVariableSymbol,
+    firDiagnostic: FirPsiDiagnostic<*>,
+    override val token: ValidityToken,
+) : KtFirDiagnostic.CapturedMemberValInitialization(), KtAbstractFirDiagnostic<KtExpression> {
+    override val firDiagnostic: FirPsiDiagnostic<*> by weakRef(firDiagnostic)
+}
+
 internal class WrongInvocationKindImpl(
    override val declaration: KtSymbol,
    override val requiredRange: EventOccurrencesRange,