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

Proposal: error on invalid assignments between classes that share a nominal heritage #7817

Closed
malibuzios opened this issue Apr 4, 2016 · 10 comments
Labels
Duplicate An existing issue was already created

Comments

@malibuzios
Copy link

This proposal serves, at this point, as an trial-and-error experience for myself and to give you guys some ideas to play with.. I understand this may not be 'it' in current form (or not at all) so don't just stamp it with that dreaded "DECLINED" thing. I'll just close it myself after you explain to me how bad it is and why.. and all the edge cases..(e.g. expressions in extends clauses etc.) or who knows, I may be even able to fix it later..

Let A and B be two classes and a: A, b: B.

For the structurally valid assignment a = b.

Either one of the following conditions should hold to error:

  1. B is a strict nominal ancestor of A.
  2. A is a nominal ancestor of B but with an incompatible generic signature.

Examples:

class Base {
    action = "START WORLD WAR III";
}

class Derived extends Base {
    action = "SAVE THE WORLD";
}

function giveMeDerived(shouldBeDerived: Derived) {
    performAction(shouldBeDerived.action);
}

giveMeDerived(new Base()); // Error! The compiler has saved the world!
class Bag<T> {
    items = [];
}

function putStringsInTheBag(bag: Bag<string>) {
    bag.items.push("a", "b", "c", "d");
}

let bagOfNumbers = new Bag<number>();
putStringsInTheBag(bagOfNumbers); // Error! no strings going in this bag.. 
class Bag<T> {
    items = [];
}

class BagWithMethod<V> extends Bag<V> {
    putItem(item: V) {
        this.items.push(item);
    }
}

let bagWithMethod = new BagWithMethod<number>();
bagWithMethod.putItem(42);

let evilBag: Bag<string> = bagWithMethod; // Error! evil bag detected!

Note: this could be opt-in through a compiler flag, e.g. --nominalClassChecks..

@mhegazy
Copy link
Contributor

mhegazy commented Apr 4, 2016

@malibuzios how is this issue different from #7792? and why not keep the question in #7792?

as mentioned in #7792 (comment)

You'd probably be interested in #202 or some discussion that happened at #6735

@mhegazy
Copy link
Contributor

mhegazy commented Apr 4, 2016

Just to be clear, the proposal:

Note: this could be opt-in through a compiler flag, e.g. --nominalClassChecks..

is a duplicate of #202

@mhegazy mhegazy added the Duplicate An existing issue was already created label Apr 4, 2016
@malibuzios
Copy link
Author

@mhegazy

This proposal is about a particular form of error checking for classes based on nominal ancestry relations. It doesn't propose any keyword or syntax. I don't believe it is a good idea to combine multiple different proposals into one issue. The issue about generics did not contain a proposal and is closed.

@malibuzios
Copy link
Author

@mhegazy

The idea here is that there would be an error only if the two classes share heritage. If they don't, it would work normally. This doesn't convert all classes to 'nominal' ones, just applies some nominal checking for ones that are related.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 4, 2016

I am not sure i follow.. how would that help you with something like:

class Base {
    action = "START WORLD WAR III";
}

class Derived extends Base {
    action = "SAVE THE WORLD";
}

class SomeRandomClassWithActionProperty {
    action = "OOPS"
}

giveMeDerived(new SomeRandomClassWithActionProperty());

Classes today are compared structurally. if i understand correctly, you would want them to be compared in a nominal fashion, which is a fair proposition.

I do not think the proposal here solves the underlying problem, it just carves out a small part of the problem space and special case it under a flag. Do not think this is the correct general solution.

We can not just switch classes to be compared nominally either, as that would be a breaking change. so it has to be an opt-in mode.

we have two options, either have a flag, as you mentioned, or a new keyword/annotation that can be attached to declarations.

If were to add support for this proposal, it would be likely a new keyword. that maintains back compat, and allows for a fine grained control over how the compiler should behave.

@malibuzios
Copy link
Author

@mhegazy

Yep, it only carves a small area. I see this as an idea that shows the granular range of checking that is possible. This is a small, but a meaningful step.

I was hoping that if, say, a nominal checker is implemented anyway, this limited form of checking could be there by default, without a flag, and wouldn't be a heavy breaking change as it is very unlikely someone would one to assign this way on purpose.

There could be some opt-out tricks to work-around the limitation (though it would probably be very rare that anyone would want to do that). Perhaps converting the class to an interface (I know this would work but I'm not sure if it'll have the expected effect)?

class A {};
interface B extends A {};

Anyway, I was giving this as an idea. Since it seems my message seem to have been successfully communicated I will close the issue.

@malibuzios
Copy link
Author

@mhegazy

Just wanted to add:

The only scenario I can think of when this check would be effective for base and derived classes is when the derived classes essentially override members without actually changing the structure (this may only last for a part of the chain):

class A {
    a = 1;
    b = 2;
}

class B extends A {
    b = 3;
}

class C extends B {
    a = 2;
}

class D extends C {
    b = 5;
}

Say someone used inheritance this way as just a form of incremental sharing where each extending class simply changed a couple of properties but didn't add anything. They can always use the earliest class in the chain with the same structure as a base reference (A in this case), as every part of the chain that 'used' to be mutually assignable would have the same structure.

Say they wrote a function that looked like this:

// What benefit does having the type C, rather than A here provides?
function iWantC(c: C) {
}

iWantC(new B()); // Error: this wouldn't work anymore with nominal checking

So the question is what benefit would it give them to use anything other than A here aside from having it as some form of documentation? (in that case, why would they pass B?) as structural typing wouldn't provide any type safety here anyway? (actually nominal typing seems to provide more safety, maybe that's what they wanted in the first place?)

I think this still needs to be investigated a bit more to decide if something like this could be introduced without a flag..

@RyanCavanaugh
Copy link
Member

@malibuzios a historical note: classes used to be nominal in TypeScript, so we've both investigated it (by virtue of having many teams, including our own, trying it out that way) and thought it about it a lot (by virtue of having decided to make a big change to structural typing of classes).

The effect of two similar things in the type system (classes, interfaces) behaving radically differently (nominal, structural) didn't lead to a good user experience. There was more confusion gained than saved by having this behavior.

@malibuzios
Copy link
Author

@RyanCavanaugh

Thanks for stopping by! The more I look at this I start to feel that if introduced something like this should be put under a flag. Someone, somewhere, may actually use classes in this unorthodox way I described (probably someone not coming from the background of nominally typed languages). I don't think it would be a good idea to break it for them. It might look a bit strange when seen abstractly but I guess it could actually fit in some pattern.

@malibuzios
Copy link
Author

@RyanCavanaugh

It is still possible that the checking for generic signatures could still be introduced without a flag. If there are examples where someone would find that behavior useful, these should be pretty insane (though possible though! :).

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants