Enhancement: make Java8 lambda steps reflection portable and use stable javac APIs

[RichardBradley]

See:

• cucumber-java8 fails on OpenJDK 1.8.0 #912: a JDK upgrade which changed class file layout broke Java8 lambda steps
• Cannot close over local variables #916: closing over local variables breaks Java8 lambda steps
  • (currently fixed on branch https://github.com/cucumber/cucumber-jvm/tree/java8-fixes , using the existing Constant Pool inspector)
• TableConverter with Java 8 Lambdas: "Not a Map or List" exception is incorrect and not helpful #936: Java 8 Lambda steps don't support Map or List arguments
  • (may not be fixable using the the existing Constant Pool inspector)

Currently, for Java8 lambda steps to work, the step runner needs to know the declared type of all arguments to the step body (see Java8StepDefinition.getParameterInfos).

The declared type can be any runtime type, including primitives like int, normal classes like String or generic types like List<String>. Users can register converters to allow any type for these step params (see cucumber.runtime.ParameterInfo.convert), including perhaps Object (although that would be a pathological case, I expect it might need special handling by the reflector and it does seem to be allowed).

It does look to me like we could replace the class ConstantPool inspector with a compile-time Compiler Annotation Processor which records the declared generic types.
See e.g. http://stackoverflow.com/questions/29922668/get-typeelement-from-generic-typeparameterelement-for-java-annotation-processor

This would mean that cucumber-jvm would be using a public supported javac API, instead of hacking about with trying to interpret the class files internals (which might change again).

We could either require Cucumber java8 users to annotate their Step files to give cucumber-jvm access to edit the source, or we could do a full project lombok and register a compiler plugin for everyone who compiles anything with cucumber-java8 on the classpath and scan for (and fixup) all calls to En.Given etc.

I'll see if I can get this to work and submit a POC.

This will need careful testing, and possibly deserve a major release version, as it will greatly change how java8 steps are compiled.
We'll need to test different compilers and different build tools (Maven, ant, manual javac) and check that they all execute the CAP correctly.
First though, I'll write a proof of concept to see if this can be made to work at all, and what the code would look like with this new approach.

[RichardBradley]

OK, here's what I have been able to find.
I have to say, this would all be easy in Scala, but is near-impossible in Java :-(

Statement of Problem

We want to be able to write steps like:

Given("^I have (\\d+) cukes in my belly$", (Long n) -> {
  ...
})

The step definitions are stored as instances of cucumber.runtime.java.StepDefinition, which has properties "pattern" (regex) as a String, and 0..N parameterInfos as a ParameterInfo, which records the declared type of each param (including generic type info, such as List<MyDomainObject>.

Therefore, we need to be able to determine the full generic parameter type of each Lambda method argument.

Java 8 does not make this info available at runtime via reflection (see detailed comments on #936).

Possible fixes:

1. Remove this impossible requirement by making the API more repetitive
2. Use the OpenJDK com.sun.source.util.Trees API to inspect the source at compile time, record the generic type info in resource files
3. Use the Project Lombok API to modify the source at compile time, to pass the generic type info as method arguments
4. Leave all this alone, stick to inspecting the class constant pool

Each is described below:

Remove this impossible requirement by making the API more repetitive

It looks like targeting the above API for Cucumber will cause ongoing issues (see "cons" below).
We could try to avoid this, by changing the Given method to something more repetitive:

// for users:
Given("^I have (\\d+) cukes in my belly$", (Long n) -> {
  ...
}, TypeToken.of(Long.class))

Given("^I have users "(.*)"$", (List<MyDomainClass> users) -> {
  ...
}, new TypeToken<List<MyDomainClass>>() {})

// in the En interface:
public <T1> void Given(String regexp, StepdefBody.A1<T1> body, TypeToken<T1> param1Type);

(TypeToken is from Guava)

Pros:

• This avoids any compile-time hacking about.
• This avoids any not supported or partially supported Java APIs

Cons:

• It does make the Cucumber Java8 Lambda APIs much harder to use.
• It will be a breaking change for anyone currently using the Cucumber Java8 Lambda APIs.

Use the OpenJDK com.sun.source.util.Trees API

We could use the OpenJDK com.sun.source.util.Trees API to inspect the source at compile time and record the generic type info in resource files.

For background, please see http://notatube.blogspot.co.uk/2010/11/project-lombok-trick-explained.html -- in short:

• You cannot inspect method bodies using vanilla Java Annotation Processors
  • (We need to inspect the method bodies to see the "Given(...)" method invocations)
• You can inspect method bodies using the OpenJDK com.sun.source.util.Trees API, but not modify them
• You can modify method bodies using Project Lombok

For the OpenJDK com.sun.source.util.Trees API approach, we'd write a Java Annotation Processor which users would have to apply to their Step definition classes.
This processor would inspect all code inside that class using a TreePathScanner

The scanner would notice all calls to Given (and friends), and would record the full generic type info in a resources file or in a new synthetic class.

At runtime, the Given method would then look up the full generic type info from that resources file, and throw a descriptive error if they were not found (like "it looks like Annotation Processors haven't worked; please check the following things...").

I have checked that this works, and I have 50% of a POC coded up for this.

Pros:

• This uses only publicly supported Java APIs (no looking in class constant pools)
• This allows us to support the desired Cucumber Java8 Lambda API

Cons:

• This will only work on the OpenJDK (although I imagine that's > 99% of users)
• I'm not sure if this will work in Eclipse, which has a custom compiler, but I think it should
• Things are a bit complicated at runtime (resources lookups etc.). The Lombok version seems much nicer in this regard.

Use the Project Lombok API

We could use the Project Lombok API to modify the source at compile time, to pass the generic type info as method arguments.

Project Lombok have a lib which allows for compile-time modification of Java source code. They have a plugin system which lets you write your own Lombok "Handlers".

We would add Project Lombok as a transitive dependency of cucumber-jvm. Any Maven users would then have Lombok added to their compile classpath, and would then see compile-time transforms using the Lombok API.

Users who are not using Maven (or Ivy etc.) but want to use cucumber-jvm would need to manually download the Project Lombok jar and add it to their compile classpath.

We could write a Project Lombok "Hander" which would notice all calls to Given (and friends), and would rewrite them to call a different method and pass the ParameterInfo as a separate method param, with the generic type info, i.e.

// The user writes:
Given("^I have users "(.*)"$", (List<MyDomainClass> users) -> {
  ...
})

// At compile-time, our Lombok Handler re-writes this to:
PostProcessedGiven("^I have users "(.*)"$", (List<MyDomainClass> users) -> {
  ...
}, new ParameterInfo(List<MyDomainClass>))

At runtime, any direct calls to the Given method would throw a descriptive error like "it looks like Lombok compile-time macros haven't worked; please check the following things...").
The other calls would have all the ParameterInfo data they need to construct a StepDefinition immediately to hand.

I haven't tested this yet, but it looks very likely to work to me.

Pros:

• This allows us to support the desired Cucumber Java8 Lambda API
• Things are very neat at runtime

Cons:

• This is a bit more invasive.
• I'm not sure that every development environment supports Project Lombok
• Development setup for non-Maven users would be greatly complicated. We'd need to write detailed usage instructions.
• We'd need to add Lombok as a transitive dependency of cucumber java8, and it's a fairly big JAR
• Future versions of the JDK might break Lombok (but it seems likely that it would be fixed quickly by the Lombok community, which is large and active)
• I'm not sure if this will work in Eclipse, which has a custom compiler, but I think it should
• There might be side-effects: Lombok might run other handers?

Leave all this alone, stick to inspecting the class constant pool

Pros:

• It works (for now)
• It's simple

Cons:

• It might break again next time the JDK change their class layout
• It probably is already broken on non-OpenJDKs
• It uses non-public APIs.

Conclusion:

I'm leaning towards the Lombok handler, although I'm worried that it might have unexpected side-effects or not work for some users.

@aslakhellesoy -- what do you think?

[RichardBradley]

Actually, now that I've written all this out, option 4, "Leave all this alone, stick to inspecting the class constant pool" is looking increasingly attractive :-)

[aslakhellesoy]

Hi @RichardBradley thanks for that very detailed analysis!

For starters I think we should add an additional API as described in option 1. It could be in cucumber.api.java8.explicit. People would be able to use it if they want. If that works out nicely we can look into option 3 (lombok) and automatically translate from the original API to the new explicit API.

The original API could maybe delegate to this API to avoid too much duplication. We could also try to improve the original API by pursuing the constant pool route.

So to sum up:

• New explicit API
• Improve original constant pool API using the proposed hacks
• Refactor original constant pool API to delegate to the new explicit API
• Write lombok code to do the translation at compile time

WDYT?

[RichardBradley]

Good plan -- I like it.

Adding a cucumber.api.java8.explicit version like Option 1 will allow a workaround for #936 quickly without any risky changes. We can use the current reflection code (with its current limitations) to delegate to that API, and then look into adding Lombok to auto-translate to that API as a second step.

What do you mean by "Improve original constant pool API using the proposed hacks"? Are you talking about this commit 046d98c ?

I think you should get that commit onto master and released ASAP, as there are lots of cucmber-jvm Java8 users suffering from #912, and treat the other changes here as a longer-term refactoring.

[bademux]

Please don't use Lombok's magic under the hood - it leads to undefined unstable behviour. It may conflict with another generators like Mapstruct (eg. github.com/mapstruct/mapstruct/issues/510).
Lombok-like libraries can be used only directly or we all can get to the situation with Unsafe, when our dependencies have feet of clay.
Please, use standard APIs like annotation processing API, and\or runtime codegeneration, if possible.

[RichardBradley]

Please don't use Lombok's magic under the hood - it leads to undefined unstable behviour. It may conflict with another generators like Mapstruct (eg. github.com/mapstruct/mapstruct/issues/510).

The way I'm proposing to use Lombok to rewrite method bodies wouldn't cause conflicts like that in the issue you linked to.
However, I do agree that it's unstable -- perhaps the current bytecode inspection is safer.

Please, use standard APIs like annotation processing API, and\or runtime codegeneration, if possible

As explained above, the annotation processing API isn't powerful enough to solve this case for us (doesn't allow inspection or modification of method bodies). I don't think runtime code generation is applicable here.

I think we should probably stick to the current constant pool reflector (i.e. option 4). We should still do option 1 to make all this more explicit and pluggable though.

[RichardBradley]

For the record, I'm not planning to do anything further on this thread, since option 4 ("Leave all this alone, stick to inspecting the class constant pool") looks to be the best, given the serious problems with the other options.

[mpkorstanje]

I am sharing the opinion of @RichardBradley and I would like to suggest replacing our current implementation with an existing one. TypeTools is a minimal light weight library that does everything our ConstantPoolInspector does and does it much better. I'll tackle this in #1178.

[RichardBradley]

Love it, great work!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.