Skip to content

Expressions in JS special assignments don't use jsdoc for contextual type #25571

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

Closed
akdor1154 opened this issue Jul 11, 2018 · 4 comments
Closed
Assignees
Labels
Bug A bug in TypeScript checkJs Relates to checking JavaScript using TypeScript Fixed A PR has been merged for this issue

Comments

@akdor1154
Copy link

TypeScript Version: 3.0.0-dev.20180710

Search Terms: tagged discriminated union property this jsdoc checkjs allowjs literal

The following typescript works great:

type DoneStatus = {
  status: 'done';
  count: number;
};

type ErrorStatus = {
  status: 'error';
  error: Error;
};

type Status = DoneStatus | ErrorStatus;

class Thing {
  s: Status;
  constructor() {
    this.s = { status: 'done', count: 3 };
  }

  fail() {
    this.s = { status: 'error', error: new Error() };
  }
}

However, the equivalent JS does not:

/**
 * @typedef DoneStatus
 * @property {'done'} status
 * @property {number} count
 */

/**
 * @typedef ErrorStatus
 * @property {'error'} status
 * @property {Error} error
 */

/** @typedef {DoneStatus | ErrorStatus} Status */

class Thing {
  constructor() {
    /** @type {Status} */
    this.s = { status: 'done', count: 3 }; // ts error
  }

  fail() {
    this.s = { status: 'error', error: new Error() }; // ts error
  }
}

Both assignments give the error:

[ts] Type 'string' is not assignable to type '"error" | "done"'.
test.js(3, 4): The expected type comes from property 'status' which is declared here on type 'DoneStatus | ErrorStatus'
(property) status: string

Expected behavior:
JS behaves the same as TS version

@mhegazy mhegazy added Bug A bug in TypeScript checkJs Relates to checking JavaScript using TypeScript labels Jul 11, 2018
@mhegazy mhegazy added this to the TypeScript 3.0.1 milestone Jul 11, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Jul 11, 2018

looks like the contextual type is not picked up correctly for the special property assignment case.

@sandersn sandersn changed the title Can't assign immediate values to tagged union properties in JS mode Expressions in JS special assignments don't use jsdoc for contextual type Jul 11, 2018
@sandersn
Copy link
Member

I'm going to make a partial fix that makes these assignments appropriately context sensitive, which fixes this bug, but then work on a permanent fix that makes the type of other assignments, like

/** @type {{ status: 'done' }} */
F.prototype = { status: 'done' }

correctly assign { status: 'done' } as the type of F.prototype. This a bigger change that is probably not appropriate for 3.0.1. Hard to say without trying it first.

sandersn added a commit that referenced this issue Jul 12, 2018
Explicitly typed special assignments should be context sensitive if they
have an explicit type tag. Previously no special assignments were
context sensitive because they are declarations, and in the common,
untyped, case we inspect the right side of the assignment to get the
type of the left side, and inspect the right side of the assignment to
get the type of the left side, etc etc.

Note that some special assignments still return `any` from
checkExpression, so still don't get the right type.

Fixes #25571
@sandersn sandersn added the Fixed A PR has been merged for this issue label Jul 12, 2018
sandersn added a commit that referenced this issue Jul 12, 2018
* Explicitly typed js assignments: context sensitive

Explicitly typed special assignments should be context sensitive if they
have an explicit type tag. Previously no special assignments were
context sensitive because they are declarations, and in the common,
untyped, case we inspect the right side of the assignment to get the
type of the left side, and inspect the right side of the assignment to
get the type of the left side, etc etc.

Note that some special assignments still return `any` from
checkExpression, so still don't get the right type.

Fixes #25571

* Change prototype property handling+update bselines

* Fix indentation in test

* Update baselines
@akdor1154
Copy link
Author

akdor1154 commented Jul 13, 2018

@sandersn is this the best way of asserting member types? i.e. the one thing I wasn't sure of was should I be using some @property assertion instead? I just couldn't find a way to get it to work - if I add a @property tag to my class then TS interprets it as a static property, not instance.

On my reading of jsdoc docs it seems using @type at the assignment point is considered correct, just wanted to check :)

@sandersn
Copy link
Member

In Javascript, the pattern that the compiler expects is modelled on Closure:

class C {
    /** @param {number} x */
    constructor(x) {
        this.x = x;
        /** @type {number | undefined} */
        this.y;
    }
}

Basically it's just looking for assignments to this.propertyName. If the assignment has an initialiser, it will use its type unless there is a type annotation. The compiler actually looks in any method, not just constructors, but assignments in the constructor are definite -- they don't include undefined in the type like assignments in methods do.

    method() {
        this.z = 'hi' // z: string | undefined
    }

We have a proposal for a @property tags that would go above the class, but those would result in slower code than just initialising properties in the constructor since the VM will end up creating fewer hidden classes for classes with uniform sets of properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript checkJs Relates to checking JavaScript using TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

3 participants