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

docs(ast): add doc comments to more AST nodes #4413

Merged
merged 12 commits into from
Jul 27, 2024
Merged

docs(ast): add doc comments to more AST nodes #4413

merged 12 commits into from
Jul 27, 2024

Conversation

DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Jul 22, 2024

What This PR Does

Adds more doc comments and examples to AST nodes

Copy link

graphite-app bot commented Jul 22, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the A-ast Area - AST label Jul 22, 2024
Copy link

codspeed-hq bot commented Jul 22, 2024

CodSpeed Performance Report

Merging #4413 will not alter performance

Comparing don/ast/docs (c5aedf8) with main (a49f491)

Summary

✅ 32 untouched benchmarks

crates/oxc_ast/src/ast/js.rs Outdated Show resolved Hide resolved
crates/oxc_ast/src/ast/js.rs Outdated Show resolved Hide resolved
crates/oxc_ast/src/ast/js.rs Outdated Show resolved Hide resolved
crates/oxc_ast/src/ast/js.rs Outdated Show resolved Hide resolved
crates/oxc_ast/src/ast/js.rs Show resolved Hide resolved
crates/oxc_ast/src/ast/ts.rs Outdated Show resolved Hide resolved
crates/oxc_ast/src/ast_impl/js.rs Outdated Show resolved Hide resolved
crates/oxc_ast/src/ast_impl/js.rs Show resolved Hide resolved
crates/oxc_ast/src/ast_impl/js.rs Outdated Show resolved Hide resolved
crates/oxc_ast/src/ast_impl/js.rs Outdated Show resolved Hide resolved
@overlookmotel
Copy link
Contributor

Thanks loads for doing this Don. I in particular will really appreciate the comments on the TS types, because I am not a habitual TS user, and so I'm constantly wracking my brains "but what is a TSIndexSignature?"

I've made a lot of comments. I hope this comes across in the spirit in which it's intended - helping spot a few small things, rather than gratuitous nit-picking. You can feel free to ignore the stylistic suggestions if you don't have time.

The main thing, though, is to please make this PR just the comments, and separate out the addition of various impls + methods into separate PRs. Some of those changes may be more questionable, and it'd be helpful to see them and discuss them in isolation. You are set up on Graphite now, right?

@DonIsaac
Copy link
Contributor Author

Yeah, I've got graphite set up. I'll create a stack with the other impls on a downstream PR

@overlookmotel
Copy link
Contributor

There's still some methods added in this PR e.g. impl<'a> Deref for BindingIdentifier<'a>. As discussed above, please move them into a separate PR.

@DonIsaac
Copy link
Contributor Author

@overlookmotel all Deref impls and new methods have been removed.

@DonIsaac
Copy link
Contributor Author

@overlookmotel bumping this

@Boshen
Copy link
Member

Boshen commented Jul 27, 2024

This conflicts with #4051

Shall we try and consolidate both? Merging this first is fine.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Jul 27, 2024
Copy link

graphite-app bot commented Jul 27, 2024

Merge activity

  • Jul 26, 9:03 PM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.

@DonIsaac DonIsaac merged commit f5f0ba8 into main Jul 27, 2024
26 checks passed
@DonIsaac DonIsaac deleted the don/ast/docs branch July 27, 2024 01:21
@oxc-bot oxc-bot mentioned this pull request Jul 27, 2024
Dunqing pushed a commit that referenced this pull request Jul 28, 2024
## [0.22.1] - 2024-07-27

### Features

- 2477330 ast: Add `AstKind::TSExportAssignment` (#4501) (Dunqing)
- aaee07e ast: Add `AstKind::AssignmentTargetPattern`,
`AstKind::ArrayAssignmentTarget` and `AstKind::ObjectAssignmentTarget`
(#4456) (Dunqing)
- fd363d1 ast: Add AstKind::get_container_scope_id (#4450) (DonIsaac)
- e2735ca span: Add `contains_inclusive` method (#4491) (DonIsaac)

### Bug Fixes

- 368112c ast: Remove `#[visit(ignore)]` from
`ExportDefaultDeclarationKind`'s `TSInterfaceDeclaration` (#4497)
(Dunqing)
- 36bb680 semantic: `TSExportAssignment` cannot reference type binding
(#4502) (Dunqing)
- cb2fa49 semantic: `typeof` operator cannot reference type-only import
(#4500) (Dunqing)
- ef0e953 semantic: Generic passed to typeof not counted as a reference
(#4499) (Dunqing)
- 40cafb8 semantic: Params in `export default (function() {})` flagged
as `SymbolFlags::Export` (#4480) (Dunqing)
- 2e01a45 semantic: Non-exported namespace member symbols flagged as
exported (#4493) (Don Isaac)
- e4ca06a semantic: Incorrect symbol’s scope_id after var hoisting
(#4458) (Dunqing)
- 77bd5f1 semantic: Use correct span for namespace symbols (#4448) (Don
Isaac)
- 5db7bed sourcemap: Fix pre-calculation of required segments for
building JSON (#4490) (overlookmotel)
- 1667491 syntax: Correct `is_reserved_keyword_or_global_object`'s
incorrect function calling. (#4484) (Ethan Goh)
- 82ba2a0 syntax: Fix unsound use of `NonZeroU32` (#4466)
(overlookmotel)
- c04b9aa transformer: Add to `SymbolTable::declarations` for all
symbols (#4460) (overlookmotel)
- ecdee88 transformer/typescript: Incorrect eliminate exports when the
referenced symbol is both value and type (#4507) (Dunqing)

### Performance

- 963a2d1 mangler: Reduce unnecessary allocation (#4498) (Dunqing)
- 868fc87 parser: Optimize conditional advance on ASCII values (#4298)
(lucab)
- 24beaeb semantic: Give `AstNodeId` a niche (#4469) (overlookmotel)
- 348c1ad semantic: Remove `span` field from `Reference` (#4464)
(overlookmotel)
- 6a9f4db semantic: Reduce storage size for symbol redeclarations
(#4463) (overlookmotel)
- 705e19f sourcemap: Reduce memory copies encoding JSON (#4489)
(overlookmotel)
- 4d10c6c sourcemap: Pre allocate String buf while encoding (#4476)
(Brooooooklyn)

### Documentation

- f5f0ba8 ast: Add doc comments to more AST nodes (#4413) (Don Isaac)
- 871b3d6 semantic: Add doc comments for SymbolTester and SemanticTester
(#4433) (DonIsaac)

### Refactor

- 9c5d2f9 ast/builder: Use `Box::new_in` over `.into_in` (#4428)
(overlookmotel)
- ccb1835 semantic: Methods take `Span` as param, not `&Span` (#4470)
(overlookmotel)
- f17254a semantic: Populate `declarations` field in
`SymbolTable::create_symbol` (#4461) (overlookmotel)
- a49f491 semantic: Re-order `SymbolTable` fields (#4459)
(overlookmotel)
- 7cd53f3 semantic: Var hoisting (#4379) (Dunqing)
- 4f5a7cb semantic: Mark SemanticTester and SymbolTester as must_use
(#4430) (DonIsaac)
- c958a55 sourcemap: `push_list` method for building JSON (#4486)
(overlookmotel)
- c99b3eb syntax: Give `ScopeId` a niche (#4468) (overlookmotel)
- 96fc94f syntax: Use `NonMaxU32` for IDs (#4467) (overlookmotel)

### Testing

- 4b274a8 semantic: Add more test cases for symbol references (#4429)
(DonIsaac)

Co-authored-by: Boshen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-ast Area - AST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants