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.

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

Feature/omit non private modifiers on private types #1398

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 30 additions & 2 deletions kotlinpoet/src/main/java/com/squareup/kotlinpoet/TypeSpec.kt
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import com.squareup.kotlinpoet.KModifier.PUBLIC
import com.squareup.kotlinpoet.KModifier.SEALED
import com.squareup.kotlinpoet.KModifier.VALUE
import java.lang.reflect.Type
import java.util.EnumSet
import javax.lang.model.element.Element
import kotlin.reflect.KClass

Expand Down Expand Up @@ -299,10 +300,10 @@ public class TypeSpec private constructor(
}

// Functions.
for (funSpec in funSpecs) {
for (funSpec in funSpecsOmittingVisibility()) {
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 it makes sense to filter out the modifiers both here and when passing the implicit modifiers to funSpec.emit() - maybe we should simply add unnecessary visibility modifiers to implicitModifiers if the type is private? Any reason why that wouldn't work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hopefully that also adds support for properties which I suspect exhibit the same behavior.

Copy link
Author

@polarene polarene Oct 28, 2022

Choose a reason for hiding this comment

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

Honestly, I don't know the library this deep... I tried to reverse engineer how the mechanism works and found out that filtering here before emit() caused the least amount of code changes. What would you suggest as a better approach? Also, I limited the scope to functions only, following the issue description, but if you want we can extend it to properties as well, need to double-check.

Copy link
Author

@polarene polarene Oct 28, 2022

Choose a reason for hiding this comment

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

I think when I did the initial implementation, I tried to add visibility modifiers to implicitModifiers but that didn't work for PUBLIC, since it undergoes a contrived logic I couldn't fully grasp and didn't feel confident modifying that, so I turned to this approach.

Copy link
Author

Choose a reason for hiding this comment

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

@Egorand so what should we do with this implementation, do you have any advice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the least amount of changes should not be the priority here, and your PR IMO adds complexity rather than reducing it. I believe implicitModifiers is the right tool for the job, and it'd be great to remove the obstacles preventing it from working correctly - e.g. the custom handling of PUBLIC should probably be removed in favour of achieving the same result with implicitModifiers. This will likely require deeper knowledge of the library's inner workings and understand if you don't have the time to invest, but I don't think current approach is the best one.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thanks for your feedback. You're right, this solution increases complexity and it was a quick workaround for implicitModifiers not working as expected. I'll try to dedicate some time to this issue and rework the solution following your guidelines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you!

if (funSpec.isConstructor) continue
if (!firstMember) codeWriter.emit("\n")
funSpec.emit(codeWriter, name, kind.implicitFunctionModifiers(modifiers + implicitModifiers), true)
funSpec.emit(codeWriter, name, implicitFunctionModifiers(implicitModifiers), true)
firstMember = false
}

Expand All @@ -324,6 +325,33 @@ public class TypeSpec private constructor(
}
}

/**
* Returns the list of [FunSpec], optionally removing all non-private visibility modifiers
* if this is a private type.
*/
private fun funSpecsOmittingVisibility(): List<FunSpec> {
return if (PRIVATE in modifiers) {
val omitted = EnumSet.of(PUBLIC, INTERNAL)
funSpecs.map {
it.toBuilder().apply { modifiers.removeAll(omitted) }.build()
}
} else {
funSpecs
}
}

/**
* Returns the implicit function modifiers for this kind, optionally removing PUBLIC
* if this is a private type.
*/
private fun implicitFunctionModifiers(implicitModifiers: Set<KModifier>): Set<KModifier> {
val fModifiers = kind.implicitFunctionModifiers(modifiers + implicitModifiers).toMutableSet()
if (PRIVATE in modifiers) {
fModifiers -= PUBLIC
}
return fModifiers
}

/** Returns the properties that can be declared inline as constructor parameters. */
private fun constructorProperties(): Map<String, PropertySpec> {
if (primaryConstructor == null) return emptyMap()
Expand Down
33 changes: 33 additions & 0 deletions kotlinpoet/src/test/java/com/squareup/kotlinpoet/TypeSpecTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5358,6 +5358,39 @@ class TypeSpecTest {
assertThat(t).hasMessageThat().contains("contextReceivers can only be applied on simple classes")
}

// https://github.com/square/kotlinpoet/issues/1301
@Test fun `function omits non-private modifiers on private type`() {
val taco = TypeSpec.classBuilder("Taco")
.addModifiers(PRIVATE)
.addFunctions(
listOf(
FunSpec.builder("f1").addModifiers(PUBLIC).build(),
FunSpec.builder("f2").addModifiers(INTERNAL).build(),
FunSpec.builder("f3").addModifiers(PRIVATE).build(),
),
)
.build()
assertThat(toString(taco)).isEqualTo(
"""
|package com.squareup.tacos
|
|import kotlin.Unit
|
|private class Taco {
| fun f1(): Unit {
| }
|
| fun f2(): Unit {
| }
|
| private fun f3(): Unit {
| }
|}
|
""".trimMargin(),
)
}

companion object {
private const val donutsPackage = "com.squareup.donuts"
}
Expand Down