Check for covariant parameters in interface implementations

[sharwell]

Rule Suggestion

Is your rule for a general problem or is it specific to your development style?

General.

What does your suggested rule do?

Report a warning when a typescript method implements an interface, but one or more parameters of the implementing method have covariant types with respect to the interface definition.

List several examples where your rule could be used

interface IRunner {
    run(s?: string): void;
}

class Runner implements IRunner {
    run(s: string): void { // Warning: should be s?: string
        console.log(s.length);
    }
}

const runner: IRunner = new Runner();
runner.run(undefined);

interface TestInterface {
	Method(text: string | undefined): void;
}

class TestClass implements TestInterface {
	public Method(text: string): void { // Warning: should be text: string|undefined
		console.log(text);
	}
}

Additional context

This is a subset of the functionality in #4175.

The compiler behavior is described in microsoft/TypeScript#27523.

[JoshuaKGoldberg]

Same as in #4175 - what bugs would this actually catch?

Another example of a place where this covariance allows a stricter parameter type in the implementing class:

interface TestInterface {
	Method(text: string | undefined): void;
}

class TestClass implements TestInterface {
	public Method(): void { // Warning: should have a text: string|undefined parameter
		console.log("zoinks");
	}
}

...but if either issue's rule were to be created, users would end up just adding unnecessary types to parameters or adding in unused parameters with names like _text just to get around it. That's not a good situation.

Unless there's a good reason (e.g. a lot of caught bugs) to enable this rule, it seems like something that would be better off in a third party package. That'd let you iterate on this idea without having to be locked into the TSLint release cycle and review system. Thoughts?

[sharwell]

I discovered this situation after enabling the strict-type-predicates rule locally. It resulted in a warning that the null check for text was unnecessary on the following line:

https://github.com/tunnelvisionlabs/antlr4ts/blob/176521ea4bd6f917def4c7093c8dd15486090679/src/CommonTokenFactory.ts#L65

It turns out the method was implemented to handle undefined argument values (which are certainly possible through the interface), but the signature of the implementation did not correctly reflect the possibility of this input. Removing the null check as a response to the tslint rule would introduce a bug that is not caught by either the TypeScript compiler or tslint.

[adidahiya]

@sharwell thanks for the code link; I think this is a valid use case for a tslint core rule and would accept PRs for #4175. Let's continue discussion there