Fix 16628: "undefined" exception when name of binding element in binding pattern is empty (microsoft#17132)

yuit · weswigham · commit 39e0cc61a7da · 2017-08-09T13:47:44.000-07:00
* Handle the case where binding pattern name element is empty

* Update tests and baselines

* Feedback from PR

* Handle empty binding patterns more generally in emitter

* Dont simply handling fo empty binding patterns and stay spec compliant

* PR feedback
diff --git a/src/compiler/declarationEmitter.ts b/src/compiler/declarationEmitter.ts
@@ -1367,6 +1367,10 @@ namespace ts {
         }
 
         function writeVariableStatement(node: VariableStatement) {
+            // If binding pattern doesn't have name, then there is nothing to be emitted for declaration file i.e. const [,] = [1,2].
+            if (every(node.declarationList && node.declarationList.declarations, decl => decl.name && isEmptyBindingPattern(decl.name))) {
+                return;
+            }
             emitJsDocComments(node);
             emitModuleElementDeclarationFlags(node);
             if (isLet(node.declarationList)) {
diff --git a/src/compiler/transformers/destructuring.ts b/src/compiler/transformers/destructuring.ts
@@ -331,11 +331,14 @@ namespace ts {
                 location
             );
         }
-        else if (numElements !== 1 && (flattenContext.level < FlattenLevel.ObjectRest || numElements === 0)) {
+        else if (numElements !== 1 && (flattenContext.level < FlattenLevel.ObjectRest || numElements === 0)
+            || every(elements, isOmittedExpression)) {
             // For anything other than a single-element destructuring we need to generate a temporary
             // to ensure value is evaluated exactly once. Additionally, if we have zero elements
             // we need to emit *something* to ensure that in case a 'var' keyword was already emitted,
             // so in that case, we'll intentionally create that temporary.
+            // Or all the elements of the binding pattern are omitted expression such as "var [,] = [1,2]",
+            // then we will create temporary variable.
             const reuseIdentifierExpressions = !isDeclarationBindingElement(parent) || numElements !== 0;
             value = ensureIdentifier(flattenContext, value, reuseIdentifierExpressions, location);
         }
diff --git a/src/compiler/transformers/es2015.ts b/src/compiler/transformers/es2015.ts
@@ -2105,13 +2105,14 @@ namespace ts {
                 setCommentRange(declarationList, node);
 
                 if (node.transformFlags & TransformFlags.ContainsBindingPattern
-                    && (isBindingPattern(node.declarations[0].name)
-                        || isBindingPattern(lastOrUndefined(node.declarations).name))) {
+                    && (isBindingPattern(node.declarations[0].name) || isBindingPattern(lastOrUndefined(node.declarations).name))) {
                     // If the first or last declaration is a binding pattern, we need to modify
                     // the source map range for the declaration list.
                     const firstDeclaration = firstOrUndefined(declarations);
-                    const lastDeclaration = lastOrUndefined(declarations);
-                    setSourceMapRange(declarationList, createRange(firstDeclaration.pos, lastDeclaration.end));
+                    if (firstDeclaration) {
+                        const lastDeclaration = lastOrUndefined(declarations);
+                        setSourceMapRange(declarationList, createRange(firstDeclaration.pos, lastDeclaration.end));
+                    }
                 }
 
                 return declarationList;
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -3856,6 +3856,20 @@ namespace ts {
         return hasModifier(node, ModifierFlags.ParameterPropertyModifier) && node.parent.kind === SyntaxKind.Constructor && isClassLike(node.parent.parent);
     }
 
+    export function isEmptyBindingPattern(node: BindingName): node is BindingPattern {
+        if (isBindingPattern(node)) {
+            return every(node.elements, isEmptyBindingElement);
+        }
+        return false;
+    }
+
+    export function isEmptyBindingElement(node: BindingElement): boolean {
+        if (isOmittedExpression(node)) {
+            return true;
+        }
+        return isEmptyBindingPattern(node.name);
+    }
+
     function walkUpBindingElementsAndPatterns(node: Node): Node {
         while (node && (node.kind === SyntaxKind.BindingElement || isBindingPattern(node))) {
             node = node.parent;
diff --git a/tests/baselines/reference/bindingPatternOmittedExpressionNesting.js b/tests/baselines/reference/bindingPatternOmittedExpressionNesting.js
@@ -0,0 +1,11 @@
+//// [bindingPatternOmittedExpressionNesting.ts]
+export let [,,[,[],,[],]] = undefined as any;
+
+//// [bindingPatternOmittedExpressionNesting.js]
+"use strict";
+exports.__esModule = true;
+exports._a = (_b = undefined, _c = _b[2], _d = _c[1], _e = _c[3]);
+var _b, _c, _d, _e;
+
+
+//// [bindingPatternOmittedExpressionNesting.d.ts]
diff --git a/tests/baselines/reference/bindingPatternOmittedExpressionNesting.symbols b/tests/baselines/reference/bindingPatternOmittedExpressionNesting.symbols
@@ -0,0 +1,4 @@
+=== tests/cases/compiler/bindingPatternOmittedExpressionNesting.ts ===
+export let [,,[,[],,[],]] = undefined as any;
+>undefined : Symbol(undefined)
+
diff --git a/tests/baselines/reference/bindingPatternOmittedExpressionNesting.types b/tests/baselines/reference/bindingPatternOmittedExpressionNesting.types
@@ -0,0 +1,9 @@
+=== tests/cases/compiler/bindingPatternOmittedExpressionNesting.ts ===
+export let [,,[,[],,[],]] = undefined as any;
+> : undefined
+> : undefined
+> : undefined
+> : undefined
+>undefined as any : any
+>undefined : undefined
+
diff --git a/tests/baselines/reference/emptyAssignmentPatterns01_ES5.js b/tests/baselines/reference/emptyAssignmentPatterns01_ES5.js
@@ -2,12 +2,15 @@
 var a: any;
 
 ({} = a);
-([] = a);
+([] = a);
+
+var [,] = [1,2];
 
 //// [emptyAssignmentPatterns01_ES5.js]
 var a;
 (a);
 (a);
+var _a = [1, 2];
 
 
 //// [emptyAssignmentPatterns01_ES5.d.ts]
diff --git a/tests/baselines/reference/emptyAssignmentPatterns01_ES5.symbols b/tests/baselines/reference/emptyAssignmentPatterns01_ES5.symbols
@@ -8,3 +8,4 @@ var a: any;
 ([] = a);
 >a : Symbol(a, Decl(emptyAssignmentPatterns01_ES5.ts, 0, 3))
 
+var [,] = [1,2];
diff --git a/tests/baselines/reference/emptyAssignmentPatterns01_ES5.types b/tests/baselines/reference/emptyAssignmentPatterns01_ES5.types
@@ -14,3 +14,9 @@ var a: any;
 >[] : undefined[]
 >a : any
 
+var [,] = [1,2];
+> : undefined
+>[1,2] : [number, number]
+>1 : 1
+>2 : 2
+
diff --git a/tests/cases/compiler/bindingPatternOmittedExpressionNesting.ts b/tests/cases/compiler/bindingPatternOmittedExpressionNesting.ts
@@ -0,0 +1,2 @@
+// @declaration: true
+export let [,,[,[],,[],]] = undefined as any;
diff --git a/tests/cases/conformance/es6/destructuring/emptyAssignmentPatterns01_ES5.ts b/tests/cases/conformance/es6/destructuring/emptyAssignmentPatterns01_ES5.ts
@@ -4,4 +4,6 @@
 var a: any;
 
 ({} = a);
-([] = a);
+([] = a);
+
+var [,] = [1,2];

Original file line number	Diff line number	Diff line change
`@@ -8,3 +8,4 @@ var a: any;`
`8`	`8`	`([] = a);`
`9`	`9`	`>a : Symbol(a, Decl(emptyAssignmentPatterns01_ES5.ts, 0, 3))`
`10`	`10`
	`11`	`+var [,] = [1,2];`