From c6298398efa13f010e0c268b05d911edd21129cc Mon Sep 17 00:00:00 2001 From: Jinseong Jeon Date: Fri, 19 Mar 2021 10:52:45 -0700 Subject: [PATCH] FIR checker: rework VAL_REASSIGNMENT_VIA_BACKING_FIELD(_ERROR) --- .../checkers/context/CheckerContext.kt | 14 +++++++- .../declaration/FirDeclarationCheckerUtils.kt | 34 ------------------- ...irValReassignmentViaBackingFieldChecker.kt | 34 +++++++++++++++++++ .../fir/checkers/CommonExpressionCheckers.kt | 5 +++ .../backingField/LocalDeclarations.fir.kt | 34 +++++++++++++++++++ .../tests/backingField/LocalDeclarations.kt | 1 - 6 files changed, 86 insertions(+), 36 deletions(-) create mode 100644 compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirValReassignmentViaBackingFieldChecker.kt create mode 100644 compiler/testData/diagnostics/tests/backingField/LocalDeclarations.fir.kt diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/context/CheckerContext.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/context/CheckerContext.kt index d3b565be268..85252687c83 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/context/CheckerContext.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/context/CheckerContext.kt @@ -37,7 +37,7 @@ abstract class CheckerContext { /** * Returns the closest to the end of context.containingDeclarations - * T instance or null if no such item could be found. + * instance of type [T] or null if no such item could be found. */ inline fun findClosest(): T? { for (it in containingDeclarations.asReversed()) { @@ -47,6 +47,18 @@ abstract class CheckerContext { return null } + /** + * Same as the one without specific [check]. For some cases, an instance of type [T] isn't good enough. E.g., property accessor is + * either getter or setter, but a type-based search could return, say, the closest setter, while we want to keep searching for a getter. + */ + inline fun findClosest(check: (T) -> Boolean): T? { + for (it in containingDeclarations.asReversed()) { + return (it as? T)?.takeIf(check) ?: continue + } + + return null + } + abstract fun addSuppressedDiagnostics( diagnosticNames: Collection, allInfosSuppressed: Boolean, diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirDeclarationCheckerUtils.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirDeclarationCheckerUtils.kt index 055cdf1e549..3ddcb9659d0 100644 --- a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirDeclarationCheckerUtils.kt +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirDeclarationCheckerUtils.kt @@ -5,11 +5,9 @@ package org.jetbrains.kotlin.fir.analysis.checkers.declaration -import org.jetbrains.kotlin.config.LanguageFeature import org.jetbrains.kotlin.descriptors.ClassKind import org.jetbrains.kotlin.descriptors.Modality import org.jetbrains.kotlin.descriptors.Visibilities -import org.jetbrains.kotlin.fir.FirElement import org.jetbrains.kotlin.fir.FirSourceElement import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext import org.jetbrains.kotlin.fir.analysis.checkers.modality @@ -18,11 +16,7 @@ import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors import org.jetbrains.kotlin.fir.analysis.diagnostics.reportOn import org.jetbrains.kotlin.fir.declarations.* import org.jetbrains.kotlin.fir.declarations.impl.FirDefaultPropertyAccessor -import org.jetbrains.kotlin.fir.expressions.FirVariableAssignment -import org.jetbrains.kotlin.fir.languageVersionSettings -import org.jetbrains.kotlin.fir.references.FirBackingFieldReference import org.jetbrains.kotlin.fir.types.FirImplicitTypeRef -import org.jetbrains.kotlin.fir.visitors.FirVisitorVoid import org.jetbrains.kotlin.lexer.KtTokens internal fun isInsideExpectClass(containingClass: FirRegularClass, context: CheckerContext): Boolean { @@ -175,34 +169,6 @@ internal fun checkPropertyAccessors( context: CheckerContext ) { if (property.isVal) { - if (property.getter != null) { - var reassignment: FirBackingFieldReference? = null - // TODO: consider replacing this with normal VariableAssignmentChecker and reporting all 'field = ...' in getter (not just 1st) - // This way is a bit hacky and does not handle diagnostic suppression normally - val visitor = object : FirVisitorVoid() { - override fun visitElement(element: FirElement) { - element.acceptChildren(this) - } - - override fun visitVariableAssignment(variableAssignment: FirVariableAssignment) { - // Report the first violation (to match FE 1.0 behavior) - if (reassignment != null) return - - val backingFieldReference = variableAssignment.lValue as? FirBackingFieldReference - if (backingFieldReference?.resolvedSymbol?.fir == property) { - reassignment = backingFieldReference - } - } - } - property.getter?.body?.accept(visitor, null) - reassignment?.source?.let { - if (context.session.languageVersionSettings.supportsFeature(LanguageFeature.RestrictionOfValReassignmentViaBackingField)) { - reporter.reportOn(it, FirErrors.VAL_REASSIGNMENT_VIA_BACKING_FIELD_ERROR, property.symbol, context) - } else { - reporter.reportOn(it, FirErrors.VAL_REASSIGNMENT_VIA_BACKING_FIELD, property.symbol, context) - } - } - } property.setter?.source?.let { reporter.reportOn(it, FirErrors.VAL_WITH_SETTER, context) } diff --git a/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirValReassignmentViaBackingFieldChecker.kt b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirValReassignmentViaBackingFieldChecker.kt new file mode 100644 index 00000000000..3b397033a23 --- /dev/null +++ b/compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/expression/FirValReassignmentViaBackingFieldChecker.kt @@ -0,0 +1,34 @@ +/* + * 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.analysis.checkers.expression + +import org.jetbrains.kotlin.config.LanguageFeature +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.FirPropertyAccessor +import org.jetbrains.kotlin.fir.expressions.FirVariableAssignment +import org.jetbrains.kotlin.fir.languageVersionSettings +import org.jetbrains.kotlin.fir.references.FirBackingFieldReference + +object FirValReassignmentViaBackingFieldChecker : FirVariableAssignmentChecker() { + override fun check(expression: FirVariableAssignment, context: CheckerContext, reporter: DiagnosticReporter) { + val backingFieldReference = expression.lValue as? FirBackingFieldReference ?: return + val property = backingFieldReference.resolvedSymbol.fir + if (property.isVar) return + val closestGetter = context.findClosest { it.isGetter } ?: return + if (property.getter != closestGetter) return + + backingFieldReference.source?.let { + if (context.session.languageVersionSettings.supportsFeature(LanguageFeature.RestrictionOfValReassignmentViaBackingField)) { + reporter.reportOn(it, FirErrors.VAL_REASSIGNMENT_VIA_BACKING_FIELD_ERROR, property.symbol, context) + } else { + reporter.reportOn(it, FirErrors.VAL_REASSIGNMENT_VIA_BACKING_FIELD, property.symbol, context) + } + } + } +} diff --git a/compiler/fir/entrypoint/src/org/jetbrains/kotlin/fir/checkers/CommonExpressionCheckers.kt b/compiler/fir/entrypoint/src/org/jetbrains/kotlin/fir/checkers/CommonExpressionCheckers.kt index 5063c6cf324..4f958d3d0fc 100644 --- a/compiler/fir/entrypoint/src/org/jetbrains/kotlin/fir/checkers/CommonExpressionCheckers.kt +++ b/compiler/fir/entrypoint/src/org/jetbrains/kotlin/fir/checkers/CommonExpressionCheckers.kt @@ -25,12 +25,17 @@ object CommonExpressionCheckers : ExpressionCheckers() { FirTypeParameterInQualifiedAccessChecker, FirSealedClassConstructorCallChecker, ) + override val functionCallCheckers: Set = setOf() override val tryExpressionCheckers: Set = setOf( FirCatchParameterChecker ) + override val variableAssignmentCheckers: Set = setOf( + FirValReassignmentViaBackingFieldChecker + ) + override val whenExpressionCheckers: Set = setOf( FirExhaustiveWhenChecker ) diff --git a/compiler/testData/diagnostics/tests/backingField/LocalDeclarations.fir.kt b/compiler/testData/diagnostics/tests/backingField/LocalDeclarations.fir.kt new file mode 100644 index 00000000000..2b29c29519e --- /dev/null +++ b/compiler/testData/diagnostics/tests/backingField/LocalDeclarations.fir.kt @@ -0,0 +1,34 @@ +// !LANGUAGE: +RestrictionOfValReassignmentViaBackingField + +class Outer { + val i: Int = 1 + get() { + class Inner { + var i: Int = 2 + get() { + field++ + return field + } + val j: Int = 3 + get() { + field = 42 + return field + } + + fun innerMember() { + field++ + } + } + return field + } + + val j: Int = 4 + get() { + fun local() { + field++ + field++ + } + local() + return field + } +} diff --git a/compiler/testData/diagnostics/tests/backingField/LocalDeclarations.kt b/compiler/testData/diagnostics/tests/backingField/LocalDeclarations.kt index 5c202ecb43c..0a7b6da116b 100644 --- a/compiler/testData/diagnostics/tests/backingField/LocalDeclarations.kt +++ b/compiler/testData/diagnostics/tests/backingField/LocalDeclarations.kt @@ -1,4 +1,3 @@ -// FIR_IDENTICAL // !LANGUAGE: +RestrictionOfValReassignmentViaBackingField class Outer {