8000 refactor(ast): get jsx types out of AstKind exceptions by ulrichstark · Pull Request #11535 · oxc-project/oxc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor(ast): get jsx types out of AstKind exceptions #11535

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

refactor(ast): get jsx types out of AstKind exceptions #11535

wants to merge 3 commits into from
+234 −253

Conversation

ulrichstark
Copy link
Contributor

This is my (not so confident) attempt to do #11490 for all jsx types.

  1. removed all jsx types from STRUCTS_BLACK_LIST and ENUMS_WHITE_LIST
  2. regenerated ast code
  3. added missing arms to AstKind::debug_name
  4. removed now broken is_jsx function, because it was unused
  5. fixed all compile errors
  6. fixed all test errors
  7. regenerated snapshots (different node ids and decreased prettier conformance)

All tests are green 🥳🎉

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic A-ast Area - AST A-ast-tools Area - AST tools A-formatter Area - Formatter labels Jun 6, 2025
@ulrichstark ulrichstark changed the title Refactor out ast kind exceptions of jsx types refactor(ast): get jsx types out of AstKind exceptions Jun 6, 2025
@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Jun 6, 2025
@codspeed-hq CodSpeed HQ
Copy link
codspeed-hq bot commented Jun 6, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #11535 will improve performances by 7.93%

Comparing ulrichstark:refactor-out-ast-kind-exceptions-of-jsx-types (541c680) with main (551cd2a)

Summary

⚡ 1 improvements
✅ 37 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ linter[RadixUIAdoptionSection.jsx] 2.8 ms 2.6 ms +7.93%

@ulrichstark ulrichstark mentioned this pull request Jun 6, 2025
Open
@ulrichstark
Copy link
Contributor Author

Less code and even some nice performance improvements 😎
image

@overlookmotel
Copy link
Contributor

⚡ linter[RadixUIAdoptionSection.jsx] 2.8 ms 2.6 ms +7.92%
⚡ semantic[RadixUIAdoptionSection.jsx] 72.4 µs 69.8 µs +3.64%

Banger!

Thanks so much for getting on this. It's a large PR, so will take me a while to get through reviewing it. But, on the face of it, looks great.

overlookmotel
overlookmotel reviewed Jun 6, 2025
View reviewed changes
@overlookmotel
Copy link
Contributor
overlookmotel commented Jun 6, 2025 •
edited
Loading

Actually, it wasn't so long. I've reviewed. From what I can see, looks good. But I'm not the best person to review the linter changes as I'm not so familiar with the linter.

  1. @camc314 Could you possibly take a look at the linter changes (and not just the ones I've commented on)? As we're planning to drop Oxlint 1.0 next week, and make a big song and dance about it, it would be a really unfortunate moment to break things!

  2. The reduced conformance pass rate in formatter suggests something may be wrong there. I know literally nothing about the formatter. @Dunqing Can you advise what we should do about that?

Note: The perf boost in Semantic I'm guessing is due to producing less AstNodes (the snapshot changes with lower NodeIds suggests that). I guess the boost in linter is just due to simpler code (less branches and lookups), but I'm amazed it has such a large effect. Bravo!

@ulrichstark Just to flag please don't get your hopes up that every further step we take on adding/removing AstKinds will get a similar perf boost. In some cases it'll probably result in more AstNodes, in others less. So perf may go up or down. But this is certainly a very pleasant first step down the road!

@overlookmotel
Copy link
Contributor
overlookmotel commented Jun 6, 2025 •
edited
Loading

Not sure what's going on with the "Check links" CI fail. I've re-run it twice and it keeps failing on https://babeljs.io/docs/babel-plugin-transform-typescript, which seems to be a valid link.

camc314
camc314 reviewed Jun 6, 2025
View reviewed changes
Copy link
Contributor
@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

linter changes look good excluding those two @overlookmotel caught

@Dunqing
Copy link
Member
Dunqing commented Jun 7, 2025

There's no need to worry about the Formatter; I've created #11511 to revamp the formatted core. I will revisit all AstKind use cases in Formatter after that work is done.

@Boshen
Copy link
Member
Boshen commented Jun 7, 2025

@camc314 merge after oxlint 1.0

@Boshen Boshen marked this pull request as draft June 7, 2025 03:58
@ulrichstark
Copy link
Contributor Author

Just did a commit to shorten the call to get the parent node kind. The other feedback around the two instances of is_within_jsx_attribute seems to be resolved? Is more work required from my side or are we good and wait for the oxlint 1.0 release?

@camc314
Copy link
Contributor
camc314 commented Jun 7, 2025

Is more work required from my side or are we good and wait for the oxlint 1.0 release?

nope, i don't think so. I've checked both changes and they look fine. thanks for your work on this!

overlookmotel
overlookmotel reviewed Jun 7, 2025
View reviewed changes
crates/oxc_semantic/src/checker/typescript.rs
Comment on lines +559 to +562
&& matches!(
ctx.nodes.parent_kind(ctx.current_node_id),
Some(AstKind::JSXAttribute(_) | AstKind::JSXSpreadAttribute(_))
)
Copy link
Contributor
@overlookmotel overlookmotel Jun 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Given the conversation above, shouldn't this change just be:

- && matches!(ctx.nodes.parent_kind(ctx.current_node_id), Some(AstKind::JSXAttributeItem(_)))
+ && matches!(ctx.nodes.parent_kind(ctx.current_node_id), Some(AstKind::JSXAttribute(_)))

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@overlookmotel overlookmotel overlookmotel left review comments

@camc314 camc314 camc314 left review comments

@Dunqing Dunqing Awaiting requested review from Dunqing Dunqing is a code owner

At least 0 approving reviews are required to merge this pull request.

Assignees

@camc314 camc314

Labels
A-ast Area - AST A-ast-tools Area - AST tools A-formatter Area - Formatter A-linter Area - Linter A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ulrichstark @overlookmotel @Dunqing @Boshen @camc314
0