-
-
Notifications
You must be signed in to change notification settings - Fork 707
feat(linter/plugins): implement SourceCode#getAllComments
#14589
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(linter/plugins): implement SourceCode#getAllComments
#14589
Conversation
SourceCode#getAllComments
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements the SourceCode#getAllComments method for linter plugins by enabling comment serialization in the AST and exposing comments through the JavaScript plugin API.
Key changes:
- Enables comment serialization in AST Program nodes by removing the
#[estree(skip)]attribute - Updates TypeScript type definitions to include Comment interface exports
- Implements the
getAllComments()method in the SourceCode API - Adds comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 10 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_ast/src/ast/js.rs | Removes estree skip attribute to enable comment serialization in Program nodes |
| crates/oxc_ast/src/serialize/mod.rs | Adds comments field serialization to Program ESTree output |
| napi/parser/src/raw_transfer.rs | Refactors comment handling to avoid unnecessary memory operations and use CloneIn trait |
| tasks/ast_tools/src/generators/typescript.rs | Updates TypeScript definitions to export Comment interface alongside Span |
| apps/oxlint/src-js/plugins/source_code.ts | Implements getAllComments method to return ast.comments array |
| apps/oxlint/test/fixtures/getAllComments/* | Adds comprehensive test fixture with plugin, test files, and expected output |
| apps/oxlint/test/e2e.test.ts | Adds end-to-end test for getAllComments functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
CodSpeed Performance ReportMerging #14589 will not alter performanceComparing Summary
Footnotes |
|
Thanks for jumping on this! This is a little tricky. Raw transfer deserializer is used in both
Hmm... let me think about this a little. Context: The slowest part of raw transfer deserializer is deserializing strings, because it often involves a call into C++ ( |
28f8d10 to
1f181fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 19 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Sorry I'm slow to come back. Here's what I think we should do:
We should be able to do that as follows: Alter codegenHere's the code which generates different variants of the deserializer for parser and oxlint: oxc/tasks/ast_tools/src/generators/raw_transfer.rs Lines 212 to 262 in 9e54002
Alter deserializer code template for
|
| #[estree(raw_deser = " | |
| const start = IS_TS ? 0 : DESER[u32](POS_OFFSET.span.start), | |
| end = DESER[u32](POS_OFFSET.span.end); | |
| const program = parent = { | |
| type: 'Program', | |
| body: null, | |
| sourceType: DESER[ModuleKind](POS_OFFSET.source_type.module_kind), | |
| hashbang: null, | |
| start, | |
| end, | |
| ...(RANGE && { range: [start, end] }), | |
| ...(PARENT && { parent: null }), | |
| }; | |
| program.hashbang = DESER[Option<Hashbang>](POS_OFFSET.hashbang); | |
| const body = program.body = DESER[Vec<Directive>](POS_OFFSET.directives); | |
| body.push(...DESER[Vec<Statement>](POS_OFFSET.body)); | |
| if (IS_TS) { | |
| let start; | |
| if (body.length > 0) { | |
| const first = body[0]; | |
| start = first.start; | |
| if (first.type === 'ExportNamedDeclaration' || first.type === 'ExportDefaultDeclaration') { | |
| const { declaration } = first; | |
| if ( | |
| declaration !== null && declaration.type === 'ClassDeclaration' | |
| && declaration.decorators.length > 0 | |
| ) { | |
| const decoratorStart = declaration.decorators[0].start; | |
| if (decoratorStart < start) start = decoratorStart; | |
| } | |
| } | |
| } else { | |
| start = end; | |
| } | |
| if (RANGE) { | |
| program.start = program.range[0] = start; | |
| } else { | |
| program.start = start; | |
| } | |
| } | |
| if (PARENT) parent = null; | |
| program | |
| ")] | |
| pub struct ProgramConverter<'a, 'b>(pub &'b Program<'a>); |
- Add
...(COMMENTS && { comments: DESER[Vec<Comment>](POS_OFFSET.comments) }),afterhashbang: null.
Codegen will set COMMENTS to false in all the deserializers except the linter one, and then minifier will shake out the dead code.
What else?
Because we're not altering the parser, that should sidestep problem of conformance tests failing.
Next steps:
Comments will have an extraneousparentfield. We'll need to remove it.- Translate
start/endof comments from UTF-8 bytes (Rust) to UTF-16 chars (JS), same as we do for rest of the AST (inLinter::run_external_rulesinoxc_linter, seenapi/parser). - Add tests for correctness of comment
start/endin files containing unicode characters. - Implement the rest of the comment-related APIs in
SourceCode.
We can deal with all of those in follow-up PRs.
How does that sound?
|
That sounds great! Thanks for the guidance, it didn't take me long to get everything up and running after it. |
6f23f83 to
7ba23b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I pushed a couple of commits - I hope you don't mind, I find it often saves time vs going back-and-forth on review. One just reverts changes to parser that we no longer need, and the other is total style nit. The latter is not at all important, and really I should have left it alone - I'm sorry, there's some tedious fastidiousness in me which I can't entirely control.
Merging now. Great!
Future PRs
RawTransferData { program.comments, comments }duplication.interface Programdoesn't include the comments type.comment.parent