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

Provide a way to manually create SampleAnalysisEnvironment #3589

Merged
merged 3 commits into from
May 7, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,12 @@ public abstract interface class org/jetbrains/dokka/analysis/kotlin/sample/Funct
public abstract fun rewrite (Ljava/util/List;Ljava/util/List;)Ljava/lang/String;
}

public abstract interface class org/jetbrains/dokka/analysis/kotlin/sample/SampleAnalysisEnvironment {
public abstract interface class org/jetbrains/dokka/analysis/kotlin/sample/SampleAnalysisEnvironment : java/io/Closeable {
public abstract fun resolveSample (Lorg/jetbrains/dokka/DokkaConfiguration$DokkaSourceSet;Ljava/lang/String;)Lorg/jetbrains/dokka/analysis/kotlin/sample/SampleSnippet;
}

public abstract interface class org/jetbrains/dokka/analysis/kotlin/sample/SampleAnalysisEnvironmentCreator {
public abstract fun create ()Lorg/jetbrains/dokka/analysis/kotlin/sample/SampleAnalysisEnvironment;
public abstract fun use (Lkotlin/jvm/functions/Function1;)Ljava/lang/Object;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package org.jetbrains.dokka.analysis.kotlin.sample

import org.jetbrains.dokka.DokkaConfiguration
import java.io.Closeable

/**
* Fully-configured and ready-to-use sample analysis environment.
Expand All @@ -17,7 +18,7 @@ import org.jetbrains.dokka.DokkaConfiguration
* in one iteration and at the same time, so that the environment is created once and lives for
* as little is possible, as opposed to creating it again and again for every individual sample.
*/
public interface SampleAnalysisEnvironment {
public interface SampleAnalysisEnvironment : Closeable {

/**
* Resolves a Kotlin sample function by its fully qualified name, and returns its import statements and body.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package org.jetbrains.dokka.analysis.kotlin.sample

import org.jetbrains.dokka.DelicateDokkaApi
import org.jetbrains.dokka.analysis.kotlin.KotlinAnalysisPlugin

/**
Expand All @@ -16,7 +17,7 @@ public interface SampleAnalysisEnvironmentCreator {
/**
* Creates and configures the sample analysis environment for a limited-time use.
*
* Configuring sample analysis environment is a rather expensive operation that takes up additional
* Configuring a sample analysis environment is a rather expensive operation that takes up additional
* resources since Dokka needs to configure and analyze source roots additional to the main ones.
* It's best to limit the scope of use and the lifetime of the created environment
* so that the resources could be freed as soon as possible.
Expand All @@ -35,4 +36,20 @@ public interface SampleAnalysisEnvironmentCreator {
* ```
*/
public fun <T> use(block: SampleAnalysisEnvironment.() -> T): T

/**
* Creates a new instance of [SampleAnalysisEnvironment].
*
* **WARNING**: This function offers a considerable amount of freedom and with it,
* the potential to misuse the API.
* A [SampleAnalysisEnvironment] once created needs to be manually closed
* otherwise it could lead to memory leaks, concurrency issues or other unexpected problems.
*
* Therefore, it's safest to use it through the [SampleAnalysisEnvironmentCreator.use]
* as it provides a controlled environment where everything is taken care of automatically.
*
* @return a new instance of [SampleAnalysisEnvironment]
*/
@DelicateDokkaApi
public fun create(): SampleAnalysisEnvironment
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package org.jetbrains.dokka.analysis.test.sample

import org.jetbrains.dokka.DelicateDokkaApi
import org.jetbrains.dokka.analysis.kotlin.sample.SampleSnippet
import org.jetbrains.dokka.analysis.test.api.kotlinJvmTestProject
import org.jetbrains.dokka.analysis.test.api.mixedJvmTestProject
Expand All @@ -14,6 +15,120 @@ import kotlin.test.*

class SampleAnalysisTest {

@OptIn(DelicateDokkaApi::class)
@Test
fun `should resolve a valid sample if the environment is created manually`() {
val testProject = kotlinJvmTestProject {
dokkaConfiguration {
kotlinSourceSet {
samples = setOf("/samples/collections.kt")
}
}
sampleFile("/samples/collections.kt", fqPackageName = "org.jetbrains.dokka.sample.collections") {
+"""
import org.jetbrains.dokka.DokkaConfiguration
import org.jetbrains.dokka.DokkaGenerator
import org.jetbrains.dokka.utilities.DokkaLogger

fun specificPositionOperations() {
val numbers = mutableListOf(1, 2, 3, 4)
numbers.add(5)
numbers.removeAt(1)
numbers[0] = 0
numbers.shuffle()
if (numbers.size > 0) {
println(numbers)
}
}
"""
}
}

testProject.useServices { context ->
val sampleAnalysisEnvironment = sampleAnalysisEnvironmentCreator.create()
try {
val sample = sampleAnalysisEnvironment.resolveSample(
sourceSet = context.singleSourceSet(),
fullyQualifiedLink = "org.jetbrains.dokka.sample.collections.specificPositionOperations"
)
assertNotNull(sample)

val expectedImports = listOf(
"org.jetbrains.dokka.DokkaConfiguration",
"org.jetbrains.dokka.DokkaGenerator",
"org.jetbrains.dokka.utilities.DokkaLogger"
)

val expectedBody = """
val numbers = mutableListOf(1, 2, 3, 4)
numbers.add(5)
numbers.removeAt(1)
numbers[0] = 0
numbers.shuffle()
if (numbers.size > 0) {
println(numbers)
}
""".trimIndent()

assertEquals(expectedImports, sample.imports)
assertEquals(expectedBody, sample.body)
} finally {
sampleAnalysisEnvironment.close()
}
}
}

@OptIn(DelicateDokkaApi::class)
@Test
fun `should resolve the same sample by multiple environments`() {
val testProject = kotlinJvmTestProject {
dokkaConfiguration {
kotlinSourceSet {
samples = setOf("/samples/collections.kt")
}
}
sampleFile("/samples/collections.kt", fqPackageName = "org.jetbrains.dokka.sample.collections") {
+"""
import org.jetbrains.dokka.DokkaConfiguration
import org.jetbrains.dokka.DokkaGenerator
import org.jetbrains.dokka.utilities.DokkaLogger

fun specificPositionOperations() {
val numbers = mutableListOf(1, 2, 3, 4)
numbers.add(5)
numbers.removeAt(1)
numbers[0] = 0
numbers.shuffle()
if (numbers.size > 0) {
println(numbers)
}
}
"""
}
}

testProject.useServices { context ->
val sampleAnalysisEnvironment1 = sampleAnalysisEnvironmentCreator.create()
val sampleAnalysisEnvironment2 = sampleAnalysisEnvironmentCreator.create()
try {
val sample1 = sampleAnalysisEnvironment1.resolveSample(
sourceSet = context.singleSourceSet(),
fullyQualifiedLink = "org.jetbrains.dokka.sample.collections.specificPositionOperations"
)
val sample2 = sampleAnalysisEnvironment2.resolveSample(
sourceSet = context.singleSourceSet(),
fullyQualifiedLink = "org.jetbrains.dokka.sample.collections.specificPositionOperations"
)
assertNotNull(sample1)
assertNotNull(sample2)
assertEquals(sample1, sample2)
} finally {
sampleAnalysisEnvironment1.close()
sampleAnalysisEnvironment2.close()
}
}
}

@Test
fun `should resolve a valid sample if set via the samples option`() {
val testProject = kotlinJvmTestProject {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import com.intellij.psi.PsiElementVisitor
import com.intellij.psi.impl.source.tree.LeafPsiElement
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
import org.jetbrains.dokka.DelicateDokkaApi
import org.jetbrains.dokka.DokkaConfiguration
import org.jetbrains.dokka.analysis.kotlin.KotlinAnalysisPlugin
import org.jetbrains.dokka.analysis.kotlin.descriptors.compiler.CompilerDescriptorAnalysisPlugin
Expand Down Expand Up @@ -49,21 +50,22 @@ internal class DescriptorSampleAnalysisEnvironmentCreator(
// avoid memory leaks through the compiler's ThreadLocals.
// Might not be relevant if the project stops using coroutines.
return runBlocking(Dispatchers.Default) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

minor question: do we still need this here?
As we now provide create() (even if it's delicate), for me it looks a little strange, that creator.use(block) and creator.create().use(block) will behave differently, and most likely users of this API need to know about internals to work with it properly.
WDYT about this?

Copy link
Member

Choose a reason for hiding this comment

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

minor question: do we still need this here?

I believe we do, otherwise it can lead to the mentioned issues :) This is the controlled environment that addresses the issues that we're aware of, and more hacks and fixes might be added here in the future

for me it looks a little strange, that creator.use(block) and creator.create().use(block) will behave differently

yeah, the naming is unfortunate :( this is a lesson for the future - do not use names that can clash with the names from stdlib. otherwise, I'd say it's expected:

most likely users of this API need to know about internals to work with it properly.

indeed, that is the whole point of create() being unsafe, delicate and NOT recommended, and use() being the safest and recommended option

  • You're using use() and have encountered a bug/leak/etc? Report it, we'll have a look and try to resolve it (most likely by adding some hacks/workarounds to use())
  • You're using create() and have encountered a bug/leak/etc? Well, we gave no guarantees and warned you, you better research and figure out how to mitigate it on your own. Or start using use(), then it becomes our problem

If we remove use(), we'll have to add the runBlocking detail (and any other) to the KDoc of create(), but I see several problems with this approach:

  • The users will have to read the docs to apply it - the chances of it are slim as it is
  • If more issues arise, the KDoc will have to be updated with the new workarounds, and the users will have to revise it every release, which is also very unlikely
  • If some issues are no longer relevant and the workarounds need to be removed - same story
  • It kinda breaks the abstraction. This API can have multiple implementations with their own limitations and workarounds. Some might apply to K1, some to K2, some to both. Not to mention any potential 3rd party ones that we're not aware of

So to me it makes sense to have use(), which takes care of all this for you


We can soft-rename use to something else to not clash with kotlin.io.use, but let's do it as a separate issue/PR

@OptIn(DokkaPluginApiPreview::class)
SamplesKotlinAnalysis(
create().use(block)
}
}

@OptIn(DokkaPluginApiPreview::class, DelicateDokkaApi::class)
override fun create(): SampleAnalysisEnvironment {
return DescriptorSampleAnalysisEnvironment(
kdocFinder = descriptorAnalysisPlugin.querySingle { kdocFinder },
kotlinAnalysis = SamplesKotlinAnalysis(
sourceSets = context.configuration.sourceSets,
context = context,
projectKotlinAnalysis = descriptorAnalysisPlugin.querySingle { kotlinAnalysis }
).use { kotlinAnalysis ->
val sampleAnalysis = DescriptorSampleAnalysisEnvironment(
kdocFinder = descriptorAnalysisPlugin.querySingle { kdocFinder },
kotlinAnalysis = kotlinAnalysis,
sampleRewriter = sampleRewriter,
dokkaLogger = context.logger
)
block(sampleAnalysis)
}
}
),
sampleRewriter = sampleRewriter,
dokkaLogger = context.logger
)
}
}

Expand Down Expand Up @@ -221,6 +223,10 @@ internal class DescriptorSampleAnalysisEnvironment(
}
return textBuilder.toString()
}

override fun close() {
kotlinAnalysis.close()
}
}

private class SampleBuilder(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import com.intellij.psi.PsiElementVisitor
import com.intellij.psi.impl.source.tree.LeafPsiElement
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
import org.jetbrains.dokka.DelicateDokkaApi
import org.jetbrains.dokka.DokkaConfiguration.DokkaSourceSet
import org.jetbrains.dokka.analysis.kotlin.KotlinAnalysisPlugin
import org.jetbrains.dokka.analysis.kotlin.sample.SampleAnalysisEnvironment
Expand Down Expand Up @@ -44,22 +45,23 @@ internal class SymbolSampleAnalysisEnvironmentCreator(
rewriters.singleOrNull()
}


override fun <T> use(block: SampleAnalysisEnvironment.() -> T): T {
return runBlocking(Dispatchers.Default) {
SamplesKotlinAnalysis(
create().use(block)
}
}

@OptIn(DelicateDokkaApi::class)
override fun create(): SampleAnalysisEnvironment {
return SymbolSampleAnalysisEnvironment(
samplesKotlinAnalysis = SamplesKotlinAnalysis(
sourceSets = context.configuration.sourceSets,
context = context
).use { samplesKotlinAnalysis ->
val sampleAnalysisEnvironment = SymbolSampleAnalysisEnvironment(
samplesKotlinAnalysis = samplesKotlinAnalysis,
projectKotlinAnalysis = projectKotlinAnalysis,
sampleRewriter = sampleRewriter,
dokkaLogger = context.logger
)
block(sampleAnalysisEnvironment)
}
}
),
projectKotlinAnalysis = projectKotlinAnalysis,
sampleRewriter = sampleRewriter,
dokkaLogger = context.logger
)
}
}

Expand Down Expand Up @@ -174,6 +176,10 @@ private class SymbolSampleAnalysisEnvironment(
}
return textBuilder.toString()
}

override fun close() {
samplesKotlinAnalysis.close()
}
}

private class SampleBuilder(
Expand Down
3 changes: 3 additions & 0 deletions dokka-subprojects/core/api/dokka-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ public final class org/jetbrains/dokka/DefaultExternalLinksKt {
public static final fun kotlinStdlib (Lorg/jetbrains/dokka/DokkaConfiguration$ExternalDocumentationLink$Companion;)Lorg/jetbrains/dokka/ExternalDocumentationLinkImpl;
}

public abstract interface annotation class org/jetbrains/dokka/DelicateDokkaApi : java/lang/annotation/Annotation {
}

public abstract interface class org/jetbrains/dokka/DokkaBootstrap {
public abstract fun configure (Ljava/lang/String;Ljava/util/function/BiConsumer;)V
public abstract fun generate ()V
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/

package org.jetbrains.dokka

/**
* Marks declarations in the Dokka that are **delicate** &mdash;
* they have limited use-case and shall be used with care in general code.
* Any use of a delicate declaration has to be carefully reviewed to make sure it is
* properly used and does not create problems like memory and resource leaks.
* Carefully read documentation of any declaration marked as `DelicateDokkaApi`.
*/
@RequiresOptIn(
level = RequiresOptIn.Level.WARNING,
message = "This is a delicate API and its use requires care." +
" Make sure you fully read and understand documentation of the declaration that is marked as a delicate API."
)
@Retention(AnnotationRetention.BINARY)
public annotation class DelicateDokkaApi
Loading