microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.08k stars 12.49k forks source link

No error on assignment between generic derived and base classes with incompatible signatures if the base class does not contain a structural reference to its generic parameter #7792

Closed malibuzios closed 8 years ago

malibuzios commented 8 years ago

TypeScript Version:

1.8.9

Code

class Base<T> { };
class Derived<U> extends Base<U> { };

let x: Base<number> = new Derived<number>(); // Works as expected.
let y: Base<string> = new Derived<number>(); // No error?

Expected behavior: The second assignment should fail as string is not assignable to number, thus Base<string> is not a valid base class for Derived<number>.

Actual behavior: No error.

Notes: If the base class contained at least one structural reference to the type parameter. It would indeed give an error:

class Base<T> { a: T }
class Derived<U> extends Base<U> {};

let y: Base<string> = new Derived<number>();
// Error: types of 'a' are incompatible, 'number' is not assignable to 'string':

However, alternatively, even if the derived class contained a reference to its type parameter, there would be no error:

class Base<T> {}
class Derived<U> extends Base<U> { a: U };

let y: Base<string> = new Derived<number>(); // No error 

More convincing example:

It turns out there could still be internal references to T in the body of a method that did not include it in its signature. Currently this compiles fine as well:

function externalFunction<T>(a: T) {
    ...
}

class Base<T> { 
    method() {
        // T is indeed referenced here, and still there is no error. 
        // I understand the 'no type based emit' principle but I think this is taking it too far!
        let x: T; 
        externalFunction(x);
    } 
}

class Derived<U> extends Base<U> { };
let x: Base<string> = new Derived<number>(); // No error!

TypeScript's first design goal is:

Statically identify constructs that are likely to be errors.

It is hard for me to see a case where this would not be a programmer's mistake (or at least an early warning sign for one). I understand this is a structural type system and understand the fact that on some level this 'shouldn't' really have practical implications at run-time if there's no structural reference to the generic type in the base class, but I don't think that would justify accepting it as a valid assignment.

malibuzios commented 8 years ago

This problem is more fundamental and also surfaces in cases where only a single class is involved:

class Bag<T> {
    items = [];
}

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

function putNumbersInTheBag(bag: Bag<number>) {
    bag.items.push(1, 2, 3, 4);
}

let bagOfStrings = new Bag<string>();
let bagOfNumbers = new Bag<number>();

putStringsInTheBag(bagOfNumbers); // Oh no!!! 

This compiles perfectly fine despite the fact that bagOfNumbers has a signature that is not compatible with Bag<string>. You might say it's the programmer's fault because they didn't specify items as items: T[]. Of course that would have been better, but the whole idea here was to design the type checker so it would catch patterns that are likely to be errors, not just blame the programmer whenever they use a pattern that the type system cannot provide sufficient safety for.

In the programmer's mind, stating bag: Bag<string> should provide a guarantee that only ancestor types with compatible signatures would be allowed to pass (to clarify: I am not describing or suggesting a form of nominal typing here, only stricter checking on type arguments when a generic ancestor is assigned). The programmer should not be required to mentally recreate the type system's inner workings and study the particular variant of structural typing it uses to consider the possibility that if the generic parameter was not structurally referenced within the class then this signature does not guarantee assignability of the type argument to string. That would be insane! For the programmer, Bag<string> literally means a bag of strings!

jeffreymorlan commented 8 years ago

I've noticed this with Promises - it's easy to accidentally create promises typed as Promise<{}>, which is assignable to any other promise type. Promise assignment should really only be covariant and not contravariant.

function f() {
    return new Promise(resolve => { resolve(42) });
    // without an explicit type argument, this ends up returning Promise<{}>
}

function g(a: Promise<string>) {
    a.then(s => { console.log(s.toUpperCase()) }).catch(e => { console.error(e) })
}

g(f()); // No error detected in assigning the Promise<{}> to Promise<string>
        // At runtime: [TypeError: s.toUpperCase is not a function]
RyanCavanaugh commented 8 years ago

https://github.com/Microsoft/TypeScript/wiki/FAQ#why-is-astring-assignable-to-anumber-for-interface-at--

Unconsumed generic type parameters are an extremely red red flag in TypeScript. The only reason this isn't an error is because it would have been a breaking change to do so.

malibuzios commented 8 years ago

@RyanCavanaugh

By using an expression like "unconsumed generic variables are an extremely red-red flag" you are basically describing exactly what I wrote: You are blaming the programmer instead of trying to provide an improvement to the type-checker that would address this.

If I believed that opening this to discussion was not productive or wouldn't be a constructive improvement to the language I would not have spent the time to research and write this. Hopefully you've read the example in my second comment, as it shows perfectly reasonable code that appears like it should provided type safety by the language but actually doesn't.

I opened this with the idea that it is, in a sense "by design", so that's not really any news for me. And I know fixing this would be a in a sense "breaking change" mostly in the sense that "bad" code would now get type-checked correctly!

Can you give examples where introducing a check for this would provide significant unuseful, spurious errors, rather than helpful ones?

In any case, please re-open this. You can leave the "By Design" tag if you wish, though I don't believe it is really helpful here, as that is pretty obvious..

(I would also like that you fast hand to the trigger here makes me feel a bit uneasy and seems reduce my motivation to contribute suggestions and ideas in the future. I think a better approach would be to become more patient and try to get to the root of what the other person is trying to convey rather than just dismissing them right away)

malibuzios commented 8 years ago

@RyanCavanaugh

Listen, I have to tell you personally that borderline rude to close and dismiss it like it was nothing! Do you really believe you're going to keep helpful participants around if you treat us like we're this "swarm" of annoying people just here to pester you by asking or suggesting obvious and unhelpful things?

You described the problem as a 'red-red flag' but then said that nothing is ever going to be done about it? What kind of impression do you think this gives about how your team works? That you basically don't care? If you don't really care about your own language, why should I care? Why would I spend my time at carefully considering how it could improved in the best way and especially consider how to solve problems with the least impact and breaking changes? because that is a part of what I'm trying to do!

malibuzios commented 8 years ago

@RyanCavanaugh

I'm not here to argue and I'm not a confrontational person (I'm telling you, I'm actually a very nice person!). So here's what I'm going to do: we'll start all over in a new issue, which would only include the example in the second comment, that I now believe was a somewhat better one, but maybe you didn't notice and possibly others wouldn't as it is surrounded by a lot of text.

You and your team will carefully read and analyze that example and try to provide a solution for it because it is, by all means a "bug". It is perfectly reasonable code that the type checker cannot provide safety for. Nothing really special about that.

DanielRosenwasser commented 8 years ago

@malibuzios sorry if you got that perception. We're not trying to brush issues like this off, and we do care. Please do understand though that we get many issues a day (to give you an idea, I have 833 notifications right now for the TypeScript GitHub repo alone), and we have answered some of the same questions/issues (@RyanCavanaugh especially!) many times. This is tough given the number of users compared to the number of members on the team, and can be very taxing for us.

So please don't take the terse responses the wrong way. We've thought about the problems here with a structural type system for years and have some thoughts on nominal typing that we've discussed (check out the most recent relevant design meeting notes at #6735).

tslint could have something to catch issues regarding unconsumed type parameters - though I don't really know how often people who already understand this issue accidentally end up writing unconsumed type params.

malibuzios commented 8 years ago

@DanielRosenwasser

I opened another issue and started receiving comments before I was able to read your response. Thank you for taking some time to answer. I appreciate that.

I'm not sure if the particular issue I'm describing is really about nominal typing. It is a particular type of checking that can be done between generic classes: themselves and their ancestors. I intentionally did not include interfaces here because I didn't think they were as important as this. If I thought this should be in a domain of a linter I would have posted this at the tslint github, it is possible that the Interface case may or may not be more relevant to it.

TypeScript is not like 'go', which doesn't have classes. When a class is declared, it has a particular and explicit implementation. The compiler knows that Bag<number> and Bag<string> share the exact same implementation code and internal logic (for simplicity I'm not including abstract/base classes and overridden methods here). The current approach to class generics treats them essentially as if they were interfaces. I don't think this really is about structural or nominal typing, it is about encoding a semantic understanding of what it means that two concrete entities are identical or share the same execution logic.

I have intentionally did not try to suggest solutions here because I believed this would require a more intricate understanding of the language, which I will probably never have compared to you or your team. To be honest I believed this would be taken more seriously than it did.

DanielRosenwasser commented 8 years ago

it is about encoding a semantic understanding of what it means that two concrete entities are identical

Generally this is what nominal typing encompasses - the idea that a type would be immediately differentiated by its type arguments. But you can't just look at that because you're not always assigning between the same class, and if we required that, you couldn't do something like the following:

interface NumberStringMap {
    [key: number]: string;
}

let arr: Array<string> = ["hello", "world"];
let m: NumberStringMap = arr;

So we look at capabilities when we do type compatibility checking because we try to be lenient.

There's also a few questions that stand out in general. Let's say we did check type arguments specifically. Take the following:

interface Box<T> { }
interface Animal { a }
interface Dog extends Animal { d }

let animalBox: Box<Animal>;
let dogBox: Box<Dog>;

Can I assign a Box<Dog> to a Box<Animal>? What about vice versa? Why or why not?

malibuzios commented 8 years ago

@DanielRosenwasser

First of all, thanks again for taking the time to answer. I understand you may be very busy. My purpose here was to try my best to demonstrate that the current approach to generics has some issues and limitations. Not to propose a solution, as that would be beyond my capabilities and the time I have to spend for this.

It may be that some innovative (perhaps "hybrid") approach or concept may need to be developed to approach this. If I have time I will try to investigate this, but this may take months or even years.

All I wanted is having the problem be known and acknowledged. It may surprise you that I may not actually be that disturbed if it would never be addressed. It does, however, disturb me if people deny there's any flaw and defensively refuse to acknowledge it.

I hope I contributed at least a little with the example I gave in my second comment, as it shows useful, functioning code that looks perfectly type safe but actually isn't - in a way that is subtle enough that even people relatively familiar with the language may not notice.

Thank you for your time!

RyanCavanaugh commented 8 years ago

@malibuzios I'm sorry to have given you that impression above. As Daniel said, we get a lot issues a day, and we do want to read and address all the comments. It's unfortunately not possible for us to spend too much time one-on-one for issues that have been logged and discussed many times before.

Rest assured that we're listening to all feedback. We do look at things like "How many times has this been seen as a bug" when considering design changes, so even an issued closed as by design is having an impact on the future direction of the language. Thanks!

malibuzios commented 8 years ago

@RyanCavanaugh @DanielRosenwasser

Just wanted to add this problem isn't really limited to generics, though you are probably familiar with this. For example if a base class has the same structure as a derived class, then the base class may be assigned to the derived class. So in this case:

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()); // OOPS?? no error?? The compiler could have saved the world :(

Now if this assignment was done erroneously, which is very likely, the type checker cannot really help here. This is actually 'worse' than all the examples I gave so far so introducing some capability for nominal typing/hybrid approach could be interesting, though I'm thinking on a more automated or error-oriented approached that wouldn't require explicit syntax..

Unused generic variables aren't really related to this and could be covered separately by a warning/linter..

RyanCavanaugh commented 8 years ago

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

vilicvane commented 8 years ago

@RyanCavanaugh I was quite surprised to find that I will just lose the type safety once I want to use Promise as return type. That's embarrassing.

Why can't we treat generics as part of the structure? I think this could be considered as proper for type aliases like:

interface Foo {
    biu: string;
}

type Bar<T> = Foo;

But for interfaces and classes, that's really awkward to me to not consider generics part of the structural stuff.