Allow FunctionalInterface conversion when the annotation is not present

[astrelsky]

I will have a look at the current tests tomorrow and see what I can add to throw the kitchen sink at this.

I tried to follow the formatting as close as possible.

Fixes #1107

[codecov]

Codecov Report

Merging #1110 (55f3b36) into master (1da1101) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
+ Coverage   88.65%   88.67%   +0.01%     
==========================================
  Files         111      111              
  Lines       10211    10211              
  Branches     4016     4016              
==========================================
+ Hits         9053     9055       +2     
+ Misses        699      698       -1     
+ Partials      459      458       -1     

Impacted Files	Coverage Δ
native/common/jp_context.cpp	81.53% <100.00%> (ø)
native/common/jp_javaframe.cpp	91.48% <100.00%> (ø)
native/common/jp_method.cpp	98.84% <0.00%> (+1.15%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Thrameos]

Does this code all work with jdk 8?

[astrelsky]

Does this cide all work with jdk 8?

It should. I don't think I used anything that wasn't available in jdk 8. The reason I got a MethodHandle to Class.isSealed is to keep jdk 8 compatibility and still keep the check to prevent a potentially confusing exception further down the road.

[Thrameos]

Okay. I will verify it on jdk8 when it is ready for review. I dont think our cloud currently tests old versions.

[astrelsky]

I added some test but unfortunately they are failing at the moment. There is something lingering elsewhere that is causing the cast to fail and I'm not sure where it is.

[astrelsky]

@Thrameos any ideas where the cast might be failing?

[Thrameos]

Should be able to look at it Friday.

[Thrameos]

So two problems.

It didn't check that the method exists for setting the FUNCTIONAL around line 460 in native/java/org/jpype/manage/TypeManager.java.

Unfortunately, doing that also exposes a race condition during initialization. Normally it won't search for the function name in the ClassDescriptor until after the type system is initialized, but once we start triggering on functions that could be considered functional but don't have the annotation we trigger on things like java.lang.invoke.TypeDescriptor which is created very early in the process before all the hooks are set up. We never explicitly load this class but it was fetched by something basic like java.lang.Class which is why the system is still booting as order is critical until Object and Class are completed. Thus we end up calling getFunctional before the hook was installed. I tried some rearrangements of the startup sequence, but no luck yet. As with all race conditions if takes a while to find the proper ordering which is a bit too challenging for most contributors (especially if I have to rearrange code to make it order free.) I will keep you posted when I work something out.

[Thrameos]

Okay I put in a pull request with the recommended fix. Hopefully it resolved the issue with the testbench.

[astrelsky]

@Thrameos I think it is ready for review. The option to request a review is not available.

[Thrameos]

It all looks fine to me. Though I am somewhat worried that we changed the definition of a public method in org.jpype.JPypeContext though I consider most of that stuff private so likely not an issue. (If I ever get helpers for the EPYPJ branch I was going to rearrange that portion anyway.) So I am going to put it to the master maintainer for review.

[astrelsky]

It all looks fine to me. Though I am somewhat worried that we changed the definition of a public method in org.jpype.JPypeContext though I consider most of that stuff private so likely not an issue. (If I ever get helpers for the EPYPJ branch I was going to rearrange that portion anyway.) So I am going to put it to the master maintainer for review.

That definition change may have been unintentional and something I did while troubleshooting. I'll look again after work but it should be trivial to avoid that change.

[Thrameos]

No, it was required because of the order of operations. When we make it recognize classes as Lambdas before java.lang.Class is constructed that means we need to create hooks before the context was finished constructing. So either I need to make that method static or I need to split the context start up into a before hooks and after hooks, or I could try to get more information passed into the JNI calls so that we don't have to probe back.

Making this method static was the shortest path to solution. And given noone is supposed to be accessing those JPypeContext methods outside of diagnostics it should be fine. The other solutions are not terribly hard to implement, but they are more involved and thus would make for a much larger PR that your original intent. So lets see if the simplest solution is acceptable first. But is the maintainer wants a better solution then I will make it so.

Sadly if you remove the static then the system will crash because with a NullPointerException or hook install failure during initialization.

[astrelsky]

Oh I didn't realize you made that change (or I did forget about it and it just worked out well)
Either way, if there is someone using it, it would still work for them and would just generate a warning about how a static method should be accessed in a static way blah blah.