fix(linter): check globals entry for no-undef, only check es2026 globals for no-extend-native and no-constant-binary-expression#20089
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
Updates eslint rule implementations to treat globals configuration entries as “defined globals” when determining whether identifiers are global for no-undef, no-extend-native, and no-constant-binary-expression.
Changes:
- Introduces
LintContext::is_global_definedto unify “global defined” checks across env globals andglobalsconfig. - Replaces direct
env_contains_varchecks in the three rules withis_global_defined. - Makes
env_contains_varan internal helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/oxc_linter/src/rules/eslint/no_undef.rs | Switches the rule’s global check to use ctx.is_global_defined. |
| crates/oxc_linter/src/rules/eslint/no_extend_native.rs | Switches the rule’s “is global object” check to use ctx.is_global_defined. |
| crates/oxc_linter/src/rules/eslint/no_constant_binary_expression.rs | Uses ctx.is_global_defined for new <Ident>() constructor global detection. |
| crates/oxc_linter/src/context/mod.rs | Adds is_global_defined and makes env_contains_var private. |
Comments suppressed due to low confidence (1)
crates/oxc_linter/src/rules/eslint/no_undef.rs:82
- After switching to
is_global_defined, the subsequentctx.globals().is_enabled(name)check becomes redundant (sinceis_global_definedalready considers non-"off" entries fromglobals). Keeping both checks makes the control flow harder to follow and suggests the two conditions differ when they currently do not in this context.
if ctx.is_global_defined(name) {
continue;
}
if ctx.globals().is_enabled(name) {
continue;
}
crates/oxc_linter/src/rules/eslint/no_constant_binary_expression.rs
Outdated
Show resolved
Hide resolved
Merging this PR will not alter performance
Comparing Footnotes
|
6259879 to
67983f3
Compare
67983f3 to
ea41224
Compare
ea41224 to
7269cc0
Compare
globals entry for no-undef, no-extend-native and no-constant-binary-expressionglobals entry for no-undef, only check es2026 globals for no-extend-native and no-constant-binary-expression
Merge activity
|
…globals for `no-extend-native` and `no-constant-binary-expression` (#20089) These 3 rules are the only one which did not checked for "globals" and only lookedup the envs. In the future PR I want to remove env from the lint context. It should be resolved into "globals" before. Added one test case where we already tested the "globals" option
7269cc0 to
a9acb2b
Compare
# Oxlint ### 🚀 Features - 04a5ce0 oxlint: Support `vite.config.ts` `.lint` field (#20214) (leaysgur) - 1735215 linter: Implement `react/no-clone-element` rule. (#20129) (connorshea) - 68e6f6f linter: Implement `react/no-react-children` rule. (#20104) (connorshea) - fe3b32e linter/plugins: Add `oxlint-plugin-eslint` package (#20009) (overlookmotel) ### 🐛 Bug Fixes - 05f6a09 linter/no-inline-comments: Deserialize rule options with serde (#20207) (camc314) - c7eb09d linter/default-case: Deserialize rule options with serde (#20206) (camc314) - f85e16c linter/plugins: Fix types for visitor compilation (#20203) (overlookmotel) - 44e24e0 linter/exhaustive-deps: Ignore type-only typeof deps (#20201) (camc314) - 0b04998 linter/no-fallthrough: Deserialize rule options with serde (#20192) (camc314) - a1031cb linter/new-cap: Deserialize rule options with serde (#20161) (camc314) - ad27fd6 linter: Add help messages to import plugin diagnostics (#20158) (John Costa) - 1340307 linter/plugins: Ensure `after` hooks always run (#20167) (overlookmotel) - c4812ec linter/plugins: Reset visitor compilation state if error during compilation (#20166) (overlookmotel) - 887eecc linter/plugins: Add license notice to `oxlint-plugin-eslint` package (#20164) (overlookmotel) - e1713a4 linter/plugins: Include common chunks in `oxlint-plugin-eslint` package (#20163) (overlookmotel) - a9acb2b linter: Check `globals` entry for `no-undef`, only check es2026 globals for `no-extend-native` and `no-constant-binary-expression` (#20089) (Sysix) - 5559f0d linter/no-unused-vars: `reportUsedIgnorePattern` should not report used rest siblings (#20108) (Don Isaac) - de7c0e2 linter/plugins: Correct error message for `markVariableAsUsed` (#20152) (overlookmotel) ### ⚡ Performance - 3a86427 linter/plugins: Pre-populate cache of `EnterExit` objects at startup (#20194) (overlookmotel) - d243391 linter/plugins: Replace arrays with `Uint8Array`s (#20190) (overlookmotel) - 8742f8b linter/plugins: Pre-populate cache of `VisitProp` objects (#20189) (overlookmotel) - 3061acb linter/plugins: Pre-populate cache of `EnterExit` objects (#20187) (overlookmotel) - c73912b linter/plugins: Free visit functions earlier (#20186) (overlookmotel) - d9f8ff4 linter/plugins: Faster reset of visitor state (#20185) (overlookmotel) - 42aff15 oxlint/lsp: Avoid computing diagnostics for non invoked code actions requests (#20080) (Sysix) ### 📚 Documentation - a080650 linter/plugins: Fix documentation of visitor compilation (#20202) (overlookmotel) - 542a04a linter: Add a link to the cyclomatic complexity Wikipedia article in `eslint/complexity` (#20174) (connorshea) # Oxfmt ### 🚀 Features - 95943aa oxfmt: Support `vite.config.*` `.fmt` field (#20197) (leaysgur) - 172fc07 oxfmt: .js/.ts config file support (#20135) (leaysgur) ### 🐛 Bug Fixes - e483569 oxfmt: Avoid double-escaping in css-in-js (#20211) (leaysgur) Co-authored-by: leaysgur <6259812+leaysgur@users.noreply.github.com>

These 3 rules are the only one which did not checked for "globals" and only lookedup the envs.
In the future PR I want to remove env from the lint context. It should be resolved into "globals" before.
Added one test case where we already tested the "globals" option