From c42dd0848e6bbcd6a6d6ebcc13f3b06f566eef81 Mon Sep 17 00:00:00 2001 From: pyos Date: Thu, 19 Jan 2023 10:57:34 +0100 Subject: [PATCH] FIR: only allow member val initialization through `this@T` class C { val x: Int init { // valid ways to initialize: x = 1 this@C.x = 1 // invalid: someOtherC.x = 1 run { /*this@run.*/x = 1 } val self = this self.x = 1 } } --- ...nMemberProperty_lateInitialization.fir.txt | 11 ++++- ...nOfNonMemberProperty_lateInitialization.kt | 11 ++++- .../PropertyInitializationInfoCollector.kt | 43 ++++++++----------- .../declaration/FirMemberPropertiesChecker.kt | 3 +- ...irReassignmentAndInvisibleSetterChecker.kt | 22 +++++----- .../tests/controlFlowAnalysis/kt6788.fir.kt | 2 +- .../propertiesInitWithOtherInstance.fir.kt | 4 +- ...ropertiesInitWithOtherInstanceInner.fir.kt | 2 +- ...rtiesInitWithOtherInstanceThisLabel.fir.kt | 4 +- 9 files changed, 57 insertions(+), 45 deletions(-) diff --git a/compiler/fir/analysis-tests/testData/resolve/cfa/reassignOfNonMemberProperty_lateInitialization.fir.txt b/compiler/fir/analysis-tests/testData/resolve/cfa/reassignOfNonMemberProperty_lateInitialization.fir.txt index 8d236efaa83..9ae7e321020 100644 --- a/compiler/fir/analysis-tests/testData/resolve/cfa/reassignOfNonMemberProperty_lateInitialization.fir.txt +++ b/compiler/fir/analysis-tests/testData/resolve/cfa/reassignOfNonMemberProperty_lateInitialization.fir.txt @@ -22,7 +22,6 @@ FILE: reassignOfNonMemberProperty_lateInitialization.kt public final val a: R|kotlin/String| = this@R|/Some|.R|kotlin/run|( = run@fun R|Some|.(): R|kotlin/String| { this@R|special/anonymous|.R|/Some.x| = String(error) - this@R|special/anonymous|.R|/Some.y| = String(ok) this@R|special/anonymous|.R|/Some.y| = String(error) this@R|special/anonymous|.R|/z| = String(error) ^ String(hello) @@ -30,6 +29,16 @@ FILE: reassignOfNonMemberProperty_lateInitialization.kt ) public get(): R|kotlin/String| + public final val b: R|kotlin/String| = Int(123).R|kotlin/run|( = run@fun R|kotlin/Int|.(): R|kotlin/String| { + this@R|/Some|.R|/Some.x| = String(error) + this@R|/Some|.R|/Some.y| = String(ok) + this@R|/Some|.R|/Some.y| = String(error) + this@R|/Some|.R|/z| = String(error) + ^ String(there) + } + ) + public get(): R|kotlin/String| + init { this@R|/Some|.R|/Some.x| = String(error) this@R|/Some|.R|/Some.y| = String(error) diff --git a/compiler/fir/analysis-tests/testData/resolve/cfa/reassignOfNonMemberProperty_lateInitialization.kt b/compiler/fir/analysis-tests/testData/resolve/cfa/reassignOfNonMemberProperty_lateInitialization.kt index 5206cc4242c..7adc3a037fd 100644 --- a/compiler/fir/analysis-tests/testData/resolve/cfa/reassignOfNonMemberProperty_lateInitialization.kt +++ b/compiler/fir/analysis-tests/testData/resolve/cfa/reassignOfNonMemberProperty_lateInitialization.kt @@ -15,11 +15,20 @@ class Some { } val a: String = run { + // these are all on this@run, which is not guaranteed to be this@Some + x = "error" + y = "error" + z = "error" + "hello" + } + + val b: String = 123.run { + // now this@run is an Int, so these are on this@Some x = "error" y = "ok" y = "error" z = "error" - "hello" + "there" } init { diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/util/PropertyInitializationInfoCollector.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/util/PropertyInitializationInfoCollector.kt index 3109761d8ba..5e5e365b4af 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/util/PropertyInitializationInfoCollector.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/util/PropertyInitializationInfoCollector.kt @@ -7,8 +7,10 @@ package org.jetbrains.kotlin.fir.analysis.cfa.util import kotlinx.collections.immutable.persistentMapOf import org.jetbrains.kotlin.contracts.description.EventOccurrencesRange +import org.jetbrains.kotlin.fir.expressions.FirThisReceiverExpression import org.jetbrains.kotlin.fir.references.toResolvedPropertySymbol import org.jetbrains.kotlin.fir.resolve.dfa.cfg.* +import org.jetbrains.kotlin.fir.symbols.FirBasedSymbol import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol class PropertyInitializationInfoData(properties: Set, graph: ControlFlowGraph) { @@ -23,8 +25,12 @@ class PropertyInitializationInfoData(properties: Set, graph: class PropertyInitializationInfoCollector( private val localProperties: Set, + private val expectedReceiver: FirBasedSymbol<*>? = null, private val declaredVariableCollector: DeclaredVariableCollector = DeclaredVariableCollector(), ) : PathAwareControlFlowGraphVisitor() { + fun getData(graph: ControlFlowGraph) = + graph.collectDataForNode(TraverseDirection.Forward, this) + companion object { private val EMPTY_INFO: PathAwarePropertyInitializationInfo = persistentMapOf(NormalPath to PropertyInitializationInfo.EMPTY) } @@ -37,12 +43,11 @@ class PropertyInitializationInfoCollector( data: PathAwarePropertyInitializationInfo ): PathAwarePropertyInitializationInfo { val dataForNode = visitNode(node, data) + val receiver = (node.fir.dispatchReceiver as? FirThisReceiverExpression)?.calleeReference?.boundSymbol + if (receiver != expectedReceiver) return dataForNode val symbol = node.fir.calleeReference.toResolvedPropertySymbol() ?: return dataForNode - return if (symbol !in localProperties) { - dataForNode - } else { - processVariableWithAssignment(dataForNode, symbol) - } + if (symbol !in localProperties) return dataForNode + return addRange(dataForNode, symbol, EventOccurrencesRange.EXACTLY_ONCE) } override fun visitVariableDeclarationNode( @@ -50,26 +55,14 @@ class PropertyInitializationInfoCollector( data: PathAwarePropertyInitializationInfo ): PathAwarePropertyInitializationInfo { val dataForNode = visitNode(node, data) - return processVariableWithAssignment( - dataForNode, - node.fir.symbol, - overwriteRange = node.fir.initializer == null && node.fir.delegate == null - ) - } - - fun getData(graph: ControlFlowGraph) = - graph.collectDataForNode(TraverseDirection.Forward, this) - - private fun processVariableWithAssignment( - dataForNode: PathAwarePropertyInitializationInfo, - symbol: FirPropertySymbol, - overwriteRange: Boolean = false, - ): PathAwarePropertyInitializationInfo { - assert(dataForNode.keys.isNotEmpty()) - return if (overwriteRange) - overwriteRange(dataForNode, symbol, EventOccurrencesRange.ZERO) - else - addRange(dataForNode, symbol, EventOccurrencesRange.EXACTLY_ONCE) + return when { + expectedReceiver != null -> + dataForNode + node.fir.initializer == null && node.fir.delegate == null -> + overwriteRange(dataForNode, node.fir.symbol, EventOccurrencesRange.ZERO) + else -> + overwriteRange(dataForNode, node.fir.symbol, EventOccurrencesRange.EXACTLY_ONCE) + } } // -------------------------------------------------- diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirMemberPropertiesChecker.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirMemberPropertiesChecker.kt index 6149d803c8b..280fd788d44 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirMemberPropertiesChecker.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirMemberPropertiesChecker.kt @@ -48,7 +48,8 @@ object FirMemberPropertiesChecker : FirClassChecker() { } if (memberPropertySymbols.isEmpty()) return null // TODO: this also visits non-constructor member functions... - return PropertyInitializationInfoCollector(memberPropertySymbols).getData(graph)[graph.exitNode]?.get(NormalPath) + // TODO: also use FirPropertyInitializationAnalyzer.PropertyReporter to report reassignments or use-before-initialization + return PropertyInitializationInfoCollector(memberPropertySymbols, symbol).getData(graph)[graph.exitNode]?.get(NormalPath) } private fun checkProperty( diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirReassignmentAndInvisibleSetterChecker.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirReassignmentAndInvisibleSetterChecker.kt index cbbeef3ca6d..4a0ee73a11d 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirReassignmentAndInvisibleSetterChecker.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirReassignmentAndInvisibleSetterChecker.kt @@ -20,6 +20,7 @@ import org.jetbrains.kotlin.fir.declarations.FirPropertyAccessor import org.jetbrains.kotlin.fir.declarations.utils.hasBackingField import org.jetbrains.kotlin.fir.declarations.utils.visibility import org.jetbrains.kotlin.fir.expressions.FirSmartCastExpression +import org.jetbrains.kotlin.fir.expressions.FirThisReceiverExpression import org.jetbrains.kotlin.fir.expressions.FirVariableAssignment import org.jetbrains.kotlin.fir.originalForSubstitutionOverride import org.jetbrains.kotlin.fir.references.FirBackingFieldReference @@ -29,10 +30,10 @@ import org.jetbrains.kotlin.fir.references.toResolvedPropertySymbol import org.jetbrains.kotlin.fir.references.toResolvedValueParameterSymbol import org.jetbrains.kotlin.fir.resolve.calls.ExpressionReceiverValue import org.jetbrains.kotlin.fir.resolve.toSymbol +import org.jetbrains.kotlin.fir.symbols.FirBasedSymbol import org.jetbrains.kotlin.fir.symbols.SymbolInternals import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol import org.jetbrains.kotlin.fir.symbols.impl.FirSyntheticPropertySymbol -import org.jetbrains.kotlin.fir.types.ConeSimpleKotlinType import org.jetbrains.kotlin.fir.types.toSymbol import org.jetbrains.kotlin.fir.visibilityChecker @@ -133,23 +134,22 @@ object FirReassignmentAndInvisibleSetterChecker : FirVariableAssignmentChecker() ) { val property = expression.lValue.toResolvedPropertySymbol() ?: return if (property.isLocal || property.isVar) return - val assignIsIllegal = when (val dispatchReceiverType = property.dispatchReceiverType) { - null -> true - else -> when { - property is FirSyntheticPropertySymbol -> true - property.hasDelegate || !property.hasBackingField -> true - property.hasInitializer -> true - else -> !isInsideInitializationOfClass(dispatchReceiverType, context) + val isLegal = when { + property is FirSyntheticPropertySymbol -> false + property.hasDelegate || !property.hasBackingField -> false + property.hasInitializer -> false + else -> { + val thisReceiverSymbol = (expression.dispatchReceiver as? FirThisReceiverExpression)?.calleeReference?.boundSymbol + thisReceiverSymbol != null && isInsideInitializationOfClass(thisReceiverSymbol, context) } } - if (assignIsIllegal) { + if (!isLegal) { reporter.reportOn(expression.lValue.source, FirErrors.VAL_REASSIGNMENT, property, context) } } - private fun isInsideInitializationOfClass(classType: ConeSimpleKotlinType, context: CheckerContext): Boolean { + private fun isInsideInitializationOfClass(classSymbol: FirBasedSymbol<*>, context: CheckerContext): Boolean { val session = context.session - val classSymbol = classType.toSymbol(session) ?: return false for (containingDeclaration in context.containingDeclarations) { val containingClassSymbol = when (containingDeclaration) { is FirProperty -> containingDeclaration.containingClassLookupTag()?.toSymbol(session) diff --git a/compiler/testData/diagnostics/tests/controlFlowAnalysis/kt6788.fir.kt b/compiler/testData/diagnostics/tests/controlFlowAnalysis/kt6788.fir.kt index 0fa9a05917e..c946e9037da 100644 --- a/compiler/testData/diagnostics/tests/controlFlowAnalysis/kt6788.fir.kt +++ b/compiler/testData/diagnostics/tests/controlFlowAnalysis/kt6788.fir.kt @@ -1,7 +1,7 @@ class A(val next: A? = null) { val x: String init { - next?.x = "a" + next?.x = "a" } } diff --git a/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstance.fir.kt b/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstance.fir.kt index 71ec7c200e0..639356afe88 100644 --- a/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstance.fir.kt +++ b/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstance.fir.kt @@ -1,12 +1,12 @@ class A(val next: A? = null) { val x: String init { - next?.x = "a" + next?.x = "a" x = "b" this.x = "c" x = "d" // don't repeat the same diagnostic again with this receiver this.x = "e" - next?.x = "f" + next?.x = "f" } } diff --git a/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstanceInner.fir.kt b/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstanceInner.fir.kt index 36e6baf0e1a..70e085e7f8c 100644 --- a/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstanceInner.fir.kt +++ b/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstanceInner.fir.kt @@ -15,7 +15,7 @@ class Outer { innerProp = "6" // do not repeat the same diagnostic with this receiver this@Inner.innerProp = "7" - inner.innerProp = "8" + inner.innerProp = "8" } } } diff --git a/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstanceThisLabel.fir.kt b/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstanceThisLabel.fir.kt index ed28c8edac7..3cd5644c1b2 100644 --- a/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstanceThisLabel.fir.kt +++ b/compiler/testData/diagnostics/tests/controlFlowAnalysis/propertiesInitWithOtherInstanceThisLabel.fir.kt @@ -1,12 +1,12 @@ class A(val next: A? = null) { val x: String init { - next?.x = "a" + next?.x = "a" this@A.x = "b" this.x = "c" x = "d" // don't repeat the same diagnostic again with this receiver this@A.x = "e" - next?.x = "f" + next?.x = "f" } }