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.

Sign up for GitHub

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

disallow recursive references for block-scoped bindings #2309

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
751b1ae
disallow recursive references for block-scoped bindings
vladima Mar 11, 2015
d3246a3
addressed PR feedback
vladima Mar 12, 2015
1ce105a
addressed PR feedback
vladima Mar 12, 2015
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
44 changes: 43 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,14 +439,56 @@ 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)) {

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?

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

succeeded

// - 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's variableDeclaration.parent? Is it the variable declaration list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I noticed parameters are not handled. Is it okay for parameters to reference themselves in their default initializers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, variable declarations are parented by variable declaration lists

// variable statement/for statement case, use site should not be inside initializer
isUsedBeforeDeclaration = isChildNode(errorLocation, variableDeclaration.initializer, container);
Copy link
Contributor

Choose a reason for hiding this comment

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

As @mhegazy said, the following case should not be an error:

let x = () => x;

}
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indentation of comment

isUsedBeforeDeclaration = isChildNode(errorLocation, (<ForInStatement>variableDeclaration.parent.parent).expression, container);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like neither of these checks handle initializers nested inside binding patterns:

let [x = x] = 0;
for (let [x = x] of []) { }

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I am not sure what is the desired behavior when a nested initializer uses a declaration from higher up in the binding pattern:

let { x, y = x } = { };
let { y = x, x } = { };
let { x, y: { z = x } } = { };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my spec reading-fu says that it should not be allowed:

let and const declarations define variables that are scoped to the running execution context’s LexicalEnvironment.
The variables are created when their containing Lexical Environment is instantiated but may not be accessed in any way until the variable’s LexicalBinding is evaluated.

and
Runtime semantic: Evaluation

LexicalBinding : BindingPattern Initializer

Let rhs be the result of evaluating Initializer.
Let value be GetValue(rhs).
ReturnIfAbrupt(value).
Let env be the running execution context’s LexicalEnvironment.
Return the result of performing BindingInitialization for BindingPattern using value and env as the arguments.

BindingInitialization step is done as a last part of evaluation of LexicalBinding, so since evaluation is not yet finished - variables cannot be accessed

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so the distinction is between BindingPattern, which is a recursive construct, and LexicalBinding, which is not. In this case, the entire LexicalBinding must precede the use.

}
if (isUsedBeforeDeclaration) {
error(errorLocation, Diagnostics.Block_scoped_variable_0_used_before_its_declaration, declarationNameToString(declaration.name));
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems very confusing to the user to call this "used before its declaration"

}
}
}
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

isDescedentNode. Child means immediate child.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually isDescendentOf. Also, I would make stopAt optional.

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 <symbol> = ...
// import <symbol> from ...
Expand Down
28 changes: 0 additions & 28 deletions src/compiler/emitter.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop

This file was deleted.

40 changes: 40 additions & 0 deletions tests/baselines/reference/recursiveLetConst.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
28 changes: 28 additions & 0 deletions tests/baselines/reference/recursiveLetConst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) {
}
11 changes: 11 additions & 0 deletions tests/cases/compiler/recursiveLetConst.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// @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) { }