From 751b1aee16135d74d9e33eb24c62a505a8de314d Mon Sep 17 00:00:00 2001 From: Vladimir Matveev Date: Wed, 11 Mar 2015 16:54:18 -0700 Subject: [PATCH 1/4] disallow recursive references for block-scoped bindings --- src/compiler/checker.ts | 44 ++++++++++++++++++- src/compiler/emitter.ts | 28 ------------ src/compiler/utilities.ts | 27 ++++++++++++ tests/baselines/reference/for-of55.errors.txt | 10 +++++ tests/baselines/reference/for-of55.types | 12 ----- .../reference/recursiveLetConst.errors.txt | 40 +++++++++++++++++ .../baselines/reference/recursiveLetConst.js | 28 ++++++++++++ tests/cases/compiler/recursiveLetConst.ts | 11 +++++ 8 files changed, 159 insertions(+), 41 deletions(-) create mode 100644 tests/baselines/reference/for-of55.errors.txt delete mode 100644 tests/baselines/reference/for-of55.types create mode 100644 tests/baselines/reference/recursiveLetConst.errors.txt create mode 100644 tests/baselines/reference/recursiveLetConst.js create mode 100644 tests/cases/compiler/recursiveLetConst.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index afcf12fed1..47f4a44db9 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -439,7 +439,33 @@ module ts { var declaration = forEach(result.declarations, d => isBlockOrCatchScoped(d) ? d : undefined); Debug.assert(declaration !== undefined, "Block-scoped variable declaration is undefined"); - if (!isDefinedBefore(declaration, errorLocation)) { + + // first check if usage is lexically located after the declaration + var isUsedBeforeDeclaration = !isDefinedBefore(declaration, errorLocation); + if (!isUsedBeforeDeclaration) { + // lexical check succedded however code still can be illegal. + // - block scoped variables cannot be used in its initializers + // let x = x; // illegal but usage is lexically after definition + // - in ForIn/ForOf statements variable cannot be contained in expression part + // for (let x in x) + // for (let x of x) + + // climb up to the variable declaration skipping binding patterns + var variableDeclaration = getAncestor(declaration, SyntaxKind.VariableDeclaration); + var container = getEnclosingBlockScopeContainer(variableDeclaration); + + if (variableDeclaration.parent.parent.kind === SyntaxKind.VariableStatement || + variableDeclaration.parent.parent.kind === SyntaxKind.ForStatement) { + // variable statement/for statement case, use site should not be inside initializer + isUsedBeforeDeclaration = isChildNode(errorLocation, variableDeclaration.initializer, container); + } + else if (variableDeclaration.parent.parent.kind === SyntaxKind.ForOfStatement || + variableDeclaration.parent.parent.kind === SyntaxKind.ForInStatement) { + // ForIn/ForOf case - use site should not be used in expression part + isUsedBeforeDeclaration = isChildNode(errorLocation, (variableDeclaration.parent.parent).expression, container); + } + } + if (isUsedBeforeDeclaration) { error(errorLocation, Diagnostics.Block_scoped_variable_0_used_before_its_declaration, declarationNameToString(declaration.name)); } } @@ -447,6 +473,22 @@ module ts { return result; } + /* Starting from 'initial' node walk up the parent chain until 'stopAt' node is reached. + * If at any point current node is equal to 'parent' node - return true. + * Return false if 'stopAt' node is reached. + */ + function isChildNode(initial: Node, parent: Node, stopAt: Node): boolean { + if (!parent) { + return false; + } + for (var current = initial; current && current !== stopAt; current = current.parent) { + if (current === parent) { + return true; + } + } + return false; + } + // An alias symbol is created by one of the following declarations: // import = ... // import from ... diff --git a/src/compiler/emitter.ts b/src/compiler/emitter.ts index 4b038ab1c4..3e7d64eedc 100644 --- a/src/compiler/emitter.ts +++ b/src/compiler/emitter.ts @@ -4116,34 +4116,6 @@ module ts { } } - function getEnclosingBlockScopeContainer(node: Node): Node { - var current = node; - while (current) { - if (isFunctionLike(current)) { - return current; - } - switch (current.kind) { - case SyntaxKind.SourceFile: - case SyntaxKind.CaseBlock: - case SyntaxKind.CatchClause: - case SyntaxKind.ModuleDeclaration: - case SyntaxKind.ForStatement: - case SyntaxKind.ForInStatement: - case SyntaxKind.ForOfStatement: - return current; - case SyntaxKind.Block: - // function block is not considered block-scope container - // see comment in binder.ts: bind(...), case for SyntaxKind.Block - if (!isFunctionLike(current.parent)) { - return current; - } - } - - current = current.parent; - } - } - - function getCombinedFlagsForIdentifier(node: Identifier): NodeFlags { if (!node.parent || (node.parent.kind !== SyntaxKind.VariableDeclaration && node.parent.kind !== SyntaxKind.BindingElement)) { return 0; diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 02ec5eb76a..b0e2540bdf 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -203,6 +203,33 @@ module ts { isCatchClauseVariableDeclaration(declaration); } + export function getEnclosingBlockScopeContainer(node: Node): Node { + var current = node; + while (current) { + if (isFunctionLike(current)) { + return current; + } + switch (current.kind) { + case SyntaxKind.SourceFile: + case SyntaxKind.CaseBlock: + case SyntaxKind.CatchClause: + case SyntaxKind.ModuleDeclaration: + case SyntaxKind.ForStatement: + case SyntaxKind.ForInStatement: + case SyntaxKind.ForOfStatement: + return current; + case SyntaxKind.Block: + // function block is not considered block-scope container + // see comment in binder.ts: bind(...), case for SyntaxKind.Block + if (!isFunctionLike(current.parent)) { + return current; + } + } + + current = current.parent; + } + } + export function isCatchClauseVariableDeclaration(declaration: Declaration) { return declaration && declaration.kind === SyntaxKind.VariableDeclaration && diff --git a/tests/baselines/reference/for-of55.errors.txt b/tests/baselines/reference/for-of55.errors.txt new file mode 100644 index 0000000000..aa1285afbd --- /dev/null +++ b/tests/baselines/reference/for-of55.errors.txt @@ -0,0 +1,10 @@ +tests/cases/conformance/es6/for-ofStatements/for-of55.ts(2,15): error TS2448: Block-scoped variable 'v' used before its declaration. + + +==== tests/cases/conformance/es6/for-ofStatements/for-of55.ts (1 errors) ==== + let v = [1]; + for (let v of v) { + ~ +!!! error TS2448: Block-scoped variable 'v' used before its declaration. + v; + } \ No newline at end of file diff --git a/tests/baselines/reference/for-of55.types b/tests/baselines/reference/for-of55.types deleted file mode 100644 index b0f5aab3fe..0000000000 --- a/tests/baselines/reference/for-of55.types +++ /dev/null @@ -1,12 +0,0 @@ -=== tests/cases/conformance/es6/for-ofStatements/for-of55.ts === -let v = [1]; ->v : number[] ->[1] : number[] - -for (let v of v) { ->v : any ->v : any - - v; ->v : any -} diff --git a/tests/baselines/reference/recursiveLetConst.errors.txt b/tests/baselines/reference/recursiveLetConst.errors.txt new file mode 100644 index 0000000000..99a904fe76 --- /dev/null +++ b/tests/baselines/reference/recursiveLetConst.errors.txt @@ -0,0 +1,40 @@ +tests/cases/compiler/recursiveLetConst.ts(2,9): error TS2448: Block-scoped variable 'x' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(3,12): error TS2448: Block-scoped variable 'x1' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(4,11): error TS2448: Block-scoped variable 'y' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(5,14): error TS2448: Block-scoped variable 'y1' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(6,14): error TS2448: Block-scoped variable 'v' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(7,16): error TS2448: Block-scoped variable 'v' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(8,15): error TS2448: Block-scoped variable 'v' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(9,15): error TS2448: Block-scoped variable 'v' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(10,17): error TS2448: Block-scoped variable 'v' used before its declaration. + + +==== tests/cases/compiler/recursiveLetConst.ts (9 errors) ==== + 'use strict' + let x = x + 1; + ~ +!!! error TS2448: Block-scoped variable 'x' used before its declaration. + let [x1] = x1 + 1; + ~~ +!!! error TS2448: Block-scoped variable 'x1' used before its declaration. + const y = y + 2; + ~ +!!! error TS2448: Block-scoped variable 'y' used before its declaration. + const [y1] = y1 + 1; + ~~ +!!! error TS2448: Block-scoped variable 'y1' used before its declaration. + for (let v = v; ; ) { } + ~ +!!! error TS2448: Block-scoped variable 'v' used before its declaration. + for (let [v] = v; ;) { } + ~ +!!! error TS2448: Block-scoped variable 'v' used before its declaration. + for (let v in v) { } + ~ +!!! error TS2448: Block-scoped variable 'v' used before its declaration. + for (let v of v) { } + ~ +!!! error TS2448: Block-scoped variable 'v' used before its declaration. + for (let [v] of v) { } + ~ +!!! error TS2448: Block-scoped variable 'v' used before its declaration. \ No newline at end of file diff --git a/tests/baselines/reference/recursiveLetConst.js b/tests/baselines/reference/recursiveLetConst.js new file mode 100644 index 0000000000..f34d85d86a --- /dev/null +++ b/tests/baselines/reference/recursiveLetConst.js @@ -0,0 +1,28 @@ +//// [recursiveLetConst.ts] +'use strict' +let x = x + 1; +let [x1] = x1 + 1; +const y = y + 2; +const [y1] = y1 + 1; +for (let v = v; ; ) { } +for (let [v] = v; ;) { } +for (let v in v) { } +for (let v of v) { } +for (let [v] of v) { } + +//// [recursiveLetConst.js] +'use strict'; +let x = x + 1; +let [x1] = x1 + 1; +const y = y + 2; +const [y1] = y1 + 1; +for (let v = v;;) { +} +for (let [v] = v;;) { +} +for (let v in v) { +} +for (let v of v) { +} +for (let [v] of v) { +} diff --git a/tests/cases/compiler/recursiveLetConst.ts b/tests/cases/compiler/recursiveLetConst.ts new file mode 100644 index 0000000000..c80aa6ef63 --- /dev/null +++ b/tests/cases/compiler/recursiveLetConst.ts @@ -0,0 +1,11 @@ +// @target:es6 +'use strict' +let x = x + 1; +let [x1] = x1 + 1; +const y = y + 2; +const [y1] = y1 + 1; +for (let v = v; ; ) { } +for (let [v] = v; ;) { } +for (let v in v) { } +for (let v of v) { } +for (let [v] of v) { } \ No newline at end of file From d3246a340ab2649a64fdbe292dad1f921e2f50fa Mon Sep 17 00:00:00 2001 From: Vladimir Matveev Date: Wed, 11 Mar 2015 23:49:35 -0700 Subject: [PATCH 2/4] addressed PR feedback --- src/compiler/checker.ts | 79 ++++++++++--------- .../reference/recursiveLetConst.errors.txt | 11 ++- .../baselines/reference/recursiveLetConst.js | 16 +++- tests/cases/compiler/recursiveLetConst.ts | 6 +- 4 files changed, 72 insertions(+), 40 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 47f4a44db9..2541be0d79 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -435,53 +435,60 @@ module ts { return undefined; } if (result.flags & SymbolFlags.BlockScopedVariable) { - // Block-scoped variables cannot be used before their definition - var declaration = forEach(result.declarations, d => isBlockOrCatchScoped(d) ? d : undefined); - - Debug.assert(declaration !== undefined, "Block-scoped variable declaration is undefined"); - - // first check if usage is lexically located after the declaration - var isUsedBeforeDeclaration = !isDefinedBefore(declaration, errorLocation); - if (!isUsedBeforeDeclaration) { - // lexical check succedded however code still can be illegal. - // - block scoped variables cannot be used in its initializers - // let x = x; // illegal but usage is lexically after definition - // - in ForIn/ForOf statements variable cannot be contained in expression part - // for (let x in x) - // for (let x of x) - - // climb up to the variable declaration skipping binding patterns - var variableDeclaration = getAncestor(declaration, SyntaxKind.VariableDeclaration); - var container = getEnclosingBlockScopeContainer(variableDeclaration); - - if (variableDeclaration.parent.parent.kind === SyntaxKind.VariableStatement || - variableDeclaration.parent.parent.kind === SyntaxKind.ForStatement) { - // variable statement/for statement case, use site should not be inside initializer - isUsedBeforeDeclaration = isChildNode(errorLocation, variableDeclaration.initializer, container); - } - else if (variableDeclaration.parent.parent.kind === SyntaxKind.ForOfStatement || - variableDeclaration.parent.parent.kind === SyntaxKind.ForInStatement) { - // ForIn/ForOf case - use site should not be used in expression part - isUsedBeforeDeclaration = isChildNode(errorLocation, (variableDeclaration.parent.parent).expression, container); - } - } - if (isUsedBeforeDeclaration) { - error(errorLocation, Diagnostics.Block_scoped_variable_0_used_before_its_declaration, declarationNameToString(declaration.name)); - } + checkResolvedBlockScopedVariable(result, errorLocation); } } return result; } + function checkResolvedBlockScopedVariable(result: Symbol, errorLocation: Node): void { + Debug.assert((result.flags & SymbolFlags.BlockScopedVariable) !== 0) + // Block-scoped variables cannot be used before their definition + var declaration = forEach(result.declarations, d => isBlockOrCatchScoped(d) ? d : undefined); + + Debug.assert(declaration !== undefined, "Block-scoped variable declaration is undefined"); + + // first check if usage is lexically located after the declaration + var isUsedBeforeDeclaration = !isDefinedBefore(declaration, errorLocation); + if (!isUsedBeforeDeclaration) { + // lexical check succeeded however code still can be illegal. + // - block scoped variables cannot be used in its initializers + // let x = x; // illegal but usage is lexically after definition + // - in ForIn/ForOf statements variable cannot be contained in expression part + // for (let x in x) + // for (let x of x) + + // climb up to the variable declaration skipping binding patterns + var variableDeclaration = getAncestor(declaration, SyntaxKind.VariableDeclaration); + var container = getEnclosingBlockScopeContainer(variableDeclaration); + + if (variableDeclaration.parent.parent.kind === SyntaxKind.VariableStatement || + variableDeclaration.parent.parent.kind === SyntaxKind.ForStatement) { + // variable statement/for statement case, + // use site should not be inside variable declaration (initializer of declaration or binding element) + isUsedBeforeDeclaration = isDescendentOf(errorLocation, variableDeclaration, container); + } + else if (variableDeclaration.parent.parent.kind === SyntaxKind.ForOfStatement || + variableDeclaration.parent.parent.kind === SyntaxKind.ForInStatement) { + // ForIn/ForOf case - use site should not be used in expression part + var expression = (variableDeclaration.parent.parent).expression; + isUsedBeforeDeclaration = isDescendentOf(errorLocation, expression, container); + } + } + if (isUsedBeforeDeclaration) { + error(errorLocation, Diagnostics.Block_scoped_variable_0_used_before_its_declaration, declarationNameToString(declaration.name)); + } + } + /* Starting from 'initial' node walk up the parent chain until 'stopAt' node is reached. * If at any point current node is equal to 'parent' node - return true. - * Return false if 'stopAt' node is reached. + * Return false if 'stopAt' node is reached or isFunctionLike(current) === true. */ - function isChildNode(initial: Node, parent: Node, stopAt: Node): boolean { + function isDescendentOf(initial: Node, parent: Node, stopAt: Node): boolean { if (!parent) { return false; } - for (var current = initial; current && current !== stopAt; current = current.parent) { + for (var current = initial; current && current !== stopAt && !isFunctionLike(current); current = current.parent) { if (current === parent) { return true; } diff --git a/tests/baselines/reference/recursiveLetConst.errors.txt b/tests/baselines/reference/recursiveLetConst.errors.txt index 99a904fe76..b02d0819a3 100644 --- a/tests/baselines/reference/recursiveLetConst.errors.txt +++ b/tests/baselines/reference/recursiveLetConst.errors.txt @@ -7,9 +7,10 @@ tests/cases/compiler/recursiveLetConst.ts(7,16): error TS2448: Block-scoped vari tests/cases/compiler/recursiveLetConst.ts(8,15): error TS2448: Block-scoped variable 'v' used before its declaration. tests/cases/compiler/recursiveLetConst.ts(9,15): error TS2448: Block-scoped variable 'v' used before its declaration. tests/cases/compiler/recursiveLetConst.ts(10,17): error TS2448: Block-scoped variable 'v' used before its declaration. +tests/cases/compiler/recursiveLetConst.ts(11,11): error TS2448: Block-scoped variable 'x2' used before its declaration. -==== tests/cases/compiler/recursiveLetConst.ts (9 errors) ==== +==== tests/cases/compiler/recursiveLetConst.ts (10 errors) ==== 'use strict' let x = x + 1; ~ @@ -37,4 +38,10 @@ tests/cases/compiler/recursiveLetConst.ts(10,17): error TS2448: Block-scoped var !!! error TS2448: Block-scoped variable 'v' used before its declaration. for (let [v] of v) { } ~ -!!! error TS2448: Block-scoped variable 'v' used before its declaration. \ No newline at end of file +!!! error TS2448: Block-scoped variable 'v' used before its declaration. + let [x2 = x2] = [] + ~~ +!!! error TS2448: Block-scoped variable 'x2' used before its declaration. + let z0 = () => z0; + let z1 = function () { return z1; } + let z2 = { f() { return z2;}} \ No newline at end of file diff --git a/tests/baselines/reference/recursiveLetConst.js b/tests/baselines/reference/recursiveLetConst.js index f34d85d86a..7d9aea3a75 100644 --- a/tests/baselines/reference/recursiveLetConst.js +++ b/tests/baselines/reference/recursiveLetConst.js @@ -8,7 +8,11 @@ for (let v = v; ; ) { } for (let [v] = v; ;) { } for (let v in v) { } for (let v of v) { } -for (let [v] of v) { } +for (let [v] of v) { } +let [x2 = x2] = [] +let z0 = () => z0; +let z1 = function () { return z1; } +let z2 = { f() { return z2;}} //// [recursiveLetConst.js] 'use strict'; @@ -26,3 +30,13 @@ for (let v of v) { } for (let [v] of v) { } +let [x2 = x2] = []; +let z0 = () => z0; +let z1 = function () { + return z1; +}; +let z2 = { + f() { + return z2; + } +}; diff --git a/tests/cases/compiler/recursiveLetConst.ts b/tests/cases/compiler/recursiveLetConst.ts index c80aa6ef63..9e00ae22e9 100644 --- a/tests/cases/compiler/recursiveLetConst.ts +++ b/tests/cases/compiler/recursiveLetConst.ts @@ -8,4 +8,8 @@ for (let v = v; ; ) { } for (let [v] = v; ;) { } for (let v in v) { } for (let v of v) { } -for (let [v] of v) { } \ No newline at end of file +for (let [v] of v) { } +let [x2 = x2] = [] +let z0 = () => z0; +let z1 = function () { return z1; } +let z2 = { f() { return z2;}} \ No newline at end of file From 36ac0c8f59c8488b017af132de2c71cbd911dd79 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 12 Mar 2015 10:16:28 -0700 Subject: [PATCH 3/4] Add additional asserts to ensure we don't create diagnostics with bogus positions. --- src/compiler/core.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 1fb73c9069..463473497c 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -269,8 +269,12 @@ module ts { export function createFileDiagnostic(file: SourceFile, start: number, length: number, message: DiagnosticMessage, ...args: any[]): Diagnostic; export function createFileDiagnostic(file: SourceFile, start: number, length: number, message: DiagnosticMessage): Diagnostic { + var end = start + length; + Debug.assert(start >= 0, "start must be non-negative, is " + start); Debug.assert(length >= 0, "length must be non-negative, is " + length); + Debug.assert(start <= file.text.length, `start must be within the bounds of the file. ${ start } > ${ file.text.length }`); + Debug.assert(end <= file.text.length, `end must be the bounds of the file. ${ end } > ${ file.text.length }`); var text = getLocaleSpecificMessage(message.key); From 1ce105ae4ba282e71bb3069746ca6d0088da4e25 Mon Sep 17 00:00:00 2001 From: Vladimir Matveev Date: Thu, 12 Mar 2015 13:03:40 -0700 Subject: [PATCH 4/4] addressed PR feedback --- src/compiler/checker.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 2541be0d79..be623d3b18 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -466,13 +466,13 @@ module ts { variableDeclaration.parent.parent.kind === SyntaxKind.ForStatement) { // variable statement/for statement case, // use site should not be inside variable declaration (initializer of declaration or binding element) - isUsedBeforeDeclaration = isDescendentOf(errorLocation, variableDeclaration, container); + isUsedBeforeDeclaration = isSameScopeDescendentOf(errorLocation, variableDeclaration, container); } else if (variableDeclaration.parent.parent.kind === SyntaxKind.ForOfStatement || variableDeclaration.parent.parent.kind === SyntaxKind.ForInStatement) { // ForIn/ForOf case - use site should not be used in expression part - var expression = (variableDeclaration.parent.parent).expression; - isUsedBeforeDeclaration = isDescendentOf(errorLocation, expression, container); + var expression = (variableDeclaration.parent.parent).expression; + isUsedBeforeDeclaration = isSameScopeDescendentOf(errorLocation, expression, container); } } if (isUsedBeforeDeclaration) { @@ -484,7 +484,7 @@ module ts { * If at any point current node is equal to 'parent' node - return true. * Return false if 'stopAt' node is reached or isFunctionLike(current) === true. */ - function isDescendentOf(initial: Node, parent: Node, stopAt: Node): boolean { + function isSameScopeDescendentOf(initial: Node, parent: Node, stopAt: Node): boolean { if (!parent) { return false; }