Rule suggestion: strict-interface-implementation

[Psidium]

Rule Suggestion

strict-interface-implementation

This have already been requested in microsoft/TypeScript#6184.

Typescript allows arguments and return type of interface and its implementation to not be strictly equal.
This is different from Java/C# where the interface is seen as a contract between two implementing classes.
I wonder if it is possible to create a new tslint rule to check that a class is not implementing the interface exactly as it is defined in the interface.

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

General.
I see the value of this rule being on future changes of interfaces, where the Rule would warn of any non-conforming implementation.

What does your suggested rule do?

Check that every class implementing interface IFoo uses the same arguments and return type as it is declared on every method of the interface.

List several examples where your rule could be used

interface IEventArgs {}
interface IDispatchable {
    dispatch(type: string, args: IEventArgs): void;
}

class B implements IDispatchable {
	// Rule would ERROR on next line
    dispatch(): number { return 1; }
}

class A implements IDispatchable {
	// no error
    dispatch(type: string, args: IEventArgs): void {}
}

Additional context

I just don't know is objects that end up implementing interfaces should be covered here or not.

[JoshuaKGoldberg]

This seems like it could have a lot of edge cases in addition to catching some interesting bugs. Let's discuss in more detail which cases it should or shouldn't flag, to make sure it doesn't become overly verbose and annoying.

Echoing RyanCavanaugh's rhetorical questions in a non-rhetorical manner: can you give examples of types of bugs this would catch? Why is B's dispatch returning a number instead of void a bad thing?

[sharwell]

I believe this rule could be overly strict in the form above. In particular, there are two situations which are safe but not accounted for above:

1. Contravariant parameters (e.g. a parameter s?: string or s: string|undefined could safely implement an interface method defined with parameter s: string).
2. Contravariant return values (e.g. a method with return type string could implement an interface method declared with return type string|undefined).

[adidahiya]

+1 for the rule suggestion, it could have options called allow-contravariant-parameters and allow-contravariant-return-values to make the checks less strict, as @sharwell suggested

edit: also, +1 to this:

Let's discuss in more detail which cases it should or shouldn't flag, to make sure it doesn't become overly verbose and annoying.

[Psidium]

I'd be down for developing it, as I've had some fun creating some pretty specific rules for my current project.

Answering some question, I believe that the most bugs occur from adding/changing parameters in interfaces, maybe not so much in return types, return types are more easily caught in TS itself.

The contra/covariant case

I cannot think in any example that modifying a return type that does not cause a compile error would be a problem. Especially if we go with the contravariant path.

But, as a contra argument, I believe we can look to other more strictly typed languages, without duck typing, to see what do they do and draw from their experience.

Expand to see how Java does this

Going deeper, I can find that Java accepts more contravariant return types:

interface Interface {
    public Object method(Integer in);
}

class Ext implements Interface {
//compiles fine
     public String method(Integer in) {
         return in.toString();
     }
}

public class MyClass {
    public static void main(String args[]) {
        Ext ext = new Ext();
        Object obj = ext.method(2);

        System.out.println(obj);
    }
}

But, it does not accept it for method parameters:

class Ext implements Interface {
//Error: class Ext does not implement interface Interface
     public String method(Object in) {
         return in.toString();
     }
}

Expand to see how Swift does it

Swift, on the other, seem to be very strict regarding protocols:

class SomeParent {
}

class SomeChild: SomeParent {
}

protocol Interface {
   func doStuff(number: Int) -> SomeParent
}

// compile error
class SomeClass: Interface {
    func doStuff(number: Int) -> SomeChild {
        return SomeChild()
    }
}

And the other way around as well.

protocol Interface {
   func doStuff(number: SomeChild) -> Void
}

class SomeClass: Interface {
    func doStuff(number: SomeParent) -> Void {
        
    }
}

Expand to see Objective C

So, contrary to Swift, Objective-C actually compiles fine when the implementation is declaring the Child as the return type while the protocol expects a Parent-class and the same with the parameter declaring to be any type.

(oh, and the reason TS's interface are protocols in ObjC/Swift is because ObjC's interfaces are actually something akin to C++ headers "declaration")

@interface Parent : NSObject
@end

@interface Child : Parent
@end

@protocol Interface <NSObject>
- (Parent*)doSomething:(NSString*)name;
@end

@interface OtherClass : NSObject <Interface>
@end

@implementation Parent
-(void)doStuff {
    OtherClass* other = [OtherClass alloc];
    [other doSomething:@""];
}
@end

@implementation Child
@end

@implementation OtherClass
- (Child*) doSomething:(NSObject *)name {
    return [Child alloc];
}
@end

So, of all languages analysed, ObjC seems to be the one closest to TypeScript on the contravariant regard

After much tough on the issue and especially after analyzing other language implementations of interface-like features, I think we can go ahead with covariance. We may honor the Type side, but lets never forget the Script side.

Test cases

I believe one simple test case might be to check that, for every parameter, the implementing parameter is of a type that can be assigned to the corresponding interface parameter, hence:

interface Interface {
    doStuff(stf: string): void;
}

export class Cs implements Interface {
    public doStuff(stf?: string): void {}
	               ~~~~~~~~~~~~ [Parameter 'stf' of type 'string | undefined' is not conformant to the parameter 'stf' of type 'string' in interface 'Interface']
}

The test would simply check if const a: string = stf; compiles in the type system or not (er this is just an example, I wouldn't intantiate a ts.SourceFile and compile it inside tslint, there's probably a better solution).

On contributing

I see that all of the tests for rules are mainly integration tests. Is this the recommended way?

[JoshuaKGoldberg]

I see that all of the tests for rules are mainly integration tests. Is this the recommended way?

Correct. Any new rule or behavior change in an existing rule should be testable in the way it reports lint complaints on code.

Also, @Psidium, it should be noted that your post is super informative & thorough. Thanks for that! 😊

[Psidium]

So, according to the spec, the ImplementsClause actually accepts interfaces AND classes (I am baffled about this).

I don't think I will be supporting this ts feature "implement a class" on the rule, only interfaces.

[Psidium]

Okay, it seems I've reached a block right now.

I've managed to extract the parameters and return types of both the interface and the implementation. But I fell short of options when trying to actually compare the two types that I have.

On the file checker.ts we have the function isRelatedTo (and the more high-level ones isTypeRelatedTo and checkTypeRelatedTo), which would be the main force of this rule, checking if the parameter is relatable/assignable to the parent one.
https://raw.githubusercontent.com/Microsoft/TypeScript/master/src/compiler/checker.ts

The problem that I have right now is that neither of the aforementioned functions are exported for us to use, as they are private to the TypeScript compiler.

We could:

• Copy part of checker.ts into tslint's codebase (hate this solution)
• Open a PR adding a assignanibility verification in tsutils
• Develop our own solution to check assignability of types inside the Rule
• Create a valid TypeScript code in a string that is generated inside the rule then use ts.createSourceFile, pass it to the ts.Program and then program.emit() to get the possible errors (then having to parse then possibly).

Do you have any other idea? What do you think would be the best option?

[Psidium]

This is what I have so far: Psidium@527a234

[JoshuaKGoldberg]

Ah, the old isRelatedTo issue! This is yet another situation in which the type relation API discussed in microsoft/TypeScript#9879 is necessary. 😢

https://github.com/runem/ts-simple-type/ is a working implementation of that API outside of TypeScript. However, it and all the other possibilities are too heavyweight to be introduced to TSLint now (#4534).

Marking this as blocked. Thanks for the investigation @Psidium!

One suggestion: if this rule is important to you, how about making it a separate, standalone package that has a dependency on ts-simple-type? That seems like the best bet for getting it to work.

[Psidium]

Humm, nice to know about those tools and that it isn't the first time this issue has arisen.

However, it and all the other possibilities are too heavyweight to be introduced to TSLint now.

Can you explain why would it be too heavyweight to add ts-simple-type as a dependency now?

By heavyweight do you mean the added time in a yarn install phase or in the linting phase?

Or do you mean the added cognitive overload of having to maintain another big tool inside tslint, given that there will be this big migration to eslint?

I believe I might be adding this rule to a separate package right now, but it seems to me that this rule is fixing something that should be an optional part of the compiler itself, so having it as a first-class citizen Rule inside the main linter seems the way to go.

[JoshuaKGoldberg]

The added time in a yarn install phase

Yes, that. This is a more strict rule that most wouldn't enable at first; adding in another npm package as a dependency for just this use case isn't something that seems necessary right now.

If the rule ends up being very useful in its own standalone package (and TSLint is still actively maintained!), that'd be good evidence that it should be ported to the core ruleset.

[JoshuaKGoldberg]

Per #4534, closing this for housekeeping - thanks for the discussion folks!