In JS, assignment to an undeclared instance property should act as its declaration

TypeScript Version: 2.7.0-dev.201xxxxx

Search Terms:
Salsa, Class member, instance

TypeScript Version
2.7.2

Code

// tsconfig
{
  "compilerOptions": {
    "target": "es5",
    "module": "commonjs",
    "allowJs": true,
    "checkJs": true,
    "outDir": "./dist",
    "strict": true,
    "esModuleInterop": true
  }
}

// index.js
class Foo {};
let f = new Foo();
f.bar = 1

Expected behavior:
No error

Actual behavior:

$ tsc --pretty
index.js:3:3 - error TS2339: Property 'bar' does not exist on type 'Foo'.

3 f.bar = 1
    ~~~

Note that this is about checking js files, not ts guess where one would declare class properties anyways

I think it's a correct error, because it's an instance variable.

class Foo { constructor () { this.f = undefined } } should be enough to declare f as an instance variable; however, its type may end up being undefined instead of any or some other appropriate type. It'd be better to annotate the this.f assignment with @type.

@Kovensky Not sure what design goals of Salsa is but this.f = undefinedin constructor seems unlike usual JavaScript.

To be honest, I think adding properties to instances after the fact isn't something we can do under our current design.

What's being proposed? Would a set of a property on a class instance never be an error? That seems like going too far.

At least in Webpack they are fine with initiating the member with undefined in constructor.

See this comment: webpack/webpack#6869 (comment)

I think a solution here is to support /** @property */ and/or /** @member */tags on classes. this way the code does not need to be changed with a this.prop = undefined, just a comment added along with a type.

similar to #15715.

A @property jsdoc may be the right solution for some code bases, but I don't think webpack is an example of one. For webpack to compile with checkJs, you have to add (at least) one line of code to fix the "Property does not exist" error. The question is which line you want to write:

class C {
  constructor() {
    this.upfront = 1
    // here?: this.late = undefined
  }
}
var c = new C();
// here?: /** @property */
c.late = 2;

Of the two, this.late = undefined is clearly better since it improves the definition of C both at edit time and compile time by making it easier for the compiler and the VM, respectively, to understand.

The only reason I can think of to prefer /** @property */ in this case is if you want to check a code base without changing anything except comments. But in that case I think you are better off inserting // @ts-ignore FIXME everywhere late is referenced, the way Google does with errors it wants to ignore. For most projects, the whole point of introducing Typescript is to make changes based on the errors it produces. If some project authors aren't on board, then they won't be happy with any changes that Typescript precipitates — some are guaranteed to be non-idiomatic Javascript.

One place where /** @property */ may make sense is an initializer that is not part of a class (or constructor function):

function C(full = true) {
  this.basics = 'just the basics'
  if (full) initC(this)
}
function initC(c) {
  c.other = 'stuff goes here'
  c.db = somedb.connect()
  c.launchMissiles = function() { /* CLASSIFIED but definitely IRREVOCABLE */ }
}
C.prototype.afterInitialisation = function() {
  // refer multiple times to c.other, c.db, c.launchMissiles
}

Since initC initialises a lot of properties, and presumably does so during construction much of the time, it wouldn't provide a runtime advantage to have a bunch of assignments like this.other = undefined in the constructor function, and the duplication would stand out in the source. Note that to get this to work, we'd need to type the parameter c as well.

/** @param {C} c */
function initC(c) {
  /** @property */
  c.other = 'stuff goes here'
  /** @property */
  c.db = somedb.connect()
  /** @property */
  c.launchMissiles = function() { /* CLASSIFIED but definitely IRREVOCABLE */ }
}

Even in this scenario, the duplication in the constructor might be just as good as the additional doc comments in initC.

My vote is to look for some other motivating example than webpack, since it looks like the webpack changes just required one or two additions per class. Plus, those additions are good for the runtime performance of webpack.

I was thinking of adding it on the class/function declaration, e.g.:

/**
  * @property {number} a
  */
class C {
    m() {
         this.a;
    }
}

or

/**
 * @class
 * @property {number} a
 */
function F() { 
    this.m = function m () { 
        this.a;
    }
}

Navigation Menu

Repository navigation

In JS, assignment to an undeclared instance property should act as its declaration #22896

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

In JS, assignment to an undeclared instance property should act as its declaration #22896

Description

Activity

marvinhagemeister commented on Mar 27, 2018

Jessidhia commented on Mar 27, 2018

mohsen1 commented on Mar 27, 2018

DanielRosenwasser commented on Mar 27, 2018

RyanCavanaugh commented on Mar 27, 2018

mohsen1 commented on Mar 27, 2018

mhegazy commented on Mar 27, 2018

sandersn commented on Mar 28, 2018

mhegazy commented on Mar 28, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions