feat(data_structures): add is_exhausted method to NonEmptyStack and SparseStack#13672
Merged
graphite-app[bot] merged 1 commit intomainfrom Sep 11, 2025
Conversation
Member
Author
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Sep 11, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds an is_exhausted method to both NonEmptyStack and SparseStack data structures to provide a clear way to check if all pushed items have been popped, returning the stack to its initial state. This is useful for debugging AST traversals where you want to ensure proper stack balance.
Key changes:
- Added
is_exhausted()method toNonEmptyStackthat checks ifcursor == start - Added
is_exhausted()method toSparseStackthat delegates to the underlyingNonEmptyStack - Added comprehensive tests for both implementations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/oxc_data_structures/src/stack/non_empty.rs | Adds is_exhausted() method and test for NonEmptyStack |
| crates/oxc_data_structures/src/stack/sparse.rs | Adds is_exhausted() method and test for SparseStack |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
CodSpeed Instrumentation Performance ReportMerging #13672 will not alter performanceComparing Summary
Footnotes |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
overlookmotel
force-pushed
the
09-11-feat_data_structures_add_is_exhausted_method_to_nonemptystack_and_sparsestack_
branch
from
d39f9d7 to
a6e1ace
Compare
Member
Author
Merge activity
|
…nd `SparseStack` (#13672) When using stacks, typically you create a stack at start of AST traversal, and then `debug_assert!` that the stack has been emptied again at the end of traversal, to make sure that the stack has not got out of sync - that every `push` is followed by a corresponding `pop`. With `NonEmptyStack` and `SparseStack` it's a little confusing how to do this, because these stacks can never be empty. `stack.is_empty()` is not correct. `stack.len() == 1` is correct, but it's unclear when reading that code what exactly this is testing. Add `is_exhausted` methods to `NonEmptyStack` and `SparseStack` to fulfil this role. Naming: `is_exhausted` is perhaps not the best name, but I couldn't come up with a better one. I considered `is_emptied`, which is more descriptive, but I think it's too close to `is_empty`. (prompted by #13632)
graphite-app
bot
force-pushed
the
09-11-feat_data_structures_add_is_exhausted_method_to_nonemptystack_and_sparsestack_
branch
from
a6e1ace to
1a58e99
Compare
graphite-app bot
pushed a commit
that referenced
this pull request
Sep 11, 2025
…time error (#13673) Continuation from #13672. Calling `NonEmptyStack::is_empty` is always a logic error. `NonEmptyStack` can never be empty, and `NonEmptyStack::is_exhausted` is very likely what was intended. Make calling `NonEmptyStack::is_empty` a compile-time error, to prevent this erroneous usage. Ideally, we'd remove the method entirely, but `&NonEmptyStack<T>` derefs to `&[T]`, so without defining `is_empty` method on `NonEmptyStack`, `stack.is_empty()` falls through to `[T]::is_empty`, which is not what we want.
graphite-app
bot
deleted the
09-11-feat_data_structures_add_is_exhausted_method_to_nonemptystack_and_sparsestack_
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When using stacks, typically you create a stack at start of AST traversal, and then
debug_assert!that the stack has been emptied again at the end of traversal, to make sure that the stack has not got out of sync - that everypushis followed by a correspondingpop.With
NonEmptyStackandSparseStackit's a little confusing how to do this, because these stacks can never be empty.stack.is_empty()is not correct.stack.len() == 1is correct, but it's unclear when reading that code what exactly this is testing.Add
is_exhaustedmethods toNonEmptyStackandSparseStackto fulfil this role.Naming:
is_exhaustedis perhaps not the best name, but I couldn't come up with a better one. I consideredis_emptied, which is more descriptive, but I think it's too close tois_empty.(prompted by #13632)