Repository: square/kotlinpoet
Issue: Duplicate public modifier when adding Modifier.PUBLIC to a Property's getter or setter

I've pushed a failing test here: https://github.com/square/kotlinpoet/compare/master...sjudd:kotlinpoet:fix_duplicate_modifiers_properties.

The test output is:

```
expected:
    var foo: kotlin.String
      public get() = "_foo"
      public set(`value`) {
      }
    
but was:
    var foo: kotlin.String
      public public get() = "_foo"
      public public set(`value`) {
      }
```

I'll take a look at fixing this as well. 

Changed files:
kotlinpoet/src/test/java/com/squareup/kotlinpoet/FunSpecTest.kt, kotlinpoet/src/test/java/com/squareup/kotlinpoet/PropertySpecTest.kt, kotlinpoet/src/main/java/com/squareup/kotlinpoet/CodeWriter.kt

diff --git a/kotlinpoet/src/main/java/com/squareup/kotlinpoet/CodeWriter.kt b/kotlinpoet/src/main/java/com/squareup/kotlinpoet/CodeWriter.kt
index 40000b5c..4f017bad 100644
--- a/kotlinpoet/src/main/java/com/squareup/kotlinpoet/CodeWriter.kt
+++ b/kotlinpoet/src/main/java/com/squareup/kotlinpoet/CodeWriter.kt
@@ -160,8 +160,12 @@ internal class CodeWriter constructor(
       emit(KModifier.PUBLIC.keyword)
       emit(" ")
     }
-    for (modifier in modifiers.toEnumSet()) {
-      if (modifier in implicitModifiers) continue
+    val uniqueNonPublicExplicitOnlyModifiers =
+      modifiers
+        .filterNot { it == KModifier.PUBLIC }
+        .filterNot { implicitModifiers.contains(it) }
+        .toEnumSet()
+    for (modifier in uniqueNonPublicExplicitOnlyModifiers) {
       emit(modifier.keyword)
       emit(" ")
     }
@@ -624,9 +628,11 @@ internal class CodeWriter constructor(
   /**
    * Returns whether a [KModifier.PUBLIC] should be emitted.
    *
-   * 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.
+   * If [modifiers] contains [KModifier.PUBLIC], this method always returns `true`.
+   *
+   * 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].
    */
   private fun shouldEmitPublicModifier(
     modifiers: Set<KModifier>,
@@ -640,10 +646,10 @@ internal class CodeWriter constructor(
       return false
     }
 
-    val containsOtherVisibility =
+    val hasOtherConsumerSpecifiedVisibility =
       modifiers.containsAnyOf(KModifier.PRIVATE, KModifier.INTERNAL, KModifier.PROTECTED)
 
-    return !containsOtherVisibility
+    return !hasOtherConsumerSpecifiedVisibility
   }
 
   /**
diff --git a/kotlinpoet/src/test/java/com/squareup/kotlinpoet/FunSpecTest.kt b/kotlinpoet/src/test/java/com/squareup/kotlinpoet/FunSpecTest.kt
index c06c3e96..1376cafc 100644
--- a/kotlinpoet/src/test/java/com/squareup/kotlinpoet/FunSpecTest.kt
+++ b/kotlinpoet/src/test/java/com/squareup/kotlinpoet/FunSpecTest.kt
@@ -674,6 +674,69 @@ class FunSpecTest {
     )
   }
 
+  @Test fun setterWithPublicModifier() {
+    val funSpec = FunSpec.setterBuilder()
+      .addParameter("value", String::class.asClassName())
+      .addStatement("this.value = this.value")
+      .addModifiers(KModifier.PUBLIC)
+      .build()
+
+    assertThat(funSpec.toString()).isEqualTo(
+      """
+      |public set(`value`) {
+      |  this.value = this.value
+      |}
+      |
+      """.trimMargin(),
+    )
+  }
+
+  @Test fun getterWithPublicModifier() {
+    val funSpec = FunSpec.getterBuilder()
+      .addStatement("return value")
+      .addModifiers(KModifier.PUBLIC)
+      .build()
+
+    assertThat(funSpec.toString()).isEqualTo(
+      """
+      |public get() = value
+      |
+      """.trimMargin(),
+    )
+  }
+
+  // This does not produce correct Kotlin, but it does at least verify that we do not drop the
+  // explicitly specified public modifier.
+  @Test fun methodWithMultipleVisibilityModifiers() {
+    val funSpec =
+      FunSpec.builder("myMethod")
+        .addModifiers(KModifier.PUBLIC, KModifier.INTERNAL, KModifier.PRIVATE)
+        .build()
+
+    assertThat(funSpec.toString()).isEqualTo(
+      """
+      |public private internal fun myMethod(): kotlin.Unit {
+      |}
+      |
+      """.trimMargin(),
+    )
+  }
+
+  @Test fun methodWithRepeatedVisibilityModifier() {
+    val funSpec =
+      FunSpec.builder("myMethod")
+        .addModifiers(KModifier.PUBLIC, KModifier.PUBLIC, KModifier.PUBLIC)
+        .build()
+
+    assertThat(funSpec.toString()).isEqualTo(
+      """
+      |public fun myMethod(): kotlin.Unit {
+      |}
+      |
+      """.trimMargin(),
+    )
+  }
+
   @Test fun thisConstructorDelegate() {
     val funSpec = FunSpec.constructorBuilder()
       .addParameter("list", List::class.parameterizedBy(Int::class))
diff --git a/kotlinpoet/src/test/java/com/squareup/kotlinpoet/PropertySpecTest.kt b/kotlinpoet/src/test/java/com/squareup/kotlinpoet/PropertySpecTest.kt
index fd61d896..5eda0f78 100644
--- a/kotlinpoet/src/test/java/com/squareup/kotlinpoet/PropertySpecTest.kt
+++ b/kotlinpoet/src/test/java/com/squareup/kotlinpoet/PropertySpecTest.kt
@@ -20,6 +20,7 @@ import com.squareup.kotlinpoet.FunSpec.Companion.GETTER
 import com.squareup.kotlinpoet.FunSpec.Companion.SETTER
 import com.squareup.kotlinpoet.KModifier.EXTERNAL
 import com.squareup.kotlinpoet.KModifier.PRIVATE
+import com.squareup.kotlinpoet.KModifier.PUBLIC
 import com.squareup.kotlinpoet.ParameterizedTypeName.Companion.parameterizedBy
 import java.io.Serializable
 import java.util.function.Function
@@ -114,6 +115,34 @@ class PropertySpecTest {
     }.hasMessageThat().isEqualTo("external getter cannot have code")
   }
 
+  @Test fun publicGetterAndSetter() {
+    val prop = PropertySpec.builder("foo", String::class)
+      .mutable()
+      .getter(
+        FunSpec.getterBuilder()
+          .addModifiers(PUBLIC)
+          .addStatement("return %S", "_foo")
+          .build(),
+      )
+      .setter(
+        FunSpec.setterBuilder()
+          .addModifiers(PUBLIC)
+          .addParameter("value", String::class)
+          .build(),
+      )
+      .build()
+
+    assertThat(prop.toString()).isEqualTo(
+      """
+      |var foo: kotlin.String
+      |  public get() = "_foo"
+      |  public set(`value`) {
+      |  }
+      |
+      """.trimMargin(),
+    )
+  }
+
   @Test fun inlineSingleAccessorVal() {
     val prop = PropertySpec.builder("foo", String::class)
       .getter(