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

Nullish coalescing should always include the type of the right operand #36393

Open
twavv opened this issue Jan 24, 2020 · 8 comments
Open

Nullish coalescing should always include the type of the right operand #36393

twavv opened this issue Jan 24, 2020 · 8 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@twavv
Copy link

twavv commented Jan 24, 2020

$ ./node_modules/.bin/tsc --version
Version 3.7.2

Search Terms:

Code

A toy example would be something like this.

let foo: string = "";
foo = "bar" ?? 123;

This one is obviously fine since "bar" is always truthy.

However, this becomes a little bit problematic when you consider the idiom of having Record objects and checking their truthiness before using them.

const elts = ["foo", "bar", "spam", "spam", "foo", "eggs"];
const counts: Record<string, number>;
for (const elt of elts) {
  // This really **should** raise an error.
  counts[elt] = counts[elt] ?? "zero";
  counts[elt] += 1;
}

Expected behavior:
An error should be raised.

Actual behavior:
Curiously, an error is not raised in strict mode but is raised in un-strict mode.

$ ./node_modules/.bin/tsc ./foo.ts 
foo.ts:5:3 - error TS2322: Type 'number | "zero"' is not assignable to type 'number'.
  Type '"zero"' is not assignable to type 'number'.

5   counts[elt] = counts[elt] ?? "zero";
    ~~~~~~~~~~~


Found 1 error.
$ ./node_modules/.bin/tsc --strict ./foo.ts
# No error, exits 0 and emits JS.

Playground Link:
Playground Link

Toggling the strictNullChecks config option will show the issue.

Related Issues:

@jcalz
Copy link
Contributor

jcalz commented Jan 24, 2020

This is probably related to or a duplicate of #13778.

With --strictNullChecks on, the compiler assumes that counts[elt] must be of type number, which is never null or undefined. If you want the compiler to notice that counts[elt] may be undefined, then you (currently) need to include undefined in the property type manually:

const counts: Record<string, number | undefined> = {};
for (const elt of elts) {
  counts[elt] = (counts[elt] ?? "zero"); // error now
  counts[elt] = (counts[elt] ?? 0) + 1; // okay
}

Playground

With --strictNullChecks off, the compiler never assumes nullish coalescing will short-circuit because it can never eliminate null and undefined. See this comment from the PR

@twavv
Copy link
Author

twavv commented Jan 24, 2020

It seems like there could be some "rightward-precedence" for this kind of thing (I'm not sure about the right terminology to describe what I mean).

What I mean is basically this:

string -> string
string ?? number -> string | number;
null ?? number -> number;

I think the algorithm would roughly be something like

  • Given a chain of A ?? B ?? C ?? ..., take the first pair (nullish coalescing is left-associative, right?)
  • Resolve the types of A and B
  • Judge the type of A ?? B to be NonNull<A> | B
  • Recurse
string ?? (string | null) ?? string[]
// Take first pair...
=> (NonNull<string> | (string | null)) ?? string[]
// Simplify the types...
=> (string | null) ?? string[]
// Recursively take first pair...
=> NonNull<string | null> | string[]
// Simplify the types...
=> string | string[]
// Done!

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Jan 27, 2020
@RyanCavanaugh RyanCavanaugh changed the title Nullish coalescing short-circuits type checking (only in strict mode) Nullish coalescing should always include the type of the right operand Jan 27, 2020
@falsandtru
Copy link
Contributor

&&, ||, and ! operators also have a similar problem.

@kherock
Copy link

kherock commented Sep 30, 2020

I just ran into this myself, but I'd say this is working as intended. Rephrasing what @jcalz found, the example in the issue description is demonstrating an example of a Record type that is too loose:

const counts: Record<string, number>;

// strictNullChecks: true
counts.foo // evaluated as (number)

// strictNullChecks: false
counts.foo // evaluated as (number | null | undefined)

Under strict mode, @typescript-eslint/no-unnecessary-condition does catch TS's short-circuiting behavior when computing the type and will yell at you:

const elts = ['foo', 'bar', 'spam', 'spam', 'foo', 'eggs'];
const counts: Record<string, number> = {};
for (const elt of elts) {
  counts[elt] = counts[elt] ?? 'zero'; /*
                ^^^^^^^^^^^
Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined. [eslint] */
  counts[elt] += 1;
}

If you explicitly widen the type to include null | undefined, you'll get the error you see in loose mode:

const elts = ['foo', 'bar', 'spam', 'spam', 'foo', 'eggs'];
const counts: Record<string, number> = {};
for (const elt of elts) {
  counts[elt] = (counts[elt] as number | null | undefined) ?? 'zero'; /*
  ^^^^^^^^^^^
Type 'number | "zero"' is not assignable to type 'number'.  Type 'string' is not assignable to type 'number'. [ts] */
  counts[elt] += 1;
}

Given that a linter is capable of pointing out the potentially unexpected behavior, I don't know if there's anything that needs to be fixed on TS's end.

@twavv
Copy link
Author

twavv commented Sep 30, 2020

I respectfully disagree (obviously - I created the issue 😉). Using too-loose Record types is a common idiom in TypeScript, and I think changing this behavior makes that idiom easier to work with.

@dmo-odoo
Copy link

dmo-odoo commented Feb 13, 2023

I think the following might be related to this issue:

function auto<T1, T2>(value: T1, fallback?: T2) {
    return value ?? fallback;
}
// Incorrectly typed to "unknown" even though the right hand path of `??` will never be taken in this case.
const f1 = auto(42);

function forced<T1, T2>(value: T1, fallback?: T2): T1 extends undefined | null ? NonNullable<T1> | T2 : T1 {
    return value ?? fallback as any;
}
// Correctly typed to "number".
const f2 = forced(42);

This can be important if your system relies on fallback values. With the current typing of ??, the system would force you to define a T2 fallback because if you don't define one then its unknown typing from being optional will pollute the final return type even in cases where T1 can never be null or undefined.

From my point of view, it seems like the typing of T1 ?? T2 could be more precise and be T1 extends undefined | null ? NonNullable<T1> | T2 : T1 instead of its current value of NonNullable<T1> | T2, but I suppose I might be missing something in my understanding ?

@fvictorio
Copy link

For the record, this:

let foo: string = "";
foo ??= 123;

has the "expected" behavior.

@duddlf23
Copy link

It seems to be resolved by turning on noUncheckedIndexedAccess config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

8 participants