Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Omit public modifier on override function or constructor parameters #1550

Merged
merged 9 commits into from
May 19, 2023
Merged

Omit public modifier on override function or constructor parameters #1550

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0cbf608
Omit public modifier on override function or constructor parameters
Omico May 16, 2023
d142e18
Fix tests in the kotlinx-metadata module
Omico May 16, 2023
9ce8fa1
Update comment
Omico May 17, 2023
51f1b28
Rollback some tests
Omico May 17, 2023
4349553
Only omit public modifier if it inside of implicit modifiers, and mod…
Omico May 17, 2023
d64e2f3
Rollback anonymousClassToString() test
Omico May 18, 2023
8a80706
Add more test
Omico May 18, 2023
8eb98ac
Remove comment
Omico May 18, 2023
ad27b03
Update comments
Omico May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 23 additions & 23 deletions ...ata/src/test/kotlin/com/squareup/kotlinpoet/metadata/specs/KotlinPoetMetadataSpecsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,14 +426,14 @@ class KotlinPoetMetadataSpecsTest : MultiClassInspectorTest() {
assertThat(typeSpec.trimmedToString()).isEqualTo(
"""
public abstract class OverriddenThings() : com.squareup.kotlinpoet.metadata.specs.KotlinPoetMetadataSpecsTest.OverriddenThingsBase(), com.squareup.kotlinpoet.metadata.specs.KotlinPoetMetadataSpecsTest.OverriddenThingsInterface {
public override var openProp: kotlin.String = throw NotImplementedError("Stub!")
override var openProp: kotlin.String = throw NotImplementedError("Stub!")

public override var openPropInterface: kotlin.String = throw NotImplementedError("Stub!")
override var openPropInterface: kotlin.String = throw NotImplementedError("Stub!")

public override fun openFunction(): kotlin.Unit {
override fun openFunction(): kotlin.Unit {
}

public override fun openFunctionInterface(): kotlin.Unit {
override fun openFunctionInterface(): kotlin.Unit {
}
}
""".trimIndent(),
Expand Down Expand Up @@ -551,13 +551,13 @@ class KotlinPoetMetadataSpecsTest : MultiClassInspectorTest() {
public val `value`: kotlin.String,
) {
FOO {
public override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
},
BAR {
public override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
},
BAZ {
public override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
},
;
}
Expand Down Expand Up @@ -616,14 +616,14 @@ class KotlinPoetMetadataSpecsTest : MultiClassInspectorTest() {
public val `value`: kotlin.String,
) {
FOO {
public override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
},
@com.squareup.kotlinpoet.metadata.specs.KotlinPoetMetadataSpecsTest.FieldAnnotation
BAR {
public override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
},
BAZ {
public override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
override fun toString(): kotlin.String = throw NotImplementedError("Stub!")
},
;
}
Expand Down Expand Up @@ -687,11 +687,11 @@ class KotlinPoetMetadataSpecsTest : MultiClassInspectorTest() {
assertThat(subInterfaceSpec.trimmedToString()).isEqualTo(
"""
public interface SubInterface : com.squareup.kotlinpoet.metadata.specs.KotlinPoetMetadataSpecsTest.TestInterface {
public override fun hasDefault(): kotlin.Unit {
override fun hasDefault(): kotlin.Unit {
}

@kotlin.jvm.JvmDefault
public override fun hasJvmDefault(): kotlin.Unit {
override fun hasJvmDefault(): kotlin.Unit {
}

public fun subInterfaceFunction(): kotlin.Unit {
Expand All @@ -706,13 +706,13 @@ class KotlinPoetMetadataSpecsTest : MultiClassInspectorTest() {
assertThat(implSpec.trimmedToString()).isEqualTo(
"""
public class TestSubInterfaceImpl() : com.squareup.kotlinpoet.metadata.specs.KotlinPoetMetadataSpecsTest.SubInterface {
public override fun noDefault(): kotlin.Unit {
override fun noDefault(): kotlin.Unit {
}

public override fun noDefaultWithInput(input: kotlin.String): kotlin.Unit {
override fun noDefaultWithInput(input: kotlin.String): kotlin.Unit {
}

public override fun noDefaultWithInputDefault(input: kotlin.String): kotlin.Unit {
override fun noDefaultWithInputDefault(input: kotlin.String): kotlin.Unit {
}
}
""".trimIndent(),
Expand Down Expand Up @@ -2100,14 +2100,14 @@ class KotlinPoetMetadataSpecsTest : MultiClassInspectorTest() {
public abstract class AbstractModalities() : com.squareup.kotlinpoet.metadata.specs.KotlinPoetMetadataSpecsTest.ModalitiesInterface {
public val implicitFinalProp: kotlin.String? = null

public override val interfaceProp: kotlin.String? = null
override val interfaceProp: kotlin.String? = null

public open val openProp: kotlin.String? = null

public fun implicitFinalFun(): kotlin.Unit {
}

public override fun interfaceFun(): kotlin.Unit {
override fun interfaceFun(): kotlin.Unit {
}

public open fun openFun(): kotlin.Unit {
Expand All @@ -2122,9 +2122,9 @@ class KotlinPoetMetadataSpecsTest : MultiClassInspectorTest() {
assertThat(finalAbstractModalities.trimmedToString()).isEqualTo(
"""
public abstract class FinalAbstractModalities() : com.squareup.kotlinpoet.metadata.specs.KotlinPoetMetadataSpecsTest.ModalitiesInterface {
public final override val interfaceProp: kotlin.String? = null
final override val interfaceProp: kotlin.String? = null

public final override fun interfaceFun(): kotlin.Unit {
final override fun interfaceFun(): kotlin.Unit {
}
}
""".trimIndent(),
Expand All @@ -2136,14 +2136,14 @@ class KotlinPoetMetadataSpecsTest : MultiClassInspectorTest() {
assertThat(modalities.trimmedToString()).isEqualTo(
"""
public class Modalities() : com.squareup.kotlinpoet.metadata.specs.KotlinPoetMetadataSpecsTest.AbstractModalities() {
public override val interfaceProp: kotlin.String? = null
override val interfaceProp: kotlin.String? = null

public override val openProp: kotlin.String? = null
override val openProp: kotlin.String? = null

public override fun interfaceFun(): kotlin.Unit {
override fun interfaceFun(): kotlin.Unit {
}

public override fun openFun(): kotlin.Unit {
override fun openFun(): kotlin.Unit {
}
}
""".trimIndent(),
Expand Down
7 changes: 7 additions & 0 deletions kotlinpoet/src/main/java/com/squareup/kotlinpoet/CodeWriter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,9 @@ internal class CodeWriter constructor(
*
* If [modifiers] contains [KModifier.PUBLIC], this method always returns `true`.
*
* Next, if [implicitModifiers] contains [KModifier.PUBLIC] and [modifiers] contains
Omico marked this conversation as resolved.
Show resolved Hide resolved
* [KModifier.OVERRIDE], this method will return `false`.
*
* Otherwise, this will return `true` when [KModifier.PUBLIC] is one of the [implicitModifiers]
* and there are no other opposing modifiers (like [KModifier.PROTECTED] etc.) supplied by the
* consumer in [modifiers].
Expand All @@ -638,6 +641,10 @@ internal class CodeWriter constructor(
return true
}

if (implicitModifiers.contains(KModifier.PUBLIC) && modifiers.contains(KModifier.OVERRIDE)) {
return false
}

if (!implicitModifiers.contains(KModifier.PUBLIC)) {
return false
}
Expand Down
4 changes: 2 additions & 2 deletions kotlinpoet/src/test/java/com/squareup/kotlinpoet/AnnotationSpecTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,9 +565,9 @@ class AnnotationSpecTest {
)

public object ExternalClassParceler : Parceler<ExternalClass> {
public override fun create(parcel: Parcel) = ExternalClass(parcel.readInt())
override fun create(parcel: Parcel) = ExternalClass(parcel.readInt())

public override fun ExternalClass.write(parcel: Parcel, flags: Int): Unit {
override fun ExternalClass.write(parcel: Parcel, flags: Int): Unit {
parcel.writeInt(value)
}
}
Expand Down
2 changes: 1 addition & 1 deletion kotlinpoet/src/test/java/com/squareup/kotlinpoet/MemberNameTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ class MemberNameTest {
import kotlin.hashCode

public class Message {
public override fun hashCode(): Int {
override fun hashCode(): Int {
var result = super.hashCode
if (result == 0) {
result = result * 37 + embedded_message.hashCode()
Expand Down
83 changes: 80 additions & 3 deletions kotlinpoet/src/test/java/com/squareup/kotlinpoet/TypeSpecTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4133,9 +4133,9 @@ class TypeSpecTest {
|import kotlin.String
|
|public data class Person(
| public override val id: Int,
| public override val name: String,
| public override val surname: String,
| override val id: Int,
| override val name: String,
| override val surname: String,
|)
|
""".trimMargin(),
Expand Down Expand Up @@ -5336,6 +5336,83 @@ class TypeSpecTest {
)
}

// https://github.com/square/kotlinpoet/issues/1548
@Test fun overrideInternalAbstractFunctionVisibility() {
val baseClass = TypeSpec.classBuilder("Base")
.addModifiers(PUBLIC, ABSTRACT)
.addFunction(
FunSpec.builder("foo")
.addModifiers(INTERNAL, ABSTRACT)
.build(),
)
.build()
assertThat(baseClass.toString()).isEqualTo(
"""
|public abstract class Base {
| internal abstract fun foo(): kotlin.Unit
|}
|
""".trimMargin(),
)
val bassClassName = ClassName("", "Base")
val exampleClass = TypeSpec.classBuilder("Example")
.addModifiers(PUBLIC)
.superclass(bassClassName)
.addFunction(
FunSpec.builder("foo")
.addModifiers(KModifier.OVERRIDE)
.build(),
)
.build()
assertThat(exampleClass.toString()).isEqualTo(
"""
|public class Example : Base() {
| override fun foo(): kotlin.Unit {
| }
|}
|
""".trimMargin(),
)
val example2Class = TypeSpec.classBuilder("Example2")
.addModifiers(PUBLIC)
.superclass(bassClassName)
.addFunction(
FunSpec.builder("foo")
.addModifiers(PUBLIC, KModifier.OVERRIDE)
.build(),
)
.build()
// Don't omit the public modifier here. The bytecode is different.
Omico marked this conversation as resolved.
Show resolved Hide resolved
assertThat(example2Class.toString()).isEqualTo(
"""
|public class Example2 : Base() {
| public override fun foo(): kotlin.Unit {
| }
|}
|
""".trimMargin(),
)
val example3Class = TypeSpec.classBuilder("Example3")
.addModifiers(INTERNAL)
.superclass(bassClassName)
.addFunction(
FunSpec.builder("foo")
.addModifiers(PUBLIC, KModifier.OVERRIDE)
.build(),
)
.build()
// We cannot omit the public modifier here. The bytecode is different.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand what "The bytecode is different" means in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is what it means. Generated by Kotlin Bytecode from IntelliJ IDEA

image

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. If the intent is to explain why the "public" modifier can't be omitted, I don't think the reference to bytecode is very helpful. I'll provide suggestions on how to reword this, but please let me know if I misunderstood your intent.

Omico marked this conversation as resolved.
Show resolved Hide resolved
assertThat(example3Class.toString()).isEqualTo(
"""
|internal class Example3 : Base() {
| public override fun foo(): kotlin.Unit {
| }
|}
|
""".trimMargin(),
)
}

@Test fun contextReceiver() {
val typeSpec = TypeSpec.classBuilder("Example")
.contextReceivers(STRING)
Expand Down