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

Adapt to removal of old JVM backend #2244

Closed
wants to merge 1 commit into from

Conversation

udalov
Copy link
Contributor

@udalov udalov commented Nov 29, 2024

We're in the process of removing old JVM backend in the Kotlin repo, which leads to removing some internal compiler API that was only used from the old backend. See KT-71197 for all the changes.

Apparently some of the utilities, specifically those in KotlinTypeMapper, were used in KSP. In this change, I've adapted KSP sources to make it compilable against Kotlin master. Mainly I copy-pasted functions from KotlinTypeMapper which were removed here and simplified them.

I have some questions regarding the semantics of, for example, the newly introduced mapPropertySignature, which either returns a generic signature or an erased JVM type descriptor, depending on whether the field is generic, but for now I just restored the old behavior without (hopefully) any changes.

I'm marking this PR as a draft to make sure that it's not merged blindly, it should only be incorporated during the next update of Kotlin in KSP.

Copy link

google-cla bot commented Nov 29, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jsjeon
Copy link
Member

jsjeon commented Dec 6, 2024

cc @ting-yuan

@jsjeon jsjeon self-requested a review December 10, 2024 22:57
@@ -356,20 +360,24 @@ class ResolverImpl(
override fun mapToJvmSignature(declaration: KSDeclaration): String? = mapToJvmSignatureInternal(declaration)

internal fun mapToJvmSignatureInternal(declaration: KSDeclaration): String? = when (declaration) {
is KSClassDeclaration -> resolveClassDeclaration(declaration)?.let { typeMapper.mapType(it).descriptor }
is KSClassDeclaration -> resolveClassDeclaration(declaration)?.let { typeMapper.mapType(it.defaultType).descriptor }
Copy link
Member

Choose a reason for hiding this comment

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

This line is long, causing linter issue:

/home/runner/work/ksp/ksp/compiler-plugin/src/main/kotlin/com/google/devtools/ksp/processing/impl/ResolverImpl.kt:363:1: Exceeded max line length (120) (cannot be auto-corrected)

Please add line wrap around braces.

is KSPropertyDeclaration -> resolvePropertyDeclaration(declaration)?.let {
typeMapper.mapFieldSignature(it.type, it) ?: typeMapper.mapType(it).descriptor
}
is KSPropertyDeclaration -> resolvePropertyDeclaration(declaration)?.let { typeMapper.mapPropertySignature(it) }
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it is exactly 120 line length, but keeping the same structure (and just rewrite mapFieldSignature with mapPropertySignature) would make the overall change look minimal.

// KotlinTypeMapper.mapSignature always uses OwnerKind.IMPLEMENTATION
typeMapper.mapFunctionName(descriptor, OwnerKind.IMPLEMENTATION)
}
return resolvePropertyAccessorDeclaration(accessor)?.let(typeMapper::mapFunctionName)
Copy link
Member

Choose a reason for hiding this comment

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

This (and below) currently failed to build:

> Task :compiler-plugin:compileKotlin FAILED
e: file://.../ksp/compiler-plugin/src/main/kotlin/com/google/devtools/ksp/processing/impl/ResolverImpl.kt:891:66 Type mismatch: inferred type is KFunction2<FunctionDescriptor, OwnerKind?, String> but (TypeVariable(T)) -> TypeVariable(R) was expected
e: file://.../ksp/compiler-plugin/src/main/kotlin/com/google/devtools/ksp/processing/impl/ResolverImpl.kt:897:86 Type mismatch: inferred type is KFunction2<FunctionDescriptor, OwnerKind?, String> but (TypeVariable(T)) -> TypeVariable(R) was expected

but that's because you didn't upgrade kotlin version: https://github.com/google/ksp/blob/main/gradle.properties#L4

Why don't you change that line to, e.g., 2.1.20-dev-5634?

I found that upgrading AA (aaKotlinBaseVersion) causes other problems, but those should be addressed separately.

@jsjeon
Copy link
Member

jsjeon commented Dec 13, 2024

FYI, I created #2255 instead where every check is passing now.

@udalov udalov closed this Dec 13, 2024
@udalov udalov deleted the remove-old-backend branch December 13, 2024 11:21
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