From f6c189be7bce2143585741650db1a8fdc668c67f Mon Sep 17 00:00:00 2001 From: Kirill Rakhman Date: Tue, 24 Jan 2023 15:38:52 +0100 Subject: [PATCH] FIR: Handle visibility of public setter of protected synthetic property like in K1 K1 allows writing access to a public setter of a protected synthetic property only if the call is inside a subclass. K2 previously allowed that unconditionally. This changes brings the behavior in line with K1. ^KT-56050 Fixed --- .../fir/java/FirJavaVisibilityChecker.kt | 13 ++- .../protectedGetterWithPublicSetter.fir.kt | 102 ++++++++++++++++-- .../protectedGetterWithPublicSetter.kt | 98 ++++++++++++++++- .../protectedGetterWithPublicSetter.txt | 55 +++++++++- .../SetterHasHigherAccess.fir.kt | 4 +- 5 files changed, 254 insertions(+), 18 deletions(-) diff --git a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/FirJavaVisibilityChecker.kt b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/FirJavaVisibilityChecker.kt index c09f1e1cfcf..09807140c85 100644 --- a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/FirJavaVisibilityChecker.kt +++ b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/FirJavaVisibilityChecker.kt @@ -9,6 +9,7 @@ import org.jetbrains.kotlin.descriptors.Visibilities import org.jetbrains.kotlin.descriptors.Visibility import org.jetbrains.kotlin.descriptors.java.JavaVisibilities import org.jetbrains.kotlin.fir.* +import org.jetbrains.kotlin.fir.declarations.FirClass import org.jetbrains.kotlin.fir.declarations.FirDeclaration import org.jetbrains.kotlin.fir.declarations.FirFile import org.jetbrains.kotlin.fir.declarations.synthetic.FirSyntheticPropertyAccessor @@ -16,8 +17,10 @@ import org.jetbrains.kotlin.fir.declarations.utils.visibility import org.jetbrains.kotlin.fir.resolve.SupertypeSupplier import org.jetbrains.kotlin.fir.resolve.calls.FirSimpleSyntheticPropertySymbol import org.jetbrains.kotlin.fir.resolve.calls.ReceiverValue +import org.jetbrains.kotlin.fir.resolve.isSubclassOf import org.jetbrains.kotlin.fir.symbols.FirBasedSymbol import org.jetbrains.kotlin.name.FqName +import org.jetbrains.kotlin.utils.addToStdlib.lastIsInstanceOrNull @NoMutableState object FirJavaVisibilityChecker : FirVisibilityChecker() { @@ -47,7 +50,7 @@ object FirJavaVisibilityChecker : FirVisibilityChecker() { // FE1.0 allows calling public setters with property assignment syntax if the getter is protected. if (!isCallToPropertySetter || symbol !is FirSimpleSyntheticPropertySymbol) return false - symbol.setterSymbol?.visibility == Visibilities.Public + symbol.setterSymbol?.visibility == Visibilities.Public && symbol.isCalledFromSubclass(containingDeclarations, session) } } @@ -56,6 +59,14 @@ object FirJavaVisibilityChecker : FirVisibilityChecker() { } } + private fun FirSimpleSyntheticPropertySymbol.isCalledFromSubclass( + containingDeclarations: List, + session: FirSession + ): Boolean { + val containingClassLookupTag = this.containingClassLookupTag() ?: return false + return containingDeclarations.any { it is FirClass && it.isSubclassOf(containingClassLookupTag, session, false) } + } + override fun platformOverrideVisibilityCheck( candidateInDerivedClass: FirBasedSymbol<*>, symbolInBaseClass: FirBasedSymbol<*>, diff --git a/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.fir.kt b/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.fir.kt index 96fb4d9a38b..d5abe62885b 100644 --- a/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.fir.kt +++ b/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.fir.kt @@ -1,9 +1,9 @@ // FILE: j/Super.java package j -public abstract class Super { - protected abstract String getName(); - public abstract void setName(String s); +public class Super { + protected String getName() { return "" }; + public void setName(String s) { } } // FILE: k/test.kt @@ -11,20 +11,110 @@ public abstract class Super { package k import j.Super -abstract class Sub: Super() { +abstract class Sub : Super() { fun test(s: Super) { s.name s.getName() s.name = "" s.name = s.name s.setName("") + + val anon1 = object : Super() { + fun testAnon() { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + + val anon2 = object { + fun testAnon() { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + } + + inner class Nested1 : Super() { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + + class Nested2 { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } +} + +abstract class NonSub { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + + val anon1 = object : Super() { + fun testAnon() { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + + val anon2 = object { + fun testAnon() { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + } + + inner class Nested1 : Super() { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + + class Nested2 { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } } } fun test(s: Super) { s.name s.getName() - s.name = "" - s.name = s.name + s.name = "" + s.name = s.name s.setName("") } diff --git a/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.kt b/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.kt index 44e1c1e1e0e..cb22e3b9077 100644 --- a/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.kt +++ b/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.kt @@ -1,9 +1,9 @@ // FILE: j/Super.java package j -public abstract class Super { - protected abstract String getName(); - public abstract void setName(String s); +public class Super { + protected String getName() { return "" }; + public void setName(String s) { } } // FILE: k/test.kt @@ -11,13 +11,103 @@ public abstract class Super { package k import j.Super -abstract class Sub: Super() { +abstract class Sub : Super() { fun test(s: Super) { s.name s.getName() s.name = "" s.name = s.name s.setName("") + + val anon1 = object : Super() { + fun testAnon() { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + + val anon2 = object { + fun testAnon() { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + } + + inner class Nested1 : Super() { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + + class Nested2 { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } +} + +abstract class NonSub { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + + val anon1 = object : Super() { + fun testAnon() { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + + val anon2 = object { + fun testAnon() { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + } + + inner class Nested1 : Super() { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } + } + + class Nested2 { + fun test(s: Super) { + s.name + s.getName() + s.name = "" + s.name = s.name + s.setName("") + } } } diff --git a/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.txt b/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.txt index b8aeb94b676..a29ae6e8fba 100644 --- a/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.txt +++ b/compiler/testData/diagnostics/tests/properties/protectedGetterWithPublicSetter.txt @@ -2,12 +2,12 @@ package package j { - public abstract class Super { + public open class Super { public constructor Super() public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean - protected/*protected and package*/ abstract fun getName(): kotlin.String! + protected/*protected and package*/ open fun getName(): kotlin.String! public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int - public abstract fun setName(/*0*/ s: kotlin.String!): kotlin.Unit + public open fun setName(/*0*/ s: kotlin.String!): kotlin.Unit public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String } } @@ -15,13 +15,58 @@ package j { package k { public fun test(/*0*/ s: j.Super): kotlin.Unit + public abstract class NonSub { + public constructor NonSub() + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public final fun test(/*0*/ s: j.Super): kotlin.Unit + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + + public final inner class Nested1 : j.Super { + public constructor Nested1() + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + protected/*protected and package*/ open override /*1*/ /*fake_override*/ fun getName(): kotlin.String! + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun setName(/*0*/ s: kotlin.String!): kotlin.Unit + public final fun test(/*0*/ s: j.Super): kotlin.Unit + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + } + + public final class Nested2 { + public constructor Nested2() + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public final fun test(/*0*/ s: j.Super): kotlin.Unit + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + } + } + public abstract class Sub : j.Super { public constructor Sub() public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean - protected/*protected and package*/ abstract override /*1*/ /*fake_override*/ fun getName(): kotlin.String! + protected/*protected and package*/ open override /*1*/ /*fake_override*/ fun getName(): kotlin.String! public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int - public abstract override /*1*/ /*fake_override*/ fun setName(/*0*/ s: kotlin.String!): kotlin.Unit + public open override /*1*/ /*fake_override*/ fun setName(/*0*/ s: kotlin.String!): kotlin.Unit public final fun test(/*0*/ s: j.Super): kotlin.Unit public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + + public final inner class Nested1 : j.Super { + public constructor Nested1() + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + protected/*protected and package*/ open override /*1*/ /*fake_override*/ fun getName(): kotlin.String! + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun setName(/*0*/ s: kotlin.String!): kotlin.Unit + public final fun test(/*0*/ s: j.Super): kotlin.Unit + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + } + + public final class Nested2 { + public constructor Nested2() + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public final fun test(/*0*/ s: j.Super): kotlin.Unit + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String + } } } + diff --git a/compiler/testData/diagnostics/tests/syntheticExtensions/javaProperties/SetterHasHigherAccess.fir.kt b/compiler/testData/diagnostics/tests/syntheticExtensions/javaProperties/SetterHasHigherAccess.fir.kt index 7283894b885..aa794ecb83e 100644 --- a/compiler/testData/diagnostics/tests/syntheticExtensions/javaProperties/SetterHasHigherAccess.fir.kt +++ b/compiler/testData/diagnostics/tests/syntheticExtensions/javaProperties/SetterHasHigherAccess.fir.kt @@ -5,8 +5,8 @@ import JavaClass fun foo(javaClass: JavaClass) { val v = javaClass.something - javaClass.something = 1 - javaClass.something++ + javaClass.something = 1 + javaClass.something++ } // FILE: JavaClass.java