Enum members should not reveal constructor calls #2468

cosinekitty · 2022-04-25T19:40:29Z

Describe the bug
I have a Kotlin project that includes several enum classes. There is a single source file astronomy.kt that contains all the code and documentation comments. I am using Dokka to generate "GitHub Flavored Markdown" (gfm). When Dokka encounters an enum declaration like this...

/**
 * The enumeration of celestial bodies supported by Astronomy Engine.
 */
enum class Body(
    internal val massProduct: Double?,
    internal val orbitalPeriod: Double?,
    internal val vsopModel: VsopModel?
) {
    /**
     * The planet Mercury.
     */
    Mercury(
        MERCURY_GM,
        87.969,
        VsopModel(vsopLonMercury, vsopLatMercury, vsopRadMercury)
    ),

    /**
     * The planet Venus.
     */
    Venus(
        VENUS_GM,
        224.701,
        VsopModel(vsopLonVenus, vsopLatVenus, vsopRadVenus)
    ),

    /*... etc ... */
}

... it inappropriately generates markdown that includes internal implementation details. Specifically, the members are listed with the internal details of the constructor calls. Enum constructors are required to be private, and in my case, the properties I am initializing are all marked internal. Here is an example of how the above Kotlin declaration for Venus gets converted to Markdown:

| [Venus](-venus/index.md) | [jvm]<br>[Venus](-venus/index.md)(VENUS_GM, 224.701, VsopModel(vsopLonVenus, vsopLatVenus, vsopRadVenus))<br>The planet Venus. |

Expected behaviour
The example for Venus above should look like this:

| [Venus](-venus/index.md) | [jvm]<br>The planet Venus. |

Outside users should not see internal implementation details of the enum member's construction. In general, I don't want source code leaking into my documentation; only the public parts of the interface should be exposed.

Screenshots
N/A

To Reproduce

Clone the Astronomy Engine repository and change into its directory.
git checkout kotlin
cd source/kotlin
./gradlew dokkaGfm
Look at the file source/kotlin/build/dokka/gfm/astronomy/io.github.cosinekitty.astronomy/-body/index.md. All of the internal constructor parameters are visible in the documentation. The same thing occurs in the markdown files for each individual enum member, such as source/kotlin/build/dokka/gfm/astronomy/io.github.cosinekitty.astronomy/-body/-venus/index.md.

Dokka configuration
Configuration of dokka used to reproduce the bug:

plugins {
    java
    kotlin("jvm") version "1.6.10"
    `maven-publish`
    id("org.jetbrains.dokka") version "1.6.10"
}

group = "io.github.cosinekitty"
version = "0.0.1"

repositories {
    mavenCentral()
}

dependencies {
    dokkaHtmlPlugin("org.jetbrains.dokka:kotlin-as-java-plugin:1.6.10")
    val junit5Version = "5.8.2"
    testImplementation("org.junit.jupiter:junit-jupiter-api:$junit5Version")
    testImplementation("org.junit.jupiter:junit-jupiter-params:$junit5Version")
    testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:$junit5Version")
    testImplementation(kotlin("test"))
}

tasks.test {
    useJUnitPlatform()
}

configure<JavaPluginExtension> {
    sourceCompatibility = JavaVersion.VERSION_1_8
}

val sourceJar by tasks.creating(Jar::class) {
    dependsOn(tasks["classes"])
    archiveClassifier.set("sources")
    from(sourceSets["main"].allSource)
}

task("fatJar", type = Jar::class) {
    from(configurations.runtimeClasspath.get().map(::zipTree))
    duplicatesStrategy = DuplicatesStrategy.EXCLUDE
    with(tasks.jar.get())
}

publishing {
    publications {
        register("mavenJava", MavenPublication::class) {
            from(components["kotlin"])
            artifact(sourceJar)
        }
    }
}

tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>().configureEach {
    kotlinOptions {
        allWarningsAsErrors = true
    }
}

tasks.dokkaGfm.configure {
    dokkaSourceSets {
        named("main") {
            includeNonPublic.set(false)
            reportUndocumented.set(true)
        }
    }
}

Installation

Operating system: macOS/Windows/Linux: The bug happens in all 3: macOS, Windows 10, and multiple flavors of Debian Linux.
Build tool: Gradle v7.4.1
Dokka version: 1.6.10

Additional context

$ ./gradlew --version

------------------------------------------------------------
Gradle 7.4.1
------------------------------------------------------------

Build time:   2022-03-09 15:04:47 UTC
Revision:     36dc52588e09b4b72f2010bc07599e0ee0434e2e

Kotlin:       1.5.31
Groovy:       3.0.9
Ant:          Apache Ant(TM) version 1.10.11 compiled on July 10 2021
JVM:          11.0.13 (JetBrains s.r.o. 11.0.13+7-b1751.25)
OS:           Linux 4.19.0-20-amd64 amd64

Are you willing to provide a PR?
I am happy to provide whatever help the maintainers could use to reproduce/diagnose this bug. Thank you!

The text was updated successfully, but these errors were encountered:

IgnatBeresnev · 2022-04-25T20:35:04Z

Hi! Thank you for so many GFM-related bug reports, we appreciate it :)

Just to let you know: right now we're focusing on making html and then javadoc formats stable, so it will take quite some time for us to get to gfm, but sooner or later it'll get some love :)

cosinekitty · 2022-04-25T21:02:32Z

Thanks @IgnatBeresnev for letting me know. I have no idea how Dokka works internally, but I wonder if these issues are really specific to GFM, or are they general to the other output formats Dokka generates?

This is a workaround for a Dokka GFM issue I reported: Kotlin/dokka#2468 I updated the format_kotlin_doc.py script to remove the internal constructor calls for members of the Body enum.

IgnatBeresnev · 2022-04-25T21:35:57Z

I wonder if these issues are really specific to GFM

Good thought, some bugs are indeed not specific to GFM and reside on the lower level. I'll check your reports for html format as well, thanks!

Enum support is somewhat poor at the moment and need to be refined, we'll definitely get to it sometime soon.

ebraminio · 2022-05-06T15:11:59Z

@IgnatBeresnev Do you think you can also give me pointers so I can fix this? :) Apparently this was supposed to be fixed by acd9234 so I tested the code in this report to see if if is there anything special about it with no luck. In order to first write an end-to-end test (from Kotlin code to resulting Gfm) I wished I could use testInline somewhere I could issue CommonmarkRenderer but found they are in separate module those confused where I can first put that test to start to see what is happening so guess some pointers on where to start and similar tests/fixes would be nice.

IgnatBeresnev · 2022-05-06T19:06:34Z

Hi! I'm not sure this is a bug that needs to be fixed

Described behavior doesn't just happen, there's logic written specifically for this, so I'm assuming it must've been for a reason. I understand it may not be relevant or desired by everyone, but still. Maybe it should be a toggable feature flag

I think this needs to be discussed first. If you want to implement it, we'll discuss internally what the desired behavior should be and I'll get back to you :)

ebraminio · 2022-05-06T19:23:54Z

The problem is I suggested to use enum constructor calls instead raw enum for the project that has the issue now! 😄 Good part is the project haven't got block to Dokka's change as cosinekitty/astronomy@737fb01 so neither me nor OP don't have an immediate use but I personally like to see Kotlin's documentation tools at the best shape! 😊

I indeed like to implement this, can you instruct me generally where to start maybe or anything regardless of intended behavior so I can work on it in parallel to your internal discussion? And I indeed won't insist much for merging the fix if I got the solution but you don't want have it in just that I'm more interested on the fix itself for now 😊. What I thought can be a start point is to find a place so I can put a Kotlin code as a string and check its Gfm result which apparently not easily feasible but sure if I look harder guess it is possible to find where I should start.

ebraminio · 2022-05-06T19:32:04Z

Or, let me first replicate this is in a minimal separate project to see whether this is Gfm exclusive or can be seen with other outputs also or not, I'll be right back 😊

cosinekitty · 2022-05-06T19:43:15Z

Hi! I'm not sure this is a bug that needs to be fixed

I thought I would chime in on this one too. The reason I consider this behavior incorrect is that enum constructors are required to be private. Dokka is generating documentation that shows how a private class constructor is being called. For example:

Venus(
        VENUS_GM,
        224.701,
        VsopModel(vsopLonVenus, vsopLatVenus, vsopRadVenus)
    )

This is leaking internal implementation details -- actual executable code -- into my documentation. It is no different than if I wrote a function, declared it private or internal, and Dokka (or jsdoc, doxygen, etc.) included a line of code where that function is called as part of the documentation.

Since enum constructors are required by the Kotlin language spec to be private, there is never a reason to emit calls to those constructors as part of the documentation. It should be a general rule that nothing private or internal can appear in the generated documentation, because an outside caller has no access to it. It is additionally odd that any executable statements should appear; these are function calls, not interface declarations.

cosinekitty · 2022-05-06T19:48:15Z

Just as a follow-up for clarification. In my example, VsopModel is an internal class. Users of my project don't know what it means, because it is never documented anywhere. I don't want this exposed in my documentation because it will just confuse people.

ebraminio · 2022-05-06T19:54:25Z

Ok just checked it, this happens to both Gfm and Jekyll outputs but not Html and Javadoc so I think maybe I can at least propose something to make the backends consistent so you maybe consider having it or not after your internal discussions and your help in the meanwhile will be awesome 🙏

ebraminio · 2022-05-06T20:40:55Z

Bare minimum to show this is gfm/jekyll exclusive,

git clone https://github.com/ebraminio/dokka2468 && cd dokka2468
./gradlew dokkaGfm dokkaHtml dokkaJavadoc dokkaJekyll
grep -r 87.969 .

which results in:

./bin/main/io/github/ebraminio/dokka2468/Body.kt:enum class Body(internal val value: Double) { Mercury(87.969) }
./build/dokka/jekyll/dokka2468/io.github.ebraminio.dokka2468/-body/-mercury/index.md:[Mercury](index.html)(87.969)
./build/dokka/jekyll/dokka2468/io.github.ebraminio.dokka2468/-body/index.md:| [Mercury](-mercury/index.html) | [jvm]<br>[Mercury](-mercury/index.html)(87.969) |
./build/dokka/gfm/dokka2468/io.github.ebraminio.dokka2468/-body/-mercury/index.md:[Mercury](index.md)(87.969)
./build/dokka/gfm/dokka2468/io.github.ebraminio.dokka2468/-body/index.md:| [Mercury](-mercury/index.md) | [jvm]<br>[Mercury](-mercury/index.md)(87.969) |
./src/main/kotlin/io/github/ebraminio/dokka2468/Body.kt:enum class Body(internal val value: Double) { Mercury(87.969) }

but internal value isn't exposed to html and javadoc one, I am now looking for a fix, so far I got lost but I shouldn't lose hope...

ebraminio · 2022-05-07T10:38:29Z

Ok, so while I was playing around this test I found regardless of visibility, constructorSignature method of that test unit prints all of the constructor signature.

So turned to my minimal reproduction of the issue, https://github.com/ebraminio/dokka2468 and made the value field on https://github.com/ebraminio/dokka2468/blob/main/src/main/kotlin/io/github/ebraminio/dokka2468/Body.kt#L3 visible and found regardless of visibility dokkaGfm/dokkaJekyll print constructor signature but dokkaHtml/dokkaJavadoc don't print enum's constructor signature.

$ git clone https://github.com/ebraminio/dokka2468 && cd dokka2468
$ git diff 
diff --git a/src/main/kotlin/io/github/ebraminio/dokka2468/Body.kt b/src/main/kotlin/io/github/ebraminio/dokka2468/Body.kt
index e3667e8..fc9b16d 100644
--- a/src/main/kotlin/io/github/ebraminio/dokka2468/Body.kt
+++ b/src/main/kotlin/io/github/ebraminio/dokka2468/Body.kt
@@ -1,3 +1,3 @@
 package io.github.ebraminio.dokka2468
 
-enum class Body(internal val value: Double) { Mercury(87.969) }
+enum class Body(val value: Double) { Mercury(value = 87.969) }
$ rm -rf build && ./gradlew dokkaGfm dokkaHtml dokkaJavadoc dokkaJekyll
...
$ grep -r 87.969 .                                                     
./bin/main/io/github/ebraminio/dokka2468/Body.kt:enum class Body(val value: Double) { Mercury(value = 87.969) }
./build/dokka/jekyll/dokka2468/io.github.ebraminio.dokka2468/-body/-mercury/index.md:[Mercury](index.html)(87.969)
./build/dokka/jekyll/dokka2468/io.github.ebraminio.dokka2468/-body/index.md:| [Mercury](-mercury/index.html) | [jvm]<br>[Mercury](-mercury/index.html)(87.969) |
./build/dokka/gfm/dokka2468/io.github.ebraminio.dokka2468/-body/-mercury/index.md:[Mercury](index.md)(87.969)
./build/dokka/gfm/dokka2468/io.github.ebraminio.dokka2468/-body/index.md:| [Mercury](-mercury/index.md) | [jvm]<br>[Mercury](-mercury/index.md)(87.969) |
./src/main/kotlin/io/github/ebraminio/dokka2468/Body.kt:enum class Body(val value: Double) { Mercury(value = 87.969) }

# note that 87.969 isn't revealed in html and javadoc results so enum's constructer isn't printed anyway.

So I think maybe constructorSignature, just like html/javadoc probably shouldn't be printed at all anyway as even having public fields, I will see how they have become different and see if I can make them consistent. @IgnatBeresnev sorry for repeated spam, hopefully this message brings something on the table instead.

IgnatBeresnev · 2022-05-12T15:54:18Z

Hi! We've disscussed this internally and agreed that exposing enum entry constructors is excessive, and we couldn't come up with any reason for why it was implemented or why it should persist in the future.

Having said that, we'd like to remove it from the content generation level (so even before rendering), which will affect all formats, not only GFM.

@ebraminio @cosinekitty is that something one of you might want to take on? :) You'd need to remove a few lines of code and a few classes, and then fix a bunch of tests, but mostly removing code. I'll add more details in case you show interest. If not, I'll we'll just do it ourselves in some time :)

cosinekitty · 2022-05-12T17:05:59Z

Hi @IgnatBeresnev, yes, I would be interested in doing this work. So yes, more details about where to look would be very helpful.

ebraminio · 2022-05-12T18:44:57Z

Same here as @cosinekitty! :)

ebraminio · 2022-05-12T19:41:15Z

#2497 hopefully is in correct direction

IgnatBeresnev · 2022-05-13T09:56:33Z

@ebraminio yep, seems like you got to it even before I did :)

cosinekitty added the bug label Apr 25, 2022

cosinekitty mentioned this issue Apr 25, 2022

Kotlin documentation cleanup cosinekitty/astronomy#206

Closed

IgnatBeresnev added the format: gfm An issue/PR related to Dokka's GFM output format label Apr 25, 2022

IgnatBeresnev removed the format: gfm An issue/PR related to Dokka's GFM output format label Apr 25, 2022

ebraminio mentioned this issue May 12, 2022

Don't expose enum constructor arguments to documentations #2497

Merged

vmishenev closed this as completed in #2497 May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum members should not reveal constructor calls #2468

Enum members should not reveal constructor calls #2468

cosinekitty commented Apr 25, 2022

IgnatBeresnev commented Apr 25, 2022

cosinekitty commented Apr 25, 2022 •

edited

IgnatBeresnev commented Apr 25, 2022 •

edited

ebraminio commented May 6, 2022 •

edited

IgnatBeresnev commented May 6, 2022

ebraminio commented May 6, 2022 •

edited

ebraminio commented May 6, 2022 •

edited

cosinekitty commented May 6, 2022

cosinekitty commented May 6, 2022

ebraminio commented May 6, 2022 •

edited

ebraminio commented May 6, 2022 •

edited

ebraminio commented May 7, 2022 •

edited

IgnatBeresnev commented May 12, 2022 •

edited

cosinekitty commented May 12, 2022

ebraminio commented May 12, 2022

ebraminio commented May 12, 2022

IgnatBeresnev commented May 13, 2022

Enum members should not reveal constructor calls #2468

Enum members should not reveal constructor calls #2468

Comments

cosinekitty commented Apr 25, 2022

IgnatBeresnev commented Apr 25, 2022

cosinekitty commented Apr 25, 2022 • edited

IgnatBeresnev commented Apr 25, 2022 • edited

ebraminio commented May 6, 2022 • edited

IgnatBeresnev commented May 6, 2022

ebraminio commented May 6, 2022 • edited

ebraminio commented May 6, 2022 • edited

cosinekitty commented May 6, 2022

cosinekitty commented May 6, 2022

ebraminio commented May 6, 2022 • edited

ebraminio commented May 6, 2022 • edited

ebraminio commented May 7, 2022 • edited

IgnatBeresnev commented May 12, 2022 • edited

cosinekitty commented May 12, 2022

ebraminio commented May 12, 2022

ebraminio commented May 12, 2022

IgnatBeresnev commented May 13, 2022

cosinekitty commented Apr 25, 2022 •

edited

IgnatBeresnev commented Apr 25, 2022 •

edited

ebraminio commented May 6, 2022 •

edited

ebraminio commented May 6, 2022 •

edited

ebraminio commented May 6, 2022 •

edited

ebraminio commented May 6, 2022 •

edited

ebraminio commented May 6, 2022 •

edited

ebraminio commented May 7, 2022 •

edited

IgnatBeresnev commented May 12, 2022 •

edited