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

Annotation to prevent type narrowing for certain types #6242

Open sadmac7000 opened 8 years ago

sadmac7000 commented 8 years ago

There's a perennial complaint that type narrowing in Ceylon can allow you to break encapsulation. For example:

class DeckOfCards() {
    shared List<Card> cards = ArrayList<Card>{*startingDeck};
    shared Card draw();
    shared void shuffle();
    shared void reset();
}

void run() {
    value deck = DeckOfCards();
    assert(is ArrayList<Card> c = deck.cards);
    c.add(Card("King of Beers"));
}

The deck of cards exposes a sane set of mutations, and also exposes the list for perusal as an immutable type, but the run method is, via a gross hack, able to discover the mutability of the deck and add a nonsense card to it.

There are various workarounds, most of which are at least somewhat aesthetically unpleasing, and there's the option of defaulting to the untyped mantra of Just Don't Do That.

I would like to propose another option: an annotation for interfaces, let's call it noNarrow for now, which prevents you from narrowing a type to include them.

So in our example, the collections library would annotate ListMutator as noNarrow, and then the code above would fail to compile at the assert because ArrayList is below ListMutator, but the type of c, List, is above it.

The cards variable would still have to become a private MutableList member with an accessor, since you couldn't narrow it inside the class either, but this is still lighter and less busy than wrapping the class, as the most common existing workaround requires. There may be further chances for improvement here by letting the type checker prefer inferred type over declared type within the scope where a variable was declared, or other such optimizations.

The weakness in this plan is union types. Interpreted strictly, the above would not allow you to narrow String|MutableList into a MutableList. The solution would be to always allow narrowing to shed union type members, regardless of if other members have noNarrow in their annotations. This proposal is quite unlivable without that, so someone more knowledgeable than I about the type system should comment on whether that behavior can be defined safely.

gavinking commented 8 years ago

But such an annotation would be trivial to circumvent, just by aliasing the original attribute:

void run() {
    value deck = DeckOfCards();
    value cards = deck.cards;
    assert(is ArrayList<Card> cards;
    c.add(Card("King of Beers"));
}
bjansen commented 8 years ago

What about an annotation like immutable, that you put on the value:

class DeckOfCards() {
    shared immutable List<Card> cards = ArrayList<Card>{*startingDeck};
    ...

Then the typechecker checks that List<Card> satisfies a given interface, like this:

shared interface ImmutableClone<Thing> given Thing satisfies ImmutableCopy<Thing> {
    shared void makeImmutableCopy();
}

Then the backend would generate something like:

public List<Card> getCards() {
    return cards.makeImmutableCopy();
}
gavinking commented 8 years ago

That seems like a whole lot of special-casey complexity just to avoid writing:

class DeckOfCards() {
    value _cards = ArrayList { *startingDeck };
    shared List<Card> cards => immutableList(_cards);
    ...
}

I gotta say it: I'm just not seeing the big problem here.

gavinking commented 8 years ago

Finally, clients that use assert to narrow the type of a List to MutableList already know they are doing something wrong, and deserve what they get. I simply don't believe that this is something that DeckOfCards really needs to protect itself from.

sadmac7000 commented 8 years ago

But such an annotation would be trivial to circumvent, just by aliasing the original attribute:

void run() {
    value deck = DeckOfCards();
    value cards = deck.cards;
    assert(is ArrayList<Card> cards;
    c.add(Card("King of Beers"));
}

Why would that work? value cards is still above ListMutator, and you still can't take it below ListMutator.

gavinking commented 8 years ago

@sadmac7000 oh you're saying that the noNarrow annotation goes on the type List? So you can never narrow any List to anything else?

sadmac7000 commented 8 years ago

No no, it goes on ListMutator. The rule is "Never narrow anything to ListMutator."

jvasileff commented 8 years ago

@sadmac7000 it seems this would give special significance to the way union types are expressed, which would be difficult to manage.

For example, List | MutableList (even though it is exactly List) would become a desirable type (to allow narrowing to MutableList). So you'd have to avoid simplifying the type, and also disallow assigning a List to a List | MutableList.

But then, if you can't make the assignment, how would you pass a List to a function that takes a List | MutableList?

Perhaps you would just disallow List | MutableList (always simplify), but then the function below, which for <String, MutableList> is perfectly valid (presumably because the two are disjoint?), might become more expensive at runtime to be sure to return null for <List, MutableList>:

U? narrowTo<T, U>(T t) {
    T | U tu = t;
    if (is U t) {
        return t;
    }
    return null;
}
sadmac7000 commented 8 years ago

Basically if you have a graph of types with directed edges indicating IS-A, we're marking some of those edges and not considering them in certain algorithms. Union simplification and narrowing specifically.

jvasileff commented 8 years ago

And disallow union complexification?

I suspect once everything was worked out, this aspect of the type system would be hard to understand.

gavinking commented 8 years ago

FTR: I still don't find this feature appealing at all. And I don't really buy that it addresses a class of bug that is at all common in practice.

quintesse commented 8 years ago

Do we close this or do we want to discuss this some more? Moving to 1.3 at least.