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

Parse jsdoc @param type literals #17352

Merged
merged 9 commits into from
Jul 26, 2017
Merged

Parse jsdoc @param type literals #17352

merged 9 commits into from
Jul 26, 2017

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Jul 21, 2017

Now Typescript supports the creation of anonymous types using successive @param lines in JSDoc:

/**
 * @param {object} o - has a string and a number
 * @param {string} o.s - the string
 * @param {number} o.n - the number
 */
function f(o) { return o.s.length + o.n; }

This is equivalent to the Typescript syntax { s: string, n: number }, but it allows per-property documentation, even for types that only need to be used in one place. (@typedef can be used for reusable types.)

If the type of the initial @param is {object[]}, then the resulting type is an array of the specified anonymous type:

/**
 * @param {Object[]} os - has a string and a number
 * @param {string} os[].s - the string
 * @param {number} os[].n - the number
 */
function f(os) { return os[0].s; }

Finally, nested anonymous types can be created by nesting the pattern:

/**
 * @param {Object[]} os - has a string and a number
 * @param {string} os[].s - the string
 * @param {object} os[].nested - it's nested because of the object type
 * @param {number} os[].nested.length - it's a number
 */
function f(os) { return os[0].nested.length; }

Implementation notes:

  1. I refactored JSDocParameterTag and JSDocPropertyTag to JSDocPropertyLikeTag and modified its parsing to be more succinct. These changes make the overall change easier to read but are not strictly required.
  2. parseJSDocEntityName accepts postfix[] as in os[].nested.length, but it doesn't check that usages are correct. Such checking would be easy to add but tedious and low-value.
  3. @typedef doesn't support nested @property tags, but does support object[] types. This is mostly a practical decision, backed up by the fact that usejsdoc.org doesn't document nested types for @typedef.

Fixes #11597

Now Typescript supports the creation of anonymous types using successive
`@param` lines in JSDoc:

```js
/**
 * @param {object} o - has a string and a number
 * @param {string} o.s - the string
 * @param {number} o.n - the number
 */
function f(o) { return o.s.length + o.n; }
```

This is equivalent to the Typescript syntax `{ s: string, n: number }`,
but it allows per-property documentation, even for types that only need
to be used in one place. (`@typedef` can be used for reusable types.)

If the type of the initial `@param` is `{object[]}`, then the resulting
type is an array of the specified anonymous type:

```js
/**
 * @param {Object[]} os - has a string and a number
 * @param {string} os[].s - the string
 * @param {number} os[].n - the number
 */
function f(os) { return os[0].s; }
```

Finally, nested anonymous types can be created by nesting the pattern:

```js
/**
 * @param {Object[]} os - has a string and a number
 * @param {string} os[].s - the string
 * @param {object} os[].nested - it's nested because of the object type
 * @param {number} os[].nested.length - it's a number
 */
function f(os) { return os[0].nested.length; }
```

Implementation notes:

1. I refactored JSDocParameterTag and JSDocPropertyTag to
JSDocPropertyLikeTag and modified its parsing to be more succinct. These
changes make the overall change easier to read but are not strictly
required.
2. parseJSDocEntityName accepts postfix[] as in `os[].nested.length`,
but it doesn't check that usages are correct. Such checking would be
easy to add but tedious and low-value.
3. `@typedef` doesn't support nested `@property` tags, but does support
`object[]` types. This is mostly a practical decision, backed up by the
fact that usejsdoc.org doesn't document nested types for `@typedef`.
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.

Generally agreeable; there are some refactorings of how jsdoc params and properties are stored which I like. Only thing I'd mention is that there are quite a few ts.isIdentifier which would normally be just isIdentifier (unless there's a reason not to).

The whole name first/name second thing feels a little hairy to deal with; I almost feel it would be neater to handle if each variant were a different SyntaxKind. (I mean, I guess they do parse differently, so it'd make sense) Up to you, though. This feels better than how it was handled before, anyway.

@@ -1553,6 +1553,10 @@ namespace ts {

/** Does the opposite of `getJSDocParameterTags`: given a JSDoc parameter, finds the parameter corresponding to it. */
export function getParameterFromJSDoc(node: JSDocParameterTag): ParameterDeclaration | undefined {
Copy link

@ghost ghost Jul 24, 2017

Choose a reason for hiding this comment

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

See the (sole) use of this function -- at x in @param obj.x we should return the symbol for x. Add a find-all-references test.

/**
 * @param obj.[|x|]
 */
function f(obj) {
    obj.[|x|];
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

return declareSymbolAndAddToSymbolTable(node as JSDocPropertyTag,
(node as JSDocPropertyTag).isBracketed || ((node as JSDocPropertyTag).typeExpression && (node as JSDocPropertyTag).typeExpression.type.kind === SyntaxKind.JSDocOptionalType) ?
return declareSymbolAndAddToSymbolTable(node as JSDocPropertyLikeTag,
(node as JSDocPropertyLikeTag).isBracketed || ((node as JSDocPropertyLikeTag).typeExpression && (node as JSDocPropertyLikeTag).typeExpression.type.kind === SyntaxKind.JSDocOptionalType) ?
Copy link

Choose a reason for hiding this comment

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

This expression is too large.

const prop = node as JSDocPropertyLikeTag;
const flags = prop.isBracketed || prop.typeExpression && prop.typeExpression.type.kind === SyntaxKind.JSDocOptionalType
    ? SymbolFlags.Property | SymbolFlags.Optional
    : SymbolFlags.Property;
return declareSymbolAndAddToSymbolTable(prop, flags, SymbolFlags.PropertyExcludes);

Also, typeExpression is declared non-optional, but here we test for its presence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

declaration = getDeclarationOfKind<TypeAliasDeclaration>(symbol, SyntaxKind.TypeAliasDeclaration);
type = getTypeFromTypeNode(declaration.type);
}
const declaration = getDeclarationOfKind<JSDocTypedefTag>(symbol, SyntaxKind.JSDocTypedefTag) ||
Copy link

Choose a reason for hiding this comment

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

getDeclarationOfKind hides a loop, so this loops twice. Could add a version of that function that takes two kinds.

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'll just use findDeclaration which takes a predicate.

Copy link

@ghost ghost Jul 25, 2017

Choose a reason for hiding this comment

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

I was going to delete that function in #17380 since it's equivalent to just find(symbol.declarations, ...) -- this would be one of only 2 uses.

typeExpression: JSDocTypeExpression;
/** Whether the property name came before the type -- non-standard for JSDoc, but Typescript-like */
isParameterNameFirst: boolean;
Copy link

Choose a reason for hiding this comment

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

If this applies to more than just parameters, it should be renamed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

visitNode(cbNode, (<JSDocParameterTag>node).typeExpression) ||
visitNode(cbNode, (<JSDocParameterTag>node).postParameterName);
case SyntaxKind.JSDocPropertyTag:
if ((node as JSDocPropertyLikeTag).isParameterNameFirst) {
Copy link

Choose a reason for hiding this comment

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

Maybe this should wait for a separate PR, but I would much prefer to see code like:

const { isParameterNameFirst, fullName, typeExpression } = node as JSDocPropertyLikeTag;

in all of these cases, instead of repeated casting.

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 will make the change here if you are willing to review it! 😅 I'll put it in a separate commit at the end which might make it easier.

Copy link

Choose a reason for hiding this comment

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

Please make a separate PR as that would be a huge diff.

visitNode(cbNode, (<JSDocTypedefTag>node).fullName) ||
visitNode(cbNode, (<JSDocTypedefTag>node).name) ||
visitNode(cbNode, (<JSDocTypedefTag>node).jsDocTypeLiteral);
if ((node as JSDocTypedefTag).typeExpression &&
Copy link

Choose a reason for hiding this comment

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

It looks like name is a pointer to a node within fullName. So it would be visited twice if we were recursively calling forEachChild.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! I added fullName later and didn't notice that name needed to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

typeExpression = tryParseTypeExpression();
}

const result = shouldParseParamTag ?
const result: JSDocPropertyLikeTag = target ?
Copy link

Choose a reason for hiding this comment

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

Since target is now an enum, use an explicit comparison instead of treating it like a boolean.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}

function parseParameterOrPropertyTag(atToken: AtToken, tagName: Identifier, shouldParseParamTag: boolean): JSDocPropertyTag | JSDocParameterTag {
function isObjectOrObjectArrayTypeReference(node: TypeNode): boolean {
return node.kind === SyntaxKind.ObjectKeyword ||
Copy link

Choose a reason for hiding this comment

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

Use a switch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link

Choose a reason for hiding this comment

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

isTypeReferenceNode just tests the syntax kind, so just make that a switch case and have default: return false;.

skipWhitespace();

let preName: Identifier, postName: Identifier;
let preName: EntityName, postName: EntityName;
Copy link

Choose a reason for hiding this comment

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

It no longer makes sense to have separate preName and postName variables. Just create isParameterNameFirst directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


function tryParseChildTag(parentTag: JSDocTypeLiteral): boolean {
function parseChildParameterOrPropertyTag(target: PropertyLikeParse.Property): JSDocTypeTag | JSDocPropertyTag | false;
Copy link

Choose a reason for hiding this comment

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

Since there are only 2 possible targets, maybe there should just be 2 different functions parseChildParmeterTag and parseChildPropertyTag that both use parseChildParameterOrPropertyTagCommon?

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 a more likely refactoring would be to pass 1 of 2 different tryParseChildTag functions instead of the target enum. But the target enum pattern is also used with parseParameterOrPropertyTag, and it's as easy to read and (probably?) as fast to run, so I think I'll keep it this way.

jsdocTypeLiteral = <JSDocTypeLiteral>createNode(SyntaxKind.JSDocTypeLiteral, start);
jsdocTypeLiteral.jsDocPropertyTags = [] as MutableNodeArray<JSDocPropertyTag>;
}
(jsdocTypeLiteral.jsDocPropertyTags as MutableNodeArray<JSDocPropertyTag>).push(child as JSDocPropertyTag);
Copy link

Choose a reason for hiding this comment

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

You could just the create the mutable array locally and assign to jsdocTypeLiteralat the end. That way you avoid casting an already-created object to mutable.
This loop could probably be its own function parseParameterTags returning NodeArray<JSDocPropertyTag> | undefined.
Also, you don't appear to be setting the position on the array -- if you don't want to do that, just declare it as an ReadonlyArray and not a NodeArray.
Also, what's with the cast on child? You passed in PropertyLikeParse.Parameter so it's surprising to see an assertion that what you got back isn't a parameter.

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 don't know a correct position to set on the child array because I don't know what would use it. I don't think I can make it a ReadonlyArray though, because forEachChild expects to iterate over a NodeArray. For now I'll still create a NodeArray with no position.

Copy link

Choose a reason for hiding this comment

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

You could just use a for loop instead of calling cbNodes, which does require a NodeArray, meaning users might be expecting it to have a position.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Done.

result.postParameterName = postName;
result.name = postName || preName;
if (typeExpression) {
result.type = typeExpression.type;
Copy link

Choose a reason for hiding this comment

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

Isn't this redundant? Why do we have both typeExpression and type?

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 wasn't sure how much like a real VariableDeclaration JSDocPropertyLikeTag should become and ended up halfway there. It turns out to be easier to continue special-casing it in getTypeOfVariableOrParameterOrProperty instead of having it become a full-fledged declaration. I dropped type in favour of typeExpression.

result.type = typeExpression.type;
}
result.fullName = postName || preName;
result.name = ts.isIdentifier(result.fullName) ? result.fullName : result.fullName.right;
Copy link

Choose a reason for hiding this comment

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

This is also redundant, I would just make this a function of the node and not a property.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was another convenience shim. I turned name into fullName and pushed the isIdentifier check into getNameOfDeclaration.

result.isBracketed = isBracketed;
return finishNode(result);

Copy link

Choose a reason for hiding this comment

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

Nit: line before a closing bracket doesn't really accomplish anything -- nothing to separate that isn't already separated by the closing bracket.

}
if (child.kind === SyntaxKind.JSDocTypeTag) {
if (alreadyHasTypeTag) {
break;
Copy link

Choose a reason for hiding this comment

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

What happens to child in this case? We just drop it with no syntax error?

Copy link

Choose a reason for hiding this comment

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

I think this comment is still unaddressed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It happens in this case:

/**
 * @typedef Name
 * @type {string}
 * @type {Oops} - this is dropped and we stop parsing the @typedef
 */

This would be a grammar error if we did more thorough checking of jsdoc structure in the checker (along with a lot of other incorrect-but-allowed jsdoc). We don't, which is OK relative to what we used to provide. We should probably improve in the future if jsdoc becomes the source of types for significant, typed codebases.

Copy link

Choose a reason for hiding this comment

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

OK, added #17442.

}
else {
if (!jsdocTypeLiteral.jsDocPropertyTags) {
jsdocTypeLiteral.jsDocPropertyTags = [] as MutableNodeArray<JSDocPropertyTag>;
Copy link

Choose a reason for hiding this comment

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

See earlier comment on casting to MutableNodeArray.

if (!typedefTag.jsDocTypeLiteral) {
typedefTag.jsDocTypeLiteral = <JSDocTypeLiteral>typeExpression.type;
if (jsdocTypeLiteral) {
if (typeExpression && typeExpression.type.kind === SyntaxKind.ArrayType) {
Copy link

Choose a reason for hiding this comment

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

What are our performance guidelines on optionally vs unconditionally assigning to a property?

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 thought the standard for booleans was to declare them as b?: boolean and only assign them when needed. What difference in performance could arise? Conditional assignment saves space (but how much I don't know) versus unconditional assignment, which could be better for [de]optimisation. @rbuckton do you have an opinion?

@@ -6647,8 +6671,61 @@ namespace ts {
}
}

function textsEqual(parent: EntityName, name: EntityName): boolean {
Copy link

Choose a reason for hiding this comment

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

Nit: would just name these variables a and b since they don't really depend on a parent-child context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

while (token() !== SyntaxKind.EndOfFileToken) {
nextJSDocToken();
switch (token()) {
case SyntaxKind.AtToken:
Copy link

Choose a reason for hiding this comment

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

Nit: indent switch body

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

const child = tryParseChildTag(target);
if (child && child.kind === SyntaxKind.JSDocParameterTag &&
(ts.isIdentifier(child.fullName) || !textsEqual(fullName, child.fullName.left))) {
break;
Copy link

@ghost ghost Jul 25, 2017

Choose a reason for hiding this comment

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

Should this be break loop? It looks like the way it is currently, it goes all the way to the end of the file, when we could stop the moment we see something that isn't an @param tag with the correct name.

Copy link
Member Author

Choose a reason for hiding this comment

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

This function is always called from in a speculative context, so I can actually just return false.

break;
}
}
scanner.setTextPos(resumePos);
Copy link

@ghost ghost Jul 25, 2017

Choose a reason for hiding this comment

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

Do we have an established pattern for re-setting the scanner? What if scanner has some other state that this doesn't reset?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this is left over from the old scanChildTags that faked its own speculative parsing. Removed.

scanner.setTextPos(resumePos);
}

function tryParseChildTag(target: PropertyLikeParse, alreadyHasTypeTag?: boolean): JSDocTypeTag | JSDocPropertyTag | JSDocParameterTag | false {
Copy link

Choose a reason for hiding this comment

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

This is only called in one place, and alreadyHasTypeTag is never provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I moved that up a couple of levels and forgot to remove it here.

// Error parsing property tag
return false;
return target === PropertyLikeParse.Property && parseParameterOrPropertyTag(atToken, tagName, target);
case "arg":
Copy link

Choose a reason for hiding this comment

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

Is the information about what tag name was used available in the AST? That might be useful for linting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, JSDocTag.tagName: Identifier, so it looks like it's preserved on all JSDocTags.

A more useful lint rule would be to look for mismatched typedef/param and param/property nestings, since the compiler currently ignores them:

/**
 * @typedef {object} x
 * @param {string} x.a - WRONG! (but ignored by the compiler)
 *
 * @param {object} y
 * @property {string} y.a - WRONG! (but ignored by the compiler)
 */

@@ -6728,6 +6794,26 @@ namespace ts {
return currentToken = scanner.scanJSDocToken();
}

function parseJSDocEntityName(createIfMissing = false): EntityName {
Copy link

Choose a reason for hiding this comment

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

This is only called in one place, and createIfMissing is always true.

Copy link

@ghost ghost Jul 25, 2017

Choose a reason for hiding this comment

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

This could use:

function createQualifiedName(entity: EntityName, name: Identifier): QualifiedName {
    const node = createNode(SyntaxKind.QualifiedName, entity.pos) as QualifiedName;
    node.left = entity;
    node.right = name;
    return finishNode(node);
}

And you can re-use createQualifiedName in parseEntityName. (parseJSDocEntityName should probably be near parseEntityName since they are similar.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I thought about merging parseEntityName and parseJSDocEntityName, but they both worry about ad-hoc jsdoc-related things, plus they call into different identifier parsers which do the same.

pushClassification(tag.postParameterName.pos, tag.postParameterName.end - tag.postParameterName.pos, ClassificationType.parameterName);
pos = tag.postParameterName.end;
if (!tag.isParameterNameFirst) {
pushCommentRange(pos, tag.name.pos - pos);
Copy link

Choose a reason for hiding this comment

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

These two if blocks are the same, so use this helper:

function pushName(name: Identifier) {
    pushCommentRange(pos, name.pos - pos);
    pushClassification(name.pos, name.end - name.pos, ClassificationType.parameterName);
    pos = name.end;
}

Copy link

Choose a reason for hiding this comment

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

Don't know why this shows as outdated, it still seems relevant.

@@ -120,7 +120,7 @@ namespace ts.JsDoc {
}

export function getJSDocParameterNameCompletions(tag: JSDocParameterTag): CompletionEntry[] {
const nameThusFar = unescapeLeadingUnderscores(tag.name.text);
const nameThusFar = isIdentifier(tag.fullName) ? unescapeLeadingUnderscores(tag.name.text) : undefined;
Copy link

Choose a reason for hiding this comment

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

I think we should bail out immediately if it's not an identifier, instead of continuing with an undefined nameThusFar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I returned emptyArray instead of undefined. I'm not sure whether that's correct.

Copy link

Choose a reason for hiding this comment

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

Should be fine.

I'll take a look at Wesley's next and see if those require any changes.
Copy link
Member Author

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Responses to review comments

return declareSymbolAndAddToSymbolTable(node as JSDocPropertyTag,
(node as JSDocPropertyTag).isBracketed || ((node as JSDocPropertyTag).typeExpression && (node as JSDocPropertyTag).typeExpression.type.kind === SyntaxKind.JSDocOptionalType) ?
return declareSymbolAndAddToSymbolTable(node as JSDocPropertyLikeTag,
(node as JSDocPropertyLikeTag).isBracketed || ((node as JSDocPropertyLikeTag).typeExpression && (node as JSDocPropertyLikeTag).typeExpression.type.kind === SyntaxKind.JSDocOptionalType) ?
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

visitNode(cbNode, (<JSDocTypedefTag>node).fullName) ||
visitNode(cbNode, (<JSDocTypedefTag>node).name) ||
visitNode(cbNode, (<JSDocTypedefTag>node).jsDocTypeLiteral);
if ((node as JSDocTypedefTag).typeExpression &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

typeExpression: JSDocTypeExpression;
/** Whether the property name came before the type -- non-standard for JSDoc, but Typescript-like */
isParameterNameFirst: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -1553,6 +1553,10 @@ namespace ts {

/** Does the opposite of `getJSDocParameterTags`: given a JSDoc parameter, finds the parameter corresponding to it. */
export function getParameterFromJSDoc(node: JSDocParameterTag): ParameterDeclaration | undefined {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

declaration = getDeclarationOfKind<TypeAliasDeclaration>(symbol, SyntaxKind.TypeAliasDeclaration);
type = getTypeFromTypeNode(declaration.type);
}
const declaration = getDeclarationOfKind<JSDocTypedefTag>(symbol, SyntaxKind.JSDocTypedefTag) ||
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'll just use findDeclaration which takes a predicate.

@sandersn
Copy link
Member Author

@weswigham

  1. The parser defines its own isIdentifier which operates while parsing an identifier, unlike the normal ts.isIdentifier.
  2. I kind of like the idea of having different syntax kinds for (1) normal @param (2) type literal @param (3) non-standard @param, but I'm not sure it's worth the effort. I'll think about it and maybe include it in this PR and maybe not.

let seenAsterisk = false;
while (true) {
nextJSDocToken();
switch (token()) {
Copy link

Choose a reason for hiding this comment

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

Can we return false; in the default case? It looks like if we keep encountering some other kind of token, we'll go all the way to the end of the 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.

Not really; we hit the default case a lot for punctuation and identifiers that we want to skip as part of the tag's description.

Note that it's the end of the comment, not the end of the whole file. And we return false as soon as tryParseChildTag encounters any tag that is not param, type or property. So runaway parsing shouldn't be a big problem.

Also note: Comments aren't handled by this PR but once they are, default will probably look like parseJSDocCommentWorker, where it just pushes the token into a comment array.

@sandersn
Copy link
Member Author

I think this is ready to go unless you have more comments.

@sandersn
Copy link
Member Author

Addressed the comment I missed previously.

@sandersn sandersn merged commit fdb4465 into master Jul 26, 2017
@sandersn sandersn deleted the jsdoc-param-type-literals branch July 26, 2017 22:17
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intellisense for object properties defined in multi-line JSDoc comments
3 participants