Skip to content

disallow recursive references for block-scoped bindings #2309

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

Merged
merged 3 commits into from
Mar 12, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 56 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -435,18 +435,67 @@ 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");
if (!isDefinedBefore(declaration, errorLocation)) {
error(errorLocation, Diagnostics.Block_scoped_variable_0_used_before_its_declaration, declarationNameToString(declaration.name));
}
checkResolvedBlockScopedVariable(result, errorLocation);
}
}
return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move everything guarded by if (nameNotFoundMessage) into another function?


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 = <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 = 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 = (<ForInStatement | ForOfStatement>variableDeclaration.parent.parent).expression;
isUsedBeforeDeclaration = isSameScopeDescendentOf(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 or isFunctionLike(current) === true.
*/
function isSameScopeDescendentOf(initial: Node, parent: Node, stopAt: Node): boolean {
if (!parent) {
return false;
}
for (var current = initial; current && current !== stopAt && !isFunctionLike(current); current = current.parent) {
if (current === parent) {
return true;
}
}
return false;
}

// An alias symbol is created by one of the following declarations:
// import <symbol> = ...
// import <symbol> from ...
Expand Down
28 changes: 0 additions & 28 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
27 changes: 27 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,33 @@ module ts {
isCatchClauseVariableDeclaration(declaration);
}

export function getEnclosingBlockScopeContainer(node: Node): Node {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did anything change here (can't tell cuz it moved)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, code was just moved from emitter to utilities

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 &&
Expand Down
10 changes: 10 additions & 0 deletions tests/baselines/reference/for-of55.errors.txt
Original file line number Diff line number Diff line change
@@ -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;
}
12 changes: 0 additions & 12 deletions tests/baselines/reference/for-of55.types

This file was deleted.

47 changes: 47 additions & 0 deletions tests/baselines/reference/recursiveLetConst.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
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(11,11): error TS2448: Block-scoped variable 'x2' used before its declaration.


==== tests/cases/compiler/recursiveLetConst.ts (10 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.
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;}}
42 changes: 42 additions & 0 deletions tests/baselines/reference/recursiveLetConst.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//// [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) { }
let [x2 = x2] = []
let z0 = () => z0;
let z1 = function () { return z1; }
let z2 = { f() { return z2;}}

//// [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) {
}
let [x2 = x2] = [];
let z0 = () => z0;
let z1 = function () {
return z1;
};
let z2 = {
f() {
return z2;
}
};
15 changes: 15 additions & 0 deletions tests/cases/compiler/recursiveLetConst.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// @target:es6
'use strict'
let x = x + 1;
let [x1] = x1 + 1;
const y = y + 2;
const [y1] = y1 + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:
'let x = ()=>x;'

This should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about let x = (()=>x)();?

That is a false negative case: it generates no TS error, but it does crash at runtime.

That false negative is expected, and therefore it is important to test that it is indeed happening as expected.

for (let v = v; ; ) { }
for (let [v] = v; ;) { }
for (let v in 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;}}