Skip to content

Check JSDoc @param tag names#18777

Merged
2 commits merged into
masterfrom
check-param
Sep 28, 2017
Merged

Check JSDoc @param tag names#18777
2 commits merged into
masterfrom
check-param

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 26, 2017

Related: #15852, but that's a broader issue so this won't fix it completely.

@ghost ghost requested a review from sandersn September 26, 2017 21:45
Copy link
Copy Markdown
Member

@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.

I don't think there should be an error in Typescript, or in cases where there is no typeExpression. Well, they should be warnings, if we had such things already.

Either way, I'd like to see tests for these cases.

Comment thread src/compiler/checker.ts
function checkJSDocParameterTag(node: JSDocParameterTag) {
checkSourceElement(node.typeExpression);
if (!getParameterSymbolFromJSDoc(node)) {
error(node.name,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a lint-style warning if there is no typeExpression, or if the typeExpression is ignored, the way it is in Typescript.
Until we have a warning infrastructure, I would rather not issue the error in those two cases.

Comment thread src/compiler/checker.ts

function checkJSDocParameterTag(node: JSDocParameterTag) {
checkSourceElement(node.typeExpression);
if (!getParameterSymbolFromJSDoc(node)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

getParameterSymbolFromJSDoc has a linear search of function.parameters, and we call this for every @param node. So it's quadratic where it could be linear if we wrote sufficiently clever code.

Might not be an issue, but I wanted to make sure you'd thought about it. (Most functions have few parameters and most @params are correct, so pathological cases should be rare.)

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 27, 2017

@sandersn Based on our discussion I think all of these points can be dealt with later? I could start working on performance tomorrow if this got in today.
To summarize:

  • Won't apply in TypeScript because we never visit the node.
  • Performance for matching @params with actual params is already terrible. We might fix this by requiring correct ordering or else issuing an error.

Copy link
Copy Markdown
Member

@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.

Yep, sounds good to me.

@ghost ghost force-pushed the check-param branch from 410dbfa to f0325ab Compare September 28, 2017 20:32
@ghost ghost merged commit 7959bd0 into master Sep 28, 2017
@ghost ghost deleted the check-param branch September 28, 2017 20:44
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
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.

1 participant