Skip to content

Commit

Permalink
Disallow let and const declarations outside blocks
Browse files Browse the repository at this point in the history
  • Loading branch information
mhegazy committed Oct 13, 2014
1 parent 778f101 commit 979d45e
Show file tree
Hide file tree
Showing 13 changed files with 1,205 additions and 26 deletions.
4 changes: 3 additions & 1 deletion src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ module ts {
var_let_or_const_expected: { code: 1152, category: DiagnosticCategory.Error, key: "'var', 'let' or 'const' expected." },
let_variable_declarations_are_only_available_when_targeting_ECMAScript_6_and_higher: { code: 1153, category: DiagnosticCategory.Error, key: "'let' variable declarations are only available when targeting ECMAScript 6 and higher." },
const_variable_declarations_are_only_available_when_targeting_ECMAScript_6_and_higher: { code: 1154, category: DiagnosticCategory.Error, key: "'const' variable declarations are only available when targeting ECMAScript 6 and higher." },
Const_must_be_intialized: { code: 1155, category: DiagnosticCategory.Error, key: "Const must be intialized." },
const_must_be_intialized: { code: 1155, category: DiagnosticCategory.Error, key: "const must be intialized." },
const_must_be_declared_inside_a_block: { code: 1156, category: DiagnosticCategory.Error, key: "const must be declared inside a block." },
let_must_be_declared_inside_a_block: { code: 1157, category: DiagnosticCategory.Error, key: "let must be declared inside a block." },
Duplicate_identifier_0: { code: 2300, category: DiagnosticCategory.Error, key: "Duplicate identifier '{0}'." },
Initializer_of_instance_member_variable_0_cannot_reference_identifier_1_declared_in_the_constructor: { code: 2301, category: DiagnosticCategory.Error, key: "Initializer of instance member variable '{0}' cannot reference identifier '{1}' declared in the constructor." },
Static_members_cannot_reference_class_type_parameters: { code: 2302, category: DiagnosticCategory.Error, key: "Static members cannot reference class type parameters." },
Expand Down
10 changes: 9 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -463,10 +463,18 @@
"category": "Error",
"code": 1154
},
"Const must be intialized.": {
"const must be intialized.": {
"category": "Error",
"code": 1155
},
"const must be declared inside a block.": {
"category": "Error",
"code": 1156
},
"let must be declared inside a block.": {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 19, 2014

Contributor

Please put single quotes around these, and consider hard coding the names 'let' and 'const'

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 19, 2014

Contributor

Also can you change "must" to "can only"?

This comment has been minimized.

Copy link
@mhegazy

mhegazy Oct 20, 2014

Author Contributor

done

"category": "Error",
"code": 1157
},

"Duplicate identifier '{0}'.": {
"category": "Error",
Expand Down
55 changes: 31 additions & 24 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2542,11 +2542,14 @@ module ts {
}

// STATEMENTS
function parseStatementAllowingLetDeclaration() {
return parseStatement(/*allowLetDeclarations*/ true);
}

function parseBlock(ignoreMissingOpenBrace: boolean, checkForStrictMode: boolean): Block {
var node = <Block>createNode(SyntaxKind.Block);
if (parseExpected(SyntaxKind.OpenBraceToken) || ignoreMissingOpenBrace) {
node.statements = parseList(ParsingContext.BlockStatements,checkForStrictMode, parseStatement);
node.statements = parseList(ParsingContext.BlockStatements, checkForStrictMode, parseStatementAllowingLetDeclaration);
parseExpected(SyntaxKind.CloseBraceToken);
}
else {
Expand Down Expand Up @@ -2592,8 +2595,8 @@ module ts {
parseExpected(SyntaxKind.OpenParenToken);
node.expression = parseExpression();
parseExpected(SyntaxKind.CloseParenToken);
node.thenStatement = parseStatement();
node.elseStatement = parseOptional(SyntaxKind.ElseKeyword) ? parseStatement() : undefined;
node.thenStatement = parseStatement(/*allowLetDeclarations*/ false);
node.elseStatement = parseOptional(SyntaxKind.ElseKeyword) ? parseStatement(/*allowLetDeclarations*/ false) : undefined;
return finishNode(node);
}

Expand All @@ -2603,7 +2606,7 @@ module ts {

var saveInIterationStatement = inIterationStatement;
inIterationStatement = ControlBlockContext.Nested;
node.statement = parseStatement();
node.statement = parseStatement(/*allowLetDeclarations*/ false);
inIterationStatement = saveInIterationStatement;

parseExpected(SyntaxKind.WhileKeyword);
Expand All @@ -2628,7 +2631,7 @@ module ts {

var saveInIterationStatement = inIterationStatement;
inIterationStatement = ControlBlockContext.Nested;
node.statement = parseStatement();
node.statement = parseStatement(/*allowLetDeclarations*/ false);
inIterationStatement = saveInIterationStatement;

return finishNode(node);
Expand Down Expand Up @@ -2692,7 +2695,7 @@ module ts {

var saveInIterationStatement = inIterationStatement;
inIterationStatement = ControlBlockContext.Nested;
forOrForInStatement.statement = parseStatement();
forOrForInStatement.statement = parseStatement(/*allowLetDeclarations*/ false);
inIterationStatement = saveInIterationStatement;

return finishNode(forOrForInStatement);
Expand Down Expand Up @@ -2803,7 +2806,7 @@ module ts {
parseExpected(SyntaxKind.OpenParenToken);
node.expression = parseExpression();
parseExpected(SyntaxKind.CloseParenToken);
node.statement = parseStatement();
node.statement = parseStatement(/*allowLetDeclarations*/ false);
node = finishNode(node);
if (isInStrictMode) {
// Strict mode code may not include a WithStatement. The occurrence of a WithStatement in such
Expand All @@ -2818,15 +2821,15 @@ module ts {
parseExpected(SyntaxKind.CaseKeyword);
node.expression = parseExpression();
parseExpected(SyntaxKind.ColonToken);
node.statements = parseList(ParsingContext.SwitchClauseStatements, /*checkForStrictMode*/ false, parseStatement);
node.statements = parseList(ParsingContext.SwitchClauseStatements, /*checkForStrictMode*/ false, parseStatementAllowingLetDeclaration);
return finishNode(node);
}

function parseDefaultClause(): CaseOrDefaultClause {
var node = <CaseOrDefaultClause>createNode(SyntaxKind.DefaultClause);
parseExpected(SyntaxKind.DefaultKeyword);
parseExpected(SyntaxKind.ColonToken);
node.statements = parseList(ParsingContext.SwitchClauseStatements, /*checkForStrictMode*/ false, parseStatement);
node.statements = parseList(ParsingContext.SwitchClauseStatements, /*checkForStrictMode*/ false, parseStatementAllowingLetDeclaration);
return finishNode(node);
}

Expand Down Expand Up @@ -2933,9 +2936,9 @@ module ts {
return token === SyntaxKind.WhileKeyword || token === SyntaxKind.DoKeyword || token === SyntaxKind.ForKeyword;
}

function parseStatementWithLabelSet(): Statement {
function parseStatementWithLabelSet(allowLetDeclarations: boolean): Statement {
labelledStatementInfo.pushCurrentLabelSet(isIterationStatementStart());
var statement = parseStatement();
var statement = parseStatement(allowLetDeclarations);
labelledStatementInfo.pop();
return statement;
}
Expand All @@ -2944,7 +2947,7 @@ module ts {
return isIdentifier() && lookAhead(() => nextToken() === SyntaxKind.ColonToken);
}

function parseLabelledStatement(): LabeledStatement {
function parseLabelledStatement(allowLetDeclarations: boolean): LabeledStatement {
var node = <LabeledStatement>createNode(SyntaxKind.LabeledStatement);
node.label = parseIdentifier();
parseExpected(SyntaxKind.ColonToken);
Expand All @@ -2956,7 +2959,7 @@ module ts {

// We only want to call parseStatementWithLabelSet when the label set is complete
// Therefore, keep parsing labels until we know we're done.
node.statement = isLabel() ? parseLabelledStatement() : parseStatementWithLabelSet();
node.statement = isLabel() ? parseLabelledStatement(allowLetDeclarations) : parseStatementWithLabelSet(allowLetDeclarations);
return finishNode(node);
}

Expand Down Expand Up @@ -3022,14 +3025,14 @@ module ts {
}
}

function parseStatement(): Statement {
function parseStatement(allowLetDeclarations: boolean): Statement {
switch (token) {
case SyntaxKind.OpenBraceToken:
return parseBlock(/* ignoreMissingOpenBrace */ false, /*checkForStrictMode*/ false);
case SyntaxKind.VarKeyword:
case SyntaxKind.LetKeyword:
case SyntaxKind.ConstKeyword:
return parseVariableStatement();
return parseVariableStatement(allowLetDeclarations);
case SyntaxKind.FunctionKeyword:
return parseFunctionDeclaration();
case SyntaxKind.SemicolonToken:
Expand Down Expand Up @@ -3063,16 +3066,12 @@ module ts {
return parseDebuggerStatement();
default:
if (isLabel()) {
return parseLabelledStatement();
return parseLabelledStatement(allowLetDeclarations);
}
return parseExpressionStatement();
}
}

function parseStatementOrFunction(): Statement {
return token === SyntaxKind.FunctionKeyword ? parseFunctionDeclaration() : parseStatement();
}

function parseAndCheckFunctionBody(isConstructor: boolean): Block {
var initialPosition = scanner.getTokenPos();
var errorCountBeforeBody = file.syntacticErrors.length;
Expand Down Expand Up @@ -3109,7 +3108,7 @@ module ts {
grammarErrorAtPos(initializerStart, initializerFirstTokenLength, Diagnostics.Initializers_are_not_allowed_in_ambient_contexts);
}
if (!inAmbientContext && !node.initializer && flags & NodeFlags.Const) {
grammarErrorOnNode(node, Diagnostics.Const_must_be_intialized);
grammarErrorOnNode(node, Diagnostics.const_must_be_intialized);
}
if (isInStrictMode && isEvalOrArgumentsIdentifier(node.name)) {
// It is a SyntaxError if a VariableDeclaration or VariableDeclarationNoIn occurs within strict code
Expand All @@ -3124,7 +3123,7 @@ module ts {
() => parseVariableDeclaration(flags, noIn), /*allowTrailingComma*/ false);
}

function parseVariableStatement(pos?: number, flags?: NodeFlags): VariableStatement {
function parseVariableStatement(allowLetDeclarations: boolean, pos?: number, flags?: NodeFlags): VariableStatement {
var node = <VariableStatement>createNode(SyntaxKind.VariableStatement, pos);
if (flags) node.flags = flags;
var errorCountBeforeVarStatement = file.syntacticErrors.length;
Expand Down Expand Up @@ -3152,6 +3151,14 @@ module ts {
grammarErrorOnNode(node, Diagnostics.const_variable_declarations_are_only_available_when_targeting_ECMAScript_6_and_higher);
}
}
else if (!allowLetDeclarations){

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 19, 2014

Contributor

Again I would guard all this by equality on the error count

if (node.flags & NodeFlags.Let) {
grammarErrorOnNode(node, Diagnostics.let_must_be_declared_inside_a_block);
}
else if (node.flags & NodeFlags.Const) {
grammarErrorOnNode(node, Diagnostics.const_must_be_declared_inside_a_block);
}
}
return node;
}

Expand Down Expand Up @@ -3784,7 +3791,7 @@ module ts {
case SyntaxKind.VarKeyword:
case SyntaxKind.LetKeyword:
case SyntaxKind.ConstKeyword:
result = parseVariableStatement(pos, flags);
result = parseVariableStatement(/*allowLetDeclarations*/ true, pos, flags);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 19, 2014

Contributor

Why is this one true? I thought let and const are only in a block, and declarations are never allowed in a block.

This comment has been minimized.

Copy link
@mhegazy

mhegazy Oct 20, 2014

Author Contributor

a module body is a function block at the end of the day. they are also allowed in the global scope

break;
case SyntaxKind.FunctionKeyword:
result = parseFunctionDeclaration(pos, flags);
Expand Down Expand Up @@ -3832,7 +3839,7 @@ module ts {
var statementStart = scanner.getTokenPos();
var statementFirstTokenLength = scanner.getTextPos() - statementStart;
var errorCountBeforeStatement = file.syntacticErrors.length;
var statement = parseStatement();
var statement = parseStatement(/*allowLetDeclarations*/ false);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 19, 2014

Contributor

This flag seems to only matter here if you are parsing a let/const/var that is not a declaration. When would that be true? I guess it would be true for a labelled statement directly parented by a module or source file, correct? It'd be worth calling out what situations would make this flag matter here.

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Oct 19, 2014

Contributor

It's set to false in cases like this:

while (expr) <parseStatement(/*allowLet:*/ false)>

This is because a let declaration here is basically worthless. Because the 'let' is scoped, you'll declare the variable, but then be unable to use it anywhere (as the 'let' will be the only statement in that scope).

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 20, 2014

Contributor

I get that, but I am asking about situations where setting it to false would matter given that we are in parseSourceElementOrModuleElement. In the while loop example, we are not calling from parseSourceElementOrModuleElement.

This comment has been minimized.

Copy link
@mhegazy

mhegazy Oct 20, 2014

Author Contributor

fixed. thanks.


if (inAmbientContext && file.syntacticErrors.length === errorCountBeforeStatement) {
grammarErrorAtPos(statementStart, statementFirstTokenLength, Diagnostics.Statements_are_not_allowed_in_ambient_contexts);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
tests/cases/compiler/constDeclarations-invalidContexts.ts(4,5): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(6,5): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(9,5): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(12,5): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(17,5): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(20,5): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(23,5): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(26,12): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(29,29): error TS1156: const must be declared inside a block.
tests/cases/compiler/constDeclarations-invalidContexts.ts(16,7): error TS2410: All symbols within a 'with' block will be resolved to 'any'.


==== tests/cases/compiler/constDeclarations-invalidContexts.ts (10 errors) ====

// Errors, const must be defined inside a block
if (true)
const c1 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.
else
const c2 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.

while (true)
const c3 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.

do
const c4 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.
while (true);

var obj;
with (obj)
~~~
!!! error TS2410: All symbols within a 'with' block will be resolved to 'any'.
const c5 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.

for (var i = 0; i < 10; i++)
const c6 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.

for (var i2 in {})
const c7 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.

if (true)
label: const c8 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.

while (false)
label2: label3: label4: const c9 = 0;
~~~~~~~~~~~~~
!!! error TS1156: const must be declared inside a block.




Loading

0 comments on commit 979d45e

Please sign in to comment.