nullish coalescing commit #32883

Kingwl · 2019-08-14T08:44:46Z

follow https://tc39.es/proposal-nullish-coalescing/

Introduction
This document specifies the nullish coalescing operator ??. See the explainer for an introduction.

The main design decisions made in this specification are:

The right argument of ?? is evaluated only if needed ("short circuiting").
?? has lower precedence than ||.
?? cannot immediately contain, or be contained within, an && or || operation.
The right argument is selected if the left argument is null or undefined.

Fixes #26578

TODO:

AlCalzone · 2019-08-14T14:11:14Z

Is this the real life? 😍

Kingwl · 2019-08-15T06:00:26Z

@typescript-bot test this.

typescript-bot · 2019-08-15T06:00:32Z

Heya @Kingwl, I've started to run the extended test suite on this PR at 2327b3b. You can monitor the build here. It should now contribute to this PR's status checks.

DanielRosenwasser · 2019-08-15T07:40:37Z

src/compiler/checker.ts

@@ -23917,6 +23918,10 @@ namespace ts {
                    return getTypeFacts(leftType) & TypeFacts.Falsy ?
                        getUnionType([removeDefinitelyFalsyTypes(leftType), rightType], UnionReduction.Subtype) :
                        leftType;
+                case SyntaxKind.QuestionQuestionToken:
+                    return getTypeFacts(leftType) & TypeFacts.EQUndefinedOrNull ?
+                        getUnionType([getNonNullableType(leftType), rightType], UnionReduction.Subtype) :


Does it make sense to also return a union type for when strictNullChecks is off?

I'd like to say yes, every value could be null/undefined if strictNullChecks is off and we cannot detect that.

same as:

declare const a: string let b = a || 1 // b is string || number

DanielRosenwasser · 2019-08-15T07:43:20Z

src/compiler/factory.ts

@@ -3044,6 +3044,10 @@ namespace ts {
        return createBinary(left, SyntaxKind.BarBarToken, right);
    }

+    export function createNullishCoalescing(left: Expression, right: Expression) {


Nit: createNullishCoalesce? I dunno, what do you think @rbuckton?

DanielRosenwasser

Progress looks great!

DanielRosenwasser · 2019-08-15T07:48:08Z

tests/baselines/reference/nullishCoalescingOperator1.js

+
+//// [nullishCoalescingOperator1.js]
+"use strict";
+var aa1 = typeof a1 !== "undefined" && a1 !== null ? a1 : 'whatever';


Just as a heads up, we might want to just go with a1 != null, since document.all is a weird anomaly of the language anyway that should ideally never be used in a ?? expression.

@DanielRosenwasser
Sorry for the late comment after TypeScript 3.7 release.
But what about the shorter implementation of ?? operator that you have suggested?
This implementation with less code is better for all of us, I think.

DanielRosenwasser · 2019-08-15T07:51:33Z

.js emit test cases I'd like you to consider when targeting ESNext:

declare let a: any, b: any, c: any;

let x1 = (a ?? b as any) || c;
let x2 = c || (a ?? b as any);
let x3 = ((a ?? b) as any) || c;
let x4 = c || ((a ?? b) as any);
let x5 = (a ?? b) as any || c;
let x6 = c || (a ?? b) as any;

let y1 = (a ?? b as any) && c;
let y2 = c && (a ?? b as any);
let y3 = ((a ?? b) as any) && c;
let y4 = c && ((a ?? b) as any);
let y5 = (a ?? b) as any && c;
let y6 = c && (a ?? b) as any;

… grammar restrictions when assertions are used.

…red). Added tests to ensure calls and property accesses are only called once.

…wl/TypeScript into nullish-coalescing-operator

dragomirtitian · 2019-08-15T16:28:08Z

@DanielRosenwasser I've added the tests, emit appears to work as expected keeping the () around the a ?? b. So as to conform to the grammar requirement that:

?? cannot immediately contain, or be contained within, an && or || operation.

Hope I didn't miss the point of the tests 😅

@rbuckton or @DanielRosenwasser
One thing I noticed was that emit is using the actual identifier in the down level version of ??. So x ?? 0 becomes typeof x !== undefined && x !== null ? x : 0.

But if x comes from an import, because of the order in which the transformations are applied, we might get side effects on the access to x:

//test2.ts
let x = { y: 1, x: 0 }
let v = 0;
Object.defineProperty(x, "x", {
    get() {
        return v++;
    }
})
export = x;
//usage.ts
import { x, y } from './test2'
let xo = x ?? "NO"
console.log(xo);
let yo = y ?? "NO"
console.log(yo);

This will output 2 and 1 because let xo = x ?? "NO" becomes var xo = typeof test2_1.x !== "undefined" && test2_1.x !== null ? test2_1.x : "NO"; which will call the getter multiple times.

I would just use the approach babel uses, use a temporary variable always ex. Thoughts ?

DanielRosenwasser · 2019-08-15T16:32:59Z

I think "use a temporary unless it's an identifier" is usually the approach we take.

dragomirtitian · 2019-08-15T16:42:13Z

@DanielRosenwasser Mkay 😕, so ignore the corner case above ?

Just to make sure I was clear in what I was trying to say x ?? "NO" is seen as using an identifier in the esnext transform, but the commonJS transformation will effectively turn it into test2_1.x ?? "NO" (with the actual result being typeof test2_1.x !== "undefined" && test2_1.x !== null ? test2_1.x : "NO"; )

Nullish coalescing operator

Kingwl · 2019-08-16T07:17:08Z

@dragomirtitian
I prefer it to be a side effect (or feature) of cjs.
not only this one but also many other syntaxes.

~~But i felt assign to temp variable is better~~

Kingwl · 2019-09-20T03:42:30Z

@typescript-bot pack this.

typescript-bot · 2019-09-20T03:42:36Z

Heya @Kingwl, I've started to run the tarball bundle task on this PR at 57be95d. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-20T03:58:18Z

Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/44785/artifacts?artifactName=tgz&fileId=64FF4F06CBE74357042C0E22BEF9D252966E889A57B59FEB9CE7822F7DA3784002&fileName=/typescript-3.7.0-insiders.20190920.tgz"
    }
}

and then running npm install.

DanielRosenwasser · 2019-09-20T07:10:00Z

src/compiler/types.ts

@@ -1502,6 +1503,7 @@ namespace ts {
    export type LogicalOperator


This is actually kind of strange because it's not really a LogicalOperator. Should it be moved out?

DanielRosenwasser

Would you be able to update the API baselines?

We'd really like to merge this in by tomorrow for the beta. There seem to be only minor issues (control flow analysis not being entirely correct), but those can be fixed by the RC.

DanielRosenwasser · 2019-09-26T21:50:07Z

src/compiler/transformers/esnext.ts

                default:
                    return visitEachChild(node, visitor, context);
            }
        }
+
+        function createNotNullCondition(node: Expression) {
+            return createBinary(


Since optional chaining uses strict equality, can you switch this to strict equality?

createBinary( createBinary( temp createToken(SyntaxKind.ExclamationEqualsEqualsToken), createNull() ), createToken(SyntaxKind.AmpersandAmpersandToken), createBinary( temp, createToken(SyntaxKind.ExclamationEqualsEqualsToken), createVoidZero() ) );

(@rbuckton)

rbuckton · 2019-09-26T22:35:02Z

src/compiler/binder.ts

@@ -943,7 +943,8 @@ namespace ts {
                else {
                    return node.kind === SyntaxKind.BinaryExpression && (
                        (<BinaryExpression>node).operatorToken.kind === SyntaxKind.AmpersandAmpersandToken ||
-                        (<BinaryExpression>node).operatorToken.kind === SyntaxKind.BarBarToken);
+                        (<BinaryExpression>node).operatorToken.kind === SyntaxKind.BarBarToken ||
+                        (<BinaryExpression>node).operatorToken.kind === SyntaxKind.QuestionQuestionToken);


?? isn't really a logical operator. This results in control-flow treating the branch as something "truthy" not something defined/undefined.

I'd like to see some tests for control flow.

Kingwl · 2019-09-27T02:38:31Z

I’ll update that today

…cript into nullish-coalescing-operator

src/compiler/checker.ts

robpalme

Some trivial things.

robpalme · 2019-09-27T16:56:06Z

src/compiler/binder.ts

@@ -1435,6 +1435,14 @@ namespace ts {
            bindCondition(node.right, trueTarget, falseTarget);
        }

+        function bindNullishCollasingExpression(node: BinaryExpression, trueTarget: FlowLabel, falseTarget: FlowLabel) {


Typo: bindNullishCollasingExpression -> bindNullishCoalescingExpression

robpalme · 2019-09-27T16:57:34Z

src/compiler/binder.ts

@@ -1461,12 +1469,15 @@ namespace ts {

        function bindBinaryExpressionFlow(node: BinaryExpression) {
            const operator = node.operatorToken.kind;
-            if (operator === SyntaxKind.AmpersandAmpersandToken || operator === SyntaxKind.BarBarToken) {
+            if (operator === SyntaxKind.AmpersandAmpersandToken || operator === SyntaxKind.BarBarToken || operator === SyntaxKind.QuestionQuestionToken) {


Nit: This if-chain would be easier to read as a switch

I prefer to keep this because of so many other code similar to that.

robpalme · 2019-09-27T17:05:09Z

src/compiler/scanner.ts

@@ -1828,6 +1831,9 @@ namespace ts {
                        pos++;
                        return token = SyntaxKind.GreaterThanToken;
                    case CharacterCodes.question:
+                        if (text.charCodeAt(pos + 1) === CharacterCodes.question) {


Nit: If you re-order the pos++ before the if, the code become marginally simpler.

I prefer to be consistent with the existed code.

# Conflicts: # src/compiler/scanner.ts # src/compiler/transformers/esnext.ts # src/compiler/utilities.ts # tests/baselines/reference/api/tsserverlibrary.d.ts # tests/baselines/reference/api/typescript.d.ts

rbuckton · 2019-10-02T00:51:49Z

@Kingwl: Thanks for the contribution!

My changes were mostly to merge the control-flow behavior of nullish-coalesce with the control-flow behavior of optional chaining and to resolve merge conflicts so that this could be merged with master for the 3.7 beta. Thanks again for all of the hard work!

wsmd · 2019-11-06T03:53:14Z

Thank you all for your hard work on this and congrats on the 3.7 release!

I have a small question out of curiosity. For foo ?? bar, was document.all the reason for choosing foo !== null && foo !== void 0 as the conditional expression in the compiled output, as opposed to the abstract equality comparison foo != null? 😅

let x = (foo !== null && foo !== undefined) ? foo : bar;
// vs
let x = foo != null ? foo : bar;

Kingwl · 2019-11-06T11:21:53Z

was document.all the reason for
@wsmd
#32883 (comment)
#32883 (comment)

Kingwl changed the title ~~migrate nullish coalescing commit~~ nullish coalescing commit Aug 14, 2019

migrate nullish coalescing commit

2ef3e0f

Kingwl force-pushed the nullish-coalescing-operator branch from b33eeac to 2ef3e0f Compare August 14, 2019 08:47

Kingwl added 5 commits August 14, 2019 17:08

add more test case

91a04ed

add branch type check test

ba39ae8

add more tests

079d002

fix nullish precedence

3822fde

update public api

f67e524

sheetalkamat self-assigned this Aug 14, 2019

add rescan question question token to fix regression

2327b3b

update public api baseline

fb0d3c9

DanielRosenwasser reviewed Aug 15, 2019

View reviewed changes

dragomirtitian added 3 commits August 15, 2019 18:02

Added tests that emit for nullish coalescing operator conforming with…

06149d0

… grammar restrictions when assertions are used.

Fixed emit to hoist temporary variables (they previously went undecla…

3d94652

…red). Added tests to ensure calls and property accesses are only called once.

Merge branch 'nullish-coalescing-operator' of https://github.com/King…

65b2008

…wl/TypeScript into nullish-coalescing-operator

Merge pull request #1 from dragomirtitian/nullish-coalescing-operator

e4497b8

Nullish coalescing operator

Kingwl mentioned this pull request Aug 16, 2019

[TypeScript] Nullish coalescing highlight support microsoft/vscode#79279

Closed

Kingwl added 2 commits August 16, 2019 15:09

use not equal to null

c2e336b

rename factory

b714a01

add grammar check

323ebf0

fix review

57be95d

DanielRosenwasser reviewed Sep 20, 2019

View reviewed changes

DanielRosenwasser reviewed Sep 26, 2019

View reviewed changes

rbuckton requested changes Sep 26, 2019

View reviewed changes

Kingwl added 4 commits September 27, 2019 10:44

update baseline

78df31b

Merge branch 'nullish-coalescing-operator' of github.com:Kingwl/TypeS…

9e3852e

…cript into nullish-coalescing-operator

update emitter and more testcase

de81520

update control flow

6418943

DanielRosenwasser reviewed Sep 27, 2019

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

Kingwl added 3 commits September 27, 2019 18:18

make linter happy

a719f66

update libs

cb187cb

avoid unnecessary assert

7fd8658

robpalme reviewed Sep 27, 2019

View reviewed changes

fix typooo

6ef160f

DanielRosenwasser approved these changes Sep 27, 2019

View reviewed changes

RyanCavanaugh added this to the TypeScript 3.7.0 milestone Sep 30, 2019

rbuckton added 2 commits September 30, 2019 14:23

Merge branch 'master' into nullish-coalescing-operator

8c572b1

# Conflicts: # src/compiler/scanner.ts # src/compiler/transformers/esnext.ts # src/compiler/utilities.ts # tests/baselines/reference/api/tsserverlibrary.d.ts # tests/baselines/reference/api/typescript.d.ts

Fixes for control-flow analysis

b030f73

rbuckton approved these changes Sep 30, 2019

View reviewed changes

RyanCavanaugh merged commit 7c50bcc into microsoft:master Sep 30, 2019

j-oliveras mentioned this pull request Oct 1, 2019

Add support for Optional Chaining #33294

Merged

Kingwl deleted the nullish-coalescing-operator branch November 6, 2019 11:20

jcalz mentioned this pull request Jan 24, 2020

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nullish coalescing commit #32883

nullish coalescing commit #32883

Kingwl commented Aug 14, 2019 •

edited

AlCalzone commented Aug 14, 2019

Kingwl commented Aug 15, 2019

typescript-bot commented Aug 15, 2019 •

edited

DanielRosenwasser Aug 15, 2019

Kingwl Aug 16, 2019

DanielRosenwasser Aug 15, 2019

DanielRosenwasser left a comment

DanielRosenwasser Aug 15, 2019 •

edited

arusakov Nov 6, 2019 •

edited

DanielRosenwasser commented Aug 15, 2019 •

edited

dragomirtitian commented Aug 15, 2019

DanielRosenwasser commented Aug 15, 2019

dragomirtitian commented Aug 15, 2019

Kingwl commented Aug 16, 2019 •

edited

Kingwl commented Sep 20, 2019

typescript-bot commented Sep 20, 2019 •

edited

typescript-bot commented Sep 20, 2019

DanielRosenwasser Sep 20, 2019

DanielRosenwasser left a comment •

edited

DanielRosenwasser Sep 26, 2019

rbuckton Sep 26, 2019

Kingwl commented Sep 27, 2019

robpalme left a comment

robpalme Sep 27, 2019

robpalme Sep 27, 2019

Kingwl Sep 27, 2019

robpalme Sep 27, 2019

Kingwl Sep 27, 2019

rbuckton commented Oct 2, 2019

wsmd commented Nov 6, 2019

Kingwl commented Nov 6, 2019

		@@ -1502,6 +1503,7 @@ namespace ts {
		export type LogicalOperator

nullish coalescing commit #32883

nullish coalescing commit #32883

Conversation

Kingwl commented Aug 14, 2019 • edited

AlCalzone commented Aug 14, 2019

Kingwl commented Aug 15, 2019

typescript-bot commented Aug 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser left a comment

Choose a reason for hiding this comment

DanielRosenwasser Aug 15, 2019 • edited

Choose a reason for hiding this comment

arusakov Nov 6, 2019 • edited

Choose a reason for hiding this comment

DanielRosenwasser commented Aug 15, 2019 • edited

dragomirtitian commented Aug 15, 2019

DanielRosenwasser commented Aug 15, 2019

dragomirtitian commented Aug 15, 2019

Kingwl commented Aug 16, 2019 • edited

Kingwl commented Sep 20, 2019

typescript-bot commented Sep 20, 2019 • edited

typescript-bot commented Sep 20, 2019

Choose a reason for hiding this comment

DanielRosenwasser left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kingwl commented Sep 27, 2019

robpalme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton commented Oct 2, 2019

wsmd commented Nov 6, 2019

Kingwl commented Nov 6, 2019

Kingwl commented Aug 14, 2019 •

edited

typescript-bot commented Aug 15, 2019 •

edited

DanielRosenwasser Aug 15, 2019 •

edited

arusakov Nov 6, 2019 •

edited

DanielRosenwasser commented Aug 15, 2019 •

edited

Kingwl commented Aug 16, 2019 •

edited

typescript-bot commented Sep 20, 2019 •

edited

DanielRosenwasser left a comment •

edited