Support @implements in JSDoc

[TimvdLippe]

We are looking at migrating from Closure Compiler JavaScript to TypeScript in Chrome DevTools. In our early experiments, we discovered that TypeScript with --checkJs does not understand @interface definitions. I originally commented on #33207 but was asked to create a separate issue instead.

You can see the error messages it would generate for DevTools in https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/1962289 (for file https://cs.chromium.org/chromium/src/third_party/devtools-frontend/src/front_end/common/EventTarget.js?rcl=32cd3a0fcc7a34ea0c3831cd447c70f359dd9400)

TypeScript Version: 3.5.3

Search Terms: jsdoc, interface, @interface,

Per #33207 (comment) I am creating a new issue. There is an existing issue that is closed for collaboration: #16142

Code

/** @interface */
class Foo {
    /** @return {number} */
    method() {}
}

/** @implements {Foo} */
class Bar {
    /** @return {string} */
    method() {
        return "bar";
    }
}

Expected behavior:
TypeScript understands that Foo is an interface and that Bar does not properly implement the interface
Actual behavior:
TypeScript does not support @interface and @implements and will throw an error on Foo and will accept the definition of Bar.
Playground Link: http://www.typescriptlang.org/play/?checkJs=true&allowJs=true&ts=3.8.0-dev.20191210&useJavaScript=true#code/PQKhAIAEEsDsBcCmAnAZgQwMaPCYAoTAG3QGdTwAxAe2vAG99xnxQJJlF4BXZWB2NwC2AIxQBfXARbghXABbUAJgAoAlA3H4t+NlGhCADkURyEFejWqS8hEuXAAhdMgZMWejl1796peMhwAOY20ixy8IqqGowyMpw8fOAARCIuyQDc7sxa4kA

Related Issues: #16142 #33207

[sandersn]

@dragomirtitian you said you were interested in implementing this, so I came back and read the example again. It looks like we only need @implements, and it should behave just like Typescript's implements, because, in TS, a class can implement any other class:

class Foo { method(): number {} }
class Bar implements Foo { method() { return 'bar' } }

This will give the desired error on Bar's method saying that string isn't assignable to numbe, plus an error on Foo's method saying that it doesn't return anything.

@TimvdLippe in your editor, what happens when you change Bar's first line to class Bar implements Foo ? Does it give you the correct error?

Edit: from local testing, implements is only checked in TS, not JS, so that'll have to change as well.

[dragomirtitian]

@sandersn One other think that should happen is ensure that it ensure that the class is not instantiated. docs

The compiler verifies that interfaces are not instantiated. If the new keyword is used with an interface function, the compiler produces a warning

Maybe make the class abstract ?

[sandersn]

abstract is close — as is declare — but Typescript doesn't allow method bodies on abstracts.

[sandersn]

For a closure → typescript conversion, it might work just to put /** @private */ on the constructor of any @interface class (or create a private constructor for any interface that doesn't have one.)

[dragomirtitian]

Private constructor could work fine as well. I was thinking an abstract class but not make the methods abstract :

abstract class Foo {
    // @ts-ignore We would need to suppress return type checks
    method(): number {}
}

class Bar implements Foo{
    method() {
        return "bar";
    }
}

Play

The error message seems better out of the box too for an abstract class: Cannot create an instance of an abstract class.

Also not sure how strict TS is with JS declared return types vs actual return types, seems to be ok with it in the playground

[sandersn]

@dragomirtitian I looked at the closure documentation. Here's what I'm thinking right now:

1. We would accept a simple-ish PR to parse @implements and make it work exactly like implements does in TS. The compiler already does this for @extends, so you can use getEffectiveBaseTypeNode as a model. There are likely to be a number of places that need to be updated in the checker, or at least tested.
2. We would accept a simple PR to parse @abstract and make it work exactly like abstract in TS. The @private PR would be a good model here, since it's likely that a few places in the checker make Typescript-like assumptions.
3. I don't like supporting JSDoc tags with the same name as TS keywords, but different semantics. And the semantics of @interface are quite different from anything already in TS. I think we'd end up in a similar situation as with @enum, which I don't want to repeat.

@TimvdLippe do you want to weigh in? Once we have (1) and (2), I'd like to know how much further you can get with devtools-frontend.

[sandersn]

Another difference is that @interface is nominal in Closure, but that's not going to result in new errors, and is inherent limitation of switching to TS since it's structural everywhere.

[dragomirtitian]

@sandersn Ok, sounds like a plan, I will get started on 1 & 2 and see how it goes. Thank you for the hints about similar features, really appreciate it 😊.

[DanielRosenwasser]

don't like supporting JSDoc tags with the same name as TS keywords, but different semantics.

Strongly agree here, @interface would be confusing for existing TypeScript users applying their knowledge in JSDoc, and JSDoc users who would learn this and realize it means something different in TypeScript.

[TimvdLippe]

DevTools frontend makes heavy use of @interface. The goal of the CL in question was to have 1 file with only @interface's that are used both in Closure and TypeScript. That way, we can migrate over one by one and keep the @interface to make sure interactions with the classes are valid.

I am not sure whether 1 and 2 will be sufficient for our case. For JS/TS "bridges" that are used in both systems, should we declare them as @abstract instead? I am not sure if I follow there.

[dragomirtitian]

@sandersn from what I gather from @TimvdLippe 's comment it sounds like the @abstract annotation will not really work for this scenario. @abstarct is already used by Closure, and Closure does not allow @abstract classes to be used in @implements clauses (ex).

This means using a single file with classes, that clojure is meant to use as interfaces, and TS is meant to use as abstract classes is not possible without TS implementing @interface in some way. (reason: If the class is marked as @abstract it will not be usable in @implements in clojure, and if is not marked as @abstract it will be usable in @implements but it can be instantiated)

I think there is value in implementing abstract and implements as discussed above, I am just not sure it will be a solution for this scenario.

[sandersn]

I still think it can work with just @implements since Typescript's implements Class semantics are so close to Closure's @implements {Class} semantics. Let me recap the options for using a single file full of @interfaces that is consumed by both systems [1]:

1. Mark the interfaces both with @interface for Closure and @abstract for Typescript. This sounds like it won't work because @abstract also has meaning in Closure.
2. Add a @private constructor to every interface. This is likely to cause problems with Closure as well, but I'm not sure.
3. Skip checking that interfaces are not instantiated in Typescript. Rely on Closure to give errors for this during the migration. After the migration is done, convert all the interfaces to Typescript interfaces by dropping the method bodies.

I think (3) is a pretty good option because people aren't likely to instantiate interfaces at all, much less while porting code.

[1] I think that's what you're saying you need to do @TimvdLippe

[TimvdLippe]

@sandersn Yes that is correct. I think option 3 makes a lot of sense and would allow us to have a smoother migration path. Thanks for working on this!

[sandersn]

Sounds like a plan then.

@dragomirtitian this sounds like a good feature for TS 3.9, so I assigned both you and me for that milestone. If you don't have time to implement @implements before the 3.9 beta is over at the beginning of February, unassign yourself and I can do it.

[dragomirtitian]

@sandersn Ok, cool, I definitely have time to do it until then. I plan to have a rough PR ready by end of next week.

[sKopheK]

Hello,
when is it available in JS files upon checkJs: true settings?

[DanielRosenwasser]

TypeScript 3.9 or typescript@next. You can also use the TS/JS nightly plugin if you use VS Code: https://marketplace.visualstudio.com/items?itemName=ms-vscode.vscode-typescript-next

[sKopheK]

Seems like type import is not allowed

[TimvdLippe]

@sandersn It seems like VS Code doesn't automatically pick up the @implements clause. E.g. with TS 3.9.2 if I try to ctrl + click on the type in an @implements, it doesn't jump to that definition. @extends works as expected.

Should I file a separate issue for this and if so, on which repository?

Also, the implementations seems to be working correctly 😄 I still have to do a couple of more checks, but it seems to be working correctly.

[TimvdLippe]

Discovered an issue that prohibits us from using this feature yet 😭: #38640

Would be great if we can figure out a workaround somehow? Downstream this is implemented in https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2207238 with TypeScript changes made in https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2207236/1

[matthew-dean]

I've yet to see a code sample of @implements that:

1. correctly defines the type on a class definition,
2. correctly type-checks a class in a .js file using JSDoc (with allowJS: true) in CLI and VSCode.

The docs have long-maintained that @implements is supported as a way to define the type of a class, but I've yet to see any evidence that this is true? Is there something fundamental I'm missing? Is there working code that demonstrates that @implements actually works?