Skip to content

Commit

Permalink
addressed PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@vladima
vladima committed Mar 12, 2015
1 parent 751b1ae commit d3246a3
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 37 deletions.
73 changes: 40 additions & 33 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
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");
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)
// 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);
// 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 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, (<ForInStatement>variableDeclaration.parent.parent).expression, container);
}
}
if (isUsedBeforeDeclaration) {
error(errorLocation, Diagnostics.Block_scoped_variable_0_used_before_its_declaration, declarationNameToString(declaration.name));
}
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)

This comment has been minimized.

Sign in to view

Copy link
@mihailik

mihailik Mar 12, 2015

Contributor

This is valid:

let x = function() { return x; }

Will this code now report a bug in that case?

This comment has been minimized.

Sign in to view

Copy link
@mihailik

mihailik Mar 12, 2015

Contributor

Hm, it looks as though even this is a valid JavaScript:

function ro() {
"use strict";
let x = Date.now() < 0 ? x : 10;
return x;
}

ro() // returns 10 in Chrome and FireFox

This comment has been minimized.

Sign in to view

Copy link
@mihailik

mihailik Mar 12, 2015

Contributor

Here's a dummy page with the sample code above and a couple of lines to dump the result or an error:

https://rawgit.com/mihailik/bac773c6cd2b5b7e35a2/raw/2fcbd90c2137a14f4af89ce39dda5f359078ebda/index.html

Passed on:

  • Chrome 41 prints 10
  • FireFox 24 prints 10
  • FireFox 36 prints 10
  • IE11 via modern.ie screenshot service prints 10

No ES6, failing on:

  • Opera 12 (modern.ie) expected expression got reserved word "let"
  • Safari/OSX (modern.ie) "Use of reserved word 'let' in strict mode eval***
  • iOS (modern.ie) Unexpected use of reserved word 'let' in strict mode eval*
  • Older V8 (Android WebKit (modern.ie), Chrome 36 (modern.ie), node v0.13-pre) Unexpected strict mode reserved word

However all the ES6-enabled browsers agree on let x = Date.now() < 0 ? x : 10 being a valid JavaScript.

This comment has been minimized.

Sign in to view

Copy link
@DickvdBrink

DickvdBrink Mar 12, 2015

Contributor

@mihailik True but if you change it to let x = Date.now() > 0 ? x : 10 (so I changed it to > you get an error in at least Chrome (ReferenceError: x is not defined)

This comment has been minimized.

Sign in to view

Copy link
@mihailik

mihailik Mar 12, 2015

Contributor

Upon further investigation, the let recursivity failures are always triggered at the moment of actual use of the corresponding variable.

If the variable used in the middle of an expression, the expression gets evaluated to the point of the variable, then the error is thrown.

Which means TS should not report a static error in this recursive case, even let x = x is no more compiler error than throw new Error('failure') is. Might be sensible to report a warning, but not an error.

This comment has been minimized.

Sign in to view

Copy link
@mihailik

mihailik Mar 12, 2015

Contributor

Or consider another one. Type it in the browser console:

function ro() {
  "use strict";
  let x = (function oh_my_this_is_bad() {
    debugger;
    return x;
  })();
  return x;
}

Then run: ro().

The debugger will stop you right before x is used, and you can try to access x:

x
> Uncaught ReferenceError: x is not defined

But then if you do x = 10 and continue, there will not be any errors thrown after all!

Clearly this is a runtime error, not compile time. We did not change the code, only modified the data in debugger -- yet the error went away.
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 = (<ForInStatement>variableDeclaration.parent.parent).expression;

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Mar 12, 2015

Contributor

ForInStatement | ForOfStatement
isUsedBeforeDeclaration = isDescendentOf(errorLocation, expression, container);
}
}
return result;
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 {

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Mar 12, 2015

Contributor

isSameScopeDescendentOf
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;
}
Expand Down
11 changes: 9 additions & 2 deletions tests/baselines/reference/recursiveLetConst.errors.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
~
Expand Down Expand Up @@ -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.
!!! 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;}}
16 changes: 15 additions & 1 deletion tests/baselines/reference/recursiveLetConst.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
}
};
6 changes: 5 additions & 1 deletion tests/cases/compiler/recursiveLetConst.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) { }
for (let [v] of v) { }
let [x2 = x2] = []
let z0 = () => z0;
let z1 = function () { return z1; }
let z2 = { f() { return z2;}}

0 comments on commit d3246a3

Please sign in to comment.