perf(linter/unicorn/prefer-dom-node-text-content): change dispatch to run only on less common node types#23897
Merged
Conversation
d51dc63 to
e30436a
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
…cturing nodes instead of identifiers
The rule's destructuring cases dispatched on `IdentifierName` and
`IdentifierReference` — two of the most common node types (every property
name and variable read) — then walked up to their parents to detect
destructuring patterns. Instead, dispatch directly on the rare parent
nodes (`BindingProperty`, `AssignmentTargetPropertyProperty`,
`AssignmentTargetPropertyIdentifier`) and read the key/binding.
Behavior is identical:
- The only `IdentifierName` that can be a direct child of `BindingProperty`
/`AssignmentTargetPropertyProperty` is the key, and the old grandparent
set always matched (`ObjectPattern`/`ObjectAssignmentTarget`), so that
branch always fired. Matching only `PropertyKey::StaticIdentifier`
preserves the computed-key exclusion.
- The `AssignmentTargetPropertyIdentifier` arm keeps the same
parent-of-`ObjectAssignmentTarget` check. The old non-APTI path was dead:
a rest target (`({...innerText} = node)`) goes through
`AssignmentTargetRest`, never a direct `ObjectAssignmentTarget` child.
On the vscode codebase, violations are unchanged (245 warnings, 47 errors),
calls drop 76% (5,996,077 -> 1,437,765), and rule time drops ~80%
(116ms -> 23ms).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e30436a to
fb728a0
Compare
camc314
added a commit
that referenced
this pull request
Jul 3, 2026
… run only on less common node types (#23897) Somewhat similar to #23867 and #23868. This was generated using Claude Code after I noticed this rule was one of the slowest on the vscode repo. I have reviewed and tested the change by running it on the vscode codebase, and the # of violations before and after is identical. All tests also pass. I've also had Claude run a battery of tests on code it generated to test as many scenarios as possible, to ensure that there were no differences in output on main vs. this branch, and confirmed that there were indeed no differences. ## What changed The rule's `NODE_TYPES` were already derived, but two of them — `IdentifierName` and `IdentifierReference` — are among the most common node types in any program (every property name, every variable read). The destructuring cases dispatched on those identifiers and then walked *up* to their parents to detect destructuring patterns. This PR instead dispatches directly on the rare parent nodes and reads the key/binding: | Old dispatch (common) | New dispatch (rare) | Covers | |---|---|---| | `IdentifierName` | `BindingProperty` (key) + `AssignmentTargetPropertyProperty` (name) | `const {innerText} = node`, `({innerText: text} = node)` | | `IdentifierReference` | `AssignmentTargetPropertyIdentifier` (binding) | `({innerText} = node)` | `StaticMemberExpression` (`node.innerText`) is unchanged. ## Why it's behavior-identical - The only `IdentifierName` that can be a direct child of `BindingProperty`/`AssignmentTargetPropertyProperty` is the key (values bottom out in `BindingIdentifier`/`IdentifierReference`). The old grandparent set always matched (`ObjectPattern`/`ObjectAssignmentTarget`), so that branch always fired — matching the new unconditional trigger. Matching only `PropertyKey::StaticIdentifier` preserves the computed-key exclusion (`{[innerText]: text}`). - The `AssignmentTargetPropertyIdentifier` arm keeps the same parent-of-`ObjectAssignmentTarget` check. The old non-APTI path was dead: a rest target (`({...innerText} = node)`) goes through `AssignmentTargetRest`, never a direct `ObjectAssignmentTarget` child. The rule test snapshot is byte-identical (no `.snap` change). ## Results Running `oxlint --debug=timings --quiet -A all -W unicorn/prefer-dom-node-text-content` on the vscode repo. Before: ``` Found 245 warnings and 47 errors. Rule Time (ms) Relative Calls Source ------------------------------------ ---------- -------- ------- ------ unicorn/prefer-dom-node-text-content 116.624 100.0% 5996077 native ``` After: ``` Found 245 warnings and 47 errors. Rule Time (ms) Relative Calls Source ------------------------------------ ---------- -------- ------- ------ unicorn/prefer-dom-node-text-content 23.631 100.0% 1437765 native ``` Violations are unchanged (245 warnings, 47 errors). Calls drop 76% (5,996,077 → 1,437,765) and rule time drops ~80% (116ms → 23ms). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Cameron <cameron.clark@hey.com>
camc314
added a commit
that referenced
this pull request
Jul 3, 2026
… run only on less common node types (#23897) Somewhat similar to #23867 and #23868. This was generated using Claude Code after I noticed this rule was one of the slowest on the vscode repo. I have reviewed and tested the change by running it on the vscode codebase, and the # of violations before and after is identical. All tests also pass. I've also had Claude run a battery of tests on code it generated to test as many scenarios as possible, to ensure that there were no differences in output on main vs. this branch, and confirmed that there were indeed no differences. ## What changed The rule's `NODE_TYPES` were already derived, but two of them — `IdentifierName` and `IdentifierReference` — are among the most common node types in any program (every property name, every variable read). The destructuring cases dispatched on those identifiers and then walked *up* to their parents to detect destructuring patterns. This PR instead dispatches directly on the rare parent nodes and reads the key/binding: | Old dispatch (common) | New dispatch (rare) | Covers | |---|---|---| | `IdentifierName` | `BindingProperty` (key) + `AssignmentTargetPropertyProperty` (name) | `const {innerText} = node`, `({innerText: text} = node)` | | `IdentifierReference` | `AssignmentTargetPropertyIdentifier` (binding) | `({innerText} = node)` | `StaticMemberExpression` (`node.innerText`) is unchanged. ## Why it's behavior-identical - The only `IdentifierName` that can be a direct child of `BindingProperty`/`AssignmentTargetPropertyProperty` is the key (values bottom out in `BindingIdentifier`/`IdentifierReference`). The old grandparent set always matched (`ObjectPattern`/`ObjectAssignmentTarget`), so that branch always fired — matching the new unconditional trigger. Matching only `PropertyKey::StaticIdentifier` preserves the computed-key exclusion (`{[innerText]: text}`). - The `AssignmentTargetPropertyIdentifier` arm keeps the same parent-of-`ObjectAssignmentTarget` check. The old non-APTI path was dead: a rest target (`({...innerText} = node)`) goes through `AssignmentTargetRest`, never a direct `ObjectAssignmentTarget` child. The rule test snapshot is byte-identical (no `.snap` change). ## Results Running `oxlint --debug=timings --quiet -A all -W unicorn/prefer-dom-node-text-content` on the vscode repo. Before: ``` Found 245 warnings and 47 errors. Rule Time (ms) Relative Calls Source ------------------------------------ ---------- -------- ------- ------ unicorn/prefer-dom-node-text-content 116.624 100.0% 5996077 native ``` After: ``` Found 245 warnings and 47 errors. Rule Time (ms) Relative Calls Source ------------------------------------ ---------- -------- ------- ------ unicorn/prefer-dom-node-text-content 23.631 100.0% 1437765 native ``` Violations are unchanged (245 warnings, 47 errors). Calls drop 76% (5,996,077 → 1,437,765) and rule time drops ~80% (116ms → 23ms). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Cameron <cameron.clark@hey.com>
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
2 participants
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.
Somewhat similar to #23867 and #23868.
This was generated using Claude Code after I noticed this rule was one of the slowest on the vscode repo. I have reviewed and tested the change by running it on the vscode codebase, and the # of violations before and after is identical. All tests also pass.
I've also had Claude run a battery of tests on code it generated to test as many scenarios as possible, to ensure that there were no differences in output on main vs. this branch, and confirmed that there were indeed no differences.
What changed
The rule's
NODE_TYPESwere already derived, but two of them —IdentifierNameandIdentifierReference— are among the most common node types in any program (every property name, every variable read). The destructuring cases dispatched on those identifiers and then walked up to their parents to detect destructuring patterns.This PR instead dispatches directly on the rare parent nodes and reads the key/binding:
IdentifierNameBindingProperty(key) +AssignmentTargetPropertyProperty(name)const {innerText} = node,({innerText: text} = node)IdentifierReferenceAssignmentTargetPropertyIdentifier(binding)({innerText} = node)StaticMemberExpression(node.innerText) is unchanged.Why it's behavior-identical
IdentifierNamethat can be a direct child ofBindingProperty/AssignmentTargetPropertyPropertyis the key (values bottom out inBindingIdentifier/IdentifierReference). The old grandparent set always matched (ObjectPattern/ObjectAssignmentTarget), so that branch always fired — matching the new unconditional trigger. Matching onlyPropertyKey::StaticIdentifierpreserves the computed-key exclusion ({[innerText]: text}).AssignmentTargetPropertyIdentifierarm keeps the same parent-of-ObjectAssignmentTargetcheck. The old non-APTI path was dead: a rest target (({...innerText} = node)) goes throughAssignmentTargetRest, never a directObjectAssignmentTargetchild.The rule test snapshot is byte-identical (no
.snapchange).Results
Running
oxlint --debug=timings --quiet -A all -W unicorn/prefer-dom-node-text-contenton the vscode repo.Before:
After:
Violations are unchanged (245 warnings, 47 errors). Calls drop 76% (5,996,077 → 1,437,765) and rule time drops ~80% (116ms → 23ms).
🤖 Generated with Claude Code