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

Bug: No null checks in type parameters #17478

Closed
mckauf opened this issue Jul 28, 2017 · 9 comments
Closed

Bug: No null checks in type parameters #17478

mckauf opened this issue Jul 28, 2017 · 9 comments
Labels
Duplicate An existing issue was already created

Comments

@mckauf
Copy link

mckauf commented Jul 28, 2017

TypeScript Version: 2.2.2

Code

interface List<T> {
    add(elem: T): void
}

function map(list: List<number>): List<number | null> {
    return list;
}

function foo(list: List<number>) {
    map(list).add(null);
}

Expected behavior:

Compiler should complain about line 6 (return list) when strictNullChecks is enabled, because add(elem: number): void has a different signature than add(elem: (number | null)): void.

Actual behavior:

This above code goes through the compiler, even though strictNullChecks, noImplicitAny et c. are all enabled. This results in that we can add null to a List<Number> in function foo and the compiler does not complain.

@ikatyang
Copy link
Contributor

It is different indeed, but it is assignable too. Why not just delete the null from return type so that it'll throw an error on .add(null)?

function map(list: List<number>): List<number> { return list; }

@mckauf
Copy link
Author

mckauf commented Jul 28, 2017

In fact, I discovered this when investigating a type bug in a bigger application where the TypeScript compiler should have complained about a type mismatch but didn't.

The above code is not the actual use case, but just the minimal example that produces the same error.

Yes, I could delete the null from the return type in the above example but this would not solve the actual bug in the compiler ;)

Why do you think that both types are assignable?

@kitsonk
Copy link
Contributor

kitsonk commented Jul 28, 2017

List<number> is assignable to List<number | null>... that does not contravene strict null checks or the type system at all. Just as number is assignable to number | null. You can always widen a type explicitly.

@mckauf
Copy link
Author

mckauf commented Jul 28, 2017

Yes, number is assignable to number | null, but with generics this is not be the case. I.e., X<number> should not be assignable to X<number | null>. Because otherwise you can construct cases where this results in number | null arguments being mapped to number arguments, but number | null is not assignable to number.

The above example illustrates this very well: we can suddenly add null elements to a list that normally should contain only number elements, and this does not require an explicite cast or anything. If this really is the expected behavior, then I would call the type system broken, because it does not guarantee type safety.

They recognized this problem in Java as well, and this is exactly the reason why you can assign Float to Number, but you cannot assign List<Float> to List<Number> in Java; namely this would result in that you can add Double values to a List<Float>.

@gcnew
Copy link
Contributor

gcnew commented Jul 28, 2017

The problem is that TypeScript doesn't support variance annotations (#1394). If the return list's parameter had been marked as covariant List<out number | null> there would have had been no problem, as it would have had been read only.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 28, 2017

If this really is the expected behavior, then I would call the type system broken, because it does not guarantee type safety.

If you are explicit about types... While you don't consider it a cast, you are relying upon asserted types and TypeScript is checking that the types are assignable, which they are. I don't see how it become un-type safe if you explicitly widen a type.

@gcnew
Copy link
Contributor

gcnew commented Jul 28, 2017

@mckauf Java has use-site variance declarations.

public static void main(String[] argv) {
    List<String> aList = new ArrayList<>();
    List<? extends String> covariantList = aList;
    List<? super String> contravariantList = aList;
    
    String o2 = covariantList.get(0); // we can read `String`/`Object`
    Object o = contravariantList.get(0); // we can read only `Object`
    
    covariantList.add(null); // we can add only `null`
    contravariantList.add("Test"); // we can add String and its derivatives (aham..)
    
    map(Arrays.asList(1.0)).add(null); // practically read-only, we can add only `null`
}

public static List<? extends Number>map(List<Double> l) {
    return l;
}

@mckauf
Copy link
Author

mckauf commented Jul 28, 2017

I don't see how it become un-type safe if you explicitly widen a type.

Because casting X<number> to X<number | null>, has nothing to do with widening; they are just two incompatible types; unlike casting number to number | null, which is indeed widening.

I have pointed out why. You can also read this to understand the problem: http://onewebsql.com/blog/generics-extends-super

In Java there are boundaries for type parameters (extends, super), which are similar to the covariant type parameters gcnew mentioned (in, out). Too bad Typescript doesn't support them. But even if it doesn't, the compiler cannot simply assume that X<number | null> includes X<number> ; this is just wrong.

@mckauf mckauf closed this as completed Jul 28, 2017
@mckauf mckauf reopened this Jul 28, 2017
@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jul 28, 2017
@RyanCavanaugh
Copy link
Member

See #1394.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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

5 participants