Mixin classes with private/protected constructors are subclassable

[rafasofizada]

Bug Report

🔎 Search Terms

mixin, private constructor

🕗 Version & Regression Information

This is the behavior in every version I tried, and I reviewed the FAQ for entries about Classes.
Versions tried:

• 3.7.4
• 4.1.3
• Nightly

⏯ Playground Link

Playground link with relevant code

💻 Code

function PreventSubclassingMixin<TBase extends new(...args: any[]) => any>(Base: TBase) {
  return class NonSubclassable extends Base {
    private constructor(...args: any[]) {
      super();
    }
  }
}

class Base {
  constructor() {}
}

class NonSubclassableThroughExtension extends Base {
  private constructor() {
    super();
  }
}

const NonSubclassableThroughMixin = PreventSubclassingMixin(Base);

new NonSubclassableThroughExtension(); // Constructor of class 'NonSubclassableThroughExtension' is private and only accessible within the class declaration.(2673)
new NonSubclassableThroughMixin(); // Doesn't throw an error

🙁 Actual behavior

NonSubclassableThroughExtension correctly throw an error — I've extended a superclass Base, which has a publicly accessible constructor, and provided a private constructor in the derived class. I can't access the derived class' constructor anymore:

new NonSubclassableThroughExtension(); // Constructor of class 'NonSubclassableThroughExtension' is private and only accessible within the class declaration.(2673)

The PreventSubclassingMixin supposedly works the same — it creates a class expression that extends the Base class with a public constructor and defines a private constructor, and returns it. The returned derived mixin class is supposedly identical to the example above. However, if I apply the mixin to the Base: const NonSubclassableThroughMixin = PreventSubclassingMixin(Base), and try to initialize it, Typescript won't throw any error.

🙂 Expected behavior

new NonSubclassableThroughMixin() should throw a Constructor of class 'NonSubclassableThroughExtension' is private and only accessible within the class declaration.(2673).

[rafasofizada]

I have found the cause of the bug.

Updated Minimal Bug Reproduction

There's no difference between subclassing through mixins vs subclassing through class extension, as I thought at the time of writing the bug report. To show this, here is an updated minimal bug reproduction, featuring two mixins:

function ShouldThrowButDoesnt<TBase extends new(...args: any[]) => any>(Base: TBase) {
  return class ShouldThrowButDoesnt extends Base {
    private constructor(...args: any[]) {
      super();
    }
  }
}

function ShouldThrowAndDoes(Base: new(...args: any[]) => any) {
  return class ShouldThrowAndDoes extends Base {
    private constructor(...args: any[]) {
      super();
    }
  }
}

class Base {
  constructor() {}
}

const shouldThrowButDoesnt = ShouldThrowButDoesnt(Base);
const shouldThrowAndDoes = ShouldThrowAndDoes(Base);

new shouldThrowButDoesnt();
new shouldThrowAndDoes();

Problem Cause

The only difference between the two mixins ShouldThrowButDoesnt and ShouldThrowAndDoes is that in the former, the class parameter is of a generic type. Because of this (and I don't know exactly why), in checker.ts > resolveNewExpression(), when obtaining constructor signatures (checker.ts, line 29642 in v4.4.2), ShouldThrowButDoesnt is represented as an intersection type with type members ShouldThrowButDoesnt and Base (this intersection is obtained by getReducedApparentType()).

Now, the cause of the problem is in how TypeScript resolves this intersection. In checker.ts > resolveIntersectionTypeMembers() (emphasis mine):

function resolveIntersectionTypeMembers(type: IntersectionType) {
    // The members and properties collections are empty for intersection types. To get all properties of an
    // intersection type use getPropertiesOfType (only the language service uses this).
    let callSignatures: Signature[] | undefined;
    let constructSignatures: Signature[] | undefined;
    let indexInfos: IndexInfo[] | undefined;
    const types = type.types;
    const mixinFlags = findMixins(types);
    const mixinCount = countWhere(mixinFlags, (b) => b);
    for (let i = 0; i < types.length; i++) {
        const t = type.types[i];
        // When an intersection type contains mixin constructor types, the construct signatures from
        //         ^^^^^^^^^^^^.              ^^^^^^^^^^^^^^^^^
        // those types are discarded and their return types are mixed into the return types of all
        //                 ^^^^^^^^^
        // other construct signatures in the intersection type. For example, the intersection type
        // '{ new(...args: any[]) => A } & { new(s: string) => B }' has a single construct signature
        // 'new(s: string) => A & B'.
        if (!mixinFlags[i]) {
            let signatures = getSignaturesOfType(t, SignatureKind.Construct);
            if (signatures.length && mixinCount > 0) {
                signatures = map(signatures, s => {
                    const clone = cloneSignature(s);
                    clone.resolvedReturnType = includeMixinType(getReturnTypeOfSignature(s), types, mixinFlags, i);
                    return clone;
                });
            }
            constructSignatures = appendSignatures(constructSignatures, signatures);
        }
        callSignatures = appendSignatures(callSignatures, getSignaturesOfType(t, SignatureKind.Call));
        indexInfos = reduceLeft(getIndexInfosOfType(t), (infos, newInfo) => appendIndexInfo(infos, newInfo, /*union*/ false), indexInfos);
    }
    setStructuredTypeMembers(type, emptySymbols, callSignatures || emptyArray, constructSignatures || emptyArray, indexInfos || emptyArray);
}

Paraphrasing what was said in the comments, the mixin constructor signatures are discarded and instead their return types are merged (because mixins are not supposed to mess with a class' constructor signature).

Therefore, mixin constructor's accessibility isn't taken into consideration at all!
@RyanCavanaugh @andrewbranch I'd appreciate it if you could review this update and assign the bug to me (I'm sorry if it's not appropriate to tag you directly).

However, I'm not really sure how a good solution would look like. Removing the !mixinFlags[i] check makes sense, but breaks a bunch of different tests. Perhaps we should only discard mixins that have a public constructor, but process all those that have a non-public one?

[andrewbranch]

We don’t assign bugs to community members. Any bug in the Backlog milestone is fair game for any community member to submit a PR, but there is no guarantee we will accept it, and your chances of getting a review and the support you need is much better if you choose an issue labeled “help wanted.” As it is, we can’t take the time to investigate this bug and answer your questions about it right now because it’s on the backlog, and we have work that is milestoned for upcoming releases that needs our attention. If you want to contribute, I would strongly recommend looking at the “help wanted” label first.

[rbuckton]

This seems like something that #41587 would solve, since we can't annotate an interface or function/constructor type with visibility modifiers like private/protected. With #41587, we could change the inferred return type of ShouldThrowButDoesnt to use a typeof class so that this works both in the current project and when imported from a declaration file:

declare function ShouldThrowButDoesent<TBase extends new (...args: any[]) => any>(Base: TBase): typeof class extends TBase {
  private constructor(...args: any[]);
};

Unfortunately, there's still a lot of work to go on #41587, judging from the notes in #41824.