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 commit #32883

Merged
merged 34 commits into from Sep 30, 2019

Conversation

@Kingwl
Copy link
Collaborator

Kingwl commented Aug 14, 2019

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:

@Kingwl Kingwl changed the title migrate nullish coalescing commit nullish coalescing commit Aug 14, 2019
@Kingwl Kingwl force-pushed the Kingwl:nullish-coalescing-operator branch from b33eeac to 2ef3e0f Aug 14, 2019
Kingwl added 5 commits Aug 14, 2019
@AlCalzone

This comment has been minimized.

Copy link

AlCalzone commented Aug 14, 2019

Is this the real life? 😍

@sheetalkamat sheetalkamat self-assigned this Aug 14, 2019
@Kingwl

This comment has been minimized.

Copy link
Collaborator Author

Kingwl commented Aug 15, 2019

@typescript-bot test this.

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Aug 15, 2019

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.

@@ -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) :

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Aug 15, 2019

Member

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

This comment has been minimized.

Copy link
@Kingwl

Kingwl Aug 16, 2019

Author Collaborator

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
@@ -3044,6 +3044,10 @@ namespace ts {
return createBinary(left, SyntaxKind.BarBarToken, right);
}

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

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Aug 15, 2019

Member

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

Copy link
Member

DanielRosenwasser left a comment

Progress looks great!


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

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Aug 15, 2019

Member

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.

This comment has been minimized.

Copy link
@arusakov

arusakov Nov 6, 2019

Contributor

@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

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Aug 15, 2019

.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.
@dragomirtitian

This comment has been minimized.

Copy link
Contributor

dragomirtitian commented Aug 15, 2019

@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

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Aug 15, 2019

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

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

dragomirtitian commented Aug 15, 2019

@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

This comment has been minimized.

Copy link
Collaborator Author

Kingwl commented Aug 16, 2019

@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

This comment has been minimized.

Copy link
Collaborator Author

Kingwl commented Sep 20, 2019

@typescript-bot pack this.

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Sep 20, 2019

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

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Sep 20, 2019

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.

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

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Sep 20, 2019

Member

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

Copy link
Member

DanielRosenwasser left a comment

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.

default:
return visitEachChild(node, visitor, context);
}
}

function createNotNullCondition(node: Expression) {
return createBinary(

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Sep 26, 2019

Member

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)

@@ -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);

This comment has been minimized.

Copy link
@rbuckton

rbuckton Sep 26, 2019

Member

?? 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

This comment has been minimized.

Copy link
Collaborator Author

Kingwl commented Sep 27, 2019

I’ll update that today

Kingwl added 4 commits Sep 27, 2019
…cript into nullish-coalescing-operator
Kingwl added 3 commits Sep 27, 2019
Copy link

robpalme left a comment

Some trivial things.

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

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

This comment has been minimized.

Copy link
@robpalme

robpalme Sep 27, 2019

Typo: bindNullishCollasingExpression -> bindNullishCoalescingExpression

@@ -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) {

This comment has been minimized.

Copy link
@robpalme

robpalme Sep 27, 2019

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

This comment has been minimized.

Copy link
@Kingwl

Kingwl Sep 27, 2019

Author Collaborator

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

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

This comment has been minimized.

Copy link
@robpalme

robpalme Sep 27, 2019

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

This comment has been minimized.

Copy link
@Kingwl

Kingwl Sep 27, 2019

Author Collaborator

I prefer to be consistent with the existed code.

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.7.0 milestone Sep 30, 2019
rbuckton added 2 commits Sep 30, 2019
# 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
@RyanCavanaugh RyanCavanaugh merged commit 7c50bcc into microsoft:master Sep 30, 2019
4 of 5 checks passed
4 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla All CLA requirements met.
Details
node10 Build #46658 succeeded
Details
node12 Build #46656 succeeded
Details
node8 Build #46657 succeeded
Details
@Kingwl

This comment has been minimized.

Copy link
Collaborator

Kingwl commented on b030f73 Oct 1, 2019

Oh......Thanks

@rbuckton

This comment has been minimized.

Copy link
Member

rbuckton commented Oct 2, 2019

@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

This comment has been minimized.

Copy link

wsmd commented Nov 6, 2019

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 Kingwl deleted the Kingwl:nullish-coalescing-operator branch Nov 6, 2019
@Kingwl

This comment has been minimized.

Copy link
Collaborator Author

Kingwl commented Nov 6, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.