#15 better support for compiler plugins #27

Foso · 2019-10-20T19:20:05Z

This PR will added the option to add a list of ComponentRegistrars to the compiler.

…tion

tschuchortdev · 2019-10-20T22:31:48Z

Looks nice. Arrow-meta was announced to the public at lambda world conf this Friday, so it's good if we have more ergonomic support for compiler plugins soon when more people will write their own.

Is this ready to be merged or more like a WIP? I will do a closer review tomorrow.

The stupid CI failed again but I think it's unrelated to these changes.

Foso · 2019-10-21T06:11:42Z

Hi, i heard about arrow-meta and i'm excited to try it out :)

This is ready to be merged, i wanted to add this feature, before i will add the support of the JSCompiler

tschuchortdev · 2019-10-21T13:23:06Z

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

 	var pluginClasspaths: List<File> = emptyList()

+	/**
+	 * Compiler plugins that should be added the compilation


tschuchortdev · 2019-10-21T13:23:50Z

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

-		if(annotationProcessors.isEmpty()) {
-			log("No services were given. Not running kapt steps.")
-			return ExitCode.OK
-		}


Why was this removed?

I removed this because it was not possible to execute the compiler plugins without also adding an annotation processor.
The problem was that the pluginClasspaths were set after this check.

I restored this and made changes. Inside stubsAndApt, only the annotation processors are added to threadLocalParameters. Then inside compileKotlin, only the compiler plugins are set, so the processors aren't getting executed twice.

Wouldn't this change the behaviour so that additional compiler plugins are only registered in the compileKotlin step? The current behaviour of #23. Is that additional plugins are executed in both steps. I don't know which way is actually right though,

Maybe you are right, we could also do it this way:

if(annotationProcessors.isEmpty() && compilerPlugins.isEmpty()) { log("No services were given. Not running kapt steps.") return ExitCode.OK }

I remove the changes in stubsAndApt and compileKotlin and just add the additional check for the compilerPlugins. Then the behaviour stays the same and it's possible to add compilerPlugins without also adding annotation processors.

I think it would be best to always add compiler plugins in the stubsAndApt step but cancel it if there are no annotation processors, like we did previously.

I'm not sure if i understand it correctly, in stubsAndApt we'll add the compiler plugins and cancel if there is no annotation processor. And then in compileKotlin we'll add the plugins again?

Otherwise i can't see how it's possible to add a compiler plugin without also having to add a annotation processor.

src/test/kotlin/com/tschuchort/compiletesting/CompilerPluginsTest.kt

tschuchortdev · 2019-10-26T13:52:53Z

src/main/kotlin/com/tschuchort/compiletesting/MainComponentRegistrar.kt

+
+    override fun registerProjectComponents(project: MockProject, configuration: CompilerConfiguration) {
+        val parameters = threadLocalParameters.get()
+        KaptComponentRegistrar(parameters.processors,parameters.kaptOptions).registerProjectComponents(project,configuration)


I read a few days ago in the Kotlin slack that the order of registering compiler plugins matters. Right now we are registering Kapt first. Could there be a problem here? For example if a compiler plugin like arrow-meta introduces some new syntax/type checking that isn't legal in vanilla Kotlin the respective plugin would have to be run before Kapt or Kapt wouldn't know how to deal with that. Any thoughts on this @raulraja?

raulraja · 2019-10-26T22:13:41Z

Unless I'm missing this is fine because meta does not rely on java stubs, it's interleaved with the compiler. But you may want to offer kapt registration as opt in and let them specify which plugin they want to run supporting a predefined set of known plugins like kapt, serialization, meta etc.

…

On Sat, Oct 26, 2019, 3:52 PM Thilo Schuchort ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/kotlin/com/tschuchort/compiletesting/MainComponentRegistrar.kt <#27 (comment)> : > + * limitations under the License. + */ + +package com.tschuchort.compiletesting + +import org.jetbrains.kotlin.base.kapt3.KaptOptions +import org.jetbrains.kotlin.com.intellij.mock.MockProject +import org.jetbrains.kotlin.compiler.plugin.ComponentRegistrar +import org.jetbrains.kotlin.config.CompilerConfiguration +import org.jetbrains.kotlin.kapt3.base.incremental.IncrementalProcessor + +internal class MainComponentRegistrar : ComponentRegistrar { + + override fun registerProjectComponents(project: MockProject, configuration: CompilerConfiguration) { + val parameters = threadLocalParameters.get() + KaptComponentRegistrar(parameters.processors,parameters.kaptOptions).registerProjectComponents(project,configuration) I read a few days ago in the Kotlin slack that the order of registering compiler plugins matters. Right now we are registering Kapt first. Could there be a problem here? For example if a compiler plugin like arrow-meta introduces some new syntax/type checking that isn't legal in vanilla Kotlin the respective plugin would have to be run before Kapt or Kapt wouldn't know how to deal with that. Any thoughts on this @raulraja <https://github.com/raulraja>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27?email_source=notifications&email_token=AADPQXDSIWXGBBN5ZQC7T63QQRDTNA5CNFSM4JCVVQ7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJKH27Q#pullrequestreview-307527038>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADPQXBKC5VCSRXP4ART2HTQQRDTNANCNFSM4JCVVQ7A> .

tschuchortdev · 2019-10-27T14:15:49Z

@raulraja That's not really what I meant. Let's assume you have some plugin that replaces every method named foo with a method bar. If the Kapt plugin is registered first, I assume that apt will see a foo method in the stub files. If Kapt is registered last, then I assume apt will see a bar method in the stub files. This could be problematic.

raulraja · 2019-10-27T16:56:28Z

Yes, I see what you mean now, sorry I misunderstood. Yes, in that case order matters.

…

On Sun, Oct 27, 2019, 3:15 PM Thilo Schuchort ***@***.***> wrote: @raulraja <https://github.com/raulraja> That's not really what I meant. Let's assume you have some plugin that replaces every method named foo with a method bar. If the Kapt plugin is registered first, I assume that apt will see a foo method in the stub files. If Kapt is registered last, then I assume apt will see a bar method in the stub files. This could be problematic. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27?email_source=notifications&email_token=AADPQXHEHLFPZBVNYNUEMHLQQWIANA5CNFSM4JCVVQ7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECK7LXY#issuecomment-546698719>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADPQXHJAPC3GSGWGAVZO7LQQWIANANCNFSM4JCVVQ7A> .

tschuchortdev · 2019-10-29T11:57:14Z

src/main/kotlin/com/tschuchort/compiletesting/KotlinCompilation.kt

-
-		/**
-		 * Here the list of compiler plugins is set
-		 *
-		 * To avoid that the annotation processors are executed twice,
-		 * the list is set to empty
-		 */
-		MainComponentRegistrar.threadLocalParameters.set(
-			MainComponentRegistrar.Parameters(
-				listOf(),
-				KaptOptions.Builder(),
-				compilerPlugins
-			)
-		)
-
-


Why did you remove this again? I think we always need to add the compiler plugins to the compileKotlin step, even if they already ran in the stubsAndApt step.

Hi @tschuchortdev , i removed it because i wanted to change a less as possible, because i couldn't guarantee that it would break any behaviour of any other compiler plugins that runs in any of the compiler phases. All i can say is, that i could test my MpApt Plugin with these changes and it didn't break any of the existing tests and Kapt was also working. I don't know much about when which compiler phase is running, because my plugin is only running in one phase.

Are you creating new source files with your plugin or simply adding to the AST?

My plugin just detects the annotations, it doesn't provide code generation

I think we still need to run the plugins in the compileKotlin step then, because all the changes done to the AST during stubsAndApt will be discarded, so a compiler plugin that changes the AST would have no effect.

But I think we also need to run the plugins in the stubsAndApt step as well, before kapt plugin is run.

I also think that we should add it to both. I could restore my deleted version.

Sounds good 👍

I restored it, and changed the check in stubsAndApt to "annotationProcessors.isEmpty()" again, because without, all added plugins were executed in both phases. In the future we need an option where you can set in which phases you want to execute the plugins, but for now i would keep it that way. If you think otherwise, just add " && compilerPlugins.isEmpty()" to the if statement again.

tschuchortdev · 2019-10-29T12:02:23Z

@Foso Can you check what the correct behaviour is when KApt interacts with compiler plugins? If you just modify your MpApt plugin to add some dummy method to all classes and then run it together with a regular annotation processor, you should be able to see the method (or not) in the AP and we can be sure. I'm not so familiar with compiler plugins unfortunately or I would do it myself.

If not, I say we just register first-party plugins before KApt in the MainComponentRegistrar and merge it. If someone complains we can always fix it later.

…ile-testing into tschuchortdev#15-betterSupportForCompilerPlugins � Conflicts: � src/test/kotlin/com/tschuchort/compiletesting/KotlinCompilationTests.kt

…ion - add plugins in compileKotlin

tschuchortdev

The arrow-meta people will be happy to have this. I will merge it manually.

Foso added 2 commits October 20, 2019 20:58

15: enable adding list of ComponentRegistrar instances to the compila…

1dd5f2b

…tion

15: enable adding list of ComponentRegistrar instances to the compila…

4bacb84

…tion

tschuchortdev approved these changes Oct 21, 2019

View reviewed changes

15: enable adding list of ComponentRegistar instances to the compilation

bd20eed

tschuchortdev reviewed Oct 26, 2019

View reviewed changes

15: enable adding list of ComponentRegistar instances to the compilation

bc48d39

tschuchortdev reviewed Oct 29, 2019

View reviewed changes

tschuchortdev self-requested a review October 29, 2019 20:23

Foso added 3 commits November 6, 2019 19:22

Merge branch 'master' of https://github.com/tschuchortdev/kotlin-comp…

c64972a

…ile-testing into tschuchortdev#15-betterSupportForCompilerPlugins � Conflicts: � src/test/kotlin/com/tschuchort/compiletesting/KotlinCompilationTests.kt

15: enable adding list of ComponentRegistar instances to the compilat…

b778d26

…ion - add plugins in compileKotlin

15: enable adding list of ComponentRegistar instances to the compilat…

725bed3

…ion - add plugins in compileKotlin

tschuchortdev approved these changes Nov 11, 2019

View reviewed changes

tschuchortdev closed this Nov 11, 2019

tschuchortdev mentioned this pull request Nov 17, 2019

Better support for compiler plugins #15

Closed

#15 better support for compiler plugins #27

#15 better support for compiler plugins #27

Uh oh!

Conversation

Foso commented Oct 20, 2019

Uh oh!

tschuchortdev commented Oct 20, 2019

Uh oh!

Foso commented Oct 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulraja commented Oct 26, 2019 via email

Uh oh!

tschuchortdev commented Oct 27, 2019

Uh oh!

raulraja commented Oct 27, 2019 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Foso Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Foso Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschuchortdev commented Oct 29, 2019

Uh oh!

tschuchortdev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Foso Nov 4, 2019 •

edited

Loading

Foso Nov 4, 2019 •

edited

Loading