Java EE-friendly compiler mode

gavinking commented 8 years ago

Thinking further about #6609 #6610, I now think we should go down a slightly different path.

The original mapping of Ceylon->Java was designed to be as "strict" as possible, allowing the minimum possible "misuse" of Ceylon APIs called from Java. More recently we weakened it slightly to make Ceylon objects serializable-by-default, which I still think was the right call.

However, the current mapping is still not perfect in the Java EE universe, where there are a bunch of patterns/rules that both our mapping, and our language defaults, get in the way of. For example,

Several Java EE specs won't let you define final members.
Some Java EE specs don't know what to do with ceylon.language.Integer, ceylon.language.String, and friends.
Some Java EE specs have problems with our late initialization checking.
JAX-RS likes having public default constructors. Grrr.

Therefore, I propose a separate mode where:

We never generate final on methods, even if they're not marked default.
We store Integer?, Float?, String?, Byte?, Boolean? fields as java.lang's Long, Double, String, Byte, Boolean.
We disable runtime assignment checking for late fields. (Hat tip: @jvasileff.)
We make the secret default constructor public instead of protected, since JAX-RS likes it that way.

Now, what triggers this mode? There's a number of possibilities I can think of. For example, it might be triggered by a persistence.xml or beans.xml in META-INF, or it might be triggered by an @Entity annotation.

On the other hand, there should probably also be a commmand line option, because "Java EE-friendly" is also probably "Spring EE-friendly".

I think this is better than what I had proposed earlier, and so I'm going to close the other issues.

gavinking commented 8 years ago

@tombentley How much work is involved in what is outlined above?

gavinking commented 8 years ago

Potential enhancement to this: we could even somehow mess with the mapping for collection-typed fields, to transform Ceylon collection-typed fields into Java collection types.

Actually we should definitely do this, though it's a little more work.

tombentley commented 8 years ago

How much work is involved

1, 3 and 4 are relatively easy. 2 is going to be more fiddly. Maybe a week in total. It should be do-able for 1.3.2. Collection conversion is way harder.

We need to put more work into figuring out when to trigger this stuff. I don't think a compiler options is the right thing, because it doesn't apply to specific program elements at all, just on whatever elements are involved in a particular compilation.

quintesse commented 8 years ago

I have the feeling this is something where we should think hard and long about the possible consequences of suddenly having several different mapping from Ceylon types to the types we finally generate. Right now it's two-pronged: boxed or unboxed, which already creates a lot of complexity. It would now become 3-pronged (while hopefully keeping compatibility with old code).

I'd almost be more inclined to just break and switch to using Java types for our boxed types. Except that would introduce the possibility of double boxing perhaps.

2 is going to be more fiddly

Just "more fiddly"? Seems like a pretty big change to me?

gavinking commented 8 years ago

We need to put more work into figuring out when to trigger this stuff.

Yeah, let me do a little research on that. I have to re-familiarize myself with some of the specs in this area (some of which I helped write, but still don't remember).

In the meantime...

1, 3 and 4 are relatively easy.

Alright, could you make a start on this bit while I figure out some rules for how it gets turned on?

2 is going to be more fiddly.

Yes, of course. We don't have to deliver this whole issue in 1.3.1 though. So let's play it by ear. It would be great to have it in but we can live without it.

Collection conversion is way harder

I think it's a bit harder, for sure, but I don't think it's all that different from 2.

tombentley commented 8 years ago

Yeah, OK maybe I was a little hasty. "Quite a lot more fiddly". It's actually an opportunity to improve the compiler backend if we can make this more pluggable and less hard-coded.

gavinking commented 8 years ago

Aren't our fields always protected by getters/setters?

gavinking commented 8 years ago

Aren't our fields always protected by getters/setters?

Ah, not quite because they're set directly from the constructor. Still, that's only three locations per field to worry about:

the constructor
the getter
the setter

I don't see how that's an enormous impact.

quintesse commented 8 years ago

Quite a lot more fiddly

The English and their understatements! rolls eyes

I don't see how that's an enormous impact.

Famous last words ;) I hope you're right but I doubt it. I'm sure we'll need changes in the model loaders for one.

gavinking commented 8 years ago

I'm sure we'll need changes in the model loaders for one.

Not true. The metadata is read from the getter, not from the field.

lucaswerkmeister commented 8 years ago

Does this have any implications for libraries? Should we, for example, compile future SDK releases in Java EE mode?

gavinking commented 8 years ago

Does this have any implications for libraries? Should we, for example, compile future SDK releases in Java EE mode?

No.

quintesse commented 8 years ago

Still, that's only three locations per field to worry about:

I think we also do direct access anywhere in the code inside the class whenever we know the getter/setter is/are final.

tombentley commented 8 years ago

If it's true, I wasn't aware of that @quintesse.

gavinking commented 8 years ago

@quintesse not that I have observed.

tombentley commented 8 years ago

We disable runtime assignment checking for late fields.

Would we do that for all late fields, or just those where we would otherwise generate an initialized flag field?

we could even somehow mess with the mapping for collection-typed fields, to transform Ceylon collection-typed fields into Java collection types.

So I assume you mean something like:

c.l.Sequential (or c.l.List-c.l.String?) →j.u.List
c.l.Map →j.u.Map
c.l.Set →j.u.Set

Obviously we'd lose identity if we're wrapping and unwrapping these things.

gavinking commented 8 years ago

Would we do that for all late fields, or just those where we would otherwise generate an initialized flag field?

Great question ... I'm gunna go with .... um ... (flips coin) ... all of them? For now?

So I assume you mean something like:

List -> j.l.List

Set -> j.l.Set

Map -> j.l.Map

String -> j.l.String (as today)

And optional types:

String? -> j.l.String
Integer? -> j.l.Long
Float? -> j.l.Double
Byte? -> j.l.Byte
Boolean? -> j.l.Boolean
Character? -> j.l.Integer

gavinking commented 8 years ago

Obviously we'd lose identity if we're wrapping and unwrapping these things.

Not a problem, since the container tends to wrap and unwrap them anyway.

Anyway, those interface types aren't Identifiable.

tombentley commented 8 years ago

We make the secret default constructor public instead of protected, since JAX-RS likes it that way.

Even for a non-shared class (where the initializer constructor would be package access)?

gavinking commented 8 years ago

Even for a non-shared class (where the initializer constructor would be package access)?

Not sure, we need to check what the JAX-RS spec says. Perhaps resources have to be public.

tombentley commented 8 years ago

They must be public, FTR

gavinking commented 8 years ago

They must be public, FTR

Then in that case it would only be necessary to do this for shared classes, it seems. And for un-shared classes we leave them package-access.

tombentley commented 8 years ago

For model loading we've previously set the Declaration.isDefault() flag when the method mirror is non-final. But in EE mode all methods are non-final, which means we should fallback to using the @Default annotation, but only when loading EE-mode classes. Which means we need to know a class was compiled in EE mode. Annoying. Or we add a @Final annotation in lieu of final. I think I prefer the latter option.

gavinking commented 8 years ago

Or we add a @Final annotation in lieu of final. I think I prefer the latter option.

I think that's reasonable. It will be more reusable if there are other cases in future where we need to suppress the final modifier.

tombentley commented 8 years ago

I have items 1 – 4 pretty much working on the ee-mode branch @gavinking, if you want to try it out. You'll need --ee on your command line (or ee in your config).

I still have to do the collection mapping. Presumably we map c.l::List<c.l::Integer> →j.l.List<j.l.Long>? What about List<List<Integer>>?

gavinking commented 8 years ago

@tombentley fantastic! That's great news.

gavinking commented 8 years ago

@tombentley it did not appear to like the default argument.

Gavins-MacBook-Pro-2:ceylon-wildfly-swarm-jaxrs gavin$ ceylon compile --ee && ceylon swarm --provided-module javax:org.wildfly.swarm:jaxrs jaxrs.example && java -jar jaxrs.example-1.0.0-swarm.jar
source/jaxrs/example/entity/Employee.ceylon:15: error: Ceylon backend error: no suitable method found for valueOf(Integer)
shared entity class Employee(name, year=null) {
                                   ^
    method Long.valueOf(String) is not applicable
      (argument mismatch; Integer cannot be converted to String)
    method Long.valueOf(long) is not applicable
      (argument mismatch; Integer cannot be converted to long)
Note: Created module jaxrs.example/1.0.0
ceylon compile: Fatal error: The compiler exited abnormally (4) due to a bug in the compiler.
Please report it:
 https://github.com/ceylon/ceylon/issues/new
Please include:

* the stacktrace printed below
* a description of what you were trying to compile.

Thank you!
com.redhat.ceylon.compiler.CompilerBugException: Bug
        at com.redhat.ceylon.compiler.CeylonCompileTool.handleExitCode(CeylonCompileTool.java:668)
        at com.redhat.ceylon.compiler.CeylonCompileTool.run(CeylonCompileTool.java:650)
        at com.redhat.ceylon.common.tools.CeylonTool.run(CeylonTool.java:524)
        at com.redhat.ceylon.common.tools.CeylonTool.execute(CeylonTool.java:405)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.redhat.ceylon.launcher.Launcher.runInJava7Checked(Launcher.java:115)
        at com.redhat.ceylon.launcher.Launcher.run(Launcher.java:41)
        at com.redhat.ceylon.launcher.Launcher.run(Launcher.java:34)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.redhat.ceylon.launcher.Bootstrap.runVersion(Bootstrap.java:162)
        at com.redhat.ceylon.launcher.Bootstrap.runInternal(Bootstrap.java:117)
        at com.redhat.ceylon.launcher.Bootstrap.run(Bootstrap.java:93)
        at com.redhat.ceylon.launcher.Bootstrap.main(Bootstrap.java:85)
Caused by: java.lang.ClassCastException: com.redhat.ceylon.langtools.tools.javac.code.Symbol$ClassSymbol cannot be cast to com.redhat.ceylon.langtools.tools.javac.code.Symbol$MethodSymbol
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.visitApply(Gen.java:1847)
        at com.redhat.ceylon.langtools.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1464)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genExpr(Gen.java:949)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.visitAssign(Gen.java:1988)
        at com.redhat.ceylon.langtools.tools.javac.tree.JCTree$JCAssign.accept(JCTree.java:1685)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genExpr(Gen.java:949)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.visitExec(Gen.java:1779)
        at com.redhat.ceylon.langtools.tools.javac.tree.JCTree$JCExpressionStatement.accept(JCTree.java:1295)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genDef(Gen.java:739)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genStat(Gen.java:774)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genStat(Gen.java:760)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genStats(Gen.java:811)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.visitBlock(Gen.java:1159)
        at com.redhat.ceylon.langtools.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:908)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genDef(Gen.java:739)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genStat(Gen.java:774)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genMethod(Gen.java:1033)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.visitMethodDef(Gen.java:996)
        at com.redhat.ceylon.langtools.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:777)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genDef(Gen.java:739)
        at com.redhat.ceylon.langtools.tools.javac.jvm.Gen.genClass(Gen.java:2461)
        at com.redhat.ceylon.compiler.java.tools.LanguageCompiler.genCodeUnlessError(LanguageCompiler.java:806)
        at com.redhat.ceylon.compiler.java.tools.LanguageCompiler.genCode(LanguageCompiler.java:772)
        at com.redhat.ceylon.langtools.tools.javac.main.JavaCompiler.generate(JavaCompiler.java:1575)
        at com.redhat.ceylon.compiler.java.tools.LanguageCompiler.generate(LanguageCompiler.java:942)
        at com.redhat.ceylon.langtools.tools.javac.main.JavaCompiler.generate(JavaCompiler.java:1539)
        at com.redhat.ceylon.langtools.tools.javac.main.JavaCompiler.compile2(JavaCompiler.java:904)
        at com.redhat.ceylon.langtools.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:862)
        at com.redhat.ceylon.compiler.java.tools.LanguageCompiler.compile(LanguageCompiler.java:271)
        at com.redhat.ceylon.compiler.java.launcher.Main.compile(Main.java:658)
        at com.redhat.ceylon.compiler.java.launcher.Main.compile(Main.java:564)
        at com.redhat.ceylon.compiler.java.launcher.Main.compile(Main.java:556)
        at com.redhat.ceylon.compiler.java.launcher.Main.compile(Main.java:545)
        at com.redhat.ceylon.compiler.CeylonCompileTool.run(CeylonCompileTool.java:649)
        ... 17 more

gavinking commented 8 years ago

That's with this:

shared entity class Employee(name, year = null) {

    generatedValue id
    shared late Integer id;

    column { length = 50; }
    shared String name;

    column
    shared variable Integer? year;

}

gavinking commented 8 years ago

@tombentley mind if I add an additional requirement at this point?

Store late primitive values using a Java wrapper class instead of an extra field. For example, late Integer id would be persisted as java.lang.Long id.

gavinking commented 8 years ago

mind if I add an additional requirement at this point?

Well, wait, perhaps that's not what I want after all. The issue is this warning:

source/jaxrs/example/entity/Employee.ceylon:10: warning: the 'late' attribute 'id' cannot be properly initialised just by setting the field value because it is erased to a primitive type: depending on the semantics of 'generatedValue' consider annotating the JavaBean Property getter with generatedValueGETTER or its setter with generatedValueSETTER or making it non-'late'
  generatedValue id

That's not quite right, since it can be properly initialized now that we got rid of the initialization checking. And I'm not sure if it merits a warning at all now.

gavinking commented 8 years ago

figure out some rules for how it gets turned on?

So this isn't actually very easy, but here's a couple of heuristics that might work:

Enable it at the class level if the class is annotated entity or xmlAccessorType, or if it has a constructor, field, or method annotated inject.
Enable it at the package level if the package is annotated xmlAccessorType.
Enable it at the module level if the module imports javax.javaeeapi or maven:"javax:javaee-api", or if it has a beans.xml or persistence.xml file in WEB-INF or META-INF.
Enable it for the whole compliation with --ee.

WDYT, @tombentley?

gavinking commented 8 years ago

Perhaps also turn it on for stateless, stateful, messageDriven, and singleton.

gavinking commented 8 years ago

Or perhaps we just autoenable it at the module level whenever the module directly imports:

Java EE,
CDI or Weld,
JPA or Hibernate,
JAXB, or
JAX-RS.

That's much simpler.

Any others that should be on that list, DYT?

tombentley commented 8 years ago

I like the idea of enabling it on a per-class basis according to annotations present. That's because we could easily make the enabling annotations configurable, so if people want EE-mode for Spring or any other thing it's not difficult for them to do it. It also means that only the required classes get EE-mode, and the rest still benefit from the ("better") default transformations. Obviously when I say "make it configurable" we could have sensible defaults so that in the usual case it "just works".

gavinking commented 8 years ago

I like the idea of enabling it on a per-class basis according to annotations present.

I agree that would be nice, but after some research I don't think it's really workable. It works well for JPA because you have the entity annotation. Also works for EJB, since you have component-defining annotations. Doesn't really work well for CDI, nor for JAXB.

gavinking commented 8 years ago

The issue is this warning ... That's not quite right, since it can be properly initialized now that we got rid of the initialization checking.

@tombentley, what's your reaction to this?

tombentley commented 8 years ago

@gavinking I'll be pushing fixes to the branch shortly.

tombentley commented 8 years ago

@gavinking I've updated the branch:

We now enable EE mode automatically when: javaee-api is a direct dependency of the module, or a class is annotated with any of various annotations.
The enabling module imports and annotations are configurable via --ee-import and --ee-annotation respectively. Setting these replaces (rather than adds to). --ee is still supported.
The warning about late won't be shown in EE mode.
Your other Employee example should now work.

Do we need to mention EE mode as a possible alternative solution in the annotated+late warning?

quintesse commented 8 years ago

@tombentley can't we do something like --ee=import and --ee=annotation where --ee would be the same as specifying --ee=import,annotation? All these extra options seem to me will only muddle the help/docs.

quintesse commented 8 years ago

Sorry forget that, I hadn't understood they took arguments.

gavinking commented 8 years ago

@tombentley excellent, thanks, I will try it out!

tombentley commented 8 years ago

@gavinking we now wrap List, Set and Map.

gavinking commented 8 years ago

Alright! Fantastic 👍

gavinking commented 8 years ago

So DYT this is finished now, @tombentley, or is there still additional work to do?

Perhaps if there is missing functionality, we should open separate issues.

tombentley commented 8 years ago

I'm happy to close this if you are @gavinking.

The only thing I think we've not done which you mentioned is to activate EE-mode based on the presence of persistence.xml or beans.xml in META-INF. If you still want this, we can open a separate issue.

gavinking commented 8 years ago

OK. So what precisely are the activation rules today? I just want to see them written down ;-)

tombentley commented 8 years ago

@gavinking https://ceylon-lang.org/documentation/1.3/reference/interoperability/ee-mode/#activating_ee_mode

gavinking commented 8 years ago

Ah, that's great that this is actually documented :-)

However, you say:

EE mode is usually activated automatically, in the presence of certain annotations on classes, or certain imports

I would like to see these "certain" things enumerated on that page ;-)

tombentley commented 8 years ago

@gavinking done

gavinking commented 8 years ago

OK, thanks, that looks good. Thanks for your hard work on this!

eclipse-archived / ceylon

Java EE-friendly compiler mode #6622