-
Notifications
You must be signed in to change notification settings - Fork 22
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
K2 plugin #116
K2 plugin #116
Conversation
e6f5291
to
6d58130
Compare
No tests will be executed for this review for two reasons:
I tested the PR locally, and it works for existing tests, I just don't want to have an intermediate state of CI configs just for this review. |
tasks.withType<ProcessResources>().configureEach { | ||
duplicatesStrategy = DuplicatesStrategy.EXCLUDE | ||
|
||
from(versionNames.map { vsResources.resolve(it) }) | ||
include { it.file.parentInAllowList(versionNames) } | ||
} | ||
// todo duplicate (or to many resources are copied, should update the algo) | ||
// tasks.withType<ProcessResources>().configureEach { | ||
// duplicatesStrategy = DuplicatesStrategy.EXCLUDE | ||
// | ||
// from(versionNames.map { vsResources.resolve(it) }) | ||
// include { it.file.parentInAllowList(versionNames) } | ||
// } |
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.
I will fix it in a separate PR
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.
Let's get someone from the compiler team to check it out as well
compiler-plugin/compiler-plugin-k2/src/main/core/kotlinx/rpc/codegen/FirRPCServiceGenerator.kt
Outdated
Show resolved
Hide resolved
@e5l do you have someone specific in mind? UPD: nevermind, I see |
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.
Generally, the approach is fine
But I recall that we discussed that the plugin also should check some invariants with checkers, and I don't see them here
Will they be implemented later, or I confused it with some other plugin?
compiler-plugin/compiler-plugin-k2/src/main/core/kotlinx/rpc/codegen/FirRPCServiceGenerator.kt
Outdated
Show resolved
Hide resolved
@Suppress("unused") | ||
private val logger: MessageCollector, | ||
) : FirDeclarationGenerationExtension(session) { | ||
private val serializationExtension = SerializationFirResolveExtension(session) |
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.
The trick with your own generator from serialization plugin is nice
Like it
compiler-plugin/compiler-plugin-k2/src/main/core/kotlinx/rpc/codegen/FirRPCServiceGenerator.kt
Outdated
Show resolved
Hide resolved
compiler-plugin/compiler-plugin-k2/src/main/core/kotlinx/rpc/codegen/FirRPCServiceGenerator.kt
Outdated
Show resolved
Hide resolved
@demiurg906 thanks for the review! Yes, you're right, we talked about checkers, they will be shipped separately |
* Introduce K2 plugin * Update IR backend * Update to 2.0 * Fix for KT-70132
* Introduce K2 plugin * Update IR backend * Update to 2.0 * Fix for KT-70132
* Introduce K2 plugin * Update IR backend * Update to 2.0 * Fix for KT-70132
Subsystem
K2 Compiler plugin
Problem Description
We are migrating from KSP code generation to compiler plugins.
Solution
This is part 2 of 3 in the migration process, which presents the K2 frontend plugin.
K2 plugin overview:
Also PR contains addition of an
compiler-plugin-common
module for shared declarations between backend and frontend(s).PR contains upgrade to Kotlin 2.0 as it required to make K2 plugin work. Project language level is set to 1.7 by default, so K2 will not be used by default. For it to be used in tests, use dedicated
kotlinx.rpc.useK2Plugin=true
in rootgradle.properties