Skip to content

Js constructor function fixes #22721

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

Merged
merged 7 commits into from
Mar 20, 2018
Merged

Js constructor function fixes #22721

merged 7 commits into from
Mar 20, 2018

Conversation

sandersn
Copy link
Member

JS constructors had a couple of bugs with strictNullChecks (which people actually do use with checkJs, see the recent post by @OliverJAsh).

  1. this-assignments in a function added undefined to the type of all properties:
function A() {
  this.x = 1
}

Incorrectly had x: number | undefined not x: number.

  1. Callable constructors, which have a check to see whether they were called instead of newed, incorrectly unioned undefined to the instance type. This was unneeded and annoying:
function A() {
  if (!(this instanceof A)) {
    return new A()
  }
  this.x = 1
}
var a = new A()

This incorrectly had a: { x: number } | undefined when it should have been a: { x: number }

Fixes #22414
Fixes #22641

Note that the second fix checks two things to distinguish a constructor function from a normal one:

  1. Whether the function is a javascript constructor
  2. Whether the function returns a type with the symbol of the constructor function itself -- that is, whether it has a statement return new A() somewhere inside function A. It doesn't check whether this statement is inside a conditional.

@sandersn sandersn requested review from weswigham and a user March 20, 2018 17:28
@@ -4259,7 +4259,12 @@ namespace ts {
}

if (isPropertyAccessExpression(expression.left) && expression.left.expression.kind === SyntaxKind.ThisKeyword) {
if (getThisContainer(expression, /*includeArrowFunctions*/ false).kind === SyntaxKind.Constructor) {
const thisContainer = getThisContainer(expression, /*includeArrowFunctions*/ false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An explanatory comment would be nice to have here

@sandersn sandersn merged commit 1074819 into master Mar 20, 2018
@sandersn sandersn deleted the js-constructor-function-fixes branch March 20, 2018 18:24
@@ -4259,7 +4259,14 @@ namespace ts {
}

if (isPropertyAccessExpression(expression.left) && expression.left.expression.kind === SyntaxKind.ThisKeyword) {
if (getThisContainer(expression, /*includeArrowFunctions*/ false).kind === SyntaxKind.Constructor) {
const thisContainer = getThisContainer(expression, /*includeArrowFunctions*/ false);
const isPrototypeProperty = isBinaryExpression(thisContainer.parent) &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only used if thisContainer.kind === SyntaxKind.FunctionExpression, might want to avoid computing this otherwise.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants