Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for @implements. #36292

Draft
wants to merge 2 commits into
base: master
from

Conversation

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Jan 18, 2020

Fixes #35629

Open issues:

  1. How to handle interfaces that can't be implemented in JS, for example those with index signatures
  2. Fix code fix that implements missing interface members (currently it does not show up, but it would be nice if it worked for JS as well)
  3. Add more tests

@sandersn For 1 I would suppress these errors for JS. For 2, do you think it is worth showing the fix? Also maybe we could have a typescript-bot pack of this so @TimvdLippe can maybe give it a try.

One issue, unrelated to @implements necessarily, is that method in the classes used as interfaces in @TimvdLippe example don't have any return statement, so any return type will cause an error. Support for @abstract was taken out of the scope of #35629, but maybe adding support for @abstract could be useful to suppress these errors. Using @ts-ignore might also be a simpler workaround.

@TimvdLippe

This comment has been minimized.

Copy link

TimvdLippe commented Jan 18, 2020

Yes I would like to try it out. I will be able to do so next week when I am back in the office.

@TimvdLippe

This comment has been minimized.

Copy link

TimvdLippe commented Jan 20, 2020

To be able to debug, I would need a bot-pack build that I can try out. Let me know when you have one available 😄

@dragomirtitian

This comment has been minimized.

Copy link
Contributor Author

dragomirtitian commented Jan 20, 2020

@TimvdLippe I unfortunately can't summon the TS bot .. let me try something, <clears-throat />

<incantation-voice>
Oh mighty and benevolent TS overlords @sandersn @orta or @DanielRosenwasser could we have a pack of this ?
</incantation-voice>

@orta

This comment has been minimized.

Copy link
Member

orta commented Jan 21, 2020

Sure!
@typescript-bot pack this

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Jan 21, 2020

Heya @orta, I've started to run the tarball bundle task on this PR at 5a7fdd5. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Jan 21, 2020

Hey @orta, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/61139/artifacts?artifactName=tgz&fileId=9B83D1D6A5EF666A0539602791C1631D23484EBBA1F4A0A5D118C77653B5832102&fileName=/typescript-3.8.0-insiders.20200121.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@sandersn

This comment has been minimized.

Copy link
Member

sandersn commented Jan 22, 2020

@dragomirtitian re your questions:

  1. Suppressing errors that can't be fixed in JS is a good idea. We can re-instate them later if we come up with a way to fix them.
  2. I believe the code you emit for the code fix would have to be different, at least in that it uses jsdoc instead of type annotations. And it may have to fail because of errors from (1). That is a lot of work so you can open a new issue instead and assign it to me (or ping me on it).

I will look at the code now.

I'm a linguist, so I prefer the traditional 3 invocations. Like this: @NoamChomsky @NoamChomsky @NoamChomsky

Copy link
Member

sandersn left a comment

Looks good so far. I have a couple of initial suggestions.

As for tests, don't forget fourslash tests -- I think we suggest completions based on implements so the same thing should work for @implements.

src/compiler/types.ts Outdated Show resolved Hide resolved
src/compiler/utilities.ts Show resolved Hide resolved
src/compiler/utilitiesPublic.ts Show resolved Hide resolved
@TimvdLippe

This comment has been minimized.

Copy link

TimvdLippe commented Jan 23, 2020

I can confirm that the updated build is catching issues like missing overrides. That said, we are hitting a couple of other issues with TypeScript (#19983) which makes it hard for us to fully test all of its internals. My understanding thus far is that this change is WAI.

One improvement that would be great is that methods with @return on an @interface do not result in the A function whose declared type is neither 'void' nor 'any' must return a value. error. At that point, I don't see any TypeScript blockers for supporting JSDoc interfaces (without special-casing for TypeScript).

Thank you for doing the work and for the quick implementation!

@dragomirtitian

This comment has been minimized.

Copy link
Contributor Author

dragomirtitian commented Jan 23, 2020

@TimvdLippe The scope of this was specifically defined as support for @implements but I saw the same issue as you with regard to the actual usability of this with functions using @return, and I mentioned it in my opening comment to this pr. If @sandersn gives me the go ahead to suppress the error for classes marked with @interface I can roll up that chage in this pr, no problem (would probably have it done by Monday).

If not I think the only workaround for now would be to @ts-ignore it, although I know that is not really a viable solution for this existing code base...

@sandersn

This comment has been minimized.

Copy link
Member

sandersn commented Jan 23, 2020

@dragomirtitian I'd like to keep support for @interface in a separate PR. Anyway, I'm not keen on the idea of ignoring errors on what would be invalid Typescript.
@TimvdLippe Is // @ts-nocheck at the beginning of the interface file a solution? I assume that it will be replaced by a d.ts once you are further along in the migration.

@TimvdLippe

This comment has been minimized.

Copy link

TimvdLippe commented Jan 23, 2020

@sandersn Given our extensive use of @interface, a no-check would probably not fly. Especially as the interfaces and usages are scattered throughout the codebase. I suppose adding ts-ignore for every single method is the least intrusive (interim) solution.

I am okay with punting that to a different PR though.

@sandersn

This comment has been minimized.

Copy link
Member

sandersn commented Jan 23, 2020

Looking at the example in the jsdoc documentation (excerpted below), I note that throwing in the interface body satisfies Typescript because throwing causes the function to return never, which is assignable to anything:

/**
 * @interface
 */
function Color() {}

/**
 * @returns {Array<number>} An array containing the red, green, and blue values, in that order.
 */
Color.prototype.rgb = function() {
    throw new Error('not implemented');
};

Does that work with Closure?

@TimvdLippe

This comment has been minimized.

Copy link

TimvdLippe commented Jan 27, 2020

@sandersn I just checked and the following does indeed compile in both Closure and TS (with this patch):

/**
 * @interface
 */
export class EventTarget {
  /**
   * @param {symbol} eventType
   * @param {function(!Common.Event)} listener
   * @param {!Object=} thisObject
   * @return {!Common.EventTarget.EventDescriptor}
   */
  addEventListener(eventType, listener, thisObject) {
    throw new Error(`Not implemented`);
  }
}
@dragomirtitian

This comment has been minimized.

Copy link
Contributor Author

dragomirtitian commented Feb 2, 2020

@sandersn I fixed the code review issues. I added a test for multiple interfaces. I ran into the following issues;

  1. I am not sure exactly how to suppress the error for missing signature implementation in JS. I mean I know how the error is produced (the error is deep in the bowels of checkTypeRelatedTo which is called when checking the heritage clauses). My issue is I am unsure how to suppress errors coming from there. I could add a parameter to checkTypeRelatedTo to skip index checks during implements checks, but that seems very ad hoc. The other option might be to capture diagnostics using errorOutputContainer but I would have to see if that could work. If you can think of an example similar to this, I could use the 'inspiration' to make sure I don't go off the rails 😂.

  2. You mentioned four-slash tests for type completions in @implements which would be similar to @extends. The issue is but I could not trigger it completions for @augments and @extends at all I would create a separate issue to fix both. Thoughts?

Also sorry for the delay, it's been a hectic week at work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.