stryker-mutator / stryker-js

Mutation testing for JavaScript and friends
https://stryker-mutator.io
Apache License 2.0
2.55k stars 242 forks source link

Must call super constructor in derived class before accessing 'this' or returning from derived constructor #4744

Closed nicojs closed 4 months ago

nicojs commented 4 months ago

Summary

Our friend "Must call super constructor in derived class before accessing 'this' or returning from derived constructor" is back.

Since TS 4.6 it is possible to call some code before super(). This is contributed by my friend @JoshuaKGoldberg 🙏

However, this causes issues when Stryker tries to mutate that code. For example:

This code:

export class UniqueKeyFailedError<T> extends UnprocessableEntityException {
  constructor(public readonly fields: ReadonlyArray<keyof T & string>) {
    const errorBody: UnprocessableEntityBody<T> = {
      status: 'uniqueness_failed',
      fields,
    };
    super(errorBody);
  }
}

Gets instrumented as:

export class UniqueKeyFailedError<T> extends UnprocessableEntityException {
  constructor(public readonly fields: ReadonlyArray<keyof T & string>) {
    if (stryMutAct_9fa48("91")) {
      {}
    } else {
      stryCov_9fa48("91");
      const errorBody: UnprocessableEntityBody<T> = stryMutAct_9fa48("92") ? {} : (stryCov_9fa48("92"), {
        status: stryMutAct_9fa48("93") ? "" : (stryCov_9fa48("93"), 'uniqueness_failed'),
        fields
      });
      super(errorBody);
    }
  }
}

Which, after transpiling to JS, results in an optional super call, which is invalid, resulting in:

ERROR [ExceptionsHandler] Must call super constructor in derived class before accessing 'this' or returning from derived constructor
ReferenceError: Must call super constructor in derived class before accessing 'this' or returning from derived constructor
    at new UniqueKeyFailedError (file:///home/nicojs/github/rock-solid/packages/backend/.stryker-tmp/sandbox1043762/dist/errors/errors.js:50:9)

This was previously a problem in #2474. We verified that the first call wasn't a call to super(). If so, we skip mutating that block. Let us do the same here, but now scan the entire block. Don't mutate before super().

@JoshuaKGoldberg if you have any better ideas, feel free to pitch in. Ideally, we would like to instrument that part of the code as well 🤷‍♀️

Reproduction example

See test/add-stryker branch in rock-solid

JoshuaKGoldberg commented 4 months ago
if (stryMutAct_9fa48("91")) {
  {}
} else {
  stryCov_9fa48("91");
  const errorBody: UnprocessableEntityBody<T> = stryMutAct_9fa48("92") ? {} : (stryCov_9fa48("92"), {
    status: stryMutAct_9fa48("93") ? "" : (stryCov_9fa48("93"), 'uniqueness_failed'),
    fields
  });
  super(errorBody);
}

🤔 Interesting. Hmm. This should be a TypeScript error, no? Since the super(errorBody) is only conditionally called, there's no guarantee the base class has finished setting up class state by the time the constructor is done. This is why the error exists - there's a legitimate hole in type safety from skipping the super call!

[TypeScript playground showing both type error and runtime crash]

I'd have thought that this type error is good and should be there. I.e. that the Stryker generates mutants that survive, but are not compiled by TypeScript FAQ covers this. If folks want their mutations to be type-error-free, they need to use a type checker plugin. No?

nicojs commented 4 months ago

Sorry for the confusion. Yes this would be a type error, but since Stryker adds // @ts-nocheck atop each file it isn't 🙈. The typescript checker plugin would't report this as an error, because it looks at each mutant in isolation, so that won't filter-out these mutants.

But even in normal JS, this way of instrumenting is invalid, since this results in a runtime error.

I'm wondering if this could be avoided somehow. So would there be a way to instrument this with mutants and have it be valid according to super() call rules.

nicojs commented 4 months ago

Indeed, this has nothing to do with the feature you've contributed to TS, it's just the way I discovered the bug, since I'm only programming in TS.

nicojs commented 4 months ago

Aha. This bug is TypeScript-specific. The problem is that TypeScript compiles it to this JS, which is invalid because if the this.fields assignment. Without it, there actually isn't an issue.

export class UniqueKeyFailedError extends UnprocessableEntityException {
    constructor(fields) {
        this.fields = fields;
        if (stryMutAct_9fa48("91")) {
            { }
        }
        else {
            stryCov_9fa48("91");
            const errorBody = stryMutAct_9fa48("92") ? {} : (stryCov_9fa48("92"), {
                status: stryMutAct_9fa48("93") ? "" : (stryCov_9fa48("93"), 'uniqueness_failed'),
                fields
            });
            super(errorBody);
        }
    }
}