microsoft / TypeScript

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

Narrowing doesn't recognize string constant as truthy #33878

Open nmain opened 5 years ago

nmain commented 5 years ago

TypeScript Version: 3.7-beta

Search Terms: cfa narrowing string truthy

Code

declare var a: { b: number } | null

!a || a.b > 3; // works
!a && true || a.b > 3 && true; // works
!a && "a was not defined" || a.b > 3 && "a.b was too big"; // error "Object is possibly null"

Expected behavior:

The narrowings all work

Actual behavior:

The third narrowing does not work and produces an error

Playground Link: Playground

Related Issues: I suspect this is a duplicate, as most CFA narrowing bugs are. But I searched as best I could and didn't find any other bugs that matched what was going on here. It seems like typescript doesn't understand that a non-empty string-literal is true in this situation?

poseidonCore commented 5 years ago

For context, this error occurs when strictNullChecks is on, but not when it is off.

However, note that here !a && "a was not defined" || a.b > 3 && "a.b was too big" is comprehended as a string, not a boolean even when strictNullChecks is off.

The problem lies in that !null && "whatever" technically evaluates as a string (because of short-circuit evaluation: true && "whatever" -> "whatever"), and null is a possibility for a ... and so the third evaluation collapses to non-empty string or boolean, which is then comprehended as string.

TS Playground

poseidonCore commented 5 years ago

Although, strangely, in the case where a is not null, the comprehension is still string, but the evaluation is boolean: eg !{b:1} && "whatever" is false because of short-circuiting.

TS Playground

RyanCavanaugh commented 5 years ago

Probably a bug in how we treat the short-circuiting operators when they're nested

poseidonCore commented 5 years ago

As I pointed out, there is an error even without nesting, which then may be feeding back into the nesting logic.

var f = !{b:1}; // :boolean=false (correct)
var m = false && "whatever"; // :boolean=false (correct)
var n = !{b:1} && "whatever"; // :string (wrong)
ahejlsberg commented 5 years ago

This is related to our unreachable code analysis in the binder. We consider false branch of true and the true branch of false to be unreachable, but we don't extend this to other literals. So, from a control flow analysis perspective, a string literal is simply a string expression that could be truthy or falsy.

It's not particularly hard to change this, but the question is where to stop. We'd of course have to check string, number, and bigint literals, but also unary - applied to a numeric literal (and, for symmetry, unary +). And, I guess, we should recognize that {...} and [...] literals are always truthy. But then there's unary ! applied to a literal, which would break a bunch of tests because we currently rely on !!true not being recognized as definitely truthy.

I'm not opposed to the change, but also don't think it is particularly high yield.