From 86b6b184dec916e378abb937c5c003891637ea0e Mon Sep 17 00:00:00 2001 From: pyos Date: Fri, 20 Jan 2023 12:32:11 +0100 Subject: [PATCH] FIR: move checkPropertyAccesses to FirPropertyInitializationAnalyzer --- ...bstractFirPropertyInitializationChecker.kt | 8 +- .../cfa/FirPropertyInitializationAnalyzer.kt | 116 ++++++++++++++++-- .../PropertyInitializationInfoCollector.kt | 6 +- .../declaration/FirMemberPropertiesChecker.kt | 109 +--------------- ...irReassignmentAndInvisibleSetterChecker.kt | 4 +- .../checkers/extended/CanBeValChecker.kt | 15 +-- .../checkers/extended/UnusedChecker.kt | 12 +- .../ControlFlowAnalysisDiagnosticComponent.kt | 2 +- 8 files changed, 126 insertions(+), 146 deletions(-) diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/AbstractFirPropertyInitializationChecker.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/AbstractFirPropertyInitializationChecker.kt index da3b2dd6d84..eab00c2a1ec 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/AbstractFirPropertyInitializationChecker.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/AbstractFirPropertyInitializationChecker.kt @@ -13,11 +13,5 @@ import org.jetbrains.kotlin.fir.resolve.dfa.cfg.ControlFlowGraph import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol abstract class AbstractFirPropertyInitializationChecker { - abstract fun analyze( - graph: ControlFlowGraph, - reporter: DiagnosticReporter, - data: PropertyInitializationInfoData, - properties: Set, - context: CheckerContext - ) + abstract fun analyze(data: PropertyInitializationInfoData, reporter: DiagnosticReporter, context: CheckerContext) } diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirPropertyInitializationAnalyzer.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirPropertyInitializationAnalyzer.kt index a6a2c3e4713..98ed59c7cf7 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirPropertyInitializationAnalyzer.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/cfa/FirPropertyInitializationAnalyzer.kt @@ -5,20 +5,116 @@ package org.jetbrains.kotlin.fir.analysis.cfa +import org.jetbrains.kotlin.contracts.description.canBeRevisited +import org.jetbrains.kotlin.contracts.description.isDefinitelyVisited +import org.jetbrains.kotlin.contracts.description.isInPlace +import org.jetbrains.kotlin.descriptors.ClassKind import org.jetbrains.kotlin.diagnostics.DiagnosticReporter +import org.jetbrains.kotlin.diagnostics.reportOn import org.jetbrains.kotlin.fir.analysis.cfa.util.PropertyInitializationInfoData import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext -import org.jetbrains.kotlin.fir.analysis.checkers.declaration.checkPropertyAccesses -import org.jetbrains.kotlin.fir.analysis.checkers.declaration.requiresInitialization -import org.jetbrains.kotlin.fir.resolve.dfa.cfg.ControlFlowGraph +import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors +import org.jetbrains.kotlin.fir.declarations.* +import org.jetbrains.kotlin.fir.declarations.utils.hasBackingField +import org.jetbrains.kotlin.fir.declarations.utils.hasExplicitBackingField +import org.jetbrains.kotlin.fir.declarations.utils.isLateInit +import org.jetbrains.kotlin.fir.expressions.FirQualifiedAccess +import org.jetbrains.kotlin.fir.expressions.FirThisReceiverExpression +import org.jetbrains.kotlin.fir.isCatchParameter +import org.jetbrains.kotlin.fir.references.toResolvedPropertySymbol +import org.jetbrains.kotlin.fir.resolve.dfa.cfg.* +import org.jetbrains.kotlin.fir.symbols.SymbolInternals import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol +import org.jetbrains.kotlin.fir.symbols.impl.FirSyntheticPropertySymbol object FirPropertyInitializationAnalyzer : AbstractFirPropertyInitializationChecker() { - override fun analyze( - graph: ControlFlowGraph, - reporter: DiagnosticReporter, - data: PropertyInitializationInfoData, - properties: Set, - context: CheckerContext - ) = graph.checkPropertyAccesses(properties.filterTo(mutableSetOf()) { it.requiresInitialization }, null, context, reporter, data) + override fun analyze(data: PropertyInitializationInfoData, reporter: DiagnosticReporter, context: CheckerContext) { + data.checkPropertyAccesses(context, reporter) + } +} + +val FirDeclaration.evaluatedInPlace: Boolean + get() = when (this) { + is FirAnonymousFunction -> invocationKind.isInPlace + is FirAnonymousObject -> classKind != ClassKind.ENUM_ENTRY + is FirConstructor -> true // child of class initialization graph + is FirFunction, is FirClass -> false + else -> true // property initializer, etc. + } + +@OptIn(SymbolInternals::class) +val FirPropertySymbol.requiresInitialization: Boolean + get() = this !is FirSyntheticPropertySymbol && !hasInitializer && !hasExplicitBackingField && + hasBackingField && fir.isCatchParameter != true + +fun PropertyInitializationInfoData.checkPropertyAccesses(context: CheckerContext, reporter: DiagnosticReporter) { + // If a property has an initializer (or does not need one), then any reads are OK while any writes are OK + // if it's a `var` and bad if it's a `val`. `FirReassignmentAndInvisibleSetterChecker` does this without a CFG. + val filtered = properties.filterTo(mutableSetOf()) { it.requiresInitialization } + if (filtered.isEmpty()) return + + checkPropertyAccesses(graph, filtered, context, reporter, null, mutableMapOf()) +} + +@OptIn(SymbolInternals::class) +private fun PropertyInitializationInfoData.checkPropertyAccesses( + graph: ControlFlowGraph, + properties: Set, + context: CheckerContext, + reporter: DiagnosticReporter, + scope: FirDeclaration?, + scopes: MutableMap, +) { + fun FirQualifiedAccess.hasCorrectReceiver() = + (dispatchReceiver as? FirThisReceiverExpression)?.calleeReference?.boundSymbol == receiver + + for (node in graph.nodes) { + when { + // TODO: `node.isUnion` - f({ x = 1 }, { x = 2 }) - which to report? + // Also this is currently indistinguishable from x = 1; f({}, {}). + + node is VariableDeclarationNode -> { + val symbol = node.fir.symbol + if (scope != null && receiver == null && node.fir.isVal && symbol in properties) { + // It's OK to initialize this variable from a nested called-in-place function, but not from + // a non-called-in-place function or a non-anonymous-object class initializer. + scopes[symbol] = scope + } + } + + node is VariableAssignmentNode -> { + val symbol = node.fir.calleeReference.toResolvedPropertySymbol() ?: continue + if (!symbol.fir.isVal || !node.fir.hasCorrectReceiver() || symbol !in properties) continue + + if (scope != scopes[symbol]) { + val error = if (receiver != null) + FirErrors.CAPTURED_MEMBER_VAL_INITIALIZATION + else + FirErrors.CAPTURED_VAL_INITIALIZATION + reporter.reportOn(node.fir.lValue.source, error, symbol, context) + } else if (getValue(node).values.any { it[symbol]?.canBeRevisited() == true }) { + reporter.reportOn(node.fir.lValue.source, FirErrors.VAL_REASSIGNMENT, symbol, context) + } + } + + node is QualifiedAccessNode -> { + val symbol = node.fir.calleeReference.toResolvedPropertySymbol() ?: continue + if (!symbol.isLateInit && node.fir.hasCorrectReceiver() && symbol in properties && + getValue(node).values.any { it[symbol]?.isDefinitelyVisited() != true } + ) { + reporter.reportOn(node.fir.source, FirErrors.UNINITIALIZED_VARIABLE, symbol, context) + } + } + + // In the class case, subgraphs of the exit node are member functions, which are considered to not + // be part of initialization, so any val is considered to be initialized there and the CFG is not + // needed. The errors on reassignments will be emitted by `FirReassignmentAndInvisibleSetterChecker`. + node is CFGNodeWithSubgraphs<*> && (receiver == null || node !== graph.exitNode) -> { + for (subGraph in node.subGraphs) { + val newScope = subGraph.declaration?.takeIf { !it.evaluatedInPlace } ?: scope + checkPropertyAccesses(subGraph, properties, context, reporter, newScope, scopes) + } + } + } + } } 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 2a3c98728c9..40a747e0dae 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 @@ -13,7 +13,11 @@ 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, receiver: FirBasedSymbol<*>?, graph: ControlFlowGraph) { +class PropertyInitializationInfoData( + val properties: Set, + val receiver: FirBasedSymbol<*>?, + val graph: ControlFlowGraph, +) { private val data by lazy(LazyThreadSafetyMode.NONE) { graph.collectDataForNode(TraverseDirection.Forward, PropertyInitializationInfoCollector(properties, receiver)) } 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 bb6f91017da..cbec8fe11a4 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 @@ -6,10 +6,7 @@ package org.jetbrains.kotlin.fir.analysis.checkers.declaration import org.jetbrains.kotlin.KtFakeSourceElementKind -import org.jetbrains.kotlin.contracts.description.canBeRevisited import org.jetbrains.kotlin.contracts.description.isDefinitelyVisited -import org.jetbrains.kotlin.contracts.description.isInPlace -import org.jetbrains.kotlin.descriptors.ClassKind import org.jetbrains.kotlin.descriptors.Visibilities import org.jetbrains.kotlin.fir.analysis.cfa.util.PropertyInitializationInfo import org.jetbrains.kotlin.fir.analysis.checkers.contains @@ -18,20 +15,15 @@ import org.jetbrains.kotlin.fir.analysis.checkers.getModifierList import org.jetbrains.kotlin.diagnostics.DiagnosticReporter import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors import org.jetbrains.kotlin.diagnostics.reportOn +import org.jetbrains.kotlin.fir.analysis.cfa.checkPropertyAccesses +import org.jetbrains.kotlin.fir.analysis.cfa.requiresInitialization import org.jetbrains.kotlin.fir.analysis.cfa.util.PropertyInitializationInfoData import org.jetbrains.kotlin.fir.declarations.* import org.jetbrains.kotlin.fir.declarations.impl.FirDefaultPropertyAccessor import org.jetbrains.kotlin.fir.declarations.utils.* -import org.jetbrains.kotlin.fir.expressions.FirQualifiedAccess -import org.jetbrains.kotlin.fir.expressions.FirThisReceiverExpression -import org.jetbrains.kotlin.fir.isCatchParameter -import org.jetbrains.kotlin.fir.references.toResolvedPropertySymbol -import org.jetbrains.kotlin.fir.resolve.dfa.cfg.* +import org.jetbrains.kotlin.fir.resolve.dfa.cfg.NormalPath import org.jetbrains.kotlin.fir.resolve.dfa.controlFlowGraph -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.lexer.KtTokens // See old FE's [DeclarationsChecker] @@ -61,7 +53,7 @@ object FirMemberPropertiesChecker : FirClassChecker() { // TODO: this also visits non-constructor member functions... // TODO: merge with `FirPropertyInitializationAnalyzer` for fewer passes. val data = PropertyInitializationInfoData(memberPropertySymbols, symbol, graph) - graph.checkPropertyAccesses(memberPropertySymbols, symbol, context, reporter, data) + data.checkPropertyAccesses(context, reporter) return data.getValue(graph.exitNode)[NormalPath] } @@ -136,96 +128,3 @@ object FirMemberPropertiesChecker : FirClassChecker() { } } } - -val FirDeclaration.evaluatedInPlace: Boolean - get() = when (this) { - is FirAnonymousFunction -> invocationKind.isInPlace - is FirAnonymousObject -> classKind != ClassKind.ENUM_ENTRY - is FirConstructor -> true // child of class initialization graph - is FirFunction, is FirClass -> false - else -> true // property initializer, etc. - } - -@OptIn(SymbolInternals::class) -val FirPropertySymbol.requiresInitialization: Boolean - get() = this !is FirSyntheticPropertySymbol && !hasInitializer && !hasExplicitBackingField && - hasBackingField && fir.isCatchParameter != true - -fun ControlFlowGraph.checkPropertyAccesses( - properties: Set, - receiver: FirBasedSymbol<*>?, - context: CheckerContext, - reporter: DiagnosticReporter, - data: PropertyInitializationInfoData -) { - // NOTE: assert(properties.all { it.requiresInitialization }) - // If a property has an initializer (or does not need one), then any reads are OK while any writes are OK - // if it's a `var` and bad if it's a `val`. `FirReassignmentAndInvisibleSetterChecker` does this without a CFG. - if (properties.isEmpty()) return - - checkPropertyAccesses(properties, receiver, context, reporter, data, null, mutableMapOf()) -} - -@OptIn(SymbolInternals::class) -private fun ControlFlowGraph.checkPropertyAccesses( - properties: Set, - receiver: FirBasedSymbol<*>?, - context: CheckerContext, - reporter: DiagnosticReporter, - data: PropertyInitializationInfoData, - scope: FirDeclaration?, - scopes: MutableMap, -) { - fun FirQualifiedAccess.hasCorrectReceiver() = - (dispatchReceiver as? FirThisReceiverExpression)?.calleeReference?.boundSymbol == receiver - - for (node in nodes) { - when { - // TODO: `node.isUnion` - f({ x = 1 }, { x = 2 }) - which to report? - // Also this is currently indistinguishable from x = 1; f({}, {}). - - node is VariableDeclarationNode -> { - val symbol = node.fir.symbol - if (scope != null && receiver == null && node.fir.isVal && symbol in properties) { - // It's OK to initialize this variable from a nested called-in-place function, but not from - // a non-called-in-place function or a non-anonymous-object class initializer. - scopes[symbol] = scope - } - } - - node is VariableAssignmentNode -> { - val symbol = node.fir.calleeReference.toResolvedPropertySymbol() ?: continue - if (!symbol.fir.isVal || !node.fir.hasCorrectReceiver() || symbol !in properties) continue - - if (scope != scopes[symbol]) { - val error = if (receiver != null) - FirErrors.CAPTURED_MEMBER_VAL_INITIALIZATION - else - FirErrors.CAPTURED_VAL_INITIALIZATION - reporter.reportOn(node.fir.lValue.source, error, symbol, context) - } else if (data.getValue(node).values.any { it[symbol]?.canBeRevisited() == true }) { - reporter.reportOn(node.fir.lValue.source, FirErrors.VAL_REASSIGNMENT, symbol, context) - } - } - - node is QualifiedAccessNode -> { - val symbol = node.fir.calleeReference.toResolvedPropertySymbol() ?: continue - if (!symbol.isLateInit && node.fir.hasCorrectReceiver() && symbol in properties && - data.getValue(node).values.any { it[symbol]?.isDefinitelyVisited() != true } - ) { - reporter.reportOn(node.fir.source, FirErrors.UNINITIALIZED_VARIABLE, symbol, context) - } - } - - // In the class case, subgraphs of the exit node are member functions, which are considered to not - // be part of initialization, so any val is considered to be initialized there and the CFG is not - // needed. The errors on reassignments will be emitted by `FirReassignmentAndInvisibleSetterChecker`. - node is CFGNodeWithSubgraphs<*> && (receiver == null || node !== exitNode) -> { - for (subGraph in node.subGraphs) { - val newScope = subGraph.declaration?.takeIf { !it.evaluatedInPlace } ?: scope - subGraph.checkPropertyAccesses(properties, receiver, context, reporter, data, newScope, scopes) - } - } - } - } -} 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 485734ea9d5..b6940a94935 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 @@ -8,10 +8,10 @@ package org.jetbrains.kotlin.fir.analysis.checkers.expression import org.jetbrains.kotlin.descriptors.Visibilities import org.jetbrains.kotlin.diagnostics.DiagnosticReporter import org.jetbrains.kotlin.diagnostics.reportOn +import org.jetbrains.kotlin.fir.analysis.cfa.evaluatedInPlace +import org.jetbrains.kotlin.fir.analysis.cfa.requiresInitialization import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext import org.jetbrains.kotlin.fir.analysis.checkers.context.findClosest -import org.jetbrains.kotlin.fir.analysis.checkers.declaration.evaluatedInPlace -import org.jetbrains.kotlin.fir.analysis.checkers.declaration.requiresInitialization import org.jetbrains.kotlin.fir.analysis.checkers.toRegularClassSymbol import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors import org.jetbrains.kotlin.fir.declarations.* diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/extended/CanBeValChecker.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/extended/CanBeValChecker.kt index 0f6a2636f79..c27cb3f3617 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/extended/CanBeValChecker.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/extended/CanBeValChecker.kt @@ -21,18 +21,12 @@ import org.jetbrains.kotlin.fir.resolve.dfa.cfg.* import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol object CanBeValChecker : AbstractFirPropertyInitializationChecker() { - override fun analyze( - graph: ControlFlowGraph, - reporter: DiagnosticReporter, - data: PropertyInitializationInfoData, - properties: Set, - context: CheckerContext - ) { + override fun analyze(data: PropertyInitializationInfoData, reporter: DiagnosticReporter, context: CheckerContext) { val unprocessedProperties = mutableSetOf() val propertiesCharacteristics = mutableMapOf() - val reporterVisitor = UninitializedPropertyReporter(data, properties, unprocessedProperties, propertiesCharacteristics) - graph.traverse(reporterVisitor) + val reporterVisitor = UninitializedPropertyReporter(data, unprocessedProperties, propertiesCharacteristics) + data.graph.traverse(reporterVisitor) for (property in unprocessedProperties) { val source = property.source @@ -74,7 +68,6 @@ object CanBeValChecker : AbstractFirPropertyInitializationChecker() { private class UninitializedPropertyReporter( val data: PropertyInitializationInfoData, - val localProperties: Set, val unprocessedProperties: MutableSet, val propertiesCharacteristics: MutableMap ) : ControlFlowGraphVisitorVoid() { @@ -82,7 +75,7 @@ object CanBeValChecker : AbstractFirPropertyInitializationChecker() { override fun visitVariableAssignmentNode(node: VariableAssignmentNode) { val symbol = node.fir.calleeReference.toResolvedPropertySymbol() ?: return - if (symbol !in localProperties) return + if (symbol !in data.properties) return unprocessedProperties.remove(symbol) val currentCharacteristic = propertiesCharacteristics.getOrDefault(symbol, EventOccurrencesRange.ZERO) diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/extended/UnusedChecker.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/extended/UnusedChecker.kt index b72623cc60c..25f1d7d4d4f 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/extended/UnusedChecker.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/extended/UnusedChecker.kt @@ -31,15 +31,9 @@ import org.jetbrains.kotlin.fir.types.coneType import org.jetbrains.kotlin.fir.types.isFunctionalType object UnusedChecker : AbstractFirPropertyInitializationChecker() { - override fun analyze( - graph: ControlFlowGraph, - reporter: DiagnosticReporter, - data: PropertyInitializationInfoData, - properties: Set, - context: CheckerContext - ) { - val ownData = ValueWritesWithoutReading(context.session, properties).getData(graph) - graph.traverse(CfaVisitor(ownData, reporter, context)) + override fun analyze(data: PropertyInitializationInfoData, reporter: DiagnosticReporter, context: CheckerContext) { + val ownData = ValueWritesWithoutReading(context.session, data.properties).getData(data.graph) + data.graph.traverse(CfaVisitor(ownData, reporter, context)) } class CfaVisitor( diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/collectors/components/ControlFlowAnalysisDiagnosticComponent.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/collectors/components/ControlFlowAnalysisDiagnosticComponent.kt index 0de95b10ca5..7e5b2bda110 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/collectors/components/ControlFlowAnalysisDiagnosticComponent.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/collectors/components/ControlFlowAnalysisDiagnosticComponent.kt @@ -34,7 +34,7 @@ class ControlFlowAnalysisDiagnosticComponent( val properties = mutableSetOf().apply { graph.traverse(LocalPropertyCollector(this)) } if (properties.isNotEmpty()) { val data = PropertyInitializationInfoData(properties, receiver = null, graph) - variableAssignmentCheckers.forEach { it.analyze(graph, reporter, data, properties, context) } + variableAssignmentCheckers.forEach { it.analyze(data, reporter, context) } } }