feat(linter): implement no-useless-assignment#15466
feat(linter): implement no-useless-assignment#15466camc314 merged 12 commits intooxc-project:mainfrom
Conversation
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. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the no-useless-assignment ESLint rule, which detects and flags assignments where the newly assigned value is never read afterward. The implementation uses control flow graph analysis to track read and write operations across different code paths.
Key changes:
- Added new linter rule for detecting useless assignments (dead stores)
- Implemented CFG-based analysis to track symbol operations across control flow paths
- Added comprehensive test coverage with 70+ test cases
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/oxc_linter/src/rules/eslint/no_useless_assignment.rs | Core implementation of the no-useless-assignment rule with CFG analysis |
| crates/oxc_linter/src/snapshots/eslint_no_useless_assignment.snap | Test snapshots showing diagnostic output for failing test cases |
| crates/oxc_linter/src/rules.rs | Registration of the new rule in the linter module system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @camc314, my PR didn’t pass just r. When I run the local CI check, it generates a bunch of snapshot updates. I’m not sure if that’s expected. |
It'd depend on the specific updates to the snapshots, but that's probably fine as long as the only snapshot it's trying to update is the one for your particular rule. |
|
You'll also want to run |
Thanks for the tip! |
|
If you use VS Code or similar, you can change the rust-analyzer extension's Unfortunately it's also a lot slower, but that should help you fix the lint errors at least. I'll let cam review further as he sees fit :) |
Thanks for the quick review! I’m new to Rust and OXC, so I didn’t really have that in mind when implementing this. I understand that performance is critical for the project — is there any way I can check benchmarks for my implementation? I’d love the chance to improve the performance. |
|
Amazing work on this! Out of interest, did you use AI? if so how did you use it and which models did you use?
I just approced the CI workflows to run, that will give us a codspeed benchmark which should highlight any performance regressions. Looks like this rule is really struggling on the react benchmark. |
Merging this PR will not alter performance
Comparing Footnotes
|
I’ll see if I can work something out to improve the performance on the React benchmark. I do use AI — mainly Deepwiki, which Boshen recommended on Discord. I ask all kinds of codebase-related questions, like “How can I detect a loop in a CFG?” or “If I have a node ID, how can I get its CFG block?” It doesn’t give me direct copy-and-paste answers every single time, but it points me to the right parts of the code I can study. For example, I asked Deepwiki, “How do I get the variable scope of a symbol?” and it gave me your code: fn get_parent_variable_scope(&self, scope_id: ScopeId) -> ScopeId {
self.scoping()
.scope_ancestors(scope_id)
.find_or_last(|scope_id| self.scoping().scope_flags(*scope_id).is_var())
.expect("scope iterator will always contain at least one element")
} |
yeahh, we may need a different algorithm. admittedly, this was in debug modes but running this rule on react.development.js was taking 1+ minutes. i had a local version of this working somewhere that used fixed point iteration instead which maybe a better approach. Thanks for sharing how you used AI, i do find it interesting how others are using it. |
I’ve pinpointed that the part slowing everything down is moving things in and out of the backtracking state. I probably need to refactor the whole thing to get rid of that. I’ll try a new approach to this, so you can close this PR for now. Would you mind sharing your solution and elaborating on what “fixed-point iteration” means? Or, if you’re already working on this, I can move on to something else. To be honest, I thought this one was labeled “good first issue,” so I expected it to be an easy one. |
177df6c to
b317e90
Compare
|
Hi @camc314, I’ve improved the performance of my previous approach — it should be much faster than before. Could you give it another shot and see if the performance looks acceptable now? Thanks! I used a bitset data structure from oxc/allocator; I’m not sure if that’s allowed. I also didn’t use a fix-point iteration to resolve the loop issue. Instead, I run a separate analysis on the loop to obtain its live-ins, which I believe should be faster. |
793be5f to
2db92d3
Compare
0e53ac4 to
86ede81
Compare
8d5f263 to
2ca49d5
Compare
|
Hi @camc314 , I’ve resolved the performance regression. Could you review it? |
2ca49d5 to
94530c7
Compare
# Oxlint ### 💥 BREAKING CHANGES - 22ce6af oxlint/lsp: [**BREAKING**] Show/fix safe suggestions by default (#19816) (Sysix) ### 🚀 Features - 7a7b7b8 oxlint/lsp: Add source.fixAllDangerous.oxc code action kind (#20526) (bab) - 9cfe57e linter/unicorn: Implement prefer-import-meta-properties rule (#20662) (Irfan - ئىرفان) - 1edb391 linter/eslint: Implement `no-restricted-exports` rule (#20592) (Nicolas Le Cam) - 0f12bcd linter/react: Implement `hook-use-state` rule (#20986) (Khaled Labeb) - 1513a9f oxlint/lsp: Show note field for lsp diagnostic (#20983) (Sysix) - 7fdf722 linter/unicorn: Implement `no-useless-iterator-to-array` rule (#20945) (Mikhail Baev) - 39c8f2c linter/jest: Implement padding-around-after-all-blocks (#21034) (Sapphire) - ac39e51 linter/eslint-vitest-plugin: Prefer importing vitest globals (#20960) (Said Atrahouch) - 0b84de1 oxlint: Support allow option for prefer-promise-reject-errors (#20934) (camc314) - 23db851 linter/consistent-return: Move rule from nursery to suspicious (#20920) (camc314) - 9a27e32 linter/no-unnecessary-type-conversion: Move rule from nursery to suspicious (#20919) (camc314) - 1ca7b58 linter/dot-notation: Move rule from nursery to style (#20918) (camc314) - 73ba81a linter/consistent-type-exports: Move rule from nursery to style (#20917) (camc314) - b9199b1 linter/unicorn: Implement switch-case-break-position (#20872) (Mikhail Baev) - 3435ff8 linter: Implements `prefer-snapshot-hint` rule in Jest and Vitest (#20870) (Said Atrahouch) - 98510d2 linter: Implement react/prefer-function-component (#19652) (Connor Shea) - 871f9d9 linter: Implement no-useless-assignment (#15466) (Zhaoting Zhou) - 0f01fbd linter: Implement eslint/object-shorthand (#17688) (yue) ### 🐛 Bug Fixes - dd2df87 npm: Export package.json for oxlint and oxfmt (#20784) (kazuya kawaguchi) - 9bc77dd linter/no-unused-private-class-members: False positive with await expr (#21067) (camc314) - 60a57cd linter/const-comparisons: Detect equality contradictions (#21065) (camc314) - 2bb2be2 linter/no-array-index-key: False positive when index is passed as function argument (#21012) (bab) - 6492953 linter/no-this-in-sfc: Only flag `this` used as member expression object (#20961) (bab) - 9446dcc oxlint/lsp: Skip `node_modules` in oxlint config walker (#21004) (copilot-swe-agent) - af89923 linter/no-namespace: Support glob pattern matching against basename (#21031) (bab) - 64a1a7e oxlint: Don't search for nested config outside base config (#21051) (Sysix) - 3b953bc linter/button-has-type: Ignore `document.createElement` calls (#21008) (Said Atrahouch) - 8c36070 linter/unicorn: Add support for `Array.from()` for `prefer-set-size` rule (#21016) (Mikhail Baev) - c1a48f0 linter: Detect vitest import from vite-plus/test (#20976) (Said Atrahouch) - 5c32fd1 lsp: Prevent corrupted autofix output from overlapping text edits (#19793) (Peter Wagenet) - ca79960 linter/no-array-index-key: Move span to `key` property (#20947) (camc314) - 2098274 linter: Add suggestion for `jest/prefer-equality-matcher` (#20925) (eryue0220) - 6eb77ec linter: Allow default-import barrels in import/named (#20757) (Bazyli Brzóska) - 9c218ef linter/eslint-vitest-plugin: Remove pending fix status for require-local-test-context-for-concurrent-snapshot (#20890) (Said Atrahouch) ### ⚡ Performance - fb52383 napi/parser, linter/plugins: Clear buffers and source texts earlier (#21025) (overlookmotel) - 3b7dec4 napi/parser, linter/plugins: Use `utf8Slice` for decoding UTF-8 strings (#21022) (overlookmotel) - 012c924 napi/parser, linter/plugins: Speed up decoding strings in raw transfer (#21021) (overlookmotel) - 55e1e9b napi/parser, linter/plugins: Initialize vars as 0 (#21020) (overlookmotel) - c25ef02 napi/parser, linter/plugins: Simplify branch condition in `deserializeStr` (#21019) (overlookmotel) - 9f494c3 napi/parser, linter/plugins: Raw transfer use `String.fromCharCode` in string decoding (#21018) (overlookmotel) - 0503a78 napi/parser, linter/plugins: Faster deserialization of `raw` fields (#20923) (overlookmotel) - a24f75e napi/parser: Optimize string deserialization for non-ASCII sources (#20834) (Joshua Tuddenham) ### 📚 Documentation - af72b80 oxlint: Fix typo for --tsconfig (#20889) (leaysgur) - 70c53b1 linter: Highlight that tsconfig is not respected in type aware linting (#20884) (camc314) # Oxfmt ### 🚀 Features - 35cf6e8 oxfmt: Add node version hint for ts config import failures (#21046) (camc314) ### 🐛 Bug Fixes - dd2df87 npm: Export package.json for oxlint and oxfmt (#20784) (kazuya kawaguchi) - 9d45511 oxfmt: Propagate file write errors instead of panicking (#20997) (leaysgur) - 139ddd9 formatter: Handle leading comment after array elision (#20987) (leaysgur) - 4216380 oxfmt: Support `.editorconfig` `tab_width` fallback (#20988) (leaysgur) - d10df39 formatter: Resolve pending space in fits measurer before expanded-mode early exit (#20954) (Dunqing) - f9ef1bd formatter: Avoid breaking after `=>` when arrow body has JSDoc type cast (#20857) (bab) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
No description provided.