ceylon / ceylon-compiler

DEPRECATED
GNU General Public License v2.0
138 stars 36 forks source link

non-null object-typed args should be checked for shared functions #2410

Open gavinking opened 8 years ago

gavinking commented 8 years ago

Something I sorta vaguely thought we already did, but don't do is properly validate arguments that might be coming from Java code. Given this:

shared void xxx(Object oo) {
    print(oo.string);
}

We should generate:

public static void xxx(final .java.lang.Object oo) {
    com.redhat.ceylon.compiler.java.Util.checkNull(oo);
    ceylon.language.print_.print(ceylon.language.String.instance(oo.toString()));
}
lucaswerkmeister commented 8 years ago

I want to disagree, sounds too expensive for too little gain. Not our fault if shitty Java authors don’t correctly call beautiful Ceylon code ;)

We should probably implement this, bench it, and then decide if it’s too expensive.

gavinking commented 8 years ago

Well if(x==null) is not very expensive. And we don't have to do it for primitives, or for nullable types.

lucaswerkmeister commented 8 years ago

But I’m pretty sure non-null non-primitive parameter types are the most common case, and the way you wrote it in the issue description, we would also (completely unnecessarily) suffer this cost for every Ceylon→Ceylon call.

jvasileff commented 8 years ago

Sure, the cost would be small, but I agree with @lucaswerkmeister, validating every argument is too much. And who knows, there could be weird hotspot issues, at least with the method call as written.

I'm also not sure that handwritten Java code calling Ceylon code is a common pattern. If true, this would mostly be for frameworks. Which you've already said should be using direct field manipulation :stuck_out_tongue_winking_eye:.

lucaswerkmeister commented 8 years ago

An alternative form to generate, without Ceylon→Ceylon cost, would be:

public static void xxx(final .java.lang.Object oo) {
    .com.redhat.ceylon.compiler.java.Util.checkNull(oo);
    .com.example.xxx$canonical$(oo);
}
public static void xxx$canonical$(final .java.lang.Object oo) {
    .ceylon.language.print_.print(.ceylon.language.String.instance(oo.toString()));
}

where all Ceylon code would directly call the $canonical$ version, but OTOH this means a lot of duplicate methods.

gavinking commented 8 years ago

I'm also not sure that handwritten Java code calling Ceylon code is a common pattern.

Well I just observed an NPE in the IDE Ceylon code.

jvasileff commented 8 years ago

How about adding @NotNull (or whatever) annotations to generated code? Do these things work in practice?