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

Do not generate source links for synthetic elements #2547

Merged
merged 1 commit into from Jun 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -68,7 +68,7 @@ abstract class AbstractTest<M : TestMethods, T : TestBuilder<M>, D : DokkaTestGe
block: T.() -> Unit
) {
val testMethods = testBuilder().apply(block).build()
val testDirPath = getTempDir(cleanupOutput).root.toPath()
val testDirPath = getTempDir(cleanupOutput).root.toPath().toAbsolutePath()
val fileMap = query.toFileMap()
fileMap.materializeFiles(testDirPath.toAbsolutePath())
if (!cleanupOutput)
Expand All @@ -85,7 +85,7 @@ abstract class AbstractTest<M : TestMethods, T : TestBuilder<M>, D : DokkaTestGe
}.toSet(),
sourceLinks = sourceSet.sourceLinks.map { link ->
link.copy(
localDirectory = testDirPath.toFile().resolve(link.localDirectory).canonicalPath
localDirectory = testDirPath.toFile().resolve(link.localDirectory).absolutePath
Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with paths was here. canonicalPath resolves symlinks, while absolutePath does not, so on windows it resulted in two different temp folder locations.

I'll refactor this bit later on in a separate PR once I get a hold of a windows machine

)
}.toSet()
)
Expand Down
Expand Up @@ -24,36 +24,47 @@ import java.io.File

class SourceLinksTransformer(val context: DokkaContext) : PageTransformer {

private val builder : PageContentBuilder = PageContentBuilder(
private val builder : PageContentBuilder = PageContentBuilder(
context.plugin<DokkaBase>().querySingle { commentsToContentConverter },
context.plugin<DokkaBase>().querySingle { signatureProvider },
context.logger
)

override fun invoke(input: RootPageNode) =
input.transformContentPagesTree { node ->
override fun invoke(input: RootPageNode): RootPageNode {
Copy link
Member Author

Choose a reason for hiding this comment

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

A bit of refactoring to make it not run if sources are not configured and to not parse configuration on every documentable

val sourceLinks = getSourceLinksFromConfiguration()
if (sourceLinks.isEmpty()) {
return input
}
return input.transformContentPagesTree { node ->
when (node) {
is WithDocumentables ->
node.documentables.filterIsInstance<WithSources>().flatMap { resolveSources(it) }
.takeIf { it.isNotEmpty() }
?.let { node.addSourcesContent(it) }
?: node
node.documentables
.filterIsInstance<WithSources>()
.flatMap { resolveSources(sourceLinks, it) }
.takeIf { it.isNotEmpty() }
?.let { node.addSourcesContent(it) }
?: node
else -> node
}
}
}

private fun getSourceLinks() = context.configuration.sourceSets
.flatMap { it.sourceLinks.map { sl -> SourceLink(sl, it) } }
private fun getSourceLinksFromConfiguration(): List<SourceLink> {
return context.configuration.sourceSets
.flatMap { it.sourceLinks.map { sl -> SourceLink(sl, it) } }
}

private fun resolveSources(documentable: WithSources) = documentable.sources
.mapNotNull { entry ->
getSourceLinks().find { File(entry.value.path).startsWith(it.path) && it.sourceSetData == entry.key }?.let {
Pair(
entry.key,
entry.value.toLink(it)
)
}
private fun resolveSources(
sourceLinks: List<SourceLink>, documentable: WithSources
): List<Pair<DokkaSourceSet, String>> {
return documentable.sources.mapNotNull { (sourceSet, documentableSource) ->
val sourceLink = sourceLinks.find { sourceLink ->
File(documentableSource.path).startsWith(sourceLink.path) && sourceLink.sourceSetData == sourceSet
} ?: return@mapNotNull null

sourceSet to documentableSource.toLink(sourceLink)
}
}

private fun ContentPage.addSourcesContent(sources: List<Pair<DokkaSourceSet, String>>) = builder
.buildSourcesContent(this, sources)
Expand Down Expand Up @@ -89,8 +100,8 @@ class SourceLinksTransformer(val context: DokkaContext) : PageTransformer {
}

private fun DocumentableSource.toLink(sourceLink: SourceLink): String {
val sourcePath = File(this.path).canonicalPath.replace("\\", "/")
val sourceLinkPath = File(sourceLink.path).canonicalPath.replace("\\", "/")
val sourcePath = File(this.path).invariantSeparatorsPath
val sourceLinkPath = File(sourceLink.path).invariantSeparatorsPath

val lineNumber = when (this) {
is DescriptorDocumentableSource -> this.descriptor
Expand Down Expand Up @@ -134,6 +145,9 @@ class SourceLinksTransformer(val context: DokkaContext) : PageTransformer {
}

private fun PsiElement.lineNumber(): Int? {
// synthetic and some light methods might return null
val textRange = textRange ?: return null

val doc = PsiDocumentManager.getInstance(project).getDocument(containingFile)
// IJ uses 0-based line-numbers; external source browsers use 1-based
return doc?.getLineNumber(textRange.startOffset)?.plus(1)
Expand Down
60 changes: 60 additions & 0 deletions plugins/base/src/test/kotlin/enums/JavaEnumsTest.kt
@@ -0,0 +1,60 @@
package enums

import org.jetbrains.dokka.SourceLinkDefinitionImpl
import org.jetbrains.dokka.base.testApi.testRunner.BaseAbstractTest
import org.junit.jupiter.api.Test
import signatures.renderedContent
import utils.TestOutputWriterPlugin
import java.net.URL
import kotlin.test.assertEquals

class JavaEnumsTest : BaseAbstractTest() {

// Shouldn't try to give source links to synthetic methods (values, valueOf) if any are present
// https://github.com/Kotlin/dokka/issues/2544
@Test
fun `java enum with configured source links should not fail build due to any synthetic methods`() {
val configuration = dokkaConfiguration {
sourceSets {
sourceSet {
sourceRoots = listOf("src/")
sourceLinks = listOf(
SourceLinkDefinitionImpl(
localDirectory = "src/main/java",
remoteUrl = URL("https://github.com/user/repo/tree/master/src/main/java"),
remoteLineSuffix = "#L"
)
)
}
}
}

val writerPlugin = TestOutputWriterPlugin()

testInline(
"""
|/src/main/java/basic/JavaEnum.java
|package testpackage
|
|public enum JavaEnum {
| ONE, TWO, THREE
|}
""".trimMargin(),
configuration,
pluginOverrides = listOf(writerPlugin)
) {
renderingStage = { _, _ ->
val enumPage = writerPlugin.writer.renderedContent("root/testpackage/-java-enum/index.html")
val sourceLink = enumPage.select("div[data-togglable=Sources]")
.select("a[href]")
.attr("href")


assertEquals(
"https://github.com/user/repo/tree/master/src/main/java/basic/JavaEnum.java#L3",
sourceLink
)
}
}
}
}
@@ -1,19 +1,23 @@
package enums

import matchers.content.*
import org.jetbrains.dokka.SourceLinkDefinitionImpl
import org.jetbrains.dokka.pages.*
import org.jetbrains.dokka.base.testApi.testRunner.BaseAbstractTest
import org.jetbrains.dokka.model.*
import org.jetbrains.kotlin.utils.addToStdlib.firstIsInstance
import org.jsoup.Jsoup
import org.jsoup.nodes.Element
import org.junit.jupiter.api.Assertions.*
import org.junit.jupiter.api.Disabled
import org.junit.jupiter.api.Test
import signatures.renderedContent
import utils.TestOutputWriter
import utils.TestOutputWriterPlugin
import java.io.File
import java.net.URL

class EnumsTest : BaseAbstractTest() {
class KotlinEnumsTest : BaseAbstractTest() {

@Test
fun `should preserve enum source ordering for documentables`() {
Expand Down Expand Up @@ -366,4 +370,52 @@ class EnumsTest : BaseAbstractTest() {
}
}
}

// Shouldn't try to give source links to synthetic methods (values, valueOf) if any are present
// Initially reported for Java, making sure it doesn't fail for Kotlin either
// https://github.com/Kotlin/dokka/issues/2544
@Test
fun `kotlin enum with configured source links should not fail the build due to synthetic methods`() {
val configuration = dokkaConfiguration {
sourceSets {
sourceSet {
sourceRoots = listOf("src/")
sourceLinks = listOf(
SourceLinkDefinitionImpl(
localDirectory = "src/main/kotlin",
remoteUrl = URL("https://github.com/user/repo/tree/master/src/main/kotlin"),
remoteLineSuffix = "#L"
)
)
}
}
}

val writerPlugin = TestOutputWriterPlugin()

testInline(
"""
|/src/main/kotlin/basic/KotlinEnum.kt
|package testpackage
|
|enum class KotlinEnum {
| ONE, TWO, THREE
|}
""".trimMargin(),
configuration,
pluginOverrides = listOf(writerPlugin)
) {
renderingStage = { _, _ ->
val sourceLink = writerPlugin.writer.renderedContent("root/testpackage/-kotlin-enum/index.html")
.select("div[data-togglable=Sources]")
.select("a[href]")
.attr("href")

assertEquals(
"https://github.com/user/repo/tree/master/src/main/kotlin/basic/KotlinEnum.kt#L3",
sourceLink
)
}
}
}
}