Optional chaining with non null operator is unsafe, because it could throw an exception

[macabeus]

TypeScript Version: 3.7.2

Search Terms: optional chaining, non null operator

Code

This input

a?.b!.c

will compile to

((_a = a) === null || _a === void 0 ? void 0 : _a.b).c;

But it's unsafe, because if a is null or undefined, will throw an exception:

   ((_a = a) === null || _a === void 0 ? void 0 : _a.b).c
=> ((_a = null) === null || _a === void 0 ? void 0 : _a.b).c
=> (null === null || _a === void 0 ? void 0 : _a.b).c
=> (true ? void 0 : _a.b).c
=> undefined.c

IMO we should not raise this exception.

Expected behavior:

Would be better to compile to

(_a = a) === null || _a === void 0 ? void 0 : _a.b.c;

So a could be null or undefined and it'll not raise an exception anymore.

Playground Link: Playground

Related Issues: There was a lot of discussion here, but was more focused in the type system, not about the code output that raise wrongly an exception.

• Non-null type assertions shouldn't affect optional chaining #34875
• Non-null assertions infringe a responsibility of optional chaining #35025
• !. after ?. should be warned #35071

Also this bug was reported in Babel and already there is a PR to fix that in Babel:

• issue: a?.b.c() produces different result than a?.b!.c() babel/babel#10959
• pr: fix: optional-chaining should work correctly with ts non-null operator babel/babel#10961

In this issue I'm saying only about the output, not about the type system.

[DanielRosenwasser]

CC @RyanCavanaugh @rbuckton

[JacksonKearl]

Is the precedence of !. vs. ?. defined somewhere? In this case it seems that it's being interpreted as (a?.b)!.c, which seems valid. The alternative might be better though, as I think it is impossible to specify via parens (a?.(b!.c)? a?(.b!.c)? ).

[alex-kinokon]

@JacksonKearl It is the explicit goal of TypeScript that the syntax should have no effect on the runtime behavior so you can’t just add a bracket out of nowhere.

Consider the following case:

function getElementById(id: string): Element | null

interface Element {
  // Only null for Document, DOCTYPE, or a Notation
  textContent: string | null
}

getElementById("div.head")?.textContent!.toUpperCase()

[JacksonKearl]

Ah my bad, yes I see what you mean.

https://www.typescriptlang.org/play/#code/DYUwLgBAhgXBDeEBGB+O8DGcB2BXAtkiAE4C+pEAPhLtgCYgBmAltiHQFAcYD22AzpEY8eEALzQUAOiQBCKRgDc3PoORRi4iAAoo0pAEp5SlQMhIoALy16ZC5UA

In this playground, adding the ! changes the emit, which it shoudn't.

[rbuckton]

I'll look into it. While the syntax is valid to parse, it is a bit nonsensical. You're asserting that a?.b will always be defined, but that won't be the case when a is possibly undefined.

[jridgewell]

You're asserting that a?.b will always be defined, but that won't be the case when a is possibly undefined.

I disagree. I think I'm asserting that if a is not nullish, then it is guaranteed to have a non-nullish b property. @proteriax's example matches my feeling perfectly. If we desugar:

// Input:
a?.b!.c

// Output:
a == null ? undefined : a.b!.c

[JacksonKearl]

@rbuckton The code should run the same with or without TS-specific syntax, right? Currently it won't; the output changes depending on the presence of the !. This is because TS (incorrectly) implicitly groups it as (a?.b)!.c, which through type-elision becomes (a?.b).c, which is clearly not the same as a?.b.c.

This further means that the code with run differently when targeting ESNext (a?.b.c;) vs anything else (((_a = a) === null || _a === void 0 ? void 0 : _a.b).c;).

[DanielRosenwasser]

As we were implementing the change, it seems that this had some undesirable properties in the type system when you change precedence. Specifically, @rbuckton's approach seemed to mean

a?.b.c!

would potentially come out as undefined. I'll defer to his explanation here.

We also checked this behavior out in C# which also has a ! operator (called the "null forgiving operator"). The way C# works seems to match the way TypeScript works today. You can witness this by playing around with the output for

#nullable enable

using System;
public class C {
    public Blah M(Blah? x) {
        var a = x?.X!.X;

        return a;
    }
}

public class Blah
{
    public Blah X = null!;
}

https://sharplab.io/#v2:CYLg1APgxAdgrgGwQQwEYIKYAIMzZgWACgABAJgEZiSBmLcrAYSwG9isOt3PuPasAQigAWWALIAKIcmEB+LAA8AlK16cOAN2QAnLMiwBeRbIB0ADQCE5gNzE16kgHY9touoC+xT0Wp0G04WI2N05+AKwzQyx4JAtXdyA

[rbuckton]

This turns out to be a much more complex and subtle issue than it seems on the surface. Currently, we parse postfix-! as a higher precedence than the ECMAScript OptionalChain, which means that a postfix-! essentially ends the chain.

Generally, the use case for postfix-! is to remove null | undefined from the expression to which it is affixed. Consider the following cases:

declare const a: number | undefined;
const x = a!; // number

Here, the user expects the type of x to be number.

declare const a: { b: number | null };
const x = a.b!; // number

Once again, the user expects the type of x to be number, as the undefined has been removed.

When optional chaining is added into the mix, we currently preserve these expectations:

declare const a: { b: number | null };
const x1 = a?.b // number | null | undefined* (* undefined added by `?.`)
const x2 = a?.b! // number

Here, we pick the type number for x2, removing null | undefined.

If we change the precedence of ! to be lower than OptionalChain, then that changes the type of x2 above to be number | undefined*, which will be surprising for users. This is because a postfix-!
encountered while parsing an OptionalChain must always be part of the OptionalChain due to grammar restrictions (checking for something that is not part of an OptionalChain following a postfix-! would require infinite lookahead since you could write a?.b!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!.c, etc.)

As a result, we need to choose one of the following different approaches:

• Change the precedence of postfix-! - This is a breaking change and we will need some time to consider:

  // ts
  a?.b!.c
  // js (es5)
  (a === null || a === undefined ? undefined ? a.b.c)
  // js (es2020)
  a?.b.c

• Leave the precedence as-is and emit parens () around the operand when --target es2020:

  // ts
  a?.b!.c
  // js (es5)
  (a === null || a === undefined ? undefined ? a.b).c
  // js (es2020)
  (a?.b).c

• Leave the precedence as-is and require parens () around an optional chain that is followed by a ! (making it an error similar to requiring parens around logical operators in conjunction with ??):

  // ts
  // a?.b!.c // error
  (a?.b)!.c
  // js (es5)
  (a === null || a === undefined ? undefined ? a.b).c
  // js (es2020)
  (a?.b).c

As a result, it is unlikely this will make our 3.8 release milestone as we need to discuss this further within the team.

Please note that C# also implements both ?. and postfix-! and is consistent with our current semantics. While C# had ?. long before JS/TS, it should be noted that C#'s implementation of postfix-! came after TS implemented it, and therefore follows the current TS precedence.

[JacksonKearl]

Leave the precedence as-is and emit parens () around the operand when --target es2020

This option is inconsistent with the general TS design goal (as I understand it) that the code should always run the same with or without TypeScript-specific add-ons.

I realize that's not explicitly stated in the design goals, but are there any other examples of TS that behaves differently if you remove the TS-specific add-ons?

[falsandtru]

Currently, we parse postfix-! as a higher precedence than the ECMAScript OptionalChain

@rbuckton A related issue #35025 should be considered together.

[DanielRosenwasser]

This option is inconsistent with the general TS design goal (as I understand it) that the code should always run the same with or without TypeScript-specific add-ons.

I don't think that's the right way to think about it because ultimately it's not really the same code, it's a distinct syntax that was parsed in a meaningfully different way. The way it's parsed typically implies a certain order of operations. If you want to desugar the current code and build a mental model around it, a?.b!.c is equivalent to

(a?.b as NonNullable<typeof a>["b"] | undefined).c

which is downeleveled to

(a?.b).c

And that code does shorten the optional chain expression's reach due to the parenthesized expression.

So the question isn't about erasability because erasure is happening either way. It's about parsing precedence.

[jridgewell]

If we change the precedence of ! to be lower than OptionalChain, then that changes the type of x2 above to be number | undefined*, which will be surprising for users.

I think this is where we disagree. I expect number | undefined* as the result.

If I wanted to unconditionally assert the return type, I would have written (foo?.bar)!.baz to begin with.

Downleveling foo?.bar!.baz into (foo?.bar).baz is really weird. It's not meaningfully different than downleveling to foo.bar.baz. Whether .bar is the thing throwing (because foo is null) or .baz throws (because foo?.bar returned undefined) is useless.

[JacksonKearl]

If I wanted to unconditionally assert the return type, I would have written (foo?.bar)!.baz to begin with.

Exactly, and given there's no way to represent what a?.b!.c looks like it means, wouldn't it make the most sense to have it default to "parse A", then users can add parens to force "parse B" -- rather than it defaults to "parse B", users can add parens to force "parse B", and "parse A" is unrepresentable?

[JacksonKearl]

@DanielRosenwasser

I don't think that's the right way to think about it because ultimately it's not really the same code, it's a distinct syntax that was parsed in a meaningfully different way.

That's all well and good if you have the TS shift-reduce conflict resolution table memorized, but if you're a random developer who encounters an error like this:

https://www.typescriptlang.org/play/?ssl=3&ssc=2&pln=1&pc=1#code/BQMw9mBcAEB2CuAbR0A+0De0BGBDATgPwxZ4BeMwAlNALwB8mAvtC0zQ5gLABQ0-0cGEIA6PPjG4y1XkyA

Realizes that they know bar will be defined any time foo is, so adds a !. The error disappears and all seems well, but actually they've taken safe JS and transformed it into error-prone JS because of TS's "warning". This is a really quite bad developer experience.

And this isn't just some hypothetical example, VSCode 1.42 will ship tomorrow with an instance of this exact bug: https://github.com/microsoft/vscode/blob/master/src/vs/base/browser/ui/tree/asyncDataTree.ts#L272

How would you propose people address the cant invoke an object which is possibly undefined error in the below:

declare const foo: { bar?: { baz?: () => {} } }
foo.bar?.baz();

given they know baz will be defined when bar is in this case (but the type system doesn't for whatever reason). The answer has always been add a !, but now that will actually add a runtime error that wasn't in the original code. And given there's no ?() syntax, and a as type assertion would have the same issues, I don't see how a fix is even representable with the current parse strategy.

This is my best shot:

declare const foo: { bar?: { baz?: () => {} } }
if (foo.bar) {
    (foo.bar.baz as Exclude<Exclude<typeof foo.bar, undefined>['baz'], undefined>)() 
}

...which is to say, get rid of the optional chaining entirely.

[DanielRosenwasser]

Yes, that's understandable, but then the argument you're giving is developer experience, it's not that TypeScript isn't providing erasable syntax on top of JS.

Maybe it seems like I'm not empathizing with the problem you're running into (I am!). I'm just trying to clarify why this isn't inconsistent with our design goals and I'd like the arguments presented here to be accurate.

I think next steps will be to

• bring this up at another design meeting to clarify whether this behavior is desirable
• chat with other language designers

[mAAdhaTTah]

And given there's no ?() syntax

There is, but it's foo?.().

...then the argument you're giving is developer experience, it's not that TypeScript isn't providing erasable syntax on top of JS.

I'd suggest it's both: It's a poor developer experience because TS isn't providing erasable syntax.

[JacksonKearl]

There is, but it's foo?.().

Ah, I knew there must be something but I blanked on what it was.

It's a poor developer experience because TS isn't providing erasable syntax.

Exactly. It's breaking the contract TS has set up with users that generally goes: "if I add TS syntax elements to my JS the JS will continue to run the exact same".

Are there any other examples in all of TS where adding some TS-specific syntax effects the emit? The only one I can think of is generators, and those at least are very explicit about being runtime features.

From TS docs:

Similar to type assertions of the forms x and x as T, the ! non-null assertion operator is simply removed in the emitted JavaScript code.

[rbuckton]

The "erasable syntax" case would be handled by forcing parens (e.g., (a?.b)!.c), since we would erase the ! (leaving (a?.b).c). It may not be the best developer experience, but ensures the semantics are well defined.

[JacksonKearl]

@rbuckton I think that's the best option if changing the precedence is too high-impact.

[DanielRosenwasser]

After our most recent design meeting, we've decided that our solution should be a slightly odd hybrid to satisfy both the a?.b!.c case and the a?.b.c! cases.

! will be special-cased so that when the next token could continue an optional chain, the ! will be parsed as part of the current chain expression (the behavior that users on this issue have wanted). Otherwise, it will still work "as expected" at the end of an optional chain expression and remove undefined from the resulting type of the entire chain.

[JacksonKearl]

Love it.

[falsandtru]

A bug of CFA #36958 should be fixed at the same time or the patch should be possible easily to be fixed after.

const m = ''.match('');
m?.[0] && m[0]; // ok
m?.[0]! && m[0]; // error
m?.[0].length! > 0 && m[0]; // error
m?.[0].split('').slice() && m[0]; // ok
m?.[0].split('')!.slice() && m[0]; // error