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

Downlevel emit for let\const #2161

Merged
merged 24 commits into from Feb 28, 2015

Conversation

Projects
None yet
6 participants
@vladima
Copy link
Contributor

vladima commented Feb 27, 2015

  • changes in tests related to computed properties are related to change createNode -> createSynthesizedNode in emitter.
  • capturing block scoped variables in closures inside loops is not supported scenario in this PR though it can be added later

vladima added some commits Feb 27, 2015

@@ -1329,6 +1330,7 @@ module ts {

// Values for enum members have been computed, and any errors have been reported for them.
EnumValuesComputed = 0x00000080,
BlockScopedBindingInLoop = 0x00000100,

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

loop body, not loop initializer, right?

This comment has been minimized.

@jods4

jods4 Feb 28, 2015

According to ES6 spec, let in the loop initializer is also scoped per iteration (so subject to the same downlevel codegen issues). Beware that browsers currently get it wrong (tested in IE11, FF33).

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 28, 2015

Contributor

Yeah I learned that while in the middle of reviewing this PR. I still do not quite understand the difference between the loop scope and the body scope, given that they are both per iteration. I suppose the difference is that you can access the loop scoped bindings in the initializer, guard, and incrementor of the loop, but they will be shadowed as soon as the body starts for that iteration. Though I still don't see how there could be a difference for for...in and for...of.

This comment has been minimized.

@jods4

jods4 Feb 28, 2015

To be honest I don't think that there is any useful difference between the two. But there is a technical difference, as the loop body is a new scope nested inside the loop scope itself. It means that you can redeclare let i both in the loop initializer and the loop body. The initializer, loop increment and loop condition share one scope, the loop body is a nested scope.

Allen makes it very clear in his answer to this question: https://esdiscuss.org/topic/in-es6-do-for-loops-with-a-let-const-initializer-create-a-separate-scope

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 28, 2015

Contributor

Got it, thanks for explaining that!

return isAnyFunction(n) || n.kind === SyntaxKind.ModuleDeclaration || n.kind === SyntaxKind.SourceFile;
}

// @internal

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

Why do we need all these internal annotations? I thought everything in utilities is automatically internal as a result of the way we build

This comment has been minimized.

@vladima

vladima Feb 27, 2015

Contributor

will remove them

@@ -1134,6 +1140,48 @@ module ts {
}

// @internal
export function nodeStartsNewLexicalEnvironment(n: Node): boolean {

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

What does this mean? Would it be correct to call this nodeStartsNewFunctionScope?

}

// @internal
export function generateUniqueName(baseName: string, isExistingName: (name: string) => boolean): string {

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

I thought @ahejlsberg added name generation in the checker

This comment has been minimized.

@vladima

vladima Feb 27, 2015

Contributor

yes, this is his code, I've just moved it to utilities so it can be shared

}
var i = 1;
while (true) {
name = baseName + i;

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

var name

This comment has been minimized.

@vladima

vladima Feb 27, 2015

Contributor

good catch

@@ -1581,6 +1581,10 @@
"category": "Error",
"code": 4081
},
"Code in the loop captures block-scoped variable '{0}' in closure. This is natively supported in ECMAScript 6 or higher.": {

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

I think the message is confusing. I'll try to think of a good phrasing

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

First, change natively to only

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

Loop contains block scoped variable referenced by a function in the loop.

@@ -77,6 +78,13 @@ module ts {
return new Symbol(flags, name);
}

function setBlockScopeContainer(node: Node, cleanLocals: boolean) {

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Feb 27, 2015

Member

I don't know what cleanLocals means without reading the function, and even then I don't know what the implications for this are. Can you leave a comment regarding the situations in which this is useful?

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

I don't understand this. Why is blockScopedContainer.locals populated? I thought it should always be empty to start.

// clean locals in block scope container if
// - current node does not have local variables
// - current node is not source file (source file always have locals)
setBlockScopeContainer(node, /*cleanLocals*/ (symbolKind & SymbolFlags.HasLocals) == 0 && node.kind !== SyntaxKind.SourceFile);

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Feb 27, 2015

Member

Remove extra space after the comment

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

"==="

This comment has been minimized.

@vladima

vladima Feb 28, 2015

Contributor

done

@@ -343,6 +354,7 @@ module ts {

function bindCatchVariableDeclaration(node: CatchClause) {
bindChildren(node, /*symbolKind:*/ 0, /*isBlockScopeContainer:*/ true);

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Feb 27, 2015

Member

Remove extra line.

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

remove space

current = current.parent;
}

var current: Node = container;

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 27, 2015

Contributor

Say what we are interested in at this point.

getNodeLinks(n).resolvedSymbol ||
resolveName(n, n.text, SymbolFlags.BlockScopedVariable | SymbolFlags.Import, /*nodeNotFoundMessage*/ undefined, /*nameArg*/ undefined);

if (symbol && (symbol.flags & SymbolFlags.BlockScopedVariable)) {

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 28, 2015

Contributor

Exclude catch variables

This comment has been minimized.

@vladima

vladima Feb 28, 2015

Contributor

done

@@ -2478,7 +2558,20 @@ module ts {
}
}

function getBlockScopedVariableId(node: Identifier): number {
// return undefined for synthesized nodes
return node.parent && resolver.getBlockScopedVariableId(node);

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 28, 2015

Contributor

nodeIsSynthesized

This comment has been minimized.

@vladima

vladima Feb 28, 2015

Contributor

done

}
}

if (startPos !== undefined) {

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 28, 2015

Contributor

Mention that this distinction has to do with source maps

? blockScopeContainer
: blockScopeContainer.parent;

var generatedName = makeUniqueName(parent, (<Identifier>node).text);

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 28, 2015

Contributor

rename makeUniqueName to generateUniqueNameForLocation

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented Feb 28, 2015

👍

vladima added a commit that referenced this pull request Feb 28, 2015

Merge pull request #2161 from Microsoft/letConstES5Minus
Downlevel emit for let\const

@vladima vladima merged commit 8abf4ff into master Feb 28, 2015

2 checks passed

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

@vladima vladima deleted the letConstES5Minus branch Feb 28, 2015

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Mar 5, 2015

this will be a good better alternative to typeguards in my code:

var foo : Fancy|SuperFancy;
if(foo.superFancy){
  let superFancy : SuperFancy = foo;
}
else{
  let fancy : Fancy = foo;
}
@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented Mar 5, 2015

@basarat, will that actually work? You can only dot into members that exist in all parts of the union.

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Mar 5, 2015

@JsonFreeman if nothing else, you don't even need the union

var foo : any; // Note
if(foo.superFancy){
  let superFancy : SuperFancy = foo;
}
else{
  let fancy : Fancy = foo;
}
@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented Mar 5, 2015

Yes, with any, that would work quite nicely.

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