eclipse-archived / ceylon

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

metamodel getMember() methods return non-members #6894

Open jvasileff opened 7 years ago

jvasileff commented 7 years ago

In ClassOrInterface.getClassOrInterface() and similar methods, results not belonging to the receiver may be returned.

Background

Due to Type<> being covariant in its type parameter, the various getMember() methods have their own Container type parameters so that the receiver type can be represented in a contravariant type parameter in the return. This allows for typesafe application of the returned value.

For example, a Type<> with the static type Type<Anything> may represent a String, but a member of String must be bound to a String, and not just some other Anything. So, Anything would not be a suitable type to use for the return value's Container.

The type parameter Container is intended to act as a filter per the documentation linked above, and as discussed in #5384.

Current Implementation

By example, for x.getAttribute<Container>("member"), the current implementation:

  1. Checks to see if x has a member named "member" and returns null if not
  2. Checks to see if Container has a member named "member" and returns null if not
  3. Return's the member found in (2)

But (3) offers no guarantees about the member's applicability to x. This is not consistent with the documented rules, and can produce surprising results.

I believe the correct algorithm would be to search for the member in the union type x | Container as suggested in this comment https://github.com/ceylon/ceylon/issues/5384#issuecomment-156671507.

Test Cases

import ceylon.language.meta.model {...}

class A() {
    shared String unrelated => "A";
    shared default String member => "A";
}

class B() extends A() {
    shared actual String member => "B";
}

class C() extends A() {
    shared actual String member => "B";
}

class U() {
    shared String unrelated => "U";
}

shared void run() {
    test1();
    test2();
    test3();
    test4();
}

void test1() {
    // x.getMember() returns a completely unrelated member that happens
    // to have the same name as a member of x.
    Attribute<U,String>? attr = `A`.getAttribute<U,String>("unrelated");
    if (!is Attribute<A,String>? attr) {
        print("oops #1"); // oops
    }
    if (exists attr) {
        print("oops #2: ``attr(U()).get()``"); // oops
    }
}
void test2() {
    // x.getMember() returns a subtype's member that can't be qualified
    // with an instance of x.
    Attribute<B,String>? attr = `A`.getAttribute<B,String>("member");
    if (!is Attribute<A,String>? attr) {
        print("oops #3"); // oops
    }
}
void test3() {
    // x.getMember() returns a supertype's member if necessary
    // (case where Container is the supertype)
    Attribute<A,String>? attr = `B`.getAttribute<A,String>("member");
    assert(exists container = attr?.container);
    if (!container.exactly(`A`)) {
        print("oops #4"); // ok
    }
}
void test4() {
    // x.getMember() returns a supertype's member if necessary
    // (case where Container is disjoint from x)
    Attribute<B,String>? attr = `C`.getAttribute<B,String>("member");
    assert(exists container = attr?.container);
    if (!container.exactly(`A`)) {
        print("oops #5"); // oops
    }
}
tombentley commented 7 years ago

The problem with looking up members in the union type is that the metamodel has to work on JS, and on JS we don't have the typechecker to rely on for forming principle instantiations, as mentioned here in #5384. So I'm not sure this one is simply a matter of diving in an hacking the metamodel into compliance.

jvasileff commented 7 years ago

For JS, 1) I didn't test it fully, but in at least a few cases it seemed to work well, and 2) longer term it might be possible for @chochos to integrate the model I've written in Ceylon into the JS backend.

I guess you're suggesting we might want to find some other approach that requires less of the model, but I'm not sure what that would be without losing capabilities (i.e. returning null too often, returning less refined declarations, or something else)

jvasileff commented 7 years ago

A couple additional comments:

  1. Regarding principle instantiations, I imagine there are several other places where this is needed, so this should not be considered a limiting factor. An example I'm aware of:
class A1<T>(T t) {
    shared class B() {
        shared T t => outer.t;
    }
}
class A2() extends A1<Integer>(1) {}
value ab = `class A1.B`.memberClassApply<A2,A2.B,[]>(`A2`)(A2());

where we need the A1 supertype of A2. Much more complex examples could be crafted.

  1. OTOH, if we wanted to consider a much bigger change... I think a meta-model v2 with an invariant Type might work out very well, and would eliminate the need for Container. This is practical since we now have use-site variance. I have the very beginnings of an API for this that wraps the current meta-model, which so far seems encouraging.