Skip to content
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

Conversation

Projects
None yet
8 participants
@mhegazy
Copy link

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
@JsonFreeman

This comment has been minimized.

Copy link
Contributor

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

There should only be one declaration, right?

This comment has been minimized.

Copy link

replied Oct 16, 2014

yes. thanks.

This comment has been minimized.

Copy link

replied Oct 17, 2014

done.

@@ -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.

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.

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.

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.

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

@@ -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.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Oct 16, 2014

Contributor

parseLabeledStatement

This comment has been minimized.

Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

done, though it is still correct :)

@@ -790,7 +793,8 @@ module ts {
Signature = CallSignature | ConstructSignature | IndexSignature,

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

This comment has been minimized.

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.

Copy link
@mhegazy

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 17, 2014

Contributor

Why doesn't Value include BlockScoped?

This comment has been minimized.

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.

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.

Copy link
@mhegazy

mhegazy Oct 17, 2014

Author

Let is variable and blockscoped.

@@ -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.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Oct 16, 2014

Contributor

indentation.

This comment has been minimized.

Copy link
@mhegazy
@ahejlsberg

This comment has been minimized.

Copy link
Member

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.

Copy link

replied Oct 17, 2014

you are absolutely right. will fix it. thanks.

@ahejlsberg

This comment has been minimized.

Copy link
Member

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.

Copy link

replied Oct 17, 2014

yes. that is the intention.

@ahejlsberg

This comment has been minimized.

Copy link
Member

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.

Copy link

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.

Copy link
Member

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.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Oct 17, 2014

Overall looks great!

Mohamed Hegazy added some commits Oct 17, 2014

Mohamed Hegazy
Fix search for shadowed const declarations by a var declarations to s…
…earch for any variable instead of only a blockScoped one to ensure we are not picking it up from a wrong scope.
Mohamed Hegazy
Allow const and let declarations to be exported in modules. Also ensu…
…re that const module elements are not used as references.
Mohamed Hegazy
Treat blockScoped variable declarations as a separate category when i…
…t comes to symbol flags, instead of compining BlockScoped and Variable
@JsonFreeman

This comment has been minimized.

Copy link
Contributor

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

The casing is not consistent between these two messages (6015 and 6047)

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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

"Block"

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

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

Why errorLocation and not location?

This comment has been minimized.

Copy link

replied Oct 20, 2014

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

This comment has been minimized.

Copy link
Contributor

replied Oct 20, 2014

Ok, please comment that.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

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 pushed a commit that referenced this pull request Oct 24, 2014

Mohamed Hegazy

@mhegazy mhegazy merged commit 290e43b into master Oct 24, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@mhegazy mhegazy deleted the letAndConst branch Oct 24, 2014

@KirillGrishin

This comment has been minimized.

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.

Copy link
Member

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 referenced this pull request Apr 20, 2015

Closed

ES6 let and const syntax #371

@nickie

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

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

@nickie

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

@sandersn FYI only Owners (i.e. us) can add Labels

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.