Skip to content

Conversation

@nhachicha
Copy link
Collaborator

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the project to Kotlin 2.1, which includes a major upgrade of the build system and language toolchain. The changes involve updating compiler plugin code to accommodate breaking API changes in Kotlin 2.1's FIR (Frontend Intermediate Representation) and IR (Intermediate Representation) components.

Key Changes:

  • Updated Kotlin from 2.0.20 to 2.1.0 and corresponding build tool versions (Gradle from 7.6 to 8.12, Android build tools from 7.3.1 to 8.7.3)
  • Modified compiler plugin code to use updated Kotlin 2.1 APIs (e.g., .type to .coneType, removed valueArgumentsCount parameters)
  • Updated JVM target from 1.8 to 11 and CI/CD infrastructure to use Java 17

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
buildSrc/src/main/kotlin/Config.kt Updated version constants for Kotlin, Android SDK, Gradle, and JVM target
packages/plugin-compiler/src/main/kotlin/io/realm/kotlin/compiler/fir/model/*.kt Adapted FIR code generation to use .coneType instead of .type and removed session parameter
packages/plugin-compiler/src/main/kotlin/io/realm/kotlin/compiler/*.kt Updated IR code to remove deprecated valueArgumentsCount parameter and add required type casts
packages/plugin-compiler/src/main/kotlin/io/realm/kotlin/compiler/IrUtils.kt Updated type references and added generic type handling for annotation values
packages/test-base/src/jvmMain/kotlin/io/realm/kotlin/test/util/Compiler.kt Set JVM target to 11 for test compilation
gradle/wrapper/gradle-wrapper.properties Updated Gradle from 7.6 to 8.12
.github/workflows/*.yml Updated CI/CD to use Java 17 and macOS 13 runners

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


// Define the link target. Empty string if there is none.
val linkPropertyName: IrConst<String> = if (type == linkingObjectType) {
val linkPropertyName: IrConst = if (type == linkingObjectType) {
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

The variable linkPropertyName should be typed as IrConst<String> instead of IrConst to maintain type safety. The generic type parameter was removed but the variable holds string constants, making the type overly broad.

Suggested change
val linkPropertyName: IrConst = if (type == linkingObjectType) {
val linkPropertyName: IrConst<String> = if (type == linkingObjectType) {

Copilot uses AI. Check for mistakes.
fun getPersistedName(declaration: IrProperty): String {
@Suppress("UNCHECKED_CAST")
return (declaration.getAnnotation(PERSISTED_NAME_ANNOTATION.asSingleFqName()).getValueArgument(0)!! as IrConstImpl<String>).value
return (declaration.getAnnotation(PERSISTED_NAME_ANNOTATION.asSingleFqName()).getValueArgument(0)!! as IrConstImpl).value?.toString() ?: ""
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

The fallback to empty string when value is null could mask genuine issues. If getValueArgument(0) returns a non-null IrConstImpl but its value is null, this suggests a problem with the annotation that should be handled explicitly rather than silently defaulting to an empty string.

Suggested change
return (declaration.getAnnotation(PERSISTED_NAME_ANNOTATION.asSingleFqName()).getValueArgument(0)!! as IrConstImpl).value?.toString() ?: ""
val annotation = declaration.getAnnotation(PERSISTED_NAME_ANNOTATION.asSingleFqName())
val valueArgument = annotation.getValueArgument(0)
if (valueArgument == null || valueArgument !is IrConstImpl<*>) {
throw IllegalStateException("PersistedName annotation is missing or malformed on property '${declaration.name.identifier}'.")
}
val value = valueArgument.value
if (value == null) {
throw IllegalStateException("PersistedName annotation value is null on property '${declaration.name.identifier}'.")
}
return value.toString()

Copilot uses AI. Check for mistakes.
return if (clazz.hasAnnotation(PERSISTED_NAME_ANNOTATION)) {
@Suppress("UNCHECKED_CAST")
return (clazz.getAnnotation(PERSISTED_NAME_ANNOTATION.asSingleFqName()).getValueArgument(0)!! as IrConstImpl<String>).value
return (clazz.getAnnotation(PERSISTED_NAME_ANNOTATION.asSingleFqName()).getValueArgument(0)!! as IrConstImpl).value?.toString() ?: ""
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

The fallback to empty string when value is null could mask genuine issues. If the annotation value is null, this likely indicates a problem that should be handled explicitly rather than silently returning an empty string, which could lead to incorrect schema generation.

Suggested change
return (clazz.getAnnotation(PERSISTED_NAME_ANNOTATION.asSingleFqName()).getValueArgument(0)!! as IrConstImpl).value?.toString() ?: ""
val annotationValue = (clazz.getAnnotation(PERSISTED_NAME_ANNOTATION.asSingleFqName()).getValueArgument(0)!! as IrConstImpl).value
if (annotationValue == null) {
fatalError("Persisted name annotation value is null for class ${clazz.name.identifier}")
}
return annotationValue.toString()

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +126
val type = modelObjectAnnotationClass.defaultType as? IrType ?: throw IllegalStateException("defaultType is not an IrType")
val primaryConstructor = modelObjectAnnotationClass.primaryConstructor ?: throw IllegalStateException("primaryConstructor is null")
val constructorSymbol = primaryConstructor.symbol as? IrConstructorSymbol ?: throw IllegalStateException("symbol is not an IrConstructorSymbol")
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

These error messages lack context. Include the class name in the error messages to help identify which annotation class is causing the problem. For example: \"defaultType is not an IrType for class ${modelObjectAnnotationClass.name}\"

Suggested change
val type = modelObjectAnnotationClass.defaultType as? IrType ?: throw IllegalStateException("defaultType is not an IrType")
val primaryConstructor = modelObjectAnnotationClass.primaryConstructor ?: throw IllegalStateException("primaryConstructor is null")
val constructorSymbol = primaryConstructor.symbol as? IrConstructorSymbol ?: throw IllegalStateException("symbol is not an IrConstructorSymbol")
val type = modelObjectAnnotationClass.defaultType as? IrType ?: throw IllegalStateException("defaultType is not an IrType for class ${modelObjectAnnotationClass.name}")
val primaryConstructor = modelObjectAnnotationClass.primaryConstructor ?: throw IllegalStateException("primaryConstructor is null for class ${modelObjectAnnotationClass.name}")
val constructorSymbol = primaryConstructor.symbol as? IrConstructorSymbol ?: throw IllegalStateException("symbol is not an IrConstructorSymbol for class ${modelObjectAnnotationClass.name}")

Copilot uses AI. Check for mistakes.
@clementetb
Copy link
Contributor

Looks good, it was not straight forward...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants