Repository navigation
Optional chaining with non null operator is unsafe, because it could throw an exception #36031
Description
Activity
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)? ).Jackson Kearl (@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()
Ah my bad, yes I see what you mean.
In this playground, adding the
!changes the emit, which it shoudn't.I'll look into it. While the syntax is valid to parse, it is a bit nonsensical. You're asserting that
a?.bwill always be defined, but that won't be the case whenais possiblyundefined.Reacted by falsandtruYou'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
ais not nullish, then it is guaranteed to have a non-nullishbproperty. @proteriax's example matches my feeling perfectly. If we desugar:// Input: a?.b!.c // Output: a == null ? undefined : a.b!.c
Reacted by falsandtru, Bruno Macabeus, Alex, Alexey Lebedev, gong, Shengming Yuan and GeoduckRon Buckton (@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 asa?.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;).Reacted by falsandtru, Bruno Macabeus, Alex and Grigory Streltsov- removed this from the TypeScript 3.8.1 milestone
on Feb 1, 2020 5 remaining items
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!.cis 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.
If we change the precedence of ! to be lower than OptionalChain, then that changes the type of
x2above to benumber | 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)!.bazto begin with.Downleveling
foo?.bar!.bazinto(foo?.bar).bazis really weird. It's not meaningfully different than downleveling tofoo.bar.baz. Whether.baris the thing throwing (becausefooisnull) or.bazthrows (becausefoo?.barreturnedundefined) is useless.Reacted by Daniel Rosenwasser, Alex and Bruno MacabeusIf 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!.clooks 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?Reacted by AlexDaniel Rosenwasser (@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:
Realizes that they know
barwill be defined any timefoois, 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 undefinederror in the below:declare const foo: { bar?: { baz?: () => {} } } foo.bar?.baz();
given they know
bazwill be defined whenbaris 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 aastype 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.
Reacted by Bruno MacabeusYes, 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
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.
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.
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.Ron Buckton (@rbuckton) I think that's the best option if changing the precedence is too high-impact.
After our most recent design meeting, we've decided that our solution should be a slightly odd hybrid to satisfy both the
a?.b!.ccase and thea?.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 removeundefinedfrom the resulting type of the entire chain.Reacted by Bruno MacabeusLove it.
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
TypeScript Version: 3.7.2
Search Terms: optional chaining, non null operator
Code
This input
will compile to
But it's unsafe, because if
aisnullorundefined, will throw an exception:IMO we should not raise this exception.
Expected behavior:
Would be better to compile to
So
acould 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.
!.after?.should be warned #35071Also this bug was reported in Babel and already there is a PR to fix that in Babel:
In this issue I'm saying only about the output, not about the type system.