Warn/error when testing a non-nullable type for null

[joshaber]

TypeScript Version: 2.0.0

TL;DR:

If a project is configured for strict null checks, checking a variable which is typed as non-null is nonsense and could indicate a type mismatch.

Code:

Here's a motivating example I experienced:

async function someFunc1(): Promise<string | null> {
  // do some stuff and return the appropriately typed promise
}

async function someFunc2(): Promise<boolean> {
  // Negating a non-null Promise is crazy talk, but here I go!
  return !!someFunc1()
}

Of course what I really meant was:

async function someFunc2(): Promise<boolean> {
  return !!(await someFunc1())
}

Expected behavior:

TypeScript warns that I'm effectively testing my non-null Promise for null, and points out that that's nonsense.

Actual behavior:

TypeScript happily accepted the code, which led to a bug.

[RyanCavanaugh]

The problem is that people will often write code like this because of external callers

function doSomething(x: number) {
  // Always be safe!
  if (x ==null) throw new Error("Must invoke with a number");
  // do stuff with x
}

If there's some subset of things we could safely disallow while still allowing people to write defensive code, that'd be something to consider. Any thoughts?

[joshaber]

Yeah, I wondered if that was why it was allowed.

To make sure I understand, the case we're concerned about is:

1. It shouldn't be annotated null because callers really shouldn't pass null.
2. But humans are gonna human and might do it anyways.
3. So check for null even if it shouldn't be possible in the type system.

A straightforward answer would be to widen the type in the function:

function doSomething(x: number) {
  const reallyX: number | null = x;
  // Always be safe!
  if (reallyX == null) throw new Error("Must invoke with a number");
  // do stuff with reallyX
}

This way we're being honest with the type system, with all the advantages that brings along.

[RyanCavanaugh]

Obviously you can work around it, but if you're moving a 100,000 line codebase to --strictNullChecks and your functions guard against their parameters, writing that workaround hundreds of times is not practical or worthwhile.

[joshaber]

Yup, certainly true. I imagine migrating a large codebase to strict null checks in a lot of work either way.

[yortus]

Note that the OP example is not testing its inputs for null, it's testing the result of a function it called. I don't think there's much code double-checking function outputs. We generally assume a function does what it says it does, or all bets are off. Could that be a case where the warning would be useful, as the OP suggests?

[RyanCavanaugh]

Not sure. I think it'd be really weird to have different type rules for parameters vs non-parameters.

[joshaber]

Alternatively—and I'm not sure what the TypeScript philosophy is on this, but—gcc, clang, etc. provide a lot of warning flags that can be turned on and off.

[axefrog]

It could be argued that not implementing a feature (at least as an optional flag) on the basis that you can override expected behaviour in JavaScript, is an argument against have any compile-time type checking at all. Why throw an error when trying to assign a null to a number, but not throw an error when trying to check that a number is not null? The two cases seem to conflict with the basic idea of using TypeScript to begin with.

function getSomeNumber(): number {
  // ... magic happens here (maybe in JS something screwy happens and a string is returned?)
}
let n = getSomeNumber()
n = null; // not allowed because the type system says no, despite what JS may have done anyway

const m = 37;
if(m === null) {} // why do we allow this, but not the earlier case?

I'd also argue that, while the large project migration case is valid, should it prevent useful type-checking features for codebases that began as TypeScript projects? For the latter, I do want the type system telling me I'm doing something redundant, because that sort of thing is exactly why I chose to use TypeScript to begin with. My thought is that useful type-safety features that are skipped because JS migration projects might trip us up, should in fact not be skipped, but at least offered as additional strict-mode flags.

Related: #15211

[evmar]

It's worth thinking hard about the migration case and figuring out an answer for that, but the code that is already migrated to strictNullChecks and being maintained will continually become a larger proportion of the code. It's worth figuring out a real solution here.

Perhaps the guard can be shortened to
if (foo as any == null) ...
which is to say, you just need to inject an as any in the cases where you are checking for null when the types don't allow it.

It's not really the same thing, but notice that this warns:

let x: boolean = true;
if (x == false) {
}

as does this:

declare let x: boolean;
if (x == 'a') {
}

[mhegazy]

checking for null and undefined is allowed specifically for defensive programming. it is common that you write an API with the type not containing null or undefined, but you have JS users that you can not control how they will call your API.

[RyanCavanaugh]

The other problem, though perhaps a transient one, is that if you have a .d.ts file that was written with strict null checks off (which is still quite common), you're going to end up having to "cast away" in perfectly correct checks in a lot of places. I ran into this quite hard when trying to implement #9041