-
Notifications
You must be signed in to change notification settings - Fork 409
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
Hack kotlin compiler to not fail when annotation changes it's type from array to non-array #3859
Changes from 3 commits
cc6f721
639682f
e457ee0
7d0fced
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
/* | ||
* Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
|
||
plugins { | ||
kotlin("multiplatform") apply false | ||
id("org.jetbrains.dokka") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/* | ||
* Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
|
||
plugins { | ||
kotlin("multiplatform") | ||
id("org.jetbrains.dokka") | ||
} | ||
|
||
kotlin { | ||
jvm() | ||
linuxX64() | ||
linuxArm64() | ||
macosX64() | ||
macosArm64() | ||
js() | ||
} |
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 it.mpp.first | ||
|
||
/** | ||
* This is an annotation which should be used for opt-in | ||
* e.g [SealedSerializationApi](https://github.com/Kotlin/kotlinx.serialization/blob/99be48514c1d0a975bb80d7bd37df429a9670064/core/commonMain/src/kotlinx/serialization/ApiLevels.kt#L50) | ||
*/ | ||
@MustBeDocumented | ||
@RequiresOptIn | ||
annotation class SomeApi | ||
|
||
/** | ||
* Class which should be somehow used by dependent module | ||
*/ | ||
@OptIn(ExperimentalSubclassOptIn::class) | ||
@SubclassOptInRequired(SomeApi::class) | ||
interface Subclass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# | ||
# Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
# | ||
|
||
kotlin.js.compiler=ir | ||
|
||
# to sync the project locally, check dokka-integration-tests/gradle/projects/README.md | ||
#dokka_it_kotlin_version= | ||
#dokka_it_dokka_version= |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
/* | ||
* Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
|
||
plugins { | ||
kotlin("multiplatform") | ||
id("org.jetbrains.dokka") | ||
} | ||
|
||
kotlin { | ||
jvm() | ||
linuxX64() | ||
linuxArm64() | ||
macosX64() | ||
macosArm64() | ||
js() | ||
|
||
sourceSets { | ||
commonMain { | ||
dependencies { | ||
api(project(":first")) | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package it.mpp.second | ||
|
||
import it.mpp.first.* | ||
|
||
/** | ||
* exposes [Subclass] from another module which has [SubclassOptInRequired] annotation present | ||
*/ | ||
class Usage( | ||
val subclass: Subclass | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
|
||
apply(from = "template.settings.gradle.kts") | ||
rootProject.name = "it-multiplatform-multimodule" | ||
|
||
include(":first") | ||
include(":second") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/* | ||
* Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
|
||
package org.jetbrains.dokka.it.gradle | ||
|
||
import org.gradle.testkit.runner.TaskOutcome.FROM_CACHE | ||
import org.gradle.testkit.runner.TaskOutcome.SUCCESS | ||
import org.junit.jupiter.api.extension.ExtensionContext | ||
import org.junit.jupiter.params.ParameterizedTest | ||
import org.junit.jupiter.params.provider.Arguments | ||
import org.junit.jupiter.params.provider.ArgumentsSource | ||
import java.io.File | ||
import java.util.stream.Stream | ||
import kotlin.test.assertTrue | ||
|
||
// SubclassOptInRequired used in test is supported starting from 1.8 | ||
internal class MultiModuleMultiplatformTestedVersionsArgumentsProvider : AllSupportedTestedVersionsArgumentsProvider() { | ||
override fun provideArguments(context: ExtensionContext?): Stream<out Arguments> { | ||
return super.provideArguments(context).filter { | ||
val buildVersions = it.get().single() as BuildVersions | ||
buildVersions.kotlinVersion >= "1.8.20" | ||
} | ||
} | ||
} | ||
|
||
class MultiModuleMultiplatformIntegrationTest : AbstractGradleIntegrationTest() { | ||
|
||
@ParameterizedTest(name = "{0}") | ||
@ArgumentsSource(MultiModuleMultiplatformTestedVersionsArgumentsProvider::class) | ||
fun execute(buildVersions: BuildVersions) { | ||
val result = createGradleRunner( | ||
buildVersions, | ||
":dokkaHtmlMultiModule", | ||
).buildRelaxed() | ||
|
||
result.shouldHaveTask(":dokkaHtmlMultiModule").shouldHaveOutcome(SUCCESS, FROM_CACHE) | ||
result.shouldHaveTask(":first:dokkaHtmlPartial").shouldHaveOutcome(SUCCESS, FROM_CACHE) | ||
result.shouldHaveTask(":second:dokkaHtmlPartial").shouldHaveOutcome(SUCCESS, FROM_CACHE) | ||
|
||
val outputDir = File(projectDir, "build/dokka/htmlMultiModule") | ||
assertTrue(outputDir.isDirectory, "Missing dokka output directory") | ||
assertTrue( | ||
outputDir.allHtmlFiles().any(), | ||
"Expected at least one html file being generated" | ||
) | ||
|
||
outputDir.allHtmlFiles().forEach { file -> | ||
assertContainsNoErrorClass(file) | ||
assertNoUnresolvedLinks(file) | ||
assertNoHrefToMissingLocalFileOrDirectory(file) | ||
assertNoEmptyLinks(file) | ||
assertNoEmptySpans(file) | ||
assertNoUnsubstitutedTemplatesInHtml(file) | ||
} | ||
|
||
val modulesFile = File(outputDir, "index.html") | ||
assertTrue(modulesFile.isFile, "Missing index.html file") | ||
|
||
val modulesFileText = modulesFile.readText() | ||
assertTrue( | ||
"first" in modulesFileText, | ||
"Expected first being mentioned in -modules.html" | ||
) | ||
assertTrue( | ||
"second" in modulesFileText, | ||
"Expected second being mentioned in -modules.html" | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
/* | ||
* Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. | ||
*/ | ||
|
||
/** | ||
* DO NOT MOVE IT | ||
* This is a hack for https://github.com/Kotlin/dokka/issues/1599 | ||
* | ||
* Copy-pasted from Kotlin compiler. | ||
* Can be removed after updating to Kotlin Compiler 2.1.0 | ||
* | ||
* fixes: | ||
* changes are highlighted by `TODO: PATCH` | ||
* | ||
*/ | ||
package org.jetbrains.kotlin.serialization.deserialization | ||
|
||
import org.jetbrains.kotlin.builtins.KotlinBuiltIns | ||
import org.jetbrains.kotlin.descriptors.* | ||
import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptor | ||
import org.jetbrains.kotlin.descriptors.annotations.AnnotationDescriptorImpl | ||
import org.jetbrains.kotlin.metadata.ProtoBuf.Annotation | ||
import org.jetbrains.kotlin.metadata.ProtoBuf.Annotation.Argument | ||
import org.jetbrains.kotlin.metadata.ProtoBuf.Annotation.Argument.Value | ||
import org.jetbrains.kotlin.metadata.ProtoBuf.Annotation.Argument.Value.Type | ||
import org.jetbrains.kotlin.metadata.deserialization.Flags | ||
import org.jetbrains.kotlin.metadata.deserialization.NameResolver | ||
import org.jetbrains.kotlin.name.ClassId | ||
import org.jetbrains.kotlin.name.Name | ||
import org.jetbrains.kotlin.resolve.DescriptorUtils | ||
import org.jetbrains.kotlin.resolve.constants.* | ||
import org.jetbrains.kotlin.types.error.ErrorUtils | ||
import org.jetbrains.kotlin.types.KotlinType | ||
|
||
public class AnnotationDeserializer(private val module: ModuleDescriptor, private val notFoundClasses: NotFoundClasses) { | ||
private val builtIns: KotlinBuiltIns | ||
get() = module.builtIns | ||
|
||
public fun deserializeAnnotation(proto: Annotation, nameResolver: NameResolver): AnnotationDescriptor { | ||
val annotationClass = resolveClass(nameResolver.getClassId(proto.id)) | ||
|
||
var arguments = emptyMap<Name, ConstantValue<*>>() | ||
if (proto.argumentCount != 0 && !ErrorUtils.isError(annotationClass) && DescriptorUtils.isAnnotationClass(annotationClass)) { | ||
val constructor = annotationClass.constructors.singleOrNull() | ||
if (constructor != null) { | ||
val parameterByName = constructor.valueParameters.associateBy { it.name } | ||
arguments = proto.argumentList.mapNotNull { resolveArgument(it, parameterByName, nameResolver) }.toMap() | ||
} | ||
} | ||
|
||
return AnnotationDescriptorImpl(annotationClass.defaultType, arguments, SourceElement.NO_SOURCE) | ||
} | ||
|
||
private fun resolveArgument( | ||
proto: Argument, | ||
parameterByName: Map<Name, ValueParameterDescriptor>, | ||
nameResolver: NameResolver | ||
): Pair<Name, ConstantValue<*>>? { | ||
val parameter = parameterByName[nameResolver.getName(proto.nameId)] ?: return null | ||
return Pair(nameResolver.getName(proto.nameId), resolveValueAndCheckExpectedType(parameter.type, proto.value, nameResolver)) | ||
} | ||
|
||
private fun resolveValueAndCheckExpectedType(expectedType: KotlinType, value: Value, nameResolver: NameResolver): ConstantValue<*> { | ||
return resolveValue(expectedType, value, nameResolver).takeIf { | ||
doesValueConformToExpectedType(it, expectedType, value) | ||
} ?: ErrorValue.create("Unexpected argument value: actual type ${value.type} != expected type $expectedType") | ||
} | ||
|
||
public fun resolveValue(expectedType: KotlinType, value: Value, nameResolver: NameResolver): ConstantValue<*> { | ||
val isUnsigned = Flags.IS_UNSIGNED.get(value.flags) | ||
|
||
return when (value.type) { | ||
Type.BYTE -> value.intValue.toByte().letIf(isUnsigned, ::UByteValue, ::ByteValue) | ||
Type.CHAR -> CharValue(value.intValue.toInt().toChar()) | ||
Type.SHORT -> value.intValue.toShort().letIf(isUnsigned, ::UShortValue, ::ShortValue) | ||
Type.INT -> value.intValue.toInt().letIf(isUnsigned, ::UIntValue, ::IntValue) | ||
Type.LONG -> value.intValue.letIf(isUnsigned, ::ULongValue, ::LongValue) | ||
Type.FLOAT -> FloatValue(value.floatValue) | ||
Type.DOUBLE -> DoubleValue(value.doubleValue) | ||
Type.BOOLEAN -> BooleanValue(value.intValue != 0L) | ||
Type.STRING -> StringValue(nameResolver.getString(value.stringValue)) | ||
Type.CLASS -> KClassValue(nameResolver.getClassId(value.classId), value.arrayDimensionCount) | ||
Type.ENUM -> EnumValue(nameResolver.getClassId(value.classId), nameResolver.getName(value.enumValueId)) | ||
Type.ANNOTATION -> AnnotationValue(deserializeAnnotation(value.annotation, nameResolver)) | ||
Type.ARRAY -> ConstantValueFactory.createArrayValue( | ||
value.arrayElementList.map { resolveValue(builtIns.anyType, it, nameResolver) }, | ||
expectedType | ||
) | ||
else -> error("Unsupported annotation argument type: ${value.type} (expected $expectedType)") | ||
} | ||
} | ||
|
||
// This method returns false if the actual value loaded from an annotation argument does not conform to the expected type of the | ||
// corresponding parameter in the annotation class. This usually means that the annotation class has been changed incompatibly | ||
// without recompiling clients, in which case we prefer not to load the annotation argument value at all, to avoid constructing | ||
// an incorrect model and breaking some assumptions in the compiler. | ||
private fun doesValueConformToExpectedType(result: ConstantValue<*>, expectedType: KotlinType, value: Value): Boolean { | ||
return when (value.type) { | ||
Type.CLASS -> { | ||
val expectedClass = expectedType.constructor.declarationDescriptor as? ClassDescriptor | ||
// We could also check that the class value's type is a subtype of the expected type, but loading the definition of the | ||
// referenced class here is undesirable and may even be incorrect (because the module might be different at the | ||
// destination where these constant values are read). This can lead to slightly incorrect model in some edge cases. | ||
expectedClass == null || KotlinBuiltIns.isKClass(expectedClass) | ||
} | ||
Type.ARRAY -> { | ||
check(result is ArrayValue && result.value.size == value.arrayElementList.size) { | ||
"Deserialized ArrayValue should have the same number of elements as the original array value: $result" | ||
} | ||
|
||
// TODO: PATCH START | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the only change to original file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate on what the patch is in KDoc? Where was it copied from? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added the link to the changes landed into kotlin.git and rephrased KDoc a bit |
||
val expectedElementType = try { | ||
builtIns.getArrayElementType(expectedType) | ||
} catch (e: IllegalStateException) { | ||
return false | ||
} | ||
// TODO: PATCH END | ||
|
||
result.value.indices.all { i -> | ||
doesValueConformToExpectedType(result.value[i], expectedElementType, value.getArrayElement(i)) | ||
} | ||
} | ||
else -> result.getType(module) == expectedType | ||
} | ||
} | ||
|
||
private inline fun <T, R> T.letIf(predicate: Boolean, f: (T) -> R, g: (T) -> R): R = | ||
if (predicate) f(this) else g(this) | ||
|
||
private fun resolveClass(classId: ClassId): ClassDescriptor { | ||
return module.findNonGenericClassAcrossDependencies(classId, notFoundClasses) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failes when generating documentation for this module
second
which depends onfirst
.It's not possible to write just a unit test for this, as we need to first compile
first
and then generate documentation forsecond
with binary dependency onfirst
to reproduce the issue.Note: to test this, I was manually set version in TestedVersions to 2.1.0-Beta2.
I will prepare separate PR to run integration tests with 2.1.0-Beta2