Skip to content
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

Add callback tag, with type parameters #23947

Merged
merged 56 commits into from
May 17, 2018
Merged

Add callback tag, with type parameters #23947

merged 56 commits into from
May 17, 2018

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented May 7, 2018

  1. Add the @callback tag.
  2. Makes @template-declared type parameters work correctly with @callback and @typedef.
  3. Parse and display comments on @callback and @typedef tags, not just their properties/parameters. Previously these were ignored.
/**
 * @callback Cb
 * @param {string} s
 * @return {number}
 */
/**
 * @template T
 * @typedef {Object} Counter
 * @property {number} n
 * @property {T} t
 */

The callback tag matches its parameters in order. The return tag must come last. Parameter names are not required to match:

/**
 * @callback Cb
 * @param {string} s
 */
/** @type {Cb} */
const f = ignored => { return; };

Note that a callback tag with a @template type parameter "uses up" that type parameter, even if the jsdoc comment is on a class or function. This only applies within a single comment, so you can get around this by using two comments:

/**
 * @template T
 * @callback Miracle
 * @return T
 */
/** @template T */
class C {
    /** @param {T} x */
    constructor(x) {
        /** @type {Miracle<T>} */ // This is the T from C<T>
        this.make = () => x
    }
}

Limitations:

  1. Templates still don't support declaring constraints on their type variables.
  2. There's no way to declare a construct signature.
  3. There's no way to declare an object with a call signature and properties using only the
  4. Nested object literal syntax doesn't work correctly. This:
/**
 * @callback {Wat}
 * @param {Object} o
 * @param {number} o.nested
 * @param {string} s
 */

is equivalent to
type Wat = (o: { nested: number, s: string }) => void not
type Wat = (o: { nested: number }, s: string) => void as it should be.

1,2 and 3 are limitations of JSDoc syntax that would require us to innovate with no real benefit. 4 might work in other jsdoc parsers, but I don't think it's that important a scenario; people who want to use @callback instead of Typescript syntax are likely to want to create a separate type alias for every type.

Fixes #23740
Fixes #23745
Fixes #7515

Edit: Now also fixes
#17339

By improving the scoping of @template tags, which was previously too wide.

sandersn added 30 commits May 1, 2018 10:16
Builds but tests still don't pass
Now I'm going to go rename some members to be more uniform. I hate
unnnecessary conditionals.
(maybe I'll rename more later)
Turns out there is a constraint in services such that they all need to
be named the same.
Also clean up some now-redundant code there to find the name of typedefs.
Also get rid of redundant typedef name code *in the binder*. It's
everywhere!
This requires almost no new code since it is basically the same as
typedefs
1. Haven't run it with all tests
2. Haven't tested typedef tags yet
3. Still allows shared symbols when on function or class declarations.
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.

Reminder: Some code introduced in #24153 should be changed before this is merged (to look up proper type params like this pr changes, and to support @callback like this pr supports), so quick info for callback tags is printed correctly. The TODO in the test in that PR should be removable once fixed.

@sandersn
Copy link
Member Author

OK, the TODO is done. @weswigham

/** @type {SharedId<number>} */
var outside = n => n + 1;

/** @type {Final<{ fantasy }, { heroes }>} */
Copy link
Member

Choose a reason for hiding this comment

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

😄

* @param {T} t
* @template {T}
Copy link
Member

Choose a reason for hiding this comment

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

Did this need to get reordered in the input file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I reordered it in the input file to show that @template works before or after. I was talking about it with @mhegazy and found that we had completely opposite intuitions about whether the usual order is before or after.

}

function getAliasTypeArgumentsForTypeNode(node: TypeNode) {
const symbol = getAliasSymbolForTypeNode(node);
function getTypeParametersForAliasSymbol(symbol: Symbol) {
Copy link
Member

Choose a reason for hiding this comment

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

This name should contain TypeArguments and not TypeParameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is an overall confusion in naming between typeArguments and typeParameters, because this function delegates to getLocalTypeParametersOfClassOrInstanceOfTypeAlias, which delegates to getEffectiveTypeParameterDeclarations. But then the result of this function is always assigned to aliasTypeArguments.

Ah, well, a big, complicated change is not the place to drop in another renaming.

Copy link
Member

Choose a reason for hiding this comment

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

Type arguments is already definitely correct here - aliasTypeArguments always hold the interstated type arguments.

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 just a some small comment about a name, but it looks decent. I do worry about how well we'll handle JSDocSignatures, though, given they don't conform to the same SignatureDeclarationBase (I'd almost rather you change SignatureDeclarationBase to accommodate JSDocSignature as a SignatureDeclaration); but w/e.

@sandersn
Copy link
Member Author

Yeah, I tried a couple of times to change SignatureDeclarationBase, but JSDocSignature was too different in shape and resulted in lots of special-case code throughout. I think this solution is slightly better.

sandersn added 2 commits May 17, 2018 09:01
The real fix is *probably* to rename Type.aliasTypeArguments to
aliasTypeParameters, but I want to make sure and then put it in a
separate PR.
@sandersn sandersn merged commit aa7e2b0 into master May 17, 2018
@TheLarkInn
Copy link
Member

This will land likely tonight I assume?

@mhegazy
Copy link
Contributor

mhegazy commented May 17, 2018

This will land likely tonight I assume?

yes

@mhegazy mhegazy deleted the jsdoc/callback branch May 17, 2018 19:11
weswigham added a commit that referenced this pull request May 17, 2018
* Optimize intersections of unions of unit types

* Add regression test

* Accept new baselines

* LEGO: check in for master to temporary branch.

* Add Unicode ThirdPartyNotice

* Properly handle edge cases

* Sort the whole diagnostic, plus giving up on isolating tests (#24186)

* Sort the whole diagnostic

* Also strip references to our repos node_modules, since removing it is hard

* LEGO: check in for master to temporary branch.

* add quick fix for import type missing typeof

* Add callback tag, with type parameters (#23947)

* Add initial tests

* Add types

* Half of parsing (builds but does not pass tests)

* Parsing done; types are uglier; doesn't crash but doesn't pass

* Bind callback tag

Builds but tests still don't pass

* Only bind param tags inside callback tags

* Fix binding switch to only handle param tags once

* Checking is 1/3 done or so.

Now I'm going to go rename some members to be more uniform. I hate
unnnecessary conditionals.

* Rename typeExpression to type (for some jsdoc)

(maybe I'll rename more later)

* Rename the rest of typeExpressions

Turns out there is a constraint in services such that they all need to
be named the same.

* Few more checker changes

* Revert "Rename the rest of typeExpressions"

This reverts commit f41a96b.

* Revert "Rename typeExpression to type (for some jsdoc)"

This reverts commit 7d2233a.

* Finish undoing typeExpression rename

* Rename and improve getTypeParametersForAliasSymbol

Plus some other small fixes

* Core checking works, but is flabbergastingly messy

I'm serious.

* Callback return types work now

* Fix crash in services

* Make github diff smaller

* Try to make github diff even smaller

* Fix rename for callback tag

* Fix nav bar for callback tag

Also clean up some now-redundant code there to find the name of typedefs.

* Handle ooorder callback tags

Also get rid of redundant typedef name code *in the binder*. It's
everywhere!

* Add ooorder callback tag test

* Parse comments for typedef/callback+display param comments

* Always export callbacks

This requires almost no new code since it is basically the same as
typedefs

* Update baselines

* Fix support for nested namespaced callbacks

And add test

* Callbacks support type parameters

1. Haven't run it with all tests
2. Haven't tested typedef tags yet
3. Still allows shared symbols when on function or class declarations.

* Template tags are now bound correctly

* Test oorder template tags

It works.

* Parser cleanup

* Cleanup types and utilities

As much as possible, and not as much as I would like.

* Handle callback more often in services

* Cleanup of binder and checker

* More checker cleanup

* Remove TODOs and one more cleanup

* Support parameter-less callback tags

* Remove extra bind call on template type parameters

* Bind template tag containers

Doesn't quite work with typedefs, but that's because it's now stricter,
without the typedef fixes. I'm going to merge with jsdoc/callback and
see how it goes.

* Fix fourslash failures

* Stop pre-binding js type aliases

Next up, stop pre-binding js type parameters

* Further cleanup of delayed js type alias binding

* Stop prebinding template tags too

This gets rid of prebinding entirely

* Remove TODO

* Fix lint

* Finish merge with use-jsdoc-aliases

* Update callback tag baselines

* Rename getTypeParametersForAliasSymbol

The real fix is *probably* to rename Type.aliasTypeArguments to
aliasTypeParameters, but I want to make sure and then put it in a
separate PR.

* moveToNewFile: Don't move imports (#24177)

* Reduce map lookups (#24203)

* Fix jsdoc type resolution [merge to master] (#24204)

* Fix JSDoc type resolution

Breaks type parameter resolution that is looked up through prototype
methods, though. I need to fix that still.

* Check for prototype method assignments first

* Undo dedupe changes to getJSDocTags

* JS Type aliases can't refer to host type params

Previously, js type aliases (@typedef and @callback) could refer to
type paremeters defined in @template tags in a *different* jsdoc tag, as
long as both tags were hosted on the same signature.

* Reduce dedupe changes+update baseline

The only reason I had undone them was to merge successfully with an
older state of master.

* Add undefined guard

* moveToNewFile: Fix bug for VariableDeclaration missing initializer (#24214)

* Use import types to refer to declarations in declaration emit (#24071)

* Stand up a simple implementation using import types for exports of modules which are otherwise inaccessible

* Ensure references exist to link to modules containing utilized ambient modules

* Accept baselines with new import type usage

* Fix lint

* Accept changed baseline (#24222)

* fixUnusedIdentifier: Don't delete node whose ancestor was already deleted (#24207)

* More robust circularity detection in node builder (#24225)

* Improve ChangeTracker#deleteNodeInList (#24221)

* moveToNewFile: Fix bug for missing importClause (#24224)

* Set startPos at EOF in jsdoc token scanner so node end positions for nodes terminated at EoF are right (#24184)

* Set startPos at EOF in jsdoc token scanner to node end positions for nodes terminated at EoF are right

* More complete nonwhitespace token check, fix syntactica jsdoc classifier

* Use loop and no nested lookahead

* Do thigns unrelated to the bug in the test

* Fix typo move return

* Patch up typedef end pos

* Fix indentation, make end pos target more obvious
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants