From 482865a4c02c5d2e83e4bc33b0f5c92226f82b54 Mon Sep 17 00:00:00 2001 From: Joe Roskopf <7951665+j-roskopf@users.noreply.github.com> Date: Fri, 12 Apr 2024 13:34:03 -0500 Subject: [PATCH 1/4] omit more permissive modifiers on restricted visibility types --- docs/changelog.md | 1 + .../com/squareup/kotlinpoet/CodeWriter.kt | 4 + .../com/squareup/kotlinpoet/TypeSpec.kt | 5 +- .../com/squareup/kotlinpoet/TypeSpecTest.kt | 90 +++++++++++++++++++ 4 files changed, 99 insertions(+), 1 deletion(-) diff --git a/docs/changelog.md b/docs/changelog.md index 2ba3014b3..cfcc9475a 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -20,6 +20,7 @@ Change Log * Fix: Throw if primary constructor delegates to other constructors (#1859). * Fix: Aliased imports with nested class (#1876). * Fix: Check for error types in `KSType.toClassName()` (#1890). +* Fix: Omit more permissive modifiers on restricted visibility types (#1301). ## Version 1.16.0 diff --git a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt index 4b855df5c..b8587ccd9 100644 --- a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt +++ b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt @@ -652,6 +652,10 @@ internal class CodeWriter constructor( return false } + if (implicitModifiers.contains(KModifier.INTERNAL)) { + return false + } + if (!implicitModifiers.contains(KModifier.PUBLIC)) { return false } diff --git a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/TypeSpec.kt b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/TypeSpec.kt index 677ab7406..9496f76f3 100644 --- a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/TypeSpec.kt +++ b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/TypeSpec.kt @@ -171,7 +171,7 @@ public class TypeSpec private constructor( codeWriter.emitAnnotations(annotations, false) codeWriter.emitModifiers( modifiers, - if (isNestedExternal) setOf(PUBLIC, EXTERNAL) else setOf(PUBLIC), + implicitModifiers + if (isNestedExternal) setOf(PUBLIC, EXTERNAL) else setOf(PUBLIC), ) codeWriter.emit(kind.declarationKeyword) if (name != null) { @@ -446,6 +446,7 @@ public class TypeSpec private constructor( ANNOTATION in modifiers -> emptySet() EXPECT in modifiers -> setOf(EXPECT) EXTERNAL in modifiers -> setOf(EXTERNAL) + INTERNAL in modifiers -> setOf(INTERNAL) else -> emptySet() } } @@ -454,6 +455,7 @@ public class TypeSpec private constructor( return defaultImplicitFunctionModifiers + when { EXPECT in modifiers -> setOf(EXPECT) EXTERNAL in modifiers -> setOf(EXTERNAL) + INTERNAL in modifiers -> setOf(INTERNAL) else -> emptySet() } } @@ -462,6 +464,7 @@ public class TypeSpec private constructor( return defaultImplicitTypeModifiers + when { EXPECT in modifiers -> setOf(EXPECT) EXTERNAL in modifiers -> setOf(EXTERNAL) + INTERNAL in modifiers -> setOf(INTERNAL) else -> emptySet() } } diff --git a/kotlinpoet/src/commonTest/kotlin/com/squareup/kotlinpoet/TypeSpecTest.kt b/kotlinpoet/src/commonTest/kotlin/com/squareup/kotlinpoet/TypeSpecTest.kt index 85c0c3b58..55083d698 100644 --- a/kotlinpoet/src/commonTest/kotlin/com/squareup/kotlinpoet/TypeSpecTest.kt +++ b/kotlinpoet/src/commonTest/kotlin/com/squareup/kotlinpoet/TypeSpecTest.kt @@ -5778,6 +5778,96 @@ class TypeSpecTest { }.hasMessageThat().isEqualTo("primary constructor can't delegate to other constructors") } + // https://github.com/square/kotlinpoet/issues/1301 + @Test fun permissiveModifiersOmittedOnRestrictedVisibilityTypesInSealedInterface() { + val pageStateClass = ClassName("", "FeaturePageState") + val type = + TypeSpec.interfaceBuilder("FeaturePageState") + .addModifiers(SEALED) + .addModifiers(INTERNAL) + .addType( + TypeSpec.objectBuilder("Loading") + .addModifiers(DATA) + .addSuperinterface(pageStateClass) + .build(), + ) + .addType( + TypeSpec.objectBuilder("Error") + .addModifiers(DATA) + .addSuperinterface(pageStateClass) + .build(), + ) + .addType( + TypeSpec.objectBuilder("Content") + .addModifiers(DATA) + .addSuperinterface(pageStateClass) + .build(), + ) + .build() + + //language=kotlin + assertThat(type.toString()).isEqualTo( + // no public visibility modifier on implicitly internal sealed types + """ + internal sealed interface FeaturePageState { + data object Loading : FeaturePageState + + data object Error : FeaturePageState + + data object Content : FeaturePageState + } + + """.trimIndent(), + ) + } + + @Test fun permissiveModifiersOmittedOnRestrictedVisibilityPropertyInClass() { + val type = TypeSpec.classBuilder("Taco") + .addModifiers(INTERNAL) + .addProperty("ints", IntArray::class) + .build() + + //language=kotlin + assertThat(toString(type)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.IntArray + + internal class Taco { + val ints: IntArray + } + + """.trimIndent(), + ) + } + + @Test fun permissiveModifiersOmittedOnRestrictedVisibilityFunctionInClass() { + val taco = TypeSpec.classBuilder("Taco") + .addModifiers(INTERNAL) + .addFunction( + FunSpec.builder("toString") + .addModifiers(KModifier.FINAL, KModifier.OVERRIDE) + .returns(String::class) + .addStatement("return %S", "taco") + .build(), + ) + .build() + + assertThat(toString(taco)).isEqualTo( + """ + |package com.squareup.tacos + | + |import kotlin.String + | + |internal class Taco { + | final override fun toString(): String = "taco" + |} + | + """.trimMargin(), + ) + } + companion object { private const val donutsPackage = "com.squareup.donuts" } From 613415d8d088166e872db09423a3f413f8603ba5 Mon Sep 17 00:00:00 2001 From: Joe Roskopf <7951665+j-roskopf@users.noreply.github.com> Date: Fri, 12 Apr 2024 16:20:19 -0500 Subject: [PATCH 2/4] additional coverage + better tests to cover matrix of 4 accessor modifiers --- .../com/squareup/kotlinpoet/CodeWriter.kt | 13 +- .../com/squareup/kotlinpoet/TypeSpec.kt | 6 + .../com/squareup/kotlinpoet/TypeSpecTest.kt | 334 ++++++++++++++++-- 3 files changed, 329 insertions(+), 24 deletions(-) diff --git a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt index b8587ccd9..44a2f499c 100644 --- a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt +++ b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt @@ -161,11 +161,19 @@ internal class CodeWriter constructor( emit(KModifier.PUBLIC.keyword) emit(" ") } + + // public always being an implicit modifier in addition to whatever we inherited. + // we don't want to throw away the implicit modifier we inherited + val implicitModifierContainsAtLeastTwoAccessMonitors = implicitModifiers.count { + it == KModifier.PUBLIC || it == KModifier.PRIVATE || it == KModifier.INTERNAL || it == KModifier.PROTECTED + } >= 2 + val uniqueNonPublicExplicitOnlyModifiers = modifiers .filterNot { it == KModifier.PUBLIC } - .filterNot { implicitModifiers.contains(it) } + .filterNot { implicitModifiers.contains(it) && implicitModifierContainsAtLeastTwoAccessMonitors.not() } .toEnumSet() + for (modifier in uniqueNonPublicExplicitOnlyModifiers) { emit(modifier.keyword) emit(" ") @@ -652,7 +660,8 @@ internal class CodeWriter constructor( return false } - if (implicitModifiers.contains(KModifier.INTERNAL)) { + if (implicitModifiers.contains(KModifier.INTERNAL) || implicitModifiers.contains(KModifier.PRIVATE) || implicitModifiers.contains(KModifier.PROTECTED)) { + // omit more permissive modifiers on more restrictive visibility modifiers return false } diff --git a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/TypeSpec.kt b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/TypeSpec.kt index 9496f76f3..0b8cfa4db 100644 --- a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/TypeSpec.kt +++ b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/TypeSpec.kt @@ -447,6 +447,8 @@ public class TypeSpec private constructor( EXPECT in modifiers -> setOf(EXPECT) EXTERNAL in modifiers -> setOf(EXTERNAL) INTERNAL in modifiers -> setOf(INTERNAL) + PRIVATE in modifiers -> setOf(PRIVATE) + PROTECTED in modifiers -> setOf(PROTECTED) else -> emptySet() } } @@ -456,6 +458,8 @@ public class TypeSpec private constructor( EXPECT in modifiers -> setOf(EXPECT) EXTERNAL in modifiers -> setOf(EXTERNAL) INTERNAL in modifiers -> setOf(INTERNAL) + PRIVATE in modifiers -> setOf(PRIVATE) + PROTECTED in modifiers -> setOf(PROTECTED) else -> emptySet() } } @@ -465,6 +469,8 @@ public class TypeSpec private constructor( EXPECT in modifiers -> setOf(EXPECT) EXTERNAL in modifiers -> setOf(EXTERNAL) INTERNAL in modifiers -> setOf(INTERNAL) + PRIVATE in modifiers -> setOf(PRIVATE) + PROTECTED in modifiers -> setOf(PROTECTED) else -> emptySet() } } diff --git a/kotlinpoet/src/commonTest/kotlin/com/squareup/kotlinpoet/TypeSpecTest.kt b/kotlinpoet/src/commonTest/kotlin/com/squareup/kotlinpoet/TypeSpecTest.kt index 55083d698..fadceb874 100644 --- a/kotlinpoet/src/commonTest/kotlin/com/squareup/kotlinpoet/TypeSpecTest.kt +++ b/kotlinpoet/src/commonTest/kotlin/com/squareup/kotlinpoet/TypeSpecTest.kt @@ -24,6 +24,7 @@ import com.squareup.kotlinpoet.KModifier.IN import com.squareup.kotlinpoet.KModifier.INNER import com.squareup.kotlinpoet.KModifier.INTERNAL import com.squareup.kotlinpoet.KModifier.PRIVATE +import com.squareup.kotlinpoet.KModifier.PROTECTED import com.squareup.kotlinpoet.KModifier.PUBLIC import com.squareup.kotlinpoet.KModifier.SEALED import com.squareup.kotlinpoet.KModifier.VARARG @@ -5822,20 +5823,155 @@ class TypeSpecTest { } @Test fun permissiveModifiersOmittedOnRestrictedVisibilityPropertyInClass() { - val type = TypeSpec.classBuilder("Taco") + val privateClass = TypeSpec.classBuilder("Taco") + .addModifiers(PRIVATE) + .addProperty("unspecifiedInts", IntArray::class) + .addProperty("privateInts", IntArray::class, PRIVATE) + .addProperty("protectedInts", IntArray::class, PROTECTED) + .addProperty("publicInts", IntArray::class, PUBLIC) + .addProperty("internalInts", IntArray::class, INTERNAL) + .build() + + //language=kotlin + assertThat(toString(privateClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.IntArray + + private class Taco { + val unspecifiedInts: IntArray + + private val privateInts: IntArray + + protected val protectedInts: IntArray + + public val publicInts: IntArray + + internal val internalInts: IntArray + } + + """.trimIndent(), + ) + + val internalClass = TypeSpec.classBuilder("Taco") .addModifiers(INTERNAL) - .addProperty("ints", IntArray::class) + .addProperty("unspecifiedInts", IntArray::class) + .addProperty("privateInts", IntArray::class, PRIVATE) + .addProperty("protectedInts", IntArray::class, PROTECTED) + .addProperty("publicInts", IntArray::class, PUBLIC) + .addProperty("internalInts", IntArray::class, INTERNAL) .build() //language=kotlin - assertThat(toString(type)).isEqualTo( + assertThat(toString(internalClass)).isEqualTo( """ package com.squareup.tacos import kotlin.IntArray internal class Taco { - val ints: IntArray + val unspecifiedInts: IntArray + + private val privateInts: IntArray + + protected val protectedInts: IntArray + + public val publicInts: IntArray + + internal val internalInts: IntArray + } + + """.trimIndent(), + ) + + val publicClass = TypeSpec.classBuilder("Taco") + .addModifiers(PUBLIC) + .addProperty("unspecifiedInts", IntArray::class) + .addProperty("privateInts", IntArray::class, PRIVATE) + .addProperty("protectedInts", IntArray::class, PROTECTED) + .addProperty("publicInts", IntArray::class, PUBLIC) + .addProperty("internalInts", IntArray::class, INTERNAL) + .build() + + //language=kotlin + assertThat(toString(publicClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.IntArray + + public class Taco { + public val unspecifiedInts: IntArray + + private val privateInts: IntArray + + protected val protectedInts: IntArray + + public val publicInts: IntArray + + internal val internalInts: IntArray + } + + """.trimIndent(), + ) + + val protectedClass = TypeSpec.classBuilder("Taco") + .addModifiers(PROTECTED) + .addProperty("unspecifiedInts", IntArray::class) + .addProperty("privateInts", IntArray::class, PRIVATE) + .addProperty("protectedInts", IntArray::class, PROTECTED) + .addProperty("publicInts", IntArray::class, PUBLIC) + .addProperty("internalInts", IntArray::class, INTERNAL) + .build() + + //language=kotlin + assertThat(toString(protectedClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.IntArray + + protected class Taco { + val unspecifiedInts: IntArray + + private val privateInts: IntArray + + protected val protectedInts: IntArray + + public val publicInts: IntArray + + internal val internalInts: IntArray + } + + """.trimIndent(), + ) + + val unspecifiedClass = TypeSpec.classBuilder("Taco") + .addProperty("unspecifiedInts", IntArray::class) + .addProperty("privateInts", IntArray::class, PRIVATE) + .addProperty("protectedInts", IntArray::class, PROTECTED) + .addProperty("publicInts", IntArray::class, PUBLIC) + .addProperty("internalInts", IntArray::class, INTERNAL) + .build() + + //language=kotlin + assertThat(toString(unspecifiedClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.IntArray + + public class Taco { + public val unspecifiedInts: IntArray + + private val privateInts: IntArray + + protected val protectedInts: IntArray + + public val publicInts: IntArray + + internal val internalInts: IntArray } """.trimIndent(), @@ -5843,28 +5979,182 @@ class TypeSpecTest { } @Test fun permissiveModifiersOmittedOnRestrictedVisibilityFunctionInClass() { - val taco = TypeSpec.classBuilder("Taco") + val unspecifiedVisibilityMethod = FunSpec.builder("unspecifiedStringMethod") + .returns(String::class) + .addStatement("return %S", "taco") + .build() + + val privateVisibilityMethod = FunSpec.builder("privateStringMethod") + .returns(String::class) + .addModifiers(PRIVATE) + .addStatement("return %S", "taco") + .build() + + val internalVisibilityMethod = FunSpec.builder("internalStringMethod") + .returns(String::class) .addModifiers(INTERNAL) - .addFunction( - FunSpec.builder("toString") - .addModifiers(KModifier.FINAL, KModifier.OVERRIDE) - .returns(String::class) - .addStatement("return %S", "taco") - .build(), - ) + .addStatement("return %S", "taco") .build() - assertThat(toString(taco)).isEqualTo( + val protectedVisibilityMethod = FunSpec.builder("protectedStringMethod") + .returns(String::class) + .addModifiers(PROTECTED) + .addStatement("return %S", "taco") + .build() + + val publicVisibilityMethod = FunSpec.builder("publicStringMethod") + .returns(String::class) + .addModifiers(PUBLIC) + .addStatement("return %S", "taco") + .build() + + val privateClass = TypeSpec.classBuilder("Taco") + .addModifiers(PRIVATE) + .addFunction(unspecifiedVisibilityMethod) + .addFunction(privateVisibilityMethod) + .addFunction(internalVisibilityMethod) + .addFunction(protectedVisibilityMethod) + .addFunction(publicVisibilityMethod) + .build() + + assertThat(toString(privateClass)).isEqualTo( """ - |package com.squareup.tacos - | - |import kotlin.String - | - |internal class Taco { - | final override fun toString(): String = "taco" - |} - | - """.trimMargin(), + package com.squareup.tacos + + import kotlin.String + + private class Taco { + fun unspecifiedStringMethod(): String = "taco" + + private fun privateStringMethod(): String = "taco" + + internal fun internalStringMethod(): String = "taco" + + protected fun protectedStringMethod(): String = "taco" + + public fun publicStringMethod(): String = "taco" + } + + """.trimIndent(), + ) + + val protectedClass = TypeSpec.classBuilder("Taco") + .addModifiers(PROTECTED) + .addFunction(unspecifiedVisibilityMethod) + .addFunction(privateVisibilityMethod) + .addFunction(internalVisibilityMethod) + .addFunction(protectedVisibilityMethod) + .addFunction(publicVisibilityMethod) + .build() + + assertThat(toString(protectedClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.String + + protected class Taco { + fun unspecifiedStringMethod(): String = "taco" + + private fun privateStringMethod(): String = "taco" + + internal fun internalStringMethod(): String = "taco" + + protected fun protectedStringMethod(): String = "taco" + + public fun publicStringMethod(): String = "taco" + } + + """.trimIndent(), + ) + + val internalClass = TypeSpec.classBuilder("Taco") + .addModifiers(INTERNAL) + .addFunction(unspecifiedVisibilityMethod) + .addFunction(privateVisibilityMethod) + .addFunction(internalVisibilityMethod) + .addFunction(protectedVisibilityMethod) + .addFunction(publicVisibilityMethod) + .build() + + assertThat(toString(internalClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.String + + internal class Taco { + fun unspecifiedStringMethod(): String = "taco" + + private fun privateStringMethod(): String = "taco" + + internal fun internalStringMethod(): String = "taco" + + protected fun protectedStringMethod(): String = "taco" + + public fun publicStringMethod(): String = "taco" + } + + """.trimIndent(), + ) + + val publicClass = TypeSpec.classBuilder("Taco") + .addModifiers(PUBLIC) + .addFunction(unspecifiedVisibilityMethod) + .addFunction(privateVisibilityMethod) + .addFunction(internalVisibilityMethod) + .addFunction(protectedVisibilityMethod) + .addFunction(publicVisibilityMethod) + .build() + + assertThat(toString(publicClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.String + + public class Taco { + public fun unspecifiedStringMethod(): String = "taco" + + private fun privateStringMethod(): String = "taco" + + internal fun internalStringMethod(): String = "taco" + + protected fun protectedStringMethod(): String = "taco" + + public fun publicStringMethod(): String = "taco" + } + + """.trimIndent(), + ) + + val unspecifiedClass = TypeSpec.classBuilder("Taco") + .addFunction(unspecifiedVisibilityMethod) + .addFunction(privateVisibilityMethod) + .addFunction(internalVisibilityMethod) + .addFunction(protectedVisibilityMethod) + .addFunction(publicVisibilityMethod) + .build() + + assertThat(toString(unspecifiedClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.String + + public class Taco { + public fun unspecifiedStringMethod(): String = "taco" + + private fun privateStringMethod(): String = "taco" + + internal fun internalStringMethod(): String = "taco" + + protected fun protectedStringMethod(): String = "taco" + + public fun publicStringMethod(): String = "taco" + } + + """.trimIndent(), ) } From 77d9465a477816362418946fa31de6c2ee465236 Mon Sep 17 00:00:00 2001 From: Joe Roskopf <7951665+j-roskopf@users.noreply.github.com> Date: Mon, 15 Apr 2024 06:53:18 -0500 Subject: [PATCH 3/4] use utility containsAnyOf --- .../src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt index 44a2f499c..d9f456d49 100644 --- a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt +++ b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt @@ -660,7 +660,7 @@ internal class CodeWriter constructor( return false } - if (implicitModifiers.contains(KModifier.INTERNAL) || implicitModifiers.contains(KModifier.PRIVATE) || implicitModifiers.contains(KModifier.PROTECTED)) { + if (implicitModifiers.containsAnyOf(KModifier.INTERNAL, KModifier.PRIVATE, KModifier.PROTECTED)) { // omit more permissive modifiers on more restrictive visibility modifiers return false } From 617f5a803c24c72e60814500e9d95ec116b16848 Mon Sep 17 00:00:00 2001 From: Joe Roskopf <7951665+j-roskopf@users.noreply.github.com> Date: Mon, 15 Apr 2024 07:36:51 -0500 Subject: [PATCH 4/4] fix modifier typo --- .../commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt index d9f456d49..28bcc6843 100644 --- a/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt +++ b/kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/CodeWriter.kt @@ -164,14 +164,14 @@ internal class CodeWriter constructor( // public always being an implicit modifier in addition to whatever we inherited. // we don't want to throw away the implicit modifier we inherited - val implicitModifierContainsAtLeastTwoAccessMonitors = implicitModifiers.count { + val implicitModifierContainsAtLeastTwoAccessModifiers = implicitModifiers.count { it == KModifier.PUBLIC || it == KModifier.PRIVATE || it == KModifier.INTERNAL || it == KModifier.PROTECTED } >= 2 val uniqueNonPublicExplicitOnlyModifiers = modifiers .filterNot { it == KModifier.PUBLIC } - .filterNot { implicitModifiers.contains(it) && implicitModifierContainsAtLeastTwoAccessMonitors.not() } + .filterNot { implicitModifiers.contains(it) && implicitModifierContainsAtLeastTwoAccessModifiers.not() } .toEnumSet() for (modifier in uniqueNonPublicExplicitOnlyModifiers) {