-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Disallow let\const declarations in the same scope with var declarations. #2027
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
Changes from all commits
a9df539
07dbd30
e5d80db
3d26fbc
941728d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8198,32 +8198,61 @@ module ts { | |
} | ||
} | ||
|
||
function checkCollisionWithConstDeclarations(node: VariableLikeDeclaration) { | ||
function checkVarDeclaredNamesNotShadowed(node: VariableDeclaration | BindingElement) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename this? |
||
// - ScriptBody : StatementList | ||
// It is a Syntax Error if any element of the LexicallyDeclaredNames of StatementList | ||
// also occurs in the VarDeclaredNames of StatementList. | ||
|
||
// - Block : { StatementList } | ||
// It is a Syntax Error if any element of the LexicallyDeclaredNames of StatementList | ||
// also occurs in the VarDeclaredNames of StatementList. | ||
|
||
// Variable declarations are hoisted to the top of their function scope. They can shadow | ||
// block scoped declarations, which bind tighter. this will not be flagged as duplicate definition | ||
// by the binder as the declaration scope is different. | ||
// A non-initialized declaration is a no-op as the block declaration will resolve before the var | ||
// declaration. the problem is if the declaration has an initializer. this will act as a write to the | ||
// block declared value. this is fine for let, but not const. | ||
// | ||
// Only consider declarations with initializers, uninitialized var declarations will not | ||
// step on a const variable. | ||
// step on a let/const variable. | ||
// Do not consider let and const declarations, as duplicate block-scoped declarations | ||
// are handled by the binder. | ||
// We are only looking for var declarations that step on const declarations from a | ||
// We are only looking for var declarations that step on let\const declarations from a | ||
// different scope. e.g.: | ||
// var x = 0; | ||
// { | ||
// const x = 0; | ||
// var x = 0; | ||
// const x = 0; // localDeclarationSymbol obtained after name resolution will correspond to this declaration | ||
// var x = 0; // symbol for this declaration will be 'symbol' | ||
// } | ||
if (node.initializer && (getCombinedNodeFlags(node) & NodeFlags.BlockScoped) === 0) { | ||
var symbol = getSymbolOfNode(node); | ||
if (symbol.flags & SymbolFlags.FunctionScopedVariable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can a block scoped declaration have a function scoped symbol? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh sorry didn't see the === 0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So when would this not be true? |
||
var localDeclarationSymbol = resolveName(node, (<Identifier>node.name).text, SymbolFlags.Variable, /*nodeNotFoundErrorMessage*/ undefined, /*nameArg*/ undefined); | ||
if (localDeclarationSymbol && localDeclarationSymbol !== symbol && localDeclarationSymbol.flags & SymbolFlags.BlockScopedVariable) { | ||
if (getDeclarationFlagsFromSymbol(localDeclarationSymbol) & NodeFlags.Const) { | ||
error(node, Diagnostics.Cannot_redeclare_block_scoped_variable_0, symbolToString(localDeclarationSymbol)); | ||
if (localDeclarationSymbol && | ||
localDeclarationSymbol !== symbol && | ||
localDeclarationSymbol.flags & SymbolFlags.BlockScopedVariable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a code example where the relevant declarations are labelled with 'localDeclarationSymbol' and 'symbol' |
||
if (getDeclarationFlagsFromSymbol(localDeclarationSymbol) & NodeFlags.BlockScoped) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When would this not be true? You already checked the symbol flags |
||
|
||
var varDeclList = getAncestor(localDeclarationSymbol.valueDeclaration, SyntaxKind.VariableDeclarationList); | ||
var container = | ||
varDeclList.parent.kind === SyntaxKind.VariableStatement && | ||
varDeclList.parent.parent; | ||
|
||
// names of block-scoped and function scoped variables can collide only | ||
// if block scoped variable is defined in the function\module\source file scope (because of variable hoisting) | ||
var namesShareScope = | ||
container && | ||
(container.kind === SyntaxKind.Block && isAnyFunction(container.parent) || | ||
(container.kind === SyntaxKind.ModuleBlock && container.kind === SyntaxKind.ModuleDeclaration) || | ||
container.kind === SyntaxKind.SourceFile); | ||
|
||
// here we know that function scoped variable is shadowed by block scoped one | ||
// if they are defined in the same scope - binder has already reported redeclaration error | ||
// otherwise if variable has an initializer - show error that initialization will fail | ||
// since LHS will be block scoped name instead of function scoped | ||
if (!namesShareScope) { | ||
var name = symbolToString(localDeclarationSymbol); | ||
error(getErrorSpanForNode(node), Diagnostics.Cannot_initialize_outer_scoped_variable_0_in_the_same_scope_as_block_scoped_declaration_1, name, name); | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -8320,7 +8349,9 @@ module ts { | |
if (node.kind !== SyntaxKind.PropertyDeclaration && node.kind !== SyntaxKind.PropertySignature) { | ||
// We know we don't have a binding pattern or computed name here | ||
checkExportsOnMergedDeclarations(node); | ||
checkCollisionWithConstDeclarations(node); | ||
if (node.kind === SyntaxKind.VariableDeclaration || node.kind === SyntaxKind.BindingElement) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What else could it be? We already checked that PropertyDeclaration and PropertySignature are false There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i.e. EnumMember or Parameter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please note that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, but what if the binding element is in the parameter? Then you want to skip it, right? |
||
checkVarDeclaredNamesNotShadowed(<VariableDeclaration | BindingElement>node); | ||
} | ||
checkCollisionWithCapturedSuperVariable(node, <Identifier>node.name); | ||
checkCollisionWithCapturedThisVariable(node, <Identifier>node.name); | ||
checkCollisionWithRequireExportsInGeneratedCode(node, <Identifier>node.name); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
tests/cases/compiler/letAndVarRedeclaration.ts(2,5): error TS2451: Cannot redeclare block-scoped variable 'e0'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(3,5): error TS2451: Cannot redeclare block-scoped variable 'e0'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(4,10): error TS2451: Cannot redeclare block-scoped variable 'e0'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(7,9): error TS2451: Cannot redeclare block-scoped variable 'x1'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(8,9): error TS2451: Cannot redeclare block-scoped variable 'x1'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(9,14): error TS2451: Cannot redeclare block-scoped variable 'x1'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(13,9): error TS2451: Cannot redeclare block-scoped variable 'x'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(15,13): error TS2451: Cannot redeclare block-scoped variable 'x'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(18,18): error TS2451: Cannot redeclare block-scoped variable 'x'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(23,9): error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(24,9): error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(25,14): error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(29,9): error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(31,13): error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(34,18): error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(38,5): error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(39,10): error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(43,9): error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(44,14): error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(49,9): error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
tests/cases/compiler/letAndVarRedeclaration.ts(50,14): error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
|
||
|
||
==== tests/cases/compiler/letAndVarRedeclaration.ts (21 errors) ==== | ||
|
||
let e0 | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'e0'. | ||
var e0; | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'e0'. | ||
function e0() { } | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'e0'. | ||
|
||
function f0() { | ||
let x1; | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x1'. | ||
var x1; | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x1'. | ||
function x1() { } | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x1'. | ||
} | ||
|
||
function f1() { | ||
let x; | ||
~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x'. | ||
{ | ||
var x; | ||
~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x'. | ||
} | ||
{ | ||
function x() { } | ||
~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x'. | ||
} | ||
} | ||
|
||
module M0 { | ||
let x2; | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
var x2; | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
function x2() { } | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
} | ||
|
||
module M1 { | ||
let x2; | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
{ | ||
var x2; | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
} | ||
{ | ||
function x2() { } | ||
~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x2'. | ||
} | ||
} | ||
|
||
let x11; | ||
~~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
for (var x11; ;) { | ||
~~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
} | ||
|
||
function f2() { | ||
let x11; | ||
~~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
for (var x11; ;) { | ||
~~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
} | ||
} | ||
|
||
module M2 { | ||
let x11; | ||
~~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
for (var x11; ;) { | ||
~~~ | ||
!!! error TS2451: Cannot redeclare block-scoped variable 'x11'. | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
//// [letAndVarRedeclaration.ts] | ||
|
||
let e0 | ||
var e0; | ||
function e0() { } | ||
|
||
function f0() { | ||
let x1; | ||
var x1; | ||
function x1() { } | ||
} | ||
|
||
function f1() { | ||
let x; | ||
{ | ||
var x; | ||
} | ||
{ | ||
function x() { } | ||
} | ||
} | ||
|
||
module M0 { | ||
let x2; | ||
var x2; | ||
function x2() { } | ||
} | ||
|
||
module M1 { | ||
let x2; | ||
{ | ||
var x2; | ||
} | ||
{ | ||
function x2() { } | ||
} | ||
} | ||
|
||
let x11; | ||
for (var x11; ;) { | ||
} | ||
|
||
function f2() { | ||
let x11; | ||
for (var x11; ;) { | ||
} | ||
} | ||
|
||
module M2 { | ||
let x11; | ||
for (var x11; ;) { | ||
} | ||
} | ||
|
||
//// [letAndVarRedeclaration.js] | ||
let e0; | ||
var e0; | ||
function e0() { } | ||
function f0() { | ||
let x1; | ||
var x1; | ||
function x1() { } | ||
} | ||
function f1() { | ||
let x; | ||
{ | ||
var x; | ||
} | ||
{ | ||
function x() { } | ||
} | ||
} | ||
var M0; | ||
(function (M0) { | ||
let x2; | ||
var x2; | ||
function x2() { } | ||
})(M0 || (M0 = {})); | ||
var M1; | ||
(function (M1) { | ||
let x2; | ||
{ | ||
var x2; | ||
} | ||
{ | ||
function x2() { } | ||
} | ||
})(M1 || (M1 = {})); | ||
let x11; | ||
for (var x11;;) { | ||
} | ||
function f2() { | ||
let x11; | ||
for (var x11;;) { | ||
} | ||
} | ||
var M2; | ||
(function (M2) { | ||
let x11; | ||
for (var x11;;) { | ||
} | ||
})(M2 || (M2 = {})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you are saying that you want vars and lets to go to one place, because you want them to collide. The choice of function locals as opposed to block locals is arbitrary, it just has to be the same for both declaration kinds. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i'd add a comment to that effect. I think using hte function as the location makes sense though, as that's where we put parameters. (we should also add tests for parameters/locals/lets (if they collide or not))