Skip to content

Deduplicate extension config from ExtensionRegistrarConfigurator #32

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Deduplicate extension config from ExtensionRegistrarConfigurator #32

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions .../org/jetbrains/kotlin/compiler/plugin/template/services/ExtensionRegistrarConfigurator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,16 @@
package org.jetbrains.kotlin.compiler.plugin.template.services

import org.jetbrains.kotlin.backend.common.extensions.IrGenerationExtension
import org.jetbrains.kotlin.compiler.plugin.CompilerPluginRegistrar
import org.jetbrains.kotlin.compiler.plugin.template.SimplePluginRegistrar
import org.jetbrains.kotlin.compiler.plugin.template.ir.SimpleIrGenerationExtension
import org.jetbrains.kotlin.compiler.plugin.template.SimplePluginComponentRegistrar
import org.jetbrains.kotlin.config.CompilerConfiguration
import org.jetbrains.kotlin.fir.extensions.FirExtensionRegistrarAdapter
import org.jetbrains.kotlin.test.model.TestModule
import org.jetbrains.kotlin.test.services.EnvironmentConfigurator
import org.jetbrains.kotlin.test.services.TestServices

class ExtensionRegistrarConfigurator(testServices: TestServices) : EnvironmentConfigurator(testServices) {
private val registrar = SimplePluginComponentRegistrar()
override fun CompilerPluginRegistrar.ExtensionStorage.registerCompilerExtensions(
module: TestModule,
configuration: CompilerConfiguration
) {
FirExtensionRegistrarAdapter.registerExtension(SimplePluginRegistrar())
IrGenerationExtension.registerExtension(SimpleIrGenerationExtension())
}
) = with(registrar) { registerExtensions(configuration) }
Copy link
Member

@bnorm bnorm Oct 22, 2025

Choose a reason for hiding this comment

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

Hmm... I'm not sure how I feel about this specifically as a pattern to recommend. While I like and agree with the overall direction of deduplicating this registration logic, delegating directly to the registerExtensions member function of SimplePluginComponentRegistrar doesn't feel like the right choice. As an example, it's common for complex compiler plugins to have lots of custom directives to control various configuration options. There might also be test specific configuration to help debug failures.

I think we should follow the Kotlin Serialization pattern here and have a companion-level function to hold the logic. A companion-level extension function should work a little cleaner as well. And name it something like registerPlugin so we don't accidentally recurse.

Copy link
Contributor Author

@kyay10 kyay10 Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at Metro's version, they prepare all these options from directives yes, but then ultimately pack it into a MetroOptions object, and the rest of the registration is almost the same logic as the normal registrar, except that they turn off the lookup tracker and expect-actual tracker. Their normal registration gets the same options thru the compiler configuration. Thus, I don't see why they couldn't just do it in configureCompilerConfiguration.
Similarly for the serialization plugin. They can add their singular option into configuration easily.
My point is that the normal registrar always gets its info from the configuration, so for tests, one should just put the necessary info into configuration, and then call registerExtensions.
In some sense, parsing directives into the compiler configuration is the same idea as parsing cli options into the compiler configuration.
Btw, there's no fear of accidental recursion since this method is called registerCompilerExtensions and the normal one is registerExtensions.

}
Loading