fix: optional-chaining should work correctly with ts non-null operator #10961

macabeus · 2020-01-04T23:57:52Z

Q	A
Fixed Issues?	Fixes #10959
Patch: Bug Fix?	👍 Yeah
Major: Breaking Change?	🧐 kind off
Minor: New Feature?	🙅‍♂️ nop
Tests Added + Pass?	👍
Any Dependency Changes?	🙅‍♂️ nop
License	MIT

This PR fix when use ! operator within an optional chaining.

a?.b!.c

should be compiled to

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

But previous this fix, it was compiling to

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

That could raise wrongly an exception if a is null or undefined.

The approach to fix this bug is unwrapping TSNonNullExpression on the OptionalMemberExpression.

JLHwung · 2020-01-05T00:15:19Z

packages/babel-plugin-proposal-optional-chaining/src/index.js

+          if (optionalPath.isTSNonNullExpression()) {
+            optionalPath.replaceWith(optionalPath.node.expression);
+
+            if (optionalPath.isOptionalMemberExpression()) {


I think we should only unwrap TSNonNullExpression and leave the node.expression as-is. Because the non-null expression does not imply any runtime checks, which is done by the optional member.

Oh, yeah. I tested now removing this if block and works fine. Okay, I'm updating this PR. Thank you.

nicolo-ribaudo

Can you also add a test with:

Optional chaining transform and typescript syntax only
Optional chaining syntax only and typescript transform

macabeus · 2020-01-05T01:40:15Z

@nicolo-ribaudo could you explain for me these transforms and syntax, please? What it's and its purposes. I still didn't understand very well about that.

JLHwung · 2020-01-05T02:31:42Z

@macabeus The transforms are the plugins that desugar OptionalChaining so that it can run on other platforms, i.e. browsers does not have optional-chaining support. The syntax are the plugins that allow the parser to parse optional-chaining, so that it will not throw unexpected token.

Optional chaining transform and typescript syntax only

So this is to say, one wants to desugar optional-chaining but keep typescript syntax intact, in this case the NonNullExpression should be preserved.

Optional chaining syntax only and typescript transform

In this case one wants to remove TypeScript stuffs but leave optional-chaining as-is.

jridgewell

Blocking this until we get confirmation from Typescript. @DanielRosenwasser?

Currently, Babel's transform matches Typescript's: http://www.typescriptlang.org/play/?ssl=1&ssc=1&pln=1&pc=8#code/IYfgdARghGDGQ

Note: I think we should change this, just need to confirm.

macabeus · 2020-01-06T18:08:25Z

I agree that we should follow the output in tsc, so I opened an issue in TypeScript repository saying about this problem.

alex-kinokon · 2020-01-06T22:40:13Z

@jridgewell It’s labeled as a bug in the TypeScript repo now. I think we’re good to go.

macabeus · 2020-01-06T23:16:45Z

Wait, no merger yet. We need to add the tests that @nicolo-ribaudo said.
I'll do it tonight.

macabeus · 2020-01-07T14:49:06Z

@nicolo-ribaudo Done. I added the two tests that you requested.
Maybe we could remove 10959-proposal-optional-chaining-and-preset-typescript, because it's redundant since there is the 10959-proposal-optional-chaining-and-typescript-syntax... What do you think?

macabeus · 2020-01-07T15:59:38Z

Some random testes failed... but it's sounds like a false negative, because I just added more tests that works fine locally. I didn't changed anymore.

JLHwung · 2020-01-07T16:27:53Z

@macabeus The CI error is not related. I will fix that later.

nicolo-ribaudo · 2020-01-07T20:40:15Z

...est/fixtures/regression/10959-syntax-optional-chaining-and-transform-typescript/options.json

+{
+  "plugins": [
+    "../../../../../babel-plugin-syntax-optional-chaining",
+    "../../../../../babel-plugin-transform-typescript"


Please use

Suggested change

"../../../../../babel-plugin-transform-typescript"

"transform-typescript"

directly (and also for the other options.json files)

nicolo-ribaudo · 2020-01-07T20:41:11Z

...ng/test/fixtures/regression/10959-proposal-optional-chaining-and-typescript-syntax/output.js

@@ -0,0 +1,3 @@
+var _a;
+
+(_a = a) === null || _a === void 0 ? void 0 : _a.b.c;


Since we aren't transforming TypeScript, this should remain

Suggested change

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

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

However, I think that no one will ever find this bug so if it's too hard to fix I'm ok with opening a new issue and not fixing it in this PR.

macabeus · 2020-03-26T00:23:13Z

The fix was just merged on TS: microsoft/TypeScript#36539

jridgewell · 2020-03-26T00:38:10Z

packages/babel-plugin-proposal-optional-chaining/src/index.js

+
+          // unwrap a TSNonNullExpression if need
+          if (optionalPath.isTSNonNullExpression()) {
+            optionalPath.replaceWith(optionalPath.node.expression);


I'm not sure that this will correctly fix the bug. We may be able to see it more in test cases like a?.b!.c.d and a?.b.c!.d

nicolo-ribaudo

Rebased and added a lot of tests!

nicolo-ribaudo · 2020-04-08T13:22:21Z

This is technically a breaking change, because it drops support for TypeScript 3.8. However, without merging this PR we can't support TypeScript 3.9.

I don't think we should wait for Babel 8: does anyone thinks that it would be better to merge this in a minor rather than in the next patch?

macabeus · 2020-04-08T15:05:10Z

@nicolo-ribaudo Well... TS already merged that on a minor release: 3.8 -> 3.9
Maybe we could follow the same approach?

existentialism · 2020-04-08T15:06:29Z

This is technically a breaking change, because it drops support for TypeScript 3.8. However, without merging this PR we can't support TypeScript 3.9.

I don't think we should wait for Babel 8: does anyone thinks that it would be better to merge this in a minor rather than in the next patch?

I'm good with a minor.

macabeus mentioned this pull request Jan 4, 2020

a?.b.c() produces different result than a?.b!.c() #10959

Closed

1 task

macabeus force-pushed the fix/optional-chaining-plus-non-null-expression branch from 49a4708 to 98c1799 Compare January 5, 2020 00:03

JLHwung reviewed Jan 5, 2020

View reviewed changes

macabeus force-pushed the fix/optional-chaining-plus-non-null-expression branch from 98c1799 to 029b658 Compare January 5, 2020 00:24

nicolo-ribaudo reviewed Jan 5, 2020

View reviewed changes

jridgewell requested changes Jan 5, 2020

View reviewed changes

macabeus mentioned this pull request Jan 6, 2020

Optional chaining with non null operator is unsafe, because it could throw an exception microsoft/TypeScript#36031

Closed

macabeus force-pushed the fix/optional-chaining-plus-non-null-expression branch from 029b658 to f56f54b Compare January 7, 2020 14:47

existentialism added area: typescript PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jan 7, 2020

nicolo-ribaudo reviewed Jan 20, 2020

View reviewed changes

jridgewell requested changes Mar 26, 2020

View reviewed changes

nicolo-ribaudo mentioned this pull request Apr 5, 2020

Unify parens printing for postfix-like expressions #11382

Merged

macabeus and others added 3 commits April 7, 2020 22:56

fix: optional-chaining should work correctly with ts non-null operator

7cff730

Keep ! when not transforming TS

2a72af8

Update tests

d490b96

nicolo-ribaudo force-pushed the fix/optional-chaining-plus-non-null-expression branch from f56f54b to d490b96 Compare April 7, 2020 21:01

nicolo-ribaudo approved these changes Apr 7, 2020

View reviewed changes

existentialism approved these changes Apr 8, 2020

View reviewed changes

jridgewell approved these changes Apr 8, 2020

View reviewed changes

nicolo-ribaudo added this to the v7.10.0 milestone Apr 8, 2020

nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Apr 8, 2020

nicolo-ribaudo merged commit 75a6530 into babel:master May 24, 2020

macabeus deleted the fix/optional-chaining-plus-non-null-expression branch May 24, 2020 21:03

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 24, 2020

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: optional-chaining should work correctly with ts non-null operator #10961

fix: optional-chaining should work correctly with ts non-null operator #10961

macabeus commented Jan 4, 2020 •

edited by JLHwung

JLHwung Jan 5, 2020 •

edited

macabeus Jan 5, 2020

nicolo-ribaudo left a comment

macabeus commented Jan 5, 2020 •

edited

JLHwung commented Jan 5, 2020

jridgewell left a comment •

edited

macabeus commented Jan 6, 2020 •

edited

alex-kinokon commented Jan 6, 2020

macabeus commented Jan 6, 2020 •

edited

macabeus commented Jan 7, 2020

macabeus commented Jan 7, 2020

JLHwung commented Jan 7, 2020

nicolo-ribaudo Jan 7, 2020

nicolo-ribaudo Jan 7, 2020

macabeus commented Mar 26, 2020

jridgewell Mar 26, 2020

nicolo-ribaudo left a comment

nicolo-ribaudo commented Apr 8, 2020

macabeus commented Apr 8, 2020

existentialism commented Apr 8, 2020

	"../../../../../babel-plugin-transform-typescript"
	"transform-typescript"

		@@ -0,0 +1,3 @@
		var _a;

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

fix: optional-chaining should work correctly with ts non-null operator #10961

fix: optional-chaining should work correctly with ts non-null operator #10961

Conversation

macabeus commented Jan 4, 2020 • edited by JLHwung

JLHwung Jan 5, 2020 • edited

Choose a reason for hiding this comment

macabeus Jan 5, 2020

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

macabeus commented Jan 5, 2020 • edited

JLHwung commented Jan 5, 2020

jridgewell left a comment • edited

Choose a reason for hiding this comment

macabeus commented Jan 6, 2020 • edited

alex-kinokon commented Jan 6, 2020

macabeus commented Jan 6, 2020 • edited

macabeus commented Jan 7, 2020

macabeus commented Jan 7, 2020

JLHwung commented Jan 7, 2020

nicolo-ribaudo Jan 7, 2020

Choose a reason for hiding this comment

nicolo-ribaudo Jan 7, 2020

Choose a reason for hiding this comment

macabeus commented Mar 26, 2020

jridgewell Mar 26, 2020

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Apr 8, 2020

macabeus commented Apr 8, 2020

existentialism commented Apr 8, 2020

macabeus commented Jan 4, 2020 •

edited by JLHwung

JLHwung Jan 5, 2020 •

edited

macabeus commented Jan 5, 2020 •

edited

jridgewell left a comment •

edited

macabeus commented Jan 6, 2020 •

edited

macabeus commented Jan 6, 2020 •

edited