Opening classes for frameworks #40

Open
wants to merge 1 commit into
from

Projects

KEEP Pending in KEEP Kotlin 1.1

@abreslav
Contributor
abreslav commented Jul 25, 2016 edited

Kotlin has classes and their members final by default. This has been causing debates that are summarized in this forum post. While all the reasoning behind this default remains valid, there are use cases that tend to be painful, most notable are frameworks using CGLib proxies (e.g. Spring AOP, Mockito) and Gradle that decorates task classes at runtime.

We propose to enable a compilation strategy that does not classes and their members final if they are marked with framework-specific annotations, such as @Service, @Repository, @Bean, @Configuration.

Open issues:

  • Support for custom notion of "indirectly annotated" (see Spring's @Component)
  • Investigate possible impact on build systems/IDE integration of having this implemented as a compiler plugin
@sdeleuze

What I like in the "META-INF file" solution is that's:

  • Not intrusive
  • Will avoid some IDE and tooling integration issues compared to other solutions
  • Allow each library to contribute its "open annotation" list without some custom configuration

We are ok to integrate such META-INF file directly in Spring Framework JAR.

Instead of listing every single annotation like @Service or @Configuration, if think that could be better to just identify classes annotated directly or indirectly by @Component (see Configuration annotated with @Component).

@abreslav
Contributor
abreslav commented Aug 9, 2016

@sdeleuze

What I like in the "META-INF file" solution is that's:

  • Will avoid some IDE and tooling integration issues compared to other solutions

Could you clarify? What issues do you mean?

  • Allow each library to contribute its "open annotation" list without some custom configuration

Isn't such file an example of custom configuration?

@abreslav
Contributor
abreslav commented Aug 9, 2016

Instead of listing every single annotation like @Service or @Configuration, if think that could be better to just identify classes annotated directly or indirectly by @Component (see Configuration annotated with @Component).

I see your point, and it's very appealing.

One problem: this brings the notion of "indirectly annotated" into the picture, and while it may be very obvious what that means for Spring, I'm not sure that it can be defined in a one-size-fits-all way. This speaks in favor of a compiler plugin where each framework could potentially define its own predicate using the full power of a programming language, not a restricted declarative format in META-INF.

@sdeleuze

One of my main concern is to make such behavior reliable and minimize the amount of customization for the end user.

I would like to avoid issues like the default open class behavior is taken in account by the Gradle/Maven but not the IDE. META-INF file solution is maybe a kind of custom configuration but it would be taken in account by any Kotlin 1.1 based compiler without any custom configuration at build tool level.

My remark about @Component is also about a similar concern, we have a lot of different @Component annotations is Spring projects, and users can define their own. So the ability to detect any kind of indirectly annotated @Component would make such support much more reliable.

If the chosen solution is a compiler plugin, what would be the requirement for us? Some kind of org.springframework.kotlin.CompilerPlugin class parameter added to the Gradle or Maven plugin configuration? Do you plan to bundle such class with Maven/Gradle Kotlin plugin in order to avoid requiring another plugin or do you expect us to provide it as an additional dependency?

@abreslav
Contributor

I would like to avoid issues like the default open class behavior is taken in account by the Gradle/Maven but not the IDE. META-INF file solution is maybe a kind of custom configuration but it would be taken in account by any Kotlin 1.1 based compiler without any custom configuration at build tool level.

Fair point, a pro for the static config file is that it can be read with equal ease by all tools. Effectively, all tools would inherit this functionality from the compiler. I'm not entirely convinced that adding such (arguably ad hoc) things to the compiler is generally a good course to take.

@abreslav
Contributor

My remark about @Component is also about a similar concern, we have a lot of different @Component annotations is Spring projects, and users can define their own. So the ability to detect any kind of indirectly annotated @Component would make such support much more reliable.

This is an important refinement of the initial requirements, we should probably think a little more about it.

If the chosen solution is a compiler plugin, what would be the requirement for us? Some kind of org.springframework.kotlin.CompilerPlugin class parameter added to the Gradle or Maven plugin configuration? Do you plan to bundle such class with Maven/Gradle Kotlin plugin in order to avoid requiring another plugin or do you expect us to provide it as an additional dependency?

I'd imagine that it should be something like apply plugin: "kotlin-spring" for Gradle, and not exactly sure what we can do for Maven, but we should definitely look more into it. Adding a <kotlin-spring/> tag somewhere would be ideal, IMO.

@sdeleuze

@abreslav Is that still expected to be available in Kotlin 1.1 ?

@cy6erGn0m
Contributor

btw, solution "META-INF file` also provides ability to specify nullable/not-nullable annotation types. Some frameworks (for example vert.x) use their own annotations so they can supply such meta-inf file for better Kotlin compatibility

@Writtscher

@abreslav Any estimation when we'll have this ..fixed''? I'd really like to apply shiny kotlin syntax to my projects. But it's a showstoppper for now as we are heavily using spring..

@abreslav
Contributor
abreslav commented Sep 30, 2016 edited

@sdeleuze @Writtscher We are currently working on a prototype with the following properties:

  • there's a compiler plugin that takes a list of "all-open" annotation class names,
  • annotated classes and their members and their descendants become open,
  • annotated annotations are added to the "all-open" list.

This plugin will be available from the command line, and in build scripts we are planning to have plugins that configure a particular framework with more or less a one-liner, like

apply plugin: 'kotlin-spring'

If everything works out alright, this will ship in 1.0.x EAP soon enough (long before 1.1).

We also plan to do the same for no-arg constructors for JPA and other serialization frameworks.

I'll update the KEEPs accrodingly next week.

@sdeleuze

That's awesome news @abreslav thanks a lot!!!

@nilsschmidthamburg

Brilliant work, thanks a lot!
Can't wait to use it. Especially when migrating big java projects to kotlin (automatically) this would avoid many problems. :-)

@abreslav abreslav modified the milestone: 11, 1.1 Oct 10, 2016
@pbartoszek

Can I check if the proposed solution will work for tests run directly from IntelliJ? For instance for spring integration tests? As far as I know IntelliJ uses Kotlin compiler directly bypassing maven/gradle

@abreslav
Contributor

@pbartoszek The IDE will run everything correctly after importing the settings from Gradle (as it routinely does for dependencies)

@pbartoszek

It looks like it's time to move from maven to Gradle :)

@abreslav
Contributor

@pbartoszek We are planning to have the same support for Maven as well

@kenyee
kenyee commented Oct 14, 2016

Looking forward to the plugin as well... Definitely one of the reasons I've been holding off on trying kotlin for a spring project 😁

@oluwasayo
oluwasayo commented Oct 15, 2016 edited

@abreslav apply plugin: 'kotlin-spring' made me cringe a bit. This is a problem faced by a wider dev community (not just spring). We're heavily invested in CDI/Weld and this is a pain point for us too. Meanwhile, with CDI when bean discovery mode is set to all in beans.xml, it is possible to not annotate the bean class at all and the CDI container treats it as @Default.

For such classes can we have a Kotlin annotation or keyword like this?
proxyable class MyBean { . . . }

@yanex
yanex commented Oct 15, 2016 edited

@oluwasayo There will be a generic plugin that uses the list of "all-open" annotations provided by the user, as well as some plugins for the popular frameworks (such as "kotlin-spring").
Also, it will be easy to write the new Gradle/Maven plugin (as an "extension" for the generic one) for any other framework.

@oluwasayo

@yanex Awesome!

@pbartoszek

Guys, do you have any update on plugin?

@yanex
yanex commented Dec 5, 2016

@pbartoszek We are planning to release it as a part of Kotlin 1.0.6 in the middle of December.

@eygraber

Any chance there will be an option to open all classes?

@yanex
yanex commented Dec 27, 2016

@eygraber Why do you need this?

@eygraber

@yanex If I want to mock a lot of classes, but I don't want to annotate each one of them.

@angryziber
angryziber commented Dec 27, 2016 edited

@eygraber - try mocking with Mockito 2.2 with mock-maker-inline enabled - works like a charm with closed by default Kotlin classes and methods.

@eygraber

@angryziber it doesn't work on Android devices

@yanex
yanex commented Dec 27, 2016

@eygraber Why don't make all mockable classes implement some interface with the all-open annotation on it?

@eygraber

@yanex I have a large number of complex classes that I would need to mock, and I don't like the idea of adding an annotation (or interface) that exists only for testing. It's much easier to just say that for testing everything should be open.

@cypressious
Contributor
@yole
Contributor
yole commented Dec 28, 2016

The appropriate solution for allowing all classes to be mocked in tests is to use a custom JUnit test runner that removes the final modifier using bytecode processing. We plan to include such a runner in the kotlin-test library.

@eygraber

@yole again that would cause issues on Android because running tests on Android uses a custom runner. I tried creating an Android transform in a gradle Plugin a while back, but I wasn't able to get it to work well (no_more_finals_plugin).

I'd say this issue on Android will most likely need to be addressed differently unless there's a method to remove final from all classes.

@eygraber

Another issue with Android is that whatever is done needs to happen at compilation time before dexing, otherwise typical bytecode manipulation won't work (because it's dex not Java bytecode).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment