X Tutup
The Wayback Machine - https://web.archive.org/web/20200805032732/https://github.com/Microsoft/TypeScript/pull/904
Skip to content

/ TypeScript

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

Let and const support #904

Merged
merged 30 commits into from Oct 24, 2014
Merged

Let and const support #904

merged 30 commits into from Oct 24, 2014
+5,766 −274

Conversation

@mhegazy
Copy link

mhegazy commented Oct 16, 2014

This change adds support for ES6-style block-scoped variable declarations. these are only available when targeting ES6. the change also adds a new target for ES6.

There are two variants, let and const. Generaly let and const behave like a var declarations with the following exceptions:

  • Both let and const can not be re-declared:
let a = 0;
a = 1;
let a; // Error: redeclaration
  • It is an error to re-declare a block-scoped variable as 'var' in the same scope:
let a = 0;
{
     var a; // Error: redeclaration, as var is hoisted to the top of the scope, and re-declares a
}
  • Let and const can not be declared in a non-guarded statements
if (true) {
    let a = 0; // OK
}

if (false) 
   let b = 0; // Error

for (let i = 0; i< 10; i++)  // OK
    console.log(i);
  • Let and const declarations are block scoped, and not hoisted to the top of the function like other JS declarations:
let a = 0;
{
    let a = "local";
    console.log(a) // local
}
console.log(a); // 0
  • Let and const can not be used before they are defined, to avoid the lexical dead zone of vars.
v = 2; // ok
var v;

a = 2;
let a; // Error, used before initialization
  • Const declarations must have an initializer, unless in ambient context
  • It is an error to write to a Const
const c = 0;
console.log(c); // OK: 0

c = 2; // Error
c++; // Error

{
    const c2 = 0;
    var c2 = 0; // not a redeclaration, as the var is hoisted out, but still a write to c2
}
  • Let and const can not be exported, only vars are allowed to.

What is left:

  • Block emit if any of the let/const errors are reported, these may be syntactic, binding, or semantic errors
  • Wire in Test262 for parser verification to ensure we are ES6 complaint
mhegazy added 15 commits Oct 11, 2014
@mhegazy
Add es6 target
873c1df
@mhegazy
Add basic parsing support for let and const
778f101
@mhegazy
Disallow let and const declarations outside blocks
979d45e
@mhegazy
Fix line endings
6f6f4af
@mhegazy
Add binder support for block scoped variable declarations
cf89f5c
@mhegazy
Do not allow use of block-scoped variable before its definition
1dde985
@mhegazy
Ensure duplicate let/const declarations accross files are reported
318575c
@mhegazy
Report duplicate identifier errors on all locations for merged declar…
cffc62a 
…ations to align with local declarations
@mhegazy
Flag assignments to a const
f5c2740
@mhegazy
Flag const declarations shodowed by var redeclarations
82f5fb4
@mhegazy
Allow const in for statements
3e45601
@mhegazy
Do not allow let and const declarations to be exported from a module
03a100d
@mhegazy
Fix emitting for const in for loops
Loading status checks…
6154923
@mhegazy
Merge branch 'master' into letAndConst
e15f4e6 
Conflicts:
	src/compiler/types.ts
@mhegazy
Add language service support for const
Loading status checks…
60bb37b
@JsonFreeman

This comment has been minimized.

Sign in to view
Copy link
Contributor

JsonFreeman commented on src/services/services.ts in 60bb37b Oct 16, 2014

There should only be one declaration, right?

This comment has been minimized.

Sign in to view
Copy link
Author

mhegazy replied Oct 16, 2014

yes. thanks.

This comment has been minimized.

Sign in to view
Copy link
Author

mhegazy replied Oct 17, 2014

done.
src/compiler/parser.ts
@@ -2827,7 +2848,7 @@ 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);

This comment has been minimized.

Sign in to view
Copy link
@CyrusNajmabadi

CyrusNajmabadi Oct 16, 2014

Contributor

i thought we would not allow 'let' here as we're not in a block scope. or do you check for that later?

This comment has been minimized.

Sign in to view
Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

i think you are correct. the ES6 spec makes it clear that lexical scopes do not include switch, try or finally blocks, section 8.1:

A Lexical Environment is a specification type used to define the association of Identifiers to specific variables and functions based upon the lexical nesting structure of ECMAScript code. A Lexical Environment consists of an Environment Record and a possibly null reference to an outer Lexical Environment. Usually a Lexical Environment is associated with some specific syntactic structure of ECMAScript code such as a FunctionDeclaration, a BlockStatement, or a Catch clause of a TryStatement and a new Lexical Environment is created each time such code is evaluated.

I will follow up to see if this is intentional or just an error in the spec and update this accordingly.

This comment has been minimized.

Sign in to view
Copy link
@JsonFreeman

JsonFreeman Oct 17, 2014

Contributor

They say "such as", which I normally interpret as "including but not limited to".

This comment has been minimized.

Sign in to view
Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

yeah. checked with @bterlson and he confirmed that that is not the intention. so keeping Switch, try and finally blocks as valid lexical scopes
src/compiler/parser.ts Outdated
@@ -2953,7 +2974,7 @@ module ts {
return isIdentifier() && lookAhead(() => nextToken() === SyntaxKind.ColonToken);
}

function parseLabelledStatement(): LabeledStatement {
function parseLabelledStatement(allowLetDeclarations: boolean): LabeledStatement {

This comment has been minimized.

Sign in to view
Copy link
@CyrusNajmabadi

CyrusNajmabadi Oct 16, 2014

Contributor

parseLabeledStatement

This comment has been minimized.

Sign in to view
Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

done, though it is still correct :)
src/compiler/types.ts Outdated
@@ -790,7 +793,8 @@ module ts {
Signature = CallSignature | ConstructSignature | IndexSignature,

ParameterExcludes = Value,
VariableExcludes = Value & ~Variable,
VariableExcludes = (Value | BlockScoped) & ~Variable,

This comment has been minimized.

Sign in to view
Copy link
@CyrusNajmabadi

CyrusNajmabadi Oct 16, 2014

Contributor

can you comment what thsi means. i.e. give an intutive explanation like "vars don't exclude vars with teh same name. but blah blah blah."

This comment has been minimized.

Sign in to view
Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

done

This comment has been minimized.

Sign in to view
Copy link
@JsonFreeman

JsonFreeman Oct 17, 2014

Contributor

Why doesn't Value include BlockScoped?

This comment has been minimized.

Sign in to view
Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

cause blockScope is not a thing by itself. so you can be a variable and blockscopes, may be you can be a class and bockscopped in the future.

This comment has been minimized.

Sign in to view
Copy link
@JsonFreeman

JsonFreeman Oct 17, 2014

Contributor

This looks like vars and lets can coexist though. Did you mean (Value & ~Variable) | BlockScoped?

This comment has been minimized.

Sign in to view
Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

Let is variable and blockscoped.
src/harness/harness.ts Outdated
@@ -685,6 +685,8 @@ module Harness {
options.target = ts.ScriptTarget.ES3;
} else if (setting.value.toLowerCase() === 'es5') {
options.target = ts.ScriptTarget.ES5;
} else if (setting.value.toLowerCase() === 'es6') {
options.target = ts.ScriptTarget.ES6;

This comment has been minimized.

Sign in to view
Copy link
@CyrusNajmabadi

CyrusNajmabadi Oct 16, 2014

Contributor

indentation.

This comment has been minimized.

Sign in to view
Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

done
@ahejlsberg

This comment has been minimized.

Sign in to view
Copy link
Member

ahejlsberg commented on src/compiler/checker.ts in 82f5fb4 Oct 17, 2014

What about this:

function outer() {
    const x = 0;
    function inner() {
        var x = 0;
    }
}

Looks to me like this will be reported as an error, but I'm not sure it should be. You probably should limit the lookup to just the current function, but I haven't carefully checked the ES6 spec.

This comment has been minimized.

Sign in to view
Copy link
Author

mhegazy replied Oct 17, 2014

you are absolutely right. will fix it. thanks.
@ahejlsberg

This comment has been minimized.

Sign in to view
Copy link
Member

ahejlsberg commented on src/compiler/types.ts in 873c1df Oct 17, 2014

Awesome that we now officially have an ES6 target. We'll need to update our class and arrow function emit to not rewrite these constructs in ES6 mode. Once this goes in we should make sure to file bugs to that effect.

This comment has been minimized.

Sign in to view
Copy link
Author

mhegazy replied Oct 17, 2014

yes. that is the intention.
@ahejlsberg

This comment has been minimized.

Sign in to view
Copy link
Member

ahejlsberg commented on src/compiler/checker.ts in cffc62a Oct 17, 2014

I like that we're now doing this. So, if for example I declare my own Array<T> with a length property I would see a duplicate error both on my declaration and the one in lib.d.ts, right? I can't tell from the tests below.

Should we also be doing this for declarations that are merged during local binding?

This comment has been minimized.

Sign in to view
Copy link
Author

mhegazy replied Oct 17, 2014

yes. @yuit checked in a fix for the local declarations. this is to ensure we are getting the same behavior while merging across files. so errors will be reported in lib.d.ts for a module "Array" and for redefining a property "length" in interface "Array"
@ahejlsberg

This comment has been minimized.

Sign in to view
Copy link
Member

ahejlsberg commented on src/compiler/parser.ts in 3e45601 Oct 17, 2014

So ES6 permits a const declaration in a for statement, but it is still an error to re-assign the const, right? Not sure why you'd ever want to do that, but if that's what ES6 says then so be it.

This comment has been minimized.

Sign in to view
Copy link
Author

mhegazy replied Oct 17, 2014

the only benefit is getting a fresh binding for every iteration, so it can be captured, but not changed later on. I could not come up with a meaningful use case that requires a new binding and read-only that is not identical to:

while (true) {
    const c = 0;
    ....
}
@ahejlsberg
Copy link
Member

ahejlsberg commented Oct 17, 2014

Overall looks great!
mhegazy added 4 commits Oct 17, 2014
@mhegazy
Fix search for shadowed const declarations by a var declarations to s…
fd469d6 
…earch for any variable instead of only a blockScoped one to ensure we are not picking it up from a wrong scope.
@mhegazy
Respond to code review comments
4ef68b9
@mhegazy
Allow const and let declarations to be exported in modules. Also ensu…
a5a6c6f 
…re that const module elements are not used as references.
@mhegazy
Treat blockScoped variable declarations as a separate category when i…
Loading status checks…
0a59cdd 
…t comes to symbol flags, instead of compining BlockScoped and Variable
@JsonFreeman

This comment has been minimized.

Sign in to view
Copy link
Contributor

JsonFreeman commented on src/compiler/diagnosticMessages.json in 873c1df Oct 19, 2014

The casing is not consistent between these two messages (6015 and 6047)
mhegazy added 2 commits Oct 20, 2014
@mhegazy
Merge branch 'master' into letAndConst
0e7d8b6
@mhegazy
Update error messages
dd5c89d
@JsonFreeman

This comment has been minimized.

Sign in to view
Copy link
Contributor

JsonFreeman commented on src/compiler/binder.ts in cf89f5c Oct 20, 2014

I would collapse this case with the case above. They have exactly the same code
@JsonFreeman

This comment has been minimized.

Sign in to view
Copy link
Contributor

JsonFreeman commented on src/compiler/binder.ts in cf89f5c Oct 20, 2014

Does this flag ever matter? A let/const would make the module instantiated, no?
@JsonFreeman

This comment has been minimized.

Sign in to view
Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 1dde985 Oct 20, 2014

"Block"
@JsonFreeman

This comment has been minimized.

Sign in to view
Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 1dde985 Oct 20, 2014

Why errorLocation and not location?

This comment has been minimized.

Sign in to view
Copy link
Author

mhegazy replied Oct 20, 2014

location changes later on. errorlocation is the one we got initally

This comment has been minimized.

Sign in to view
Copy link
Contributor

JsonFreeman replied Oct 20, 2014

Ok, please comment that.
@JsonFreeman

This comment has been minimized.

Sign in to view
Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 1dde985 Oct 20, 2014

I prefer:

if (declarationSourceFile === referenceSourceFile) {
    if (declaration.pos > errorLocation.pos) {
        ...
    }
}
else if (compilerOptions.out) {
    ...
}

So you don't have to worry about the compilerOptions.out case if it was in the same file.
mhegazy added 9 commits Oct 20, 2014
@mhegazy
Simplify the binder logic for managing blockScopeContainer
91f4098
@mhegazy
allow let and const declarations in module bodies under labels
d5fe43b
@mhegazy
Create a new flag for diagnostics 'isEarly' and disable emit if this …
Loading status checks…
dd7ca69 
…flag is set. Set the flag by default on all let and const errors to ensure we are not emitting invalid JS code.
@mhegazy
respond to code review comments
Loading status checks…
373dc76
@mhegazy
Merge branch 'master' into letAndConst
Loading status checks…
9353c11 
Conflicts:
	src/compiler/types.ts
@mhegazy
Merge branch 'master' into letAndConst
Loading status checks…
d1858d0
@mhegazy
Ensure let and const declarations in labels are parsed correctelly
e4a2084
@mhegazy
Only check for collisions with variabels and not properties
67c78a2
@mhegazy
Merge branch 'master' into letAndConst
Loading status checks…
51e101c
mhegazy added a commit that referenced this pull request Oct 24, 2014
@mhegazy
Merge pull request #904 from Microsoft/letAndConst
Loading status checks…
290e43b 
Let and const support
@mhegazy mhegazy merged commit 290e43b into master Oct 24, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@mhegazy mhegazy deleted the letAndConst branch Oct 24, 2014
This was referenced Oct 25, 2014
Closed
Closed
@NoelAbrahams NoelAbrahams mentioned this pull request Jan 12, 2015
Closed
@KirillGrishin

This comment has been minimized.

Sign in to view
Copy link

KirillGrishin commented on tests/baselines/reference/es6-amd.js in 873c1df Mar 11, 2015

Do I understand correctly that currently (TS1.4.1) even with target ES6 the output will not be ES6 classes?

This comment has been minimized.

Sign in to view
Copy link
Member

ahejlsberg replied Mar 11, 2015

That is correct, but 1.5 will support proper ES6 output (i.e. output a class as a class).
@adidahiya adidahiya mentioned this pull request Apr 20, 2015
Closed
@nickie
Copy link

nickie commented Feb 1, 2016

TypeScript's specification seems to allow a missing initializer for destructuring lexical bindings, whereas the EC262 specification does not. Is this intentional? In that case, I believe it should only be allowed in the case that a type annotation is present, e.g., in let [x, y]: number[]; The current playground implementation seems to disallow this.
@sandersn
Copy link
Member

sandersn commented Feb 1, 2016

@nickie, can you report a new bug for this, with the label Spec?
@nickie
Copy link

nickie commented Feb 1, 2016

@sandersn, done (6785) but I don't know how to add the label. Please add it, or advise me how to. Thanks.
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Feb 1, 2016

@sandersn FYI only Owners (i.e. us) can add Labels
@myitcv myitcv mentioned this pull request Apr 7, 2016
Closed
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@mhegazy @ahejlsberg @nickie @sandersn @RyanCavanaugh @CyrusNajmabadi @JsonFreeman @KirillGrishin
You can’t perform that action at this time.
X Tutup