Kotlin compiler plugin #470
Conversation
neeme-praks-sympower
force-pushed
the
compiler-plugin
branch
3 times, most recently
from
231dec5 to
e77837f
Compare
neeme-praks-sympower
force-pushed
the
compiler-plugin
branch
from
e77837f to
b0d19db
Compare
|
As @oshai is probably not very familiar with Kotlin compiler plugins, I reached out via an acquaintance to JetBrains to see if anyone with good technical knowledge about such plugins could have a look. @bnorm (Brian Norman), author of https://github.com/bnorm/kotlin-ir-plugin-template promised to have a glance at this some time next week. |
|
Thank you, sound good! |
|
@bnorm, have you had a chance to have a look? |
There was a problem hiding this comment.
Overall looks pretty good. I took a pretty high-level pass over most things, but I wasn't able to spend as much time as I would have liked. Might have more time next week. In the meantime though, if there's an area you have specific questions about, tag me in a comment on the relevant code and I will focus on that in my next pass.
For a sample project, my typical approach is the following: https://github.com/bnorm/piecemeal/tree/main/sample. The key is the includeBuild("..") in the settings.gradle.kts. It makes it so built dependencies from the parent directory are injected into the project. So you can use the compiler plugin and Gradle plugin without having to publish them.
| val disableAll: Property<String> = objects.property(String::class.java) | ||
| val disableTransformingDeprecatedApi: Property<String> = objects.property(String::class.java) | ||
| val disableTransformingNotImplementedApi: Property<String> = objects.property(String::class.java) | ||
| val disableTransformingEntryExitApi: Property<String> = objects.property(String::class.java) | ||
| val disableTransformingThrowingCatchingApi: Property<String> = | ||
| objects.property(String::class.java) | ||
| val disableCollectingCallSiteInformation: Property<String> = objects.property(String::class.java) |
There was a problem hiding this comment.
I would make these boolean properties and then convert to and from a String when creating the SubpluginOptions. That will make it more clear how to configure the Gradle options.
Also, if you want to provide a default, you can use .convention(false) after the property.
objectFactory.property(Boolean::class.java).convention(false)
| import org.gradle.api.model.ObjectFactory | ||
| import org.gradle.api.provider.Property | ||
|
|
||
| // Based on https://github.com/bnorm/kotlin-ir-plugin-template |
There was a problem hiding this comment.
If you want to look at something a bit more recent, you can check out piecemeal, a compiler-plugin I've been slowly working on myself.
| extra["kotlin_plugin_id"] = "io.github.oshai.kotlinlogging.kotlin-ir-plugin" | ||
| extra["kotlin_plugin_package_name"] = "io.github.oshai.kotlinlogging.irplugin" |
There was a problem hiding this comment.
I would drop the IR naming of the plugin and just call it "kotlin plugin" or something like that. I really should update that template and drop the IR part of the name. There's many things you can do now with a compiler plugin that have nothing to do with IR.
| disableAll = false | ||
| disableTransformingDeprecatedApi = false | ||
| disableTransformingNotImplementedApi = false | ||
| disableTransformingEntryExitApi = false | ||
| disableTransformingThrowingCatchingApi = false | ||
| disableCollectingCallSiteInformation = false |
There was a problem hiding this comment.
Just note that this is currently inaccurate. Currently you would need to do "true" or "false" and they are all required values. The Kotlin compiler plugin may not require them, but they way you have Gradle configured does make them required.
The suggestions I made for the Gradle extension will make it accurate though.
| private var parentDeclarations = ArrayDeque<IrDeclarationBase>() | ||
| private var parentDeclaration: IrDeclarationParent? = null | ||
| private var parentClass: IrClass? = null |
There was a problem hiding this comment.
IrElementTransformerVoidWithContext should provide much of this functionality already, as it maintains a stack of elements as it navigates. You should have access to properties like currentClass which will give you the current IrClass or currentDeclarationParent which gives you the current IrDeclarationParent.
| ) : IrGenerationExtension { | ||
| override fun generate(moduleFragment: IrModuleFragment, pluginContext: IrPluginContext) { | ||
| messageCollector.report(CompilerMessageSeverity.INFO, "Plugin config: $config") | ||
| for (file in moduleFragment.files) { |
There was a problem hiding this comment.
Instead of checking if (config.disableAll) in each visit overload, it would be better to check and early return here. Then that check can be omitted from each of the visit overloads.
| CompilerMessageSeverity.INFO, | ||
| "Error while processing call: ${e.message}", | ||
| ) | ||
| messageCollector.report( |
There was a problem hiding this comment.
While I would recommend just letting the exception propagate, I would at least attach a CompilerMessageSourceLocation to this warning. Should be easy to add to SourceFile:
fun getCompilerMessageLocation(element: IrElement): CompilerMessageSourceLocation {
val info = getSourceRangeInfo(element)
val lineContent = getText(info)
return CompilerMessageLocationWithRange.create(irFile.path, info.startLineNumber, info.startColumnNumber, info.endLineNumber, info.endColumnNumber, lineContent)!!
}
| originalLogExpression.endOffset, | ||
| ) | ||
| val eventBuilderLambdaArgument = atCall.valueArguments.last() as IrFunctionExpression | ||
| val eventBuilderLambdaBody = eventBuilderLambdaArgument.function.body!! |
There was a problem hiding this comment.
Cast body to IrBlockBody and you won't need to cast eventBuilderLambdaBody.statements to a MutableList.
| ) | ||
| val eventBuilderLambdaArgument = atCall.valueArguments.last() as IrFunctionExpression | ||
| val eventBuilderLambdaBody = eventBuilderLambdaArgument.function.body!! | ||
| (eventBuilderLambdaBody.statements as MutableList).add( |
There was a problem hiding this comment.
It's possible an early return is present in the statement list. Maybe setting the internal data should be added as the first statement?
|
The other thing I will mention here is that it might be wise to place the entire compiler plugin in a separate repository. Each time a new version of Kotlin is released, changes may need to be made to the compiler plugin to make it compatible. This means the compiler plugin is intrinsically tied to the version of Kotlin. You'll have to maintain a compatibility list for which version must be used. So maybe it's best to have a |
|
Thanks a lot for your suggestions, @bnorm!
@oshai, is it fine by you if I just move the compiler plugin to a new repo on my personal GitHub account? Some API changes will still have to land in |
|
I moved changes to |
I am ok with having a separate repo, sure. No issue with having it in your account. I hope all public changes will make sense. |
Kotlin compiler plugin for #449
See
kotlin-ir-plugin/README.mdfor details.Publishing is still a bit of a mystery. I would like to have a sample project where we could attach the compiler plugin and demonstrate the features. However, I'm not sure how to attach the plugin - what version to use, etc. Or should we put the sample project in a different (independent) repository?