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

Traverse JSDoc nodes if present #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sandersn
Copy link

Fixes #18

This change has two open questions that manifest as ugly code:

  1. The jsDoc property is not exposed in the Typescript API because the intended interface is getJSDocTags, which does a lot more work to provide a list of tags that are collected directly from a node, or any one of several parent nodes. I use type assertions to any to access the property without checking.
  2. The jsDoc property is of type JSDoc[], not Node, so it can't be traversed directly. I decided to add the discovered JSDoc nodes as children before the actual children, since that's where JSDoc appears syntactically. But it's odd that that each tag appears as an individual child instead of appearing under some synthetic jsdoc node. Unfortunately, I don't know tsquery well enough to know if such a synthetic node is possible, or a good idea.

Both problems boil down to "JSDoc is stored on a side-band to the AST", which leads to problems parsing and representing it as part of the AST. Given (1) my inexperience with tsquery and (2) my hacky solutions to the two open questions, you should probably think of this PR as a starting point for discussion rather than something to merge.

This change has two open questions that manifest as ugly code:

1. The jsDoc property is not exposed in the Typescript API because the
intended interface is `getJSDocTags`, which does a lot more work to
provide a list of tags that are collected directly from a node, or any
one of several parent nodes. I use type assertions to any to access the
property without checking.
2. The jsDoc property is of type JSDoc[], not Node, so it can't be
traversed directly. I decided to add the discovered JSDoc
nodes as children before the actual children, since that's where JSDoc
appears syntactically. But it's odd that that each tag appears as an
individual child instead of appearing under some synthetic jsdoc node.
Unfortunately, I don't know tsquery well enough to know if such a
synthetic node is possible, or a good idea.

Both problems boil down to "JSDoc is stored on a side-band to the AST",
which leads to problems parsing and representing it as part of the AST.
@sandersn sandersn mentioned this pull request Jun 15, 2018
@phenomnomnominal
Copy link
Owner

Thanks for this, I'll check it out this weekend properly when I get a sec. My first thoughts are to dig into the "side-band to the AST" thing - is that a unique property of the JSDoc nodes? Or is that also similar to how the type nodes are stored? I did a little experiment of swapping out forEachChild with getChildren().forEach(, which would be roughly equivalent I think? It'd change the behaviour of TSQuery quite a bit, but perhaps that's what people really want? What do you think?

@sandersn
Copy link
Author

I looked at the source for getChildren and it does quite a bit more work than forEachChild. It does adds jsdocs, which is nice since tsquery wouldn't have to add handling for it. Basically, forEachChild is compiler code and reflects the compiler's treatment of the AST as an abstract syntax tree intended for type checking, whereas getChildren is language service code and actually makes the AST less abstract in order to extract meaning from all parts of the source.

After writing that, I think that getChildren is probably a better fit for tsquery, but

  1. it might be slower since it essentially rebuilds the tree with extra information.
  2. the extra code might be less reliable than forEachChild's static walk over the tree
  3. existing consumers of tsquery might depend too heavily on the specifics of forEachChild.

@phenomnomnominal
Copy link
Owner

Yeah I've done a bit of thinking about this too, and I think I agree with you. I'd be happy to make the breaking change if it makes this whole thing more useful. Interested in what @urish thinks too though, as he is the main downstream consumer right now. My one remaining reservation is with the new map stuff we're working on, where we're using the transformer API, and visitEachChild, which is essentially the same as forEachChild in terms of traversal.

I'm wondering if there is any way you can query that side-band information? That way it could be enabled by some sort of flag, and everyone wouldn't have to take the perf hit.

@sandersn
Copy link
Author

sandersn commented Jul 5, 2018

Well there's the private way that I use in this PR, which accesses the jsDoc property directly to visit the comment and from there, the tags, which are its children. It's similar to the way getChildren (which is not configurable) works. The more abstract way is getJSDocTags, which skips comments and also traverses the tree looking for tags. For example, given the following input:

/**
 * @param {string} x
 * @return {number}
 */
function f(/** @type {number} */x) {
}

x.jsDoc is a jsdoc comment with only one tag, @type {number}. f.jsDoc is a jsdoc comment with two tags, @param {string} x and @return {number}.

In contrast, getJSDocTags(x) returns two tags: @type {number} and @param {string} x, while getJSDocTags(f) returns one tag: @return {number}.

I personally would prefer the less abstract behaviour when querying a syntax tree, since I feel confident that I could identify interesting nodes to later call getJSDocTags on myself. And I would be fine passing a flag to get the information.

I have mixed feelings about making the jsDoc property public, though, because it would add an obvious, and wrong, way to check whether a node has jsDoc type tags. I think it would make it harder for new users to pick up the compiler API and use it for type checking. But I don't know if type checking usage is more common than syntax usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants