Fix 16628: "undefined" exception when name of binding element in binding pattern is empty #17132
Conversation
src/compiler/declarationEmitter.ts
Outdated
| @@ -1367,6 +1367,11 @@ 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 (node.declarationList.declarations.length && isBindingPattern(node.declarationList.declarations[0].name) && | |||
There was a problem hiding this comment.
This needs to be a recursive check, as binding patterns can nest infinitely, and of not just omitted expressions, but omitted expressions or binding patterns containing only the aforementioned.
Also, a test for that:
// @declaration: true
export let [,,[,[],,[],]] = undefined as any;| @@ -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, (element) => isOmittedExpression(element))) { | |||
There was a problem hiding this comment.
This... might? Need to be recursive? Depends on how the transform works.
There was a problem hiding this comment.
Answer: No. To be spec compliant, we still emit temporaries in js for empty binding patterns in order to attempt to get the elements for the sub-binding patterns/invoke their side-effects (and potentially error if they do not exist).
src/compiler/transformers/es2015.ts
Outdated
| // 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 && lastDeclaration) { |
There was a problem hiding this comment.
We should probably bail much before here - up before createVariableDeclarationList if declarations is empty.
src/compiler/transformers/es2015.ts
Outdated
| if (node.transformFlags & TransformFlags.ContainsBindingPattern | ||
| && (isBindingPattern(node.declarations[0].name) | ||
| || isBindingPattern(lastOrUndefined(node.declarations).name))) { | ||
| const firstDeclaration = firstOrUndefined(declarations); |
There was a problem hiding this comment.
Why look up the declarations before checking if there is work to do? If we need to ensure firstDeclaration is not undefined, add another if inside of the existing block.
Also, you shouldn't need to check both firstDeclaration and lastDeclaration as both will be undefined if declarations.length === 0 and both will have values if declarations.length > 0.
| @@ -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, (element) => isOmittedExpression(element))) { | |||
There was a problem hiding this comment.
Can we just use every(elements, isOmittedExpression)? No need to introduce a new function here.
|
@rbuckton Done |
…ing pattern is empty (microsoft#17132) * 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
Fix #16628