Skip to content
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

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

Closed
nicojs opened this issue Feb 20, 2024 · 4 comments · Fixed by #4757 · May be fixed by victorperin/qr-scanner-cli#225 or victorperin/qr-scanner-cli#230
Labels
🐛 Bug Something isn't working

Comments

@nicojs
Copy link
Member

nicojs commented Feb 20, 2024

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

@nicojs nicojs added the 🐛 Bug Something isn't working label Feb 20, 2024
@JoshuaKGoldberg
Copy link
Contributor

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
Copy link
Member Author

nicojs commented Feb 20, 2024

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
Copy link
Member Author

nicojs commented Feb 20, 2024

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
Copy link
Member Author

nicojs commented Feb 25, 2024

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);
        }
    }
}

nicojs added a commit that referenced this issue Feb 25, 2024
…super()`

Support constructors in TypeScript that have some code before the `super()` call and have constructor properties or initialized class properties. In such cases, the block statement mutator is not applied.

For more info, see #4744
nicojs added a commit that referenced this issue Feb 25, 2024
…super()` (#4757)

Support constructors in TypeScript that have some code before the `super()` call and have constructor properties or initialized class properties. In such cases, the block statement mutator is not applied.

For more info, see #4744
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment