fix(linter/plugins): reset ancestors if error during AST walk#17207
Merged
graphite-app[bot] merged 1 commit intomainfrom Dec 21, 2025
Merged
Conversation
github-actions
bot
added
A-linter
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 Dec 21, 2025
overlookmotel
force-pushed
the
om/12-20-fix_linter_plugins_reset_ancestors_if_error_during_ast_walk
branch
from
387bfc9 to
b5ac1d4
Compare
overlookmotel
force-pushed
the
om/12-20-refactor_linter_plugins_refactor_state_reset_after_errors_during_linting
branch
from
10800a6 to
1fbf13b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the ancestors array (used internally by esquery for selector matching) was not being reset after an error during AST traversal. This caused the array to retain state from the failed file, affecting selector behavior when processing subsequent files.
Key changes:
- Added debug assertions to verify
ancestorsis in sync during AST traversal - Reset
ancestorsarray inresetStateAfterError()function - Added comprehensive test to verify the fix works correctly with multi-file scenarios
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tasks/ast_tools/src/generators/estree_visit.rs |
Added generator code for debug check function and ancestorsLen tracking |
apps/oxlint/src-js/generated/walk.js |
Generated JavaScript with debug assertions for each AST node walker |
apps/oxlint/src-js/plugins/lint.ts |
Added ancestors array reset in error handler and debug assertions before/after AST walk |
apps/oxlint/test/fixtures/lint_visit_error/plugin.ts |
Updated test to verify ancestors is properly reset after errors |
apps/oxlint/test/fixtures/lint_visit_error/output.snap.md |
Updated expected test output |
apps/oxlint/test/fixtures/lint_visit_error/options.json |
Added single-thread option for deterministic test execution |
apps/oxlint/test/fixtures/lint_visit_error/files/1.js |
Added first test file that triggers error |
apps/oxlint/test/fixtures/lint_visit_error/files/2.js |
Added second test file to verify reset works |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
overlookmotel
force-pushed
the
om/12-20-refactor_linter_plugins_refactor_state_reset_after_errors_during_linting
branch
from
1fbf13b to
ebd3750
Compare
overlookmotel
force-pushed
the
om/12-20-fix_linter_plugins_reset_ancestors_if_error_during_ast_walk
branch
from
b5ac1d4 to
059ddae
Compare
Member
Author
Merge activity
|
While walking the AST, `ancestors` array is pushed to when entering a node, and popped again when exiting. `ancestors` is empty at start of AST traversal and, because each push is followed by a pop, `ancestors` is empty again at the end of AST traversal. `ancestors` is a singleton, reused for every AST traversal. `ancestors` is used by `esquery` when determining if a node matches a selector. Note that it's not the same as what's returned from `sourceCode.getAncestors()`. `ancestors` is used only internally by `esquery`, so only way to see what it contains is by observing the behavior of selectors. This PR fixes a bug where `ancestors` was not reset (emptied) if an error is thrown during AST traversal. In that case `ancestors` was left in the state it was in at time that the error was thrown, and this affects behavior of selectors when visiting the *next* AST. Fix it by resetting `ancestors` of there's an error. Also add a bunch of debug assertions to ensure `ancestors` remains in sync with AST traversal at all times.
graphite-app bot
pushed a commit
that referenced
this pull request
Dec 21, 2025
#17205) In JS plugins tests, it's sometimes required to ensure that files are linted in deterministic order. For example, #17207 fixes a bug where some internal global state was not reset correctly after an error is thrown during visiting the AST. This affects what happens when linting the *next* file. In order to write a test for this, and produces deterministic output in the snapshot file, the order files are linted in needs to be deterministic. This PR provides this determinism by: 1. Adding a `force_deterministic_order` cargo feature which is enabled in test build. 2. When this feature is enabled *and* Oxlint is run with `--threads 1`, file paths are sorted before linting. 3. Adding an option for test fixtures `singleThread`. When `true`, the fixture is run with `--threads 1`. Aim is to only enable deterministic ordering in test fixtures which *need* determinism, so that most test fixtures are still testing Oxlint running as it does usually for a user.
graphite-app
bot
force-pushed
the
om/12-20-refactor_linter_plugins_refactor_state_reset_after_errors_during_linting
branch
from
ebd3750 to
6f05a99
Compare
graphite-app
bot
force-pushed
the
om/12-20-fix_linter_plugins_reset_ancestors_if_error_during_ast_walk
branch
from
059ddae to
4027039
Compare
Base automatically changed from
om/12-20-refactor_linter_plugins_refactor_state_reset_after_errors_during_linting
to
main
December 21, 2025 14:39
graphite-app
bot
deleted the
om/12-20-fix_linter_plugins_reset_ancestors_if_error_during_ast_walk
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
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.
While walking the AST,
ancestorsarray is pushed to when entering a node, and popped again when exiting.ancestorsis empty at start of AST traversal and, because each push is followed by a pop,ancestorsis empty again at the end of AST traversal.ancestorsis a singleton, reused for every AST traversal.ancestorsis used byesquerywhen determining if a node matches a selector. Note that it's not the same as what's returned fromsourceCode.getAncestors().ancestorsis used only internally byesquery, so only way to see what it contains is by observing the behavior of selectors.This PR fixes a bug where
ancestorswas not reset (emptied) if an error is thrown during AST traversal. In that caseancestorswas left in the state it was in at time that the error was thrown, and this affects behavior of selectors when visiting the next AST.Fix it by resetting
ancestorsif there's an error. Also add a bunch of debug assertions to ensureancestorsremains in sync with AST traversal at all times.