Skip to content

Fix @implements emit for namespaced base types #38688

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

Merged
merged 8 commits into from
Jun 12, 2020
Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented May 20, 2020

Fixes #38640

Couple of outstanding questions:

  1. Whether an inaccessible base type is possible -- currently we only allow type references here, which is fine for chrome-devtools, but doesn't work for the pure JS case where somebody wants to declare that they implement a class that they acquired via require -- require doesn't create a real alias today, but a value, which cannot be used for implements.
  2. serializeImplementedType contains a duplicate of trySerializeTypeReference, except that it calls isTypeSymbolAccessible instead of getSymbolAccessibilityChain. Both should call the same function, and then the code should be de-duped. But I'm not at all sure whether getSymbolAccessibilityChain should be expanded or whether both should call of isSymbolAccessible.

If (1) is impossible, then I can delete a lot of error reporting machinery that I currently haven't found a way to test.

Here's some more detail:

Inaccessible base type

@implements' type reference doesn't seem to add Value to name lookup the way the rest of jsdoc lookups do. I'm not sure why yet. However, chrome-devtools is referring to types from a .d.ts or global classes, as far as I know. However, the following code should probably work too:

const C = require('m')
/** @implements {C} */
class D { }

I just don't think there's much demand for that, considering that I didn't observe much usage of @implements outside closure codebases, and if people write new jsdoc, they can work around the limitation for now by creating a type alias for the type of the class: @typedef C {typeof import('m').C}.

Duplication in serializeImplementedType

I'm going to try switching trySerializeTypeReference to isValueSymbolAccessible, and pass allowModules: false.

sandersn added 4 commits May 20, 2020 14:43
1. Switch to isSymbolAccessible for both types and values, then unify to
a single function.
2. Remove inaccesible base error. We can put it back after making
@implements type reference lookup looser (which may or may not happen).
@sandersn
Copy link
Member Author

@weswigham I decided to try isSymbolAccessible for everything, and it passes all of suite 0. What additional tests even use d.ts emit? We don't have good coverage here.

if (ref) {
return ref;
}
return createExpressionWithTypeArguments(/*typeArgs*/ undefined, symbolToExpression(t.symbol, context, SymbolFlags.Type));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If t.symbol is undefined (which is often can be), I'm pretty sure this crashes? Or makes a node which'll crash later?

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this looks now, though I am concerned with the unguarded t.symbol reference (since it only actually exists on anonymous object types and interface types).

@weswigham
Copy link
Member

What additional tests even use d.ts emit?

JS .d.ts emit is just the relevant conformance folder, mostly. It has a fair quantity of tests in it; but the area to cover is massive.

@TimvdLippe
Copy link
Contributor

This PR appears to be approved, but not merged yet. Is there anything I could help out with to get this over the line? Not sure how best we could test this in our repository, I assume we need it packed for that?

@sandersn
Copy link
Member Author

Sorry, casualty of some urgent internal work that came up. I'll see if I can address @weswigham's comment about undefined t.symbol.

@sandersn sandersn merged commit 45e6805 into master Jun 12, 2020
@sandersn sandersn deleted the fix-implements-tag-emit branch June 12, 2020 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declaration file for @implements of type specified in .d.ts file includes broken syntax
4 participants