feat(linter): implement prefer-const#15707
Closed
connorshea wants to merge 36 commits intooxc-project:mainfrom
Closed
feat(linter): implement prefer-const#15707connorshea wants to merge 36 commits intooxc-project:mainfrom
connorshea wants to merge 36 commits intooxc-project:mainfrom
Conversation
connorshea
changed the title
Contributor
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. |
CodSpeed Performance ReportMerging #15707 will not alter performanceComparing Summary
Footnotes
|
camc314
reviewed
Nov 15, 2025
Entirely done by GitHub Copilot w/ Claude Sonnet 4.5.
Unclear why this was done this way in eslint's tests originally.
…hings here now...
connorshea
force-pushed
the
implement-prefer-const
branch
from
320c6ca to
1d22e29
Compare
connorshea
changed the title
Member
Author
|
I'm marking this as ready for review since I've gotten the tests all passing with cam's fix for the ast. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Member
Author
|
Tested enabling the rule on mastodon, which currently does not have it enabled and so has some violations. And I got identical results as far as I can tell :D 48 errors for each. eslint: oxlint: |
Member
Author
|
Compared the violations for VS Code between ESLint and oxlint since VS Code is huge and has this rule enabled. EDIT: actually I think disregard the examples after this, some of these may be invalid code if changed to const, so this part still needs to be fixed and these examples should be added to the tests. incorrect observatione.g. for (let match: RegExpExecArray | null; (match = ELISION_REGEX.exec(line));) {
locations.push({
targetRange,
targetSelectionRange: new vscode.Range(lineNumber, offset, lineNumber, offset),
targetUri: currentTarget,
originSelectionRange: new vscode.Range(i, lastEnd, i, ELISION_REGEX.lastIndex - match[0].length),
});
offset += (ELISION_REGEX.lastIndex - lastEnd - match[0].length) + Number(match[1]);
lastEnd = ELISION_REGEX.lastIndex;
}and then So that's neat. I didn't see any obvious false-positives. |
Also use backticks for code references.
`let foo;` is valid, but `const foo;` is not. So this kind of cases should be a violation.
…valid. Add a test for this case so it doesn't regress.
Just to be sure that we don't misinterpret commented out code as actual code.
…sed. inside for statements, it's not really practical to change let to const like this. Also add some basic cases for typescript code.
Member
Author
|
Got it down to two cases where the rule raises a violation in the vscode codebase: And I think these are both correct/fine. The first is just because of a difference in where the highlighted violation occurs, it's disabled on a different line than is necessary for the oxlint rule, because the oxlint rule highlights the export function autorunSelfDisposable(fn: (reader: IReaderWithDispose) => void, debugLocation = DebugLocation.ofCaller()): IDisposable {
let ar: IDisposable | undefined;
let disposed = false;
// eslint-disable-next-line prefer-const
ar = autorun(reader => {
fn({
delayedStore: reader.delayedStore,
store: reader.store,
readObservable: reader.readObservable.bind(reader),
dispose: () => {
ar?.dispose();
disposed = true;
}
});
}, debugLocation);
if (disposed) {
ar.dispose();
}
return ar;
}And the second seems like it works fine if for (let seen = new Set(); result && !seen.has(resolvedType);) {
seen.add(resolvedType);
result = await resolveDebugConfigurationForType(resolvedType, result);
result = await resolveDebugConfigurationForType('*', result);
resolvedType = result?.type ?? type!;
}
return result;So I think we're fully good to go now :) |
camc314
reviewed
Nov 16, 2025
camc314
reviewed
Nov 16, 2025
Comment on lines
369
to
377
| let has_read_before_write = references.iter().any(|r| { | ||
| if !r.is_read() || r.node_id() == write_node_id { | ||
| return false; | ||
| } | ||
| // Simple span comparison - if read comes before write in source | ||
| let read_span = ctx.nodes().get_node(r.node_id()).kind().span(); | ||
| let write_span = ctx.nodes().get_node(write_node_id).kind().span(); | ||
| read_span.start < write_span.start | ||
| }); |
Contributor
There was a problem hiding this comment.
how does eslint do this. Relying on spans will be unreliable as functions are hoisted ect.
Member
Author
There was a problem hiding this comment.
I didn't even think about hoisted functions breaking here while going through the code tbh, good point.
I tried adding a test case for ensuring this works with a basic hoisted function, but it wasn't able to trigger an issue with the previous implementation on it, so it's probably insufficient as a test. I'd appreciate any example you can give me of code where this'd be problematic :)
I've updated it to avoid using span comparison and I think it now will work correctly because this should now go through it in semantic order. But I'm relying heavily on claude here, so I'm not 100% sure.
This is the ESLint implementation: https://github.com/eslint/eslint/blob/ca4d3b40085de47561f89656a2207d09946ed45e/lib/rules/prefer-const.js#L239-L241
I assume this part should also be updated to avoid span start comparisons? https://github.com/connorshea/oxc/blob/328ec563a69a8763fbb0f3d7892f088945227eb7/crates/oxc_linter/src/rules/eslint/prefer_const.rs#L331-L344
Contributor
There was a problem hiding this comment.
🫠 looks like eslint relies on the order of the references being in the same order as they will be evaluated.
Let me think about this more.
There was a problem hiding this comment.
After an attempt of implementing this rule I discovered this PR 😂 So I wanna see it get finished...
I haven't looked into the source code for ESLint too deeply, but tested the core hoisting behaviour of both ESLint and this implementation and they both behave the same in the following tests:
vec![
(
"function foo() { bar(x); } let x = 0;",
Some(serde_json::json!([{ "ignoreReadBeforeAssign": true }])),
), // Pass
(
"let x = 0; function foo() { bar(x); }",
Some(serde_json::json!([{ "ignoreReadBeforeAssign": true }])),
), // Fail (Missing exact test in PR)
(
"function foo() { bar(x); } let x = 0;",
Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])),
), // Fail
(
"let x = 0; function foo() { bar(x); }",
Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])),
), // Fail
]
camc314
reviewed
Nov 16, 2025
Avoid using `collect()` when unnecessary while checking for write-only refs.
Comparing span starts doesn't necessarily work, because of hoisting, so compare nodes instead to rely on the semantic order.
graphite-app bot
pushed a commit
that referenced
this pull request
Dec 3, 2025
Dogfood Oxlint JS plugins by enabling ESLint's `prefer-const` rule, running as a JS plugin. #15707 will implement this rule natively in Oxlint, but in meantime this is a good one to use to test JS plugins.
taearls
pushed a commit
to taearls/oxc
that referenced
this pull request
Dec 11, 2025
Dogfood Oxlint JS plugins by enabling ESLint's `prefer-const` rule, running as a JS plugin. oxc-project#15707 will implement this rule natively in Oxlint, but in meantime this is a good one to use to test JS plugins.
web-dev-sam
reviewed
Jan 1, 2026
| "let { name, ...otherStuff } = obj; otherStuff = {};", | ||
| Some(serde_json::json!([{ "destructuring": "any" }])), | ||
| ), // { "parser": require(fixtureParser("babel-eslint5/destructuring-object-spread"), ), }, | ||
| ("let x; function foo() { bar(x); } x = 0;", None), |
There was a problem hiding this comment.
Duplicate test
Suggested change
| ("let x; function foo() { bar(x); } x = 0;", None), |
Comment on lines
+962
to
+975
| ( | ||
| "function square(n) { x * x } | ||
| let x = 5; | ||
| console.log(square(4));", | ||
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | ||
| ), | ||
| ( | ||
| "function foo() { bar(x); } let x = 0;", | ||
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | ||
| ), | ||
| ( | ||
| "let x = 0; function foo() { bar(x); }", | ||
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | ||
| ), |
There was a problem hiding this comment.
Test for the last hoisting/configuration possibility/direction:
Suggested change
| ( | |
| "function square(n) { x * x } | |
| let x = 5; | |
| console.log(square(4));", | |
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | |
| ), | |
| ( | |
| "function foo() { bar(x); } let x = 0;", | |
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | |
| ), | |
| ( | |
| "let x = 0; function foo() { bar(x); }", | |
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | |
| ), | |
| ( | |
| "function square(n) { x * x } | |
| let x = 5; | |
| console.log(square(4));", | |
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | |
| ), | |
| ( | |
| "function foo() { bar(x); } let x = 0;", | |
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | |
| ), | |
| ( | |
| "let x = 0; function foo() { bar(x); }", | |
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": false }])), | |
| ), | |
| ( | |
| "let x = 0; function foo() { bar(x); }", | |
| Some(serde_json::json!([{ "ignoreReadBeforeAssign": true }])), | |
| ), |
Member
|
closing in favor of #18687 |
graphite-app bot
pushed a commit
that referenced
this pull request
Jan 29, 2026
- closes #12645 - redux of #15707 A slightly refreshed version of @connorshea's PR here: #15707. This includes a set of oxc-exclusive tests that try to test a few additional scenarios. This PR also includes a conditional fix which the previous PR did not. This is currently passing all of the ESLint tests, plus our own oxc tests too.
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
4 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.
Taking a stab at implementing prefer-const, but I could probably use someone else's help to finish this up.
This was primarily implemented on a whim a few weeks ago, using GitHub Copilot and Claude Sonnet 4.5. I've gotten all the tests passing, so I think this is ready for review. Unfortunately as a Rust noob I have little ability to tell whether this code is following best practices (it almost certainly isn't, it feels a bit janky even as someone unfamiliar with Rust). I've done some refactoring based on code smells I've noticed, but it's still pretty smelly.
I did test it on the mastodon repo as well to ensure it detects problematic code correctly, and it gets identical results to what the eslint rule reports: #15707 (comment)
Fixes #12645.