Fix Object.extend arguments not being type-checked

[dwickern]

/cc @dfreeman

This is something I noticed after merging #11. extend lets you pass any argument, even if the types don't match the definition:

// given that Controller is defined like...
class Controller extends Object {
    needs: string[];
}

class X extends Controller {
    needs: 42 // error: types not compatible
}

Controller.extend({
    needs: 42 // this PR makes this an error again
});

It was caused by MixinOrLiteral:

type MixinOrLiteral<T, Base> = Ember.Mixin<T, Base> | T;

Since Ember.Mixin had nothing on its prototype, every extend invocation matches the first case.

I don't think I can write a test for it because we would be checking that the code doesn't compile.

[dfreeman]

Ahh, structural typing strikes again! Thanks for fixing this.

Given that the entire point of a static type system is to exclude code that's known to be invalid at compile time, having negative test cases seems like it would be really valuable. Otherwise it seems like it would be really easy to regress on things like this without realizing it.

I know DT's tests don't include coverage for this kind of thing, but do you think it would make sense to augment the suite in this repo?

[dwickern]

I did experiment with negative test cases in my ember-types repo which preceded this repo: test runner / test case. Maybe we could work it in somehow and still be compatible with the DT structure.

I think we'll need a custom test runner anyway: once we start shipping typings with ember addons, we'll need to test them somehow. I don't think DT's test suite will work for that.

[dfreeman]

That custom runner is really nice!

It would also be awesome to be able to annotate expressions with an expected inferred type, similar to your expected error codes. Today's assertType<T>() gets us most of the way there, but it's really only checking that the type of the given expression is assignable to T, which isn't super helpful if the inferred type is e.g. any.

[dwickern]

They do have an API to walk the AST, maybe it could check that assertType is never called with any.

[dfreeman]

Cool. Looks like we could even pretty easily take it one step further and verify that the inferred type of the expression matches the passed type argument exactly with something like:

sourceFile.forEachChild(function verifyTypeAssertions(node) {
    if (ts.isCallExpression(node)) {
        if (ts.isIdentifier(node.expression) && node.expression.text === 'assertType') {
            const tc = program.getTypeChecker();
            const expectedType = tc.getTypeFromTypeNode(node.typeArguments[0]);
            const actualType = tc.getTypeAtLocation(node.arguments[0]);
            console.log('expected:', tc.typeToString(expectedType), 'actual:', tc.typeToString(actualType));
        }
    } else {
        node.forEachChild(verifyTypeAssertions);
    }
});

[theroncross]

__ember_mixin_ is public and showing in autocomplete suggestions. Can you make it private?

[dfreeman]

FWIW the TS compiler itself uses the term brand for this pattern, which they have sprinkled around pretty liberally e.g. https://github.com/Microsoft/TypeScript/blob/master/src/compiler/types.ts#L621

[dwickern]

I wonder if we could implement the negative case as a function and handle it with the compiler API

shouldNotCompile(`
  Controller.extend({ needs: 42 });
`);

Then the code would be ignored if you used a different test runner