Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add annotations to java.lang for Java modifiers #2324

Closed
CeylonMigrationBot opened this issue Sep 29, 2015 · 31 comments
Closed

add annotations to java.lang for Java modifiers #2324

CeylonMigrationBot opened this issue Sep 29, 2015 · 31 comments

Comments

@CeylonMigrationBot
Copy link

@CeylonMigrationBot CeylonMigrationBot commented Sep 29, 2015

[@gavinking] We should add the following annotations into java.base: java.lang.Transient, java.lang.Volatile, java.lang.Synchronized, java.lang.Native, java.lang.Strictfp.

This would be a priority issue for Ceylon 1.3 since it addresses interop problems.

[Migrated from ceylon/ceylon-compiler#2324]

@CeylonMigrationBot
Copy link
Author

@CeylonMigrationBot CeylonMigrationBot commented Sep 29, 2015

[@tombentley] Wouldn't native need some support from the tc, because such a thing would need to be allowed to not have a body, etc.

Loading

@CeylonMigrationBot
Copy link
Author

@CeylonMigrationBot CeylonMigrationBot commented Sep 29, 2015

[@gavinking]
@tombentley well yes, I guess, but that's not impossible.

Or we could just force you to add both native annotations...

Loading

@FroMage
Copy link
Contributor

@FroMage FroMage commented Aug 31, 2016

Should we consider this for 1.3.0? We have a user who's using Java serialisation, and although his problem would not be solved by a transient annotation, he wanted to use it.

The issue with adding new crap to java.lang is that it usually impacts the IDEs too. Perhaps a temporary hack would be to add compiler annotations for this?

WDYT @tombentley @gavinking?

Loading

@FroMage
Copy link
Contributor

@FroMage FroMage commented Aug 31, 2016

My fear is that without being able to implement readResolve and friends, this will only get you half-way, no?

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Aug 31, 2016

I think a "temporary hack" using compiler annotations is a terrible idea, because it would likely not be temporary enough. I don't find someone wanting to use something, but it not actually solving their problem to indicate a pressing need for action.

And readResolve()/writeReplace() are just ordinary methods IIRC, we're not stopping people from writing them if they need.

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Aug 31, 2016

The issue with adding new crap to java.lang is that it usually impacts the IDEs too.

Well I don't really see how this could pose a big problem. It's not like we need this stuff in the model. It's just a compiler thing, right? If the annotation is there we add the modifier. It never affects ceylon semantics.

Loading

@FroMage
Copy link
Contributor

@FroMage FroMage commented Aug 31, 2016

not actually solving their problem to indicate a pressing need for action.

Well, true, let's put this back for 1.4.

And readResolve()/writeReplace() are just ordinary methods IIRC, we're not stopping people from writing them if they need.

Oh, I didn't realise, fine.

It's just a compiler thing, right? If the annotation is there we add the modifier. It never affects ceylon semantics.

Well no, it needs completion and metamodel support, so it's always a bit more annoying. Not impossible, but I feel we shouldn't rush this so close to the release without thinking it through and properly testing it.

Loading

@FroMage FroMage added this to the 1.4 milestone Aug 31, 2016
@FroMage FroMage removed this from the 1.3.0 milestone Aug 31, 2016
@gavinking
Copy link
Member

@gavinking gavinking commented Aug 31, 2016

There's no reason we can't do this in 1.3.1, AFAICT. I don't think it blocks 1.3.0.

Loading

@FroMage
Copy link
Contributor

@FroMage FroMage commented Aug 31, 2016

Yeah, what I figured too.

Loading

@FroMage
Copy link
Contributor

@FroMage FroMage commented Aug 31, 2016

Thanks guys.

Loading

@gavinking gavinking added this to the 1.3.2 milestone Nov 22, 2016
@gavinking gavinking removed this from the 1.4 milestone Nov 22, 2016
@gavinking
Copy link
Member

@gavinking gavinking commented Nov 22, 2016

@tombentley you want to take a look at this one? How hard do you think it is?

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 22, 2016

Do we need to support transient on toplevel or static values? javac doesn't complain, because of the way JLS is worded, but java.io serialization will ignore static fields?

What should we do about volatile on a non-variable? Non-variables get transformed to final fields, and javac rejects a volatile final. But it's not possible to restrict the annotation usage using Ceylon annotation constraints because there's no meta.declaration for variable.

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 22, 2016

Oh, and what about volatile on a toplevel? Those are already volatile, so I guess we can just silently ignore the annotation in that case.

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 22, 2016

Hmm the semantics of synchronized are a bit interesting wrt invocation. If I have

synchronized void f(String s=whatever()) {}

then presumably we mark the overloads of f synchronized too? Because in general we can't know what shared state whatever is accessing. But then a named argument invocation such as:

f{};

consists of two sequential accesses, one to the default value method f$s() followed by another to f(String), which means another thread might mutate state between the two! So I suppose the correct thing to do in that case would be to explicitly synchronize on the container of f for the whole of the named argument invocation.

Loading

@jvasileff
Copy link
Contributor

@jvasileff jvasileff commented Nov 22, 2016

explicitly synchronize on the container of f for the whole of the named argument invocation

Will that introduce a deadlock risk if a synchronized method also attempts a named arg invocation?

Perhaps synchronized should be forbidden on methods that introduce defaulted parameters for now?

Loading

@quintesse
Copy link
Contributor

@quintesse quintesse commented Nov 22, 2016

Or have them only on the method itself, not on the retrieval of the default value (for now)

Loading

@jvasileff
Copy link
Contributor

@jvasileff jvasileff commented Nov 22, 2016

It would be nice if these annotations could be used on cross platform declarations and ignored by other backends.

Loading

@gavinking
Copy link
Member

@gavinking gavinking commented Nov 22, 2016

Seems to me that a transient static field makes no sense.

Loading

@gavinking
Copy link
Member

@gavinking gavinking commented Nov 22, 2016

I mean, statics are implicitly transient, no?

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 23, 2016

transient is specified quite vaguely: http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.3.1.3, so java.io.Serializable is just an example of a "system service" which respects transient. Who knows, maybe there's something out there which does something with a transient static. But it's easier for me to not support adding transient to toplevels, so until someone comes up with a reason I'll continue to not support it.

I'm more interested in your thoughts about synchronized @gavinking

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 23, 2016

@quintesse given this class (it's Ceylon's native):

class Native(s, shared native String u) {
    shared native String s;
    shared native String t;
    shared native String m();
}

It seems we generate a getter for u, and fields for u and s, but nothing else. I don't really understand why, but I need to if we're going to support javaNative native declarations. Can you enlighten me?

Loading

@quintesse
Copy link
Contributor

@quintesse quintesse commented Nov 23, 2016

@tombentley I don't really understand that class, you can't have native parameters, that just doesn't make sense

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 23, 2016

Well they're not prevented by the typechecker. If they don't make sense then they should be.

I don't really understand why we don't emit anything for a native declaration, but I suppose that I'll need to change the backend to emit a declaration for a javaNative native declaration.

Loading

@quintesse
Copy link
Contributor

@quintesse quintesse commented Nov 23, 2016

Well you need to think there are really 2 different kinds of native, the ones we've had since the beginning to be able to compile the language module and the ones I added to be able to compile Ceylon code for the different backends. But they're handled quite differently.

The first kind does just enough to make the typechecker do its work. If it generates code it will most probably be nonsense because we then "exclude" them in the build and replace them with our own classes compiled from Java code.

The second kind does not generate code for native at all, only for native("xxx").

Loading

@gavinking
Copy link
Member

@gavinking gavinking commented Nov 23, 2016

@tombentley personally I would not worry about synchronizing evaluation of default arguments (or the overloads that just evaluate them). I think it's clear that they're evaluated before entering the synchronized function.

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 23, 2016

@gavinking that means it not possible to evaluate a default argument expression synchronized-ly without pulling it into a separate function. But I'm happy with that, I guess.

@quintesse I think what I'm trying to do amounts to supporting javaNative native rather than javaNative native("jvm"). With a small change in acceptDeclaration() I can generate declaration for javaNative native class members. Though I still need to make a few codegen changes to avoid generating fields. But it seems that while it's OK to write a native toplevel function it's not possible to write a native toplevel getter:

jni native String nativeGetter;
jni native assign nativeGetter;
jni native variable String nativeVariable;

I get "no native implementation for backend..." for all three.

Also how do I write an interface member with a native concrete implementation:

interface NativeInterface {
    shared jni native String t;
    shared jni native String m(String s = "");
}

I'm wondering whether it would be best to not shoehorn javaNative on top of native, but instead teach the typechecker that javaNative declaration is OK and doesn't have a body, assuming @gavinking is OK with that.

Loading

@quintesse
Copy link
Contributor

@quintesse quintesse commented Nov 23, 2016

I get "no native implementation for backend..." for all three.

But do you have the appropriate .java files that implement those types?

Loading

@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 24, 2016

Hmm. I would need to add a flag to FunctionOrValueFlags, but then we run out of bits in the int. I could bump the int to a long, at the cost of adding an extra 4 bytes to every Declaration.

Loading

@FroMage
Copy link
Contributor

@FroMage FroMage commented Nov 24, 2016

We could do something smart and create a protected long getModifiers() which would be implemented with int modifiers in Declaration and augmented with int moreModifiers in FunctionOrValueFlags with protected long getModifiers(){ return moreModifiers << 32 | super.getModifiers(); }.

But honestly I'd just move it to a long and call it a day myself.

Loading

tombentley added a commit that referenced this issue Nov 25, 2016
tombentley added a commit that referenced this issue Nov 25, 2016
tombentley added a commit that referenced this issue Nov 25, 2016
…rictfp modifiers in EeVisitor

The code generator doesn't always have a Tree to hand when transforming
tombentley added a commit that referenced this issue Nov 25, 2016
* treat javaNative much like formal
* need to make Declaration.flags long
tombentley added a commit that referenced this issue Nov 25, 2016
@tombentley
Copy link
Contributor

@tombentley tombentley commented Nov 25, 2016

Quite a lot more fiddly than anticipated.

Loading

@tombentley tombentley closed this Nov 25, 2016
@gavinking
Copy link
Member

@gavinking gavinking commented Nov 25, 2016

Yay!!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants