JavaScript: ignore self-assignments with a JSDoc comment

[asger-semmle]

Fixes a technically correct but seemingly unfixable alert in type-checked JS code, like this:

class C extends Q {
  constructor() {
    super(...);
    initStuff();

    /**
     * An important field initialized somewhere else.
     * @type { string }
     */
    this.x = this.x;
  }
}

TypeScript can check JS code, and recognizes JSDoc-annotated assignments in the constructor as fields. If these fields are initialized elsewhere, however, a self-assignment may be necessary to have a place to "declare" the field.

I still think it's fishy, but I think it's fairly safe to turn it off in this case. The real problems are unlikely to have a JSDoc comment in front of them.

[ghost]

I agree about the fishyness,
I think we should tie the heuristic as close as possible to the specific TypeScript use case, see comments.

[ghost]

Perhaps:

      // exclude self-assignments that have been inserted to satisfy the TypeScript type checker

[ghost]

Can we make this even more specific?

      not e.getAssignment().getParent().(ExprStmt).getDocumentation().getATag().getName() = "type"

[ghost]

Great.
I think this deserves a change note, it may be a more widespread problem than we are aware of.

[Zarel]

Modern versions of TypeScript also support

    /**
     * An important field initialized somewhere else.
     * @type { string }
     */
    this.x;

Which is probably clearer.

[asger-semmle]

That's a very good point @Zarel, thank you much for bringing it up.

That case falls under the purview of the "expression has no effect" query, which already has the same exception for JSDoc comments. Maybe it's worth reconsidering the exception here, though, since there actually is a more reasonable alternative (@esben-semmle).

@Zarel was there a reason you didn't use the short form in Pokemon-Showdown?

[Zarel]

@asger-semmle We started using TypeScript's checkJs support when it was very experimental, and didn't support that form. TypeScript's checkJs support is honestly still pretty bad, and I'm probably going to need to transition to using .ts files at some point.

I'll probably switch to using the shorter form at some point.

[ghost]

Maybe it's worth reconsidering the exception here, though, since there actually is a more reasonable alternative (@esben-semmle).

I agree. Let us keep the whitelist simple, and drop this PR.

[Zarel]

Someone mentioned one reason to do it this way.

If you use

this.x = this.x;

then the property will be initialized to undefined if it doesn't already exist. This will make V8's JIT optimize access to it, but if you don't, every access will be deoptimized.

[xiemaisi]

True, but this.x = undefined; achieves the same effect and is arguably clearer.

[Zarel]

this.x = undefined will set this.x to undefined always. The use-case is to set this.x to undefined only when it doesn't already exist.

The use-case is something like:

class Foo {
  constructor(options = {}) {
    Object.assign(this, options);
    /** @type {string | undefined} */
    this.x = this.x;
  }
}
let a = new Foo();
let b = new Foo({x: "bar"});

[xiemaisi]

Right, fair enough. If people feel that this is a common enough idiom to whitelist it, we can do so.

[Zarel]

I honestly wouldn't know. I would guess it's really uncommon. I'm using it to strongly type the data I get from data files, but there's probably a better way to do it.

[xiemaisi]

OK, that sounds like something people might reasonably do. @asger-semmle, @esben-semmle, do you think it is safe to whitelist this.prop = this.prop assignments or might we lose too many interesting results that way?

[ghost]

I am not sure.
At the very least, we should restrict the whitelist to be for constructors.
My main fear is that the presence of another this.propA = o.propA assignment in the constructor may indicate that we are hiding a copy/paste error, unfortunately that is a wobbly heuristic to de-whitelist on.

[xiemaisi]

OK. How about resurrecting this PR with its idea to only whitelist self-assignments that have a @type annotation, potentially even restricting it to self-assignments on this? We should then also add an explanation of his whitelisting to the qhelp.

[asger-semmle]

The @type jsdoc comment seems like a safe enough whitelisting to me.

[xiemaisi]

LGTM, but could you also update the qhelp to mention this additional whitelisting condition, perhaps in the "how to fix" section? It gives people a semantic way of addressing the alert without having to resort to suppression comments.

[asger-semmle]

Will rebase after #388 lands, as they both touch the change notes.

[xiemaisi]

#388 has landed; time for rebase?

[asger-semmle]

Rebased

[xiemaisi]

LGTM, modulo one niggle which I won't insist on.

[xiemaisi]

Spurious comma before "and".