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

feat(ast,parser): parse jsdoc #168

Closed
Boshen opened this issue Mar 12, 2023 · 10 comments
Closed

feat(ast,parser): parse jsdoc #168

Boshen opened this issue Mar 12, 2023 · 10 comments
Assignees
Labels
A-linter Area - Linter A-parser Area - Parser E-Help Wanted Experience level - For the experienced collaborators P-high Priority - High

Comments

@Boshen
Copy link
Member

Boshen commented Mar 12, 2023

I see there are some demand for reading jsdoc information, let's do this in two steps:

  1. parse jsdoc into structured data, this should be lazy.
  2. save them into trivias and have them accessible. I haven't figured out the best way to do this, but see AST Trivia handling #6 for disucssion.

Note: This is for parsing jsdoc comments to structured data. This is not about wiring up all the jsdoc comments into a document tree (which is the main purpose for jsdoc).

To parse jsdoc comments, we need to:

  1. add a new routine to parse jsdoc in oxc_parser, this should be a separate routine so we can parse jsdoc lazily
  2. add the jsdoc AST to oxc_ast
  3. setup jsdoc tests (I'm unsure on how to do this properly yet).

References:

@Boshen Boshen added this to the AST / Lexer / Parser milestone Mar 12, 2023
@Boshen Boshen added the E-Help Wanted Experience level - For the experienced collaborators label Mar 12, 2023
@Boshen Boshen self-assigned this Mar 19, 2023
@Boshen
Copy link
Member Author

Boshen commented Mar 19, 2023

Prioritizing because @thepassle need this. See https://twitter.com/passle_/status/1637446645104164865

@Boshen Boshen added the P-high Priority - High label Mar 21, 2023
@Boshen Boshen modified the milestones: AST / Lexer / Parser, Linter Mar 22, 2023
@Boshen
Copy link
Member Author

Boshen commented Mar 23, 2023

Update:

The original intent was to come up with a generic comment attachment algorithm, where we attach comments to some AST node driven by a set of rules. After some research and reading about it in the ESLint and Babel codebases, I conclude that it is incomprehensible for me right now.

So instead, let's reduce the problem space down to "attach jsdoc comments to specific AST node". This will be much more approachable.

We will build the jsdoc data structure inside the semantic analyzer. When a jsdoc targeting AST node such as a Statement or Declaration is visited, we will ask Trivias to get us a leading comment. The jsdoc data structure will save the comment span and mark the Statement (SemanticNode) to indicate it has a jsdoc. A getter will be provided to parse the jsdoc lazily in a OnceCell.

@Boshen Boshen removed this from the AST / Lexer / Parser milestone Mar 29, 2023
@Boshen Boshen added the A-parser Area - Parser label Mar 29, 2023
@Boshen Boshen added this to the 0.0.3 milestone Mar 29, 2023
@Boshen Boshen added the A-linter Area - Linter label Mar 29, 2023
@Boshen Boshen removed this from the 0.0.3 milestone Apr 2, 2023
@ematipico
Copy link

I want to help somehow. What do you need or what can I do? :)

@Boshen
Copy link
Member Author

Boshen commented Jun 20, 2023

I want to help somehow. What do you need or what can I do? :)

Hi Ema! I've forgotten everything we wrote about jsdocs, but would you be interested in getting a https://github.com/gajus/eslint-plugin-jsdoc rule to work? I can guide you on the missing pieces in the discord channel. I think we had some of the infra working, targeting a lint rule would be the easiest to fill in the gaps.

@leaysgur

This comment was marked as outdated.

@lukeed
Copy link
Contributor

lukeed commented Feb 5, 2024

I'd love to be able to just see & access comment values instead of skipping them outright.

I'd definitely consider it a bonus step for oxc to parse any JSDocs and translate its meaning(s) to the AST, but there are lots of instances where users either invent directives or just use a standard comment to transfer additional information

Quick examples:

/** @table "users" */
type User = { ... }

import(/* webpackChunkName: "my-chunk" */ "foobar");

// comptime
const DAY = ms(1, "day")

@leaysgur
Copy link
Contributor

We talked a bit about this in #2437 ...,

  • There are many use cases for JSDoc, but for now, aim to implement eslint-plugin-jsdoc
  • To know the details, read the source of eslint-plugin-jsdoc first

So, I spent these days reading through the code.

  • eslint-plugin-jsdoc itself
  • jsdoccomment, comment-parser and jsdoc-type-pratt-parser, which are heavily dependent

(There are 3 articles on my blog if you're interested. Sorry it's in Japanese.)

As a result, although IMO, I'm not sure we should aim for 100% compatibility for this.

For a number of reasons,

@Boshen Sorry for the long lines, then I'd like to confirm,

  • Were you originally going to re-implement eslint-plugin-jsdoc with compatibility, no matter how hard it was?
  • Do you think it would be worthwhile to just omit the heavy functionality
    • and provide rules/oxc_jsdoc like rules/oxc?
    • or keep the label eslint-plugin-jsdoc?
  • (Maybe we haven't seen this yet, but) Should we leave this to external plugins and spend our time on other things?
    • e.g. Like TypeScript, put JSDoc in the AST?(although compatibility with ESTree would be an issue)
    • e.g. Just provide a generic API like getLeadingComments(node) and leave it at all

What do you think? 👀

@Boshen
Copy link
Member Author

Boshen commented Feb 22, 2024

@leaysgur Oh wow I didn't expect 3 blog posts on this topic, I thought jsdoc is a solved problem 😰

So in summary, it seems like the hardest part about jsdoc is comment attachment to AST nodes.

We can leave this part out and focus our task on just jsdoc content rules, which is quiet easy as all we need to do is finish the jsdoc parser and run rules against these parsed jsdocs.

then I'd like to confirm

The intention was to pass all the tests, but we don't really need to do it if it's not a fun task.


And if you're sick of jsdoc after looking at it for 3 days ... you may also join me on the eslint-plugin-import task.

@leaysgur
Copy link
Contributor

leaysgur commented Feb 22, 2024

For future reference, let me elaborate.

the hardest part about jsdoc is comment attachment to AST nodes.

This wasn't particularly difficult, at least if we trust the logic of eslint-plugin-jsdoc, or rather, jsdoccomment.

https://github.com/es-joy/jsdoccomment/blob/6aae5ea306015096e3d58cd22257e5222c54e3b4/src/jsdoccomment.js#L283

To put it simply, it was just this:

// findJSDocComment(node, sourceCode): Comment | null;
const beforeTokens = sourceCode.getTokensBefore(node, { includeComments: true });
while (let token = beforeTokens.pop()) {
  if (token.type === 'Block' && token.value.startsWith('*')) return token;
}

return null;

And I believe this behavior was already implemented in #2437 .


The part I found to be the hardest was that some(about half of) rules can:

  • 1️⃣ freely determine which astNode to execute the above logic
if (
  esquery.matches(
    astNode,
    // from rule config
    `MethodDefinition:not([accessibility="public"]):has(JsdocBlock)`
  )
) {
  const jsdocComment = findJSDocComment(astNode, sourceCode);
}
  • 2️⃣ And also based on the Comment obtained, freely determine whether to execute rule handler
const jsdocAstNode = toESTreeLikeAST(jsdocComment);
if (
  esquery.matches(
    jsdocAstNode,
    // from rule config
    `JsdocBlock[postDelimiter=""]:has(JsdocTypeUnion > JsdocTypeName[value="Bar"]:nth-child(1))`
  )
) {
  ruleHandler({ astNode, jsdocAstNode });
}

https://github.com/gajus/eslint-plugin-jsdoc/blob/main/docs/advanced.md

(Actually, they seemed to have a bit more complicated logic...)


About rest half of the rules seem to simply check "only the text of all JSDoc comments in the source".

However, their implementation was like

for (const { astNode, jsdocAstNode } of jsdocNodesWithAttachedNode)
  // Why astNode required...??
  ruleHandler({ astNode, jsdocAstNode });
// ...
for (const { jsdocAstNode } of jsdocNodesWithoutAttachedNode)
  ruleHandler({ astNode: null, jsdocAstNode });

they are called differently for some reason.

https://github.com/gajus/eslint-plugin-jsdoc/blob/e948bee821e964a92fbabc01574eca226e9e1252/src/iterateJsdoc.js#L2279-L2328

And when I tried to replace astNode of the former to null, the tests started to FAIL...! 😇

💡 After additional research, I finally solved this mystery.

  • check-tag-names
  • informative-docs
  • no-undefined-types

It seems that these 3 rules perform extra linting if node exists. Other all rule's tests pass without node!


you may also join me on the eslint-plugin-import task.

That sounds interesting as well.

Either way, I'll think a bit more about how to conclude #2437 .

Boshen pushed a commit that referenced this issue Feb 24, 2024
Partial fix for #168 

- [x] Fix general finding behavior for leading comments
- [x] Accept multiple jsdoc comments per node
- [x] Provide `get_one` and also `get_all` 
- [x] Add `iter_all()` for non-node related usage
- [x] Limit AST node kinds to parse
@Boshen
Copy link
Member Author

Boshen commented Apr 15, 2024

@leaysgur is carrying this task 👍 . Future issues can be created separately now.

@Boshen Boshen closed this as completed Apr 15, 2024
IWANABETHATGUY pushed a commit to IWANABETHATGUY/oxc that referenced this issue May 29, 2024
Partial fix for oxc-project#168 

- [x] Fix general finding behavior for leading comments
- [x] Accept multiple jsdoc comments per node
- [x] Provide `get_one` and also `get_all` 
- [x] Add `iter_all()` for non-node related usage
- [x] Limit AST node kinds to parse
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter A-parser Area - Parser E-Help Wanted Experience level - For the experienced collaborators P-high Priority - High
Projects
None yet
Development

No branches or pull requests

5 participants