Skip to content

refactor(semantic): remove Option from parent_* methods#12087

Merged
Boshen merged 4 commits intooxc-project:mainfrom
ulrichstark:Remove-Option-from-parent_--methods
Jul 7, 2025
Merged

refactor(semantic): remove Option from parent_* methods#12087
Boshen merged 4 commits intooxc-project:mainfrom
ulrichstark:Remove-Option-from-parent_--methods

Conversation

@ulrichstark
Copy link
Contributor

@ulrichstark ulrichstark commented Jul 5, 2025

Closes #11946

@camchenry in #11946:
Originally suggested by @overlookmotel. Currently, all of the methods relating to getting a node's parent ID/kind/node return Option<&AstNode>. However, we know that all AST nodes should have a parent, with the exception of Program. In this case, we could have Program be self-referential: it will return its parent ID as itself, and then we can remove Option from the return type of all methods fetching parent data. This would remove a lot of boilerplate code in the linter and elsewhere. Could potentially improve performance as well by removing lots of unnecessary branches which means fewer instructions.

This required updating logic in multiple cases, because parent of Program is now Program itself. I managed to get all tests to pass, but it's very likely that I have broken something untested. Broken logic will surface as incorrect results, stack overflows or infinite loops.

Edit:
Seems like semantic benchmarks have improved a bit (+0.3%) and linter benchmarks have slightly regressed (-0.1%). I suspect the now required additional checks in ancestors, ancestor_kinds and ancestor_ids for the slight slowdown of the linter benchmarks.

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Jul 5, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Jul 5, 2025

CodSpeed Instrumentation Performance Report

Merging #12087 will not alter performance

Comparing ulrichstark:Remove-Option-from-parent_--methods (c3393e1) with main (d732e85)

Summary

✅ 38 untouched benchmarks

@Boshen Boshen requested a review from camchenry July 6, 2025 06:36
@overlookmotel
Copy link
Member

We might be able to improve perf of AstNodes::ancestor_ids a little by using a custom iterator, instead of std::iter::successors.

Also, node_id == NodeId::DUMMY would be a cheaper check than parent_id == node_id. The NodeId of Program in practice is always NodeId::DUMMY (0), but we don't have a static guarantee of that at present. But we could get that guarantee by adding assert!(self.parent_ids.is_empty()) to the top of AstNodes::add_program_node (or maybe just debug_assert!).

But anyway, the main reason I suggested this change was to remove tons of boilerplate code from linter, rather than the perf impact.

Copy link
Member

@camchenry camchenry left a comment

Choose a reason for hiding this comment

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

I think the improvements in readability of code in the linter alone are worth it. I think the change in the benchmarks is reasonable, and probably within what I would consider noise. Changes >=1% are a better signal typically.

I think my only hesitation is whether it makes sense change this API contract and whether we feel like it is confusing to not return Option on these functions. Would we ever want to return None in the future?

@camchenry
Copy link
Member

Also, node_id == NodeId::DUMMY would be a cheaper check than parent_id == node_id. The NodeId of Program in practice is always NodeId::DUMMY (0), but we don't have a static guarantee of that at present. But we could get that guarantee by adding assert!(self.parent_ids.is_empty()) to the top of AstNodes::add_program_node (or maybe just debug_assert!).

We should do this in this PR, or at least as a follow-up PR.

@Boshen Boshen merged commit 54cf5cb into oxc-project:main Jul 7, 2025
24 checks passed
@oxc-bot oxc-bot mentioned this pull request Jul 7, 2025
@ulrichstark ulrichstark deleted the Remove-Option-from-parent_--methods branch July 7, 2025 05:46
camc314 pushed a commit that referenced this pull request Jul 8, 2025
…de (#12123)

Follow on to #12087

> We might be able to improve perf of AstNodes::ancestor_ids a little by
using a custom iterator, instead of std::iter::successors.
> Also, node_id == NodeId::DUMMY would be a cheaper check than parent_id
== node_id. The NodeId of Program in practice is always NodeId::DUMMY
(0), but we don't have a static guarantee of that at present. But we
could get that guarantee by adding assert!(self.parent_ids.is_empty())
to the top of AstNodes::add_program_node (or maybe just debug_assert!).
> @overlookmotel in
#12087 (comment)

My changes:
- added constant NodeId::ROOT (same value as NodeId::DUMMY, but leads to
more intuitive code)
- added asserts in `add_program_node` that nodes are empty and node kind
that's being added is Program
- ancestor_kinds and ancestors now build upon ancestor_ids, making
std::iter::successors implementation obsolete
- make AstNodeParentIter simpler by iterating over node ids and by
making use of previous assertions of `add_program_node`
- remove root, root_node and root_node_mut functions and replace their
use with either new program function or NodeId::ROOT

Results in general much less code, especially less boilerplate code in
the linter around getting the program node. Also might hopefully yield
some small benchmark improvements.

Edit:
Only very minor if not zero improvements on the linter benchmarks :(

I also noticed while working on this that the ancestor iterator methods
where documented as "The first node produced by this iterator is the
first parent of the node pointed to by node_id". That statement was
incorrect because all the iterators didn't yield the parent of node_id,
but node_id itself as first item. I updated the documentation of these
methods to correctly reflect their implementation.
Boshen pushed a commit that referenced this pull request Jul 11, 2025
…iterators (#12164)

This simplifies the implementation and uses of `ancestor_ids`,
`ancestor_kinds` and `ancestors` iterators by changing its first
returned item from the provided node to the first parent of this node.
This makes the implementation of those iterators simpler and makes code
in most lint rules shorter by mainly removing the often called
`.skip(1)`.

All tests are green, but as always I probably broke something untested
because of the scale and impact of this change. I'm not quite happy with
me having to resort to `iter::once` twice in `no_unused_vars` lint rule,
but I couldn't come up with a better solution. But having much less
code, simpler implementation of the iterators and hopefully a
performance gain in the linter benchmarks is definitely worth the
change.

Continuation of #12123 and #12087
Closes #12151
graphite-app bot pushed a commit that referenced this pull request Jul 25, 2025
#12087 changed parentage of AST nodes so `Program` is now its own parent. Update an outdated comment to reflect this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

semantic: Remove Option from parent_* methods

4 participants