microsoft / TypeScript

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

`strictNullChecks` short-circuits the subsequent type checks #53162

Open PPpro opened 1 year ago

PPpro commented 1 year ago

Bug Report

hello, I encounter a strange bug when I use strictNullChecks: true, and here is an example:

interface A {
  a: number;
}
const list: A[] = [];
list[0] ??= {};  // throw an error: Property 'a' is missing in type '{}' but required in type 'A'
list[0] = list[0] ?? {};  // the type checking not work !
list[0] = list[0] || {};  // the type checking not work !

πŸ”Ž Search Terms

πŸ•— Version & Regression Information

It seems the type checking works after I close the strictNullChecks options Playground link with relevant code

πŸ’» Code

interface A {
  a: number;
}
const list: A[] = [];
list[0] ??= {};  // throw an error: Property 'a' is missing in type '{}' but required in type 'A'
list[0] = list[0] ?? {};  // the type checking not work !
list[0] = list[0] || {};  // the type checking not work !

πŸ™ Actual behavior

list[0] = list[0] ?? {}; doens't throw any errors

πŸ™‚ Expected behavior

I expect that list[0] = list[0] ?? {}; should throw an error

jakebailey commented 1 year ago

Any value which implements the interface A must be truthy and non-null/undefined, so I don't think it makes sense to emit an error here unless you have noUncheckedIndexedAccess enabled, in which case you do see an error for the cases you've noted: Playground Link

PPpro commented 1 year ago

@jakebailey thanks for reply, then one thing bothers me that should the statement list[0] ??= {}; emit an error ? should we keep the behavior the same as list[0] = list[0] ?? {};

fatcerberus commented 1 year ago

@jakebailey It does seem inconsistent though that list[0] ??= {} produces an error, since the RHS is equally irrelevant by the same reasoning.

jakebailey commented 1 year ago

Ah. I see what you mean.

PPpro commented 1 year ago

Sorry, I thought I could give a simpler example of what I want to discuss in this issue.

In this example, when strictNullChecks is true, only the statement a = a ? a : {}; emit an error. while when strictNullChecks is false, all the three emit errors, and that is what I expect. I thought that object {} should not be assigned to a boolean variable

let a: boolean = true;
a = a || {};
a = a ?? {};
a = a ? a : {};

here is the playground link

In this case, it seems that we should close strictNullChecks to make it more type-safe.

Andarist commented 1 year ago

Re last one: checkConditionalExpression produces a union of both true/false types: https://github.dev/microsoft/TypeScript/blob/746a6feb2e7ba6987b6c72db538dd498b35cd461/src/compiler/checker.ts#L37036

Binary expressions take into account control flow analysis. So in this case, it's known that || {} and ?? {} can't be chosen by those expressions and thus their type is simply true and thus assignable to a.

I don't know why conditional expressions don't apply the same control flow-based "narrowing" though.

Re first one: it's likely a weird (at first glance) byproduct of the same control flow analysis. In all of those cases TypeScript checks if the RHS is assignable to the left side. It just happens that RHS type isn't the same in all of them because the variants with || {} and ?? {} are known to be of type A since the first operand of those is truthy and CFA knows that the second operand can't be selected (according to the TS rules around indexed access on arrays). In the ??= the RHS is different - it's actually {} and that type is validated against the assignment target. It's surprising... but makes sense πŸ˜…

fatcerberus commented 1 year ago

In this case, it seems that we should close strictNullChecks to make it more type-safe.

I assume by "close" you mean "disable". Definitely don't do that as it will make undefined and null silently assignable to all types. You only get more errors here because by turning it off the compiler knows a can now be falsy and thus the RHS of ?? becomes reachable.

Technically a ?? b should be an unconditional error if the compiler knows a is always truthy. It's clearly already doing that check anyway.

fatcerberus commented 1 year ago

@Andarist @jakebailey The a ??= not_a case should probably be caught because someone could conceivably have written that to harden against unsafe JS callers (IIRC it's been mentioned in the past that TS code should still be able to do null-checks without getting a never for that reason). The lack of typechecking is thus counterproductive.

RyanCavanaugh commented 1 year ago

If you take the view that an expression of the form e ? b : c always includes reachable c even if e is truthy, then you have to have a coherent view of the world of what e.g. the expression e ? b : e?.a means, and it's not obvious how you should do that when the only legal CFA path to get there involves contradictions. Let's say e started as a parameter of type null | { a: string } and you wrote

if (e !== null) {
  e ||= e.a;

If we assume that e.a has to be reachable... what type does e have when we're evaluating it? Including null here seems bananas since we know the code just tested for that. Including { a: string } seems like the worst option, since the code only runs when it isn't that. If the programmer wrote the code on purpose, then there's some other value that e has, but we just have no idea which.

I don't think this is solvable in a meaningful sense apart from just saying "You seem to have written unreachable code". Maybe with the existence of NUIA we could have a flag that does warn you if you test for an always-truthy value and see what that does; maybe it'd be useful or maybe not.

Andarist commented 1 year ago

If you take the view that an expression of the form e ? b : c always includes reachable c even if e is truthy

My q here would be - why that view is taken by TS if other logical-like expressions don't take that view? I don't intend to argue with the choice. It's just something that I encountered in the past already and didn't know how to answer this for myself.