eclipse-archived / ceylon

The Ceylon compiler, language module, and command line tools
http://ceylon-lang.org
Apache License 2.0
399 stars 62 forks source link

Java 8: support auto-SMI of our Callable types #1617

Closed CeylonMigrationBot closed 8 years ago

CeylonMigrationBot commented 10 years ago

[@FroMage] In theory we should be able to pretend that any method in Java which accepts an SMI (Single-Method Interface) accepts a union of the SMI and its corresponding Ceylon Callable, and do the magic auto-conversion in our backend, when provided with a Callable.

Similarly we should be able to pretend that any method in Java which returns an SMI returns an intersection of the SMI and its corresponding Ceylon Callable, and do the magic auto-conversion in our backend.

I'm not 100% convinced we can do this due to the usual issues with types and bounds and type-arguments, but we can try and investigate.

I highly doubt we'll have time to do this for 1.1, so marking 1.2

[Migrated from ceylon/ceylon-compiler#1617]

CeylonMigrationBot commented 10 years ago

[@FroMage] I started looking into how to implement this in the 1627 branch, but it's rather useless to implement until we support Java wildcards a lot better, as most SMI types involve wildcards so even if we can bump Predicate<? super T> into Predicate<Nothing>|Callable<Boolean,[Nothing]>, this is rather useless.

Things left to be done:

fwgreen commented 8 years ago

Could this be partially implemented sooner than 1.3? Maybe some sort of hack in combination with https://github.com/ceylon/ceylon/issues/5739 that only works for things annotated with @FunctionalInterface? Using the features of JDK 8 would be much easier right now. Here is a painful example in JAX-RS

@GET
@Produces({"application/json"})
public void list(@Suspended AsyncResponse response) {
    CompletableFuture.supplyAsync(repo::list).thenApply(this::generic).thenAccept(response::resume);
}

private GenericEntity<List<Location>> generic(List<Location> list) {
    return new GenericEntity<List<Location>>(list) {};
}

The Ceylon version works, but is hard to live with.

get 
produces({"application/json"})
shared default void list(suspended AsyncResponse response) {
    object supplier satisfies Supplier<List<Location>> { get() => repo.list(); }
    object consumer satisfies Consumer<GenericEntity<List<Location>>> { accept(GenericEntity<List<Location>> list) => response.resume(list); }
    object transform satisfies Function<List<Location>, GenericEntity<List<Location>>> { apply(List<Location> list) => generic(list); }
    CompletableFuture.supplyAsync(supplier).thenApply(transform).thenAccept(consumer);
}

GenericEntity<List<Location>> generic(List<Location> list) => object extends GenericEntity<List<Location>>(list) {};
FroMage commented 8 years ago

I agree we should try something.

gavinking commented 8 years ago

that only works for things annotated with @FunctionalInterface?

OMG, I had no idea that this thing existed:

https://docs.oracle.com/javase/8/docs/api/java/lang/FunctionalInterface.html

Then I agree. The model loader should treat parameters whose type is annotated @FunctionalInterface as being of the corresponding Ceylon function type.

Perhaps it could also perform the same transformation in other locations, but I bet it would be perfectly "good enough" to do it only for parameters.

FroMage commented 8 years ago

I pushed a prototype in the 1617 branch. It transforms parameters of a SAM type into a union of the SAM and equivalent Callable type. Then the backend applies auto-conversions from Callable to SAM when required. For lambdas this is mostly just implementing the SAM rather than AbstractCallable and adding the right method. For method references, this means adding a SAM wrapper class that delegates to the Callable method. It's buggy, slow and shitty but it serves as a proof of concept.

ATM lambda parameter inference does not work before the LHS is of type SAM|Callable and I suspect the typechecker won't infer the parameter types in that case. It would have to change.

ATM we have tests that fail (and the SDK does not compile) because we have some Ceylon implementations of SAM interfaces that fail because the declared Ceylon parameter types (SAM) do not correspond with the super-interface's parameter types (SAM|Callable). This still has to be figured out.

Also, I strongly suspect all boxing/erasure rules to be invalidated by this because we have union types that we do not erase to Object but to the SAM. The effects are hard to measure and will be hell to fix.

Anyway, try it out for size?

Here's a valid example:

import java.util.\ifunction {
    Consumer, IntConsumer
}
import java.util {
    ArrayList
}

void toplevel(Integer i) => print(i);

void lambdas() {
    value j = LambdasJava();
    j.consumer((Boolean b) => print(b), true);
    j.\ifunction((Boolean b) => b, true);
    function f(Integer i) => print(i);
    j.intConsumer(f);
    value fval = (Integer i) => print(i);
    j.intConsumer(fval);
    fval(1);

    j.intConsumer(toplevel);

    j.intConsumer((Integer i) => print(i));
    j.intSupplier(() => 1);

    value l = ArrayList<Integer>();
    value s = l.stream().filter((Integer i) => i.positive)
            .mapToInt((Integer i) => i)
            .sum();
}
import java.util.function.*;

public class LambdasJava {

    public LambdasJava(){}
    public LambdasJava(IntConsumer consumer){}

    public <T> void consumer(Consumer<T> consumer, T t){
        consumer.accept(t);
    }

    public <T,R> R function(Function<T,R> function, T t){
        return function.apply(t);
    }

    public void intConsumer(IntConsumer consumer){
        consumer.accept(1);
    }

    public int intSupplier(IntSupplier supplier){
        return supplier.getAsInt();
    }
}

BTW, can't we fix the parser to allow import java.util.function without the keyword quoting? It's annoying :( j.\ifunction is bad enough…

fwgreen commented 8 years ago
import java.util { ArrayList }
import java.util.stream { ... }

value numbers = ArrayList<Integer>();
value list = numbers.stream().map((Integer i) => i + 1).collect(Collectors.toList<Integer>());
error: argument must be assignable to parameter 'arg0' of 'collect' in 'Stream<Integer>':
'Collector<Integer,out Object,List<Integer>>' is not assignable to 'Collector<in Integer,Object,List<Integer>>?'
FroMage commented 8 years ago

Ah, lemme look

FroMage commented 8 years ago

OK, so I have an alternate prototype where we don't fuck around with the model (fixes inheritance issues) and the typechecker has a list of things it can coerce from/to. This alternate proto supports the exact same test, although my type argument inference algo is simplistic and does not support nested type parameters yet. I'll polish that and push it to another branch.

The backend deals with coercions exactly the same way in both prototypes. The only change is that during model loading I mark SAM classes/interfaces and what their abstract member is.

gavinking commented 8 years ago

the typechecker has a list of things it can coerce from/to

Negative. Implicit type conversions introduce ambiguities into the type system.

FroMage commented 8 years ago

Well, so far every possible solution has drawbacks. Perhaps we should compare drawbacks and pick the ones that are less intrusive? What sort of ambiguities are we talking about? How do they manifest?

gavinking commented 8 years ago

Look, implicit type conversions are simply terrible when combined with type inference. We've been over this approximately 1007 times. We're just not going down that path.

There is only one way to make this idea work, and that is the one proposed in #6189. But that's not what you've implemented.

FroMage commented 8 years ago

Well, that other issue also has drawbacks, they've been documented, as have the model loader solution. Can you refresh my memory on the drawbacks wrt coercion please?

FroMage commented 8 years ago

Proto pushed to the 1617-coercion branch.

FroMage commented 8 years ago

Looking at @fwgreen 's issue, this is the signature of Collectors.toList():

public static <T> Collector<T,?,List<T>> toList();

So we map Collectors.toList<Integer>'s return type as Collector<Integer,out Object,List<Integer>>.

As for the signature of Stream.collect():

interface Stream<T>{
<R,A> R collect(Collector<? super T,A,R> collector);
}

So given a Stream<Integer> we infer R to be List<Integer>, and A to be Object. And due to ? super T as first type param, we get in Integer, so yeah we try to put a Collector<Integer,out Object,List<Integer>> into a Collector<in Integer,Object,List<Integer>>. At this point, my understanding of both subtyping wrt use-site variance and Java wildcard collapses and I've no clue why this is an issue or how we could solve this. @gavinking any idea?

gavinking commented 8 years ago

A Collector<Integer,out Object,List<Integer>> isn't a Collector<in Integer,Object,List<Integer>> because of the second type argument.

For any type X<T>, X<out Y> is a strict supertype of X<Y>.

Perhaps this is a case that Java's wildcard capture can handle (Java can sometimes transform a method signature involving unbound type parameters into one involving wildcards). Ceylon doesn't support wildcard capture.

FroMage commented 8 years ago

Perhaps

Well, I suppose so, otherwise those methods would not be any use.

The question is how can we support this?

gavinking commented 8 years ago

Well, you could futz with the signature of collect().

gavinking commented 8 years ago

You could notice that A is a completely unconstrained type parameter (WTF is it even there for?!) and replace it with in Nothing or out Object.

What an idiotic signature that method has.

gavinking commented 8 years ago

It quite clearly should be:

interface Stream<T>{
    <R> R collect(Collector<? super T,?,R> collector);
}
FroMage commented 8 years ago

OK thanks.

What sort of rules express that it's not constrained? Can't the typechecker notice that and infer something more useful for A?

gavinking commented 8 years ago

OIC, from the point of view of the implementors of this method, apparently they sometimes need to pass As in and out of the Collector. Except that most Collectors apparently don't even have an "intermediate accumulation type", and just completely hide A behind a wildcard.

Yew.

gavinking commented 8 years ago

What sort of rules express that it's not constrained?

Good question. I suppose something like:

it occurs only once in the parameter list, and does not occur in the return type

Except that there is another similar situation where you could say the same thing. Ignoring type constraints:

it occurs only contravariantly or only covariantly in the parameter list and does not occur in the return type.

But of course in Java signatures we're not going to encounter covariant or contravariant occurrences of a type parameter, so perhaps this case isn't interesting.

gavinking commented 8 years ago

But yeah, I suppose, in principle, that the typechecker could quite reasonably infer in Nothing as the type argument for any type parameter that occurs just once in the parameter list and never in the return type.

gavinking commented 8 years ago

I have taken a look at this. While it doesn't look hard to detect the very simplest case and infer a use-site-variant type (I managed to implement that much already), it's not at all a small change to the language since we currently don't support use-site variance in value expressions at all. We only support it in type expressions. Furthermore, it's not entirely clear what the runtime semantics are: what reified type does A refer to at runtime?

I would therefore much prefer to hack this in the model loader than to implement something totally dodgy in the typechecker.

jvasileff commented 8 years ago

what reified type does A refer to at runtime?

I thought this would just be allowed when calling isJava() methods. For Ceylon, it seems the type argument would have to be determined at runtime (e.g. theCollector.A or even theMutableList.Element).

FroMage commented 8 years ago

I would therefore much prefer to hack this in the model loader than to implement something totally dodgy in the typechecker.

OK, sounds reasonable I guess, but which of the two signatures should the model loader change? Caller or callee?

FroMage commented 8 years ago

I've spun this into #6370 please continue discussion around this there.

FroMage commented 8 years ago

@gavinking can you refresh my memory wrt the drawbacks of coercion for java types and inference? I'm not at all questioning that the drawbacks are real and would cause the end of the world as we know it, but I just forgot them and would appreciate to remember them.

FroMage commented 8 years ago

Merged master on the branch, and added support for String/CharSequence, SAM->Callable, fixed issues with method references and primitive types, added checks for type compat between SAM/Callable.

FroMage commented 8 years ago

Note that this work in conversions is useful no matter which of the three solutions we end up with.

FroMage commented 8 years ago

With the tests passing now, that's better. Also start not coercing unless the typechecker told us to.

bjansen commented 8 years ago

Can't wait to see this in master!

FroMage commented 8 years ago

Well, if you have an opinion, make it known in #6189. So far we're still assessing the pros/cons of three strategies.

fwgreen commented 8 years ago

This works on the 1617 branch but fails on the 1617-overload branch

import spark { Spark { ... }, ... }

shared void run() {
    get("/", (Request req, Response res) => "Hello, world");
}
error: compiler bug: null at com.redhat.ceylon.compiler.java.codegen.PositionalInvocation.unfake(Invocation.java:884)
error: compiler bug: visitor didn't yield any result at com.redhat.ceylon.compiler.java.codegen.ExpressionTransformer.transformExpression(ExpressionTransformer.java:369)
error: compiler bug: visitor didn't yield any result at unknown
FroMage commented 8 years ago

Thanks, I'll test this.

FroMage commented 8 years ago

OK @fwgreen should work now.

fwgreen commented 8 years ago

@FroMage It works again! Many thanks.

FroMage commented 8 years ago

OK so all the tests pass. I still haven't implemented anything to do with variadics (especially methods that take SAM... or SAM methods which are variadic, but that can come later. At this point I'm ready for review from you @gavinking especially regarding the new members of Declaration and their names. Then we can merge to master.

fwgreen commented 8 years ago
CompletableFuture.supplyAsync(() => repo.listAll())...

works without issues, but...

resource late ManagedExecutorService mes;

CompletableFuture.supplyAsync(() => repo.listAll(), mes)...
error: illegal argument types in invocation of overloaded method or class: there must be exactly one overloaded declaration of 'supplyAsync' which accepts the given argument types 'List<Person>(), ManagedExecutorService'
        CompletableFuture.supplyAsync(() => repo.listAll(), mes)
FroMage commented 8 years ago

@fwgreen OK will test.

FroMage commented 8 years ago

I merged this in master. I suppose I still have to do something about the metamodel, to ignore those overloads.

FroMage commented 8 years ago

I also have to support overloading selection with null arguments.

FroMage commented 8 years ago

I also have to support overloading selection with null arguments.

Done

FroMage commented 8 years ago

@gavinking this is where overloads fail:

ExecutorService mes = nothing;

CompletableFuture.supplyAsync(() => 2, mes);

Fails because ExecutorService is itself also a SAM (damnit), but one which we want to pass as instance, not as a Callable. How can we fix this?

gavinking commented 8 years ago

@FroMage I don't follow. AFAICT, it's impossible that a Java method accepts Callable, so the first argument should be sufficient to disambiguate the overload.

FroMage commented 8 years ago

It's not about being ambiguous: we have the real method, and one overload with f(Callable, Callable) type. We said we would not generate f(SAM, Callable) and f(Callable, SAM) due to combinatorial explosion, so the method we want to call does not exist in the model.

gavinking commented 8 years ago

@FroMage Oh, OK, I see. So just write:

Executor ex = .... ;
CompletableFuture.supplyAsync(() => 2, ex.execute);

Seems pretty straightforward.

FroMage commented 8 years ago

If they don't do any instanceof and downcasts on the other side, I guess…

Can you try that @fwgreen ?

FroMage commented 8 years ago

I think we still have an issue with static refs of the form Type.member.

FroMage commented 8 years ago

And even qualified members don't work ATM, lemme fix both.