diff --git a/docs/changelog.md b/docs/changelog.md index 2ba3014b39..cfcc9475a9 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 4b855df5cc..28bcc68437 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 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) } + .filterNot { implicitModifiers.contains(it) && implicitModifierContainsAtLeastTwoAccessModifiers.not() } .toEnumSet() + for (modifier in uniqueNonPublicExplicitOnlyModifiers) { emit(modifier.keyword) emit(" ") @@ -652,6 +660,11 @@ internal class CodeWriter constructor( return false } + if (implicitModifiers.containsAnyOf(KModifier.INTERNAL, KModifier.PRIVATE, KModifier.PROTECTED)) { + // omit more permissive modifiers on more restrictive visibility modifiers + 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 677ab74067..0b8cfa4db2 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,9 @@ public class TypeSpec private constructor( ANNOTATION in modifiers -> emptySet() 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() } } @@ -454,6 +457,9 @@ public class TypeSpec private constructor( return defaultImplicitFunctionModifiers + when { 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() } } @@ -462,6 +468,9 @@ public class TypeSpec private constructor( return defaultImplicitTypeModifiers + when { 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 85c0c3b58a..fadceb8740 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 @@ -5778,6 +5779,385 @@ 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 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("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(internalClass)).isEqualTo( + """ + package com.squareup.tacos + + import kotlin.IntArray + + internal class Taco { + 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(), + ) + } + + @Test fun permissiveModifiersOmittedOnRestrictedVisibilityFunctionInClass() { + 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) + .addStatement("return %S", "taco") + .build() + + 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 + + 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(), + ) + } + companion object { private const val donutsPackage = "com.squareup.donuts" }