-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add support for 'new.target' meta-property #12783
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
Conversation
@@ -568,6 +568,8 @@ namespace ts { | |||
* is created if `value` was appended. | |||
* @param value The value to append to the array. If `value` is `undefined`, nothing is | |||
* appended. | |||
* @param copyOnWrite Indicates whether to return a fresh array rather than modify the | |||
* existing array. |
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.
Looks like this can be removed.
@@ -883,6 +883,18 @@ namespace ts { | |||
return false; | |||
} | |||
|
|||
export function getAllLabeledStatements(node: LabeledStatement): { statement: Statement; labeledStatements: LabeledStatement[]; } { |
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.
Where is this used?
|
||
// Subtree facts | ||
NewTarget = 1 << 14, // Contains a 'new.target' meta-property | ||
NewTargetInComputedPropertyName = 1 << 15, // Contains a 'new.target' meta-property in a computed property name. |
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.
Why is it important that this be distinct from NewTarget
?
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.
When we compute flags for a member, such as a method or accessor, it both has its own lexical scope (for the parameters and body) as well as shares the lexical scope of its container (due to the computed property name). Without this flag, encountering a NewTarget in a computed property name would incorrectly indicate that NewTarget was set in the body of the method, not in the container.
I'll have to dive in, but can you explain the es2015 transform changes you needed to make? |
Regarding the changes to the transformer, I am trying to clean up the es2015 transformer and remove some of the variables we use to track state as we descend the tree, this should reduce the overall memory footprint due to values saved on the stack. Ideally I can use this same mechanism to eventually remove some I can isolate these changes to a separate PR if it would be more clear. |
break; | ||
} | ||
|
||
const captureNewTargetStatement = createVariableStatement( |
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.
newTarget
could be undefined
if you're somehow not handling a case - consider adding a
Debug.assert(!!newTarget, "'newTarget' should be defined for " + (ts.SyntaxKind as any)[node.kind])
or something similar.
return node; | ||
if (convertedLoopState) { | ||
if (hierarchyFacts & HierarchyFacts.ArrowFunction) { | ||
// if the enclosing function is an ArrowFunction is then we use the captured 'this' keyword. |
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.
"is then" -> ", then"
@@ -900,6 +900,17 @@ namespace ts { | |||
return false; | |||
} | |||
|
|||
export function unwrapInnermostStatmentOfLabel(node: LabeledStatement, beforeUnwrapLabelCallback?: (node: LabeledStatement) => void) { |
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.
You only use this function once - do we anticipate using it again?
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.
Yes, I also am using this in the asyncGenerators branch
@@ -3848,6 +3874,7 @@ namespace ts { | |||
|| kind === SyntaxKind.TrueKeyword | |||
|| kind === SyntaxKind.SuperKeyword | |||
|| kind === SyntaxKind.NonNullExpression | |||
|| kind === SyntaxKind.MetaProperty |
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.
Do we want to just make this a switch
? I think it's asking for it at this point.
I get the crux of the changes which look alright to me - I'm not an expert at the loop conversion code, so @vladima may be a good person to check in with. |
This adds support for the
new.target
meta-property introduced in ES6. This also adds best-effort down-level emit for ES5/3.NewTarget meta-property
The
new.target
meta-property is new syntax introduced in ES6. When you create an instance of a constructor vianew
, the value ofnew.target
is set to be a reference to the constructor function initially used to allocate the instance. If a function is called rather than constructed vianew
,new.target
is set toundefined
. In addition,new.target
is lexically scoped to the body of the function. As such, it can be captured inside an arrow function.While it is legal to use
new.target
in methods and accessors in ES6, those members cannot be called vianew
as they are not constructors. As such, we have chosen to make it an error to referencenew.target
in the body of a method or accessor.ES5 emit
new.target
is a pure ES6 syntactic construct that can only be emitted down-level on a best-effort basis. While our emit fornew.target
is sufficient for ES5 code that interacts with other ES5 code, it does not fully emulatenew.target
in an ES6 host environment. What this means is that ournew.target
emit is not compatible withReflect.construct
as we have no way to intercept this API. This emit also is intolerant of explicit changes to theconstructor
property of the constructor's prototype.That said, the reason we are introducing best-effort down-level emit for
new.target
are for several specific use cases pairing ES5 emit for classes withObject.setPrototypeOf
or__proto__
. One such use case is inheriting fromError
in NodeJS v4 and higher:Also, while we can use
this.constructor
to emulatenew.target
for classes, we must be more defensive fornew.target
in a function declaration or function expression. The reason for this is that calling an imported function in a CommonJS, AMD, or System module results in thethis
argument of the function being bound to the module object (i.e.func()
becomespackage_1.func()
). Also, function expressions are often assigned to a property of an object which could in turn become thethis
if the function is called as a member.To defend against this case, the down-level emit for
new.target
in functions is the following:Fixes #2551
Related: