GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add support for 'new.target' meta-property by rbuckton · Pull Request #12783 · microsoft/TypeScript

Conversation

@rbuckton

@rbuckton Ron Buckton (rbuckton) commented Dec 9, 2016 •
edited by DanielRosenwasser
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 via new, the value of new.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 via new, new.target is set to undefined. 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 via new as they are not constructors. As such, we have chosen to make it an error to reference new.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 for new.target is sufficient for ES5 code that interacts with other ES5 code, it does not fully emulate new.target in an ES6 host environment. What this means is that our new.target emit is not compatible with Reflect.construct as we have no way to intercept this API. This emit also is intolerant of explicit changes to the constructor 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 with Object.setPrototypeOf or __proto__. One such use case is inheriting from Error in NodeJS v4 and higher:

// ts
class CustomError extends Error {
  constructor(message?: string) {
    super(message); // 'Error' breaks prototype chain here
    (<any>this).__proto__ = new.target.prototype; // restore prototype chain
  }
}

// js
var CustomError = (function (_super) {
  __extends(CustomError, _super);
  function CustomError() {
    var _newTarget = this.constructor;
    var _this = _super.apply(this, arguments);  // 'Error' breaks prototype chain here
    _this.__proto__ = _newTarget.prototype; // restore prototype chain
    return _this;
  }
  return CustomError;
})(Error);

Also, while we can use this.constructor to emulate new.target for classes, we must be more defensive for new.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 the this argument of the function being bound to the module object (i.e. func() becomes package_1.func()). Also, function expressions are often assigned to a property of an object which could in turn become the this 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:

// ts
function f() {
  if (new.target) { /* called via 'new' */ }
}

// js
function f() {
  var _newTarget = this && this instanceof f ? this.constructor : void 0;
  if (_newTarget) { /* called via 'new' */ }
}

Fixes #2551

Related:

  • https://tc39.github.io/ecma262/#prod-NewTarget - Specification for NewTarget
  • Documented breaking change for subclassing built-ins. TypeScript-wiki#100 - Documented breaking change for subclassing built-ins
  • Wrong constructor of Error subclass from 2.1.1 #12660 - Wrong constructor of Error subclass from 2.1.1
  • Bug or breaking change in super constructors? #12669 - Bug or breaking change in super constructors?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

DanielRosenwasser, HerringtonDarkholme, yortus, saschanaz, JoshuaKGoldberg, Jessidhia, Artazor, GoToLoop, and rictic reacted with thumbs up emoji
All reactions
  • 👍 9 reactions
@rbuckton
Adds support for new.target
6a1ccd8
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Dec 9, 2016
@rbuckton
Ron Buckton (rbuckton) requested review from Daniel Rosenwasser (DanielRosenwasser), Anders Hejlsberg (ahejlsberg), Mohamed Hegazy (mhegazy), Nathan Shively-Sanders (sandersn) and Vladimir Matveev (vladima) December 9, 2016 02:21
@rbuckton
Revert changes to core.ts
c0a73b2
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested changes Dec 13, 2016
View reviewed changes
Comment thread src/compiler/core.ts Outdated
* @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.

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 12, 2016

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/compiler/utilities.ts Outdated
return false;
}

export function getAllLabeledStatements(node: LabeledStatement): { statement: Statement; labeledStatements: LabeledStatement[]; } {

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 12, 2016

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/compiler/transformers/es2015.ts Outdated

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

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 12, 2016

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@rbuckton Ron Buckton (rbuckton) Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Dec 13, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I'll have to dive in, but can you explain the es2015 transform changes you needed to make?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rbuckton

Ron Buckton (rbuckton) commented Dec 19, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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 TransformFlags values (such as LexicalThis capturing) and instead explicitly track the value as we walk back up the spine.

I can isolate these changes to a separate PR if it would be more clear.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ron Buckton (rbuckton) added 2 commits December 19, 2016 11:29
@rbuckton
PR cleanup
97f48e8
@rbuckton
Merge branch 'master' into es6-new-target
5f32d3b
@rbuckton Ron Buckton (rbuckton) mentioned this pull request Dec 20, 2016
Clean up for ES2015 transforms #13040
Merged
@rbuckton
Merge branch 'es2015-cleanup' into es6-new-target
c53ae34
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested changes Dec 20, 2016
View reviewed changes
Comment thread src/compiler/transformers/es2015.ts
break;
}

const captureNewTargetStatement = createVariableStatement(

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 20, 2016

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/compiler/transformers/es2015.ts Outdated
return node;
if (convertedLoopState) {
if (hierarchyFacts & HierarchyFacts.ArrowFunction) {
// if the enclosing function is an ArrowFunction is then we use the captured 'this' keyword.

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 20, 2016

Copy link
Copy Markdown
Member

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"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/compiler/utilities.ts
return false;
}

export function unwrapInnermostStatmentOfLabel(node: LabeledStatement, beforeUnwrapLabelCallback?: (node: LabeledStatement) => void) {

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 20, 2016

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@rbuckton Ron Buckton (rbuckton) Dec 21, 2016

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/compiler/utilities.ts Outdated
|| kind === SyntaxKind.TrueKeyword
|| kind === SyntaxKind.SuperKeyword
|| kind === SyntaxKind.NonNullExpression
|| kind === SyntaxKind.MetaProperty

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 20, 2016

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Dec 20, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I get the crux of the changes which look alright to me - I'm not an expert at the loop conversion code, so Vladimir Matveev (@vladima) may be a good person to check in with.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rbuckton
PR Feedback
4620fbd
mhegazy
Mohamed Hegazy (mhegazy) approved these changes Dec 26, 2016
View reviewed changes
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) approved these changes Dec 28, 2016
View reviewed changes
@rbuckton
Merge branch 'master' into es6-new-target
4098058
vladima
Vladimir Matveev (vladima) approved these changes Dec 28, 2016
View reviewed changes
@rbuckton
Ron Buckton (rbuckton) merged commit 2db8d80 into master Dec 28, 2016
@rbuckton
Ron Buckton (rbuckton) deleted the es6-new-target branch December 28, 2016 21:53
@soda0289 Reyad Attiyat (soda0289) mentioned this pull request Mar 18, 2017
Fix: Convert MetaProperty (new.target) nodes correcly (fixes #194) eslint/typescript-eslint-parser#195
Merged
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser approved these changes
@ahejlsberg Anders Hejlsberg (ahejlsberg) Awaiting requested review from ahejlsberg
@sandersn Nathan Shively-Sanders (sandersn) Awaiting requested review from sandersn
+2 more reviewers
@mhegazy Mohamed Hegazy (mhegazy) mhegazy approved these changes
@vladima Vladimir Matveev (vladima) vladima approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@rbuckton @DanielRosenwasser @mhegazy @vladima @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.