Skip to content

refactor(linter): resolve globals from env on build#20100

Open
Sysix wants to merge 1 commit intomainfrom
03-07-refactor_linter_resolve_globals_from_env_on_build
Open

refactor(linter): resolve globals from env on build#20100
Sysix wants to merge 1 commit intomainfrom
03-07-refactor_linter_resolve_globals_from_env_on_build

Conversation

@Sysix
Copy link
Member

@Sysix Sysix commented Mar 7, 2026

Refactored how env and globals worked together. Before we checked each individual entry if it is enabled/disabled, but they are working "hand in hand". This was a problem for overriding global with env. An example:

{
  "globals": {
    "window": "off"
  },
  "overrides": [
   {
      "files": ["*.ts"],
      "env": {
         "browser": true
       }
    }
  ]
}

This config would tell the lint rule that window is still off, but in real we re-enabled it with the env setting.


Separated to logic to resolve the globals at the beginning and avoiding to have a "OxlintEnv" instance in the resolved config. We could probably rename OxlintEnv to OxlintrcEnv now.

@github-actions github-actions bot added the A-linter Area - Linter label Mar 7, 2026
Copy link
Member Author

Sysix commented Mar 7, 2026


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of 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.

@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Mar 7, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 7, 2026

Merging this PR will not alter performance

✅ 53 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing 03-07-refactor_linter_resolve_globals_from_env_on_build (7f99530) with main (a9acb2b)

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Sysix Sysix force-pushed the 03-07-refactor_linter_resolve_globals_from_env_on_build branch 2 times, most recently from 3449eab to b1b3ac0 Compare March 7, 2026 17:53
@Sysix Sysix requested a review from Copilot March 7, 2026 17:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors linter configuration handling so that environment presets (env) are resolved into concrete globals during config build time, removing runtime env lookups in linting/context code.

Changes:

  • Remove env from LintConfig/ContextHost and stop checking enabled envs at lint-time (globals are expected to be pre-resolved).
  • Resolve env presets into OxlintGlobals when building LintConfig and when resolving overrides.
  • Update/add unit tests to assert env-to-globals resolution and override behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/oxc_linter/src/external_linter.rs Adjusts the serialized config payload sent to JS plugins to no longer include envs (now globals-only).
crates/oxc_linter/src/context/mod.rs Removes env-based global resolution from LintContext global queries.
crates/oxc_linter/src/context/host.rs Removes env() accessor from ContextHost.
crates/oxc_linter/src/config/mod.rs Drops env from LintConfig and resolves env → globals during From<Oxlintrc>.
crates/oxc_linter/src/config/globals.rs Adds internal insertion helper to support env → globals expansion.
crates/oxc_linter/src/config/env.rs Implements env → globals expansion logic (override_globals).
crates/oxc_linter/src/config/config_store.rs Removes env override merging; overrides now apply via pre-resolved globals.
crates/oxc_linter/src/config/config_builder.rs Resolves base + override env/global settings into a single globals map; adds tests for override behavior.

@Sysix Sysix force-pushed the 03-07-refactor_linter_resolve_globals_from_env_on_build branch from b1b3ac0 to fe8dce4 Compare March 7, 2026 18:28
@github-actions github-actions bot added A-cli Area - CLI A-ast-tools Area - AST tools A-linter-plugins Area - Linter JS plugins labels Mar 7, 2026
@Sysix Sysix force-pushed the 03-07-fix_linter_check_globals_entry_for_no-undef_no-extend-native_and_no-constant-binary-expression_ branch from 67983f3 to ea41224 Compare March 7, 2026 18:30
@Sysix Sysix force-pushed the 03-07-refactor_linter_resolve_globals_from_env_on_build branch 2 times, most recently from e233211 to 78e61c4 Compare March 7, 2026 18:41
@Sysix Sysix requested a review from Copilot March 7, 2026 18:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 23 changed files in this pull request and generated 3 comments.

Comment on lines +51 to +59
let Some(globals_entries) = GLOBALS.get(name) else {
continue;
};
for (env, supported) in globals_entries.entries() {
if *active {
globals_to_override.insert(env.to_string(), GlobalValue::from(*supported));
} else {
globals_to_override.insert(env.to_string(), GlobalValue::Off);
}
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OxlintEnv::override_globals currently inserts GlobalValue::Off for every global in an env when that env is set to false. This changes semantics vs the previous lookup-based approach: if a global is provided by multiple envs (e.g. setTimeout exists in both node and browser), disabling one env can incorrectly force that global to off even though another enabled env still provides it. Consider computing resolved globals as the union of enabled envs (ignore disabled envs), then apply explicit globals overrides last; for overrides that disable envs, this likely requires recomputing the resolved globals for that override rather than representing env: false as globals: off entries.

Suggested change
let Some(globals_entries) = GLOBALS.get(name) else {
continue;
};
for (env, supported) in globals_entries.entries() {
if *active {
globals_to_override.insert(env.to_string(), GlobalValue::from(*supported));
} else {
globals_to_override.insert(env.to_string(), GlobalValue::Off);
}
// Only enabled environments contribute globals; disabled envs are ignored here.
if !*active {
continue;
}
let Some(globals_entries) = GLOBALS.get(name) else {
continue;
};
for (env, supported) in globals_entries.entries() {
globals_to_override.insert(env.to_string(), GlobalValue::from(*supported));

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +44
let mut globals = OxlintGlobals::default();
config.env.override_globals(&mut globals);
config.globals.override_globals(&mut globals);
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LintConfig::from expands env presets into OxlintGlobals, which can make config.globals extremely large (builtins + enabled envs). This map is cloned in Config::apply_overrides and serialized per-file for JS plugins (see crates/oxc_linter/src/config/config_store.rs and crates/oxc_linter/src/lib.rs), so this change likely introduces a significant per-file CPU/memory regression. Consider caching a pre-serialized globals JSON per resolved config, using shared/immutable maps (e.g. Arc-backed), or keeping a more compact representation for builtins/env presets when communicating with JS.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@overlookmotel I am afraid this is a real problem for JS side? The snapshots-diff looks big for "node": true.
But in my opinion there should be no concept of "env" for the lint context. This is for me only a helper section to avoid defining all globals per line. With ESLint flat config and our js config, most users are using the globals npm package with destructruring like {globals: {...npm-globals['vue'], ...npm-globals['node']}.
So maybe this is at the end already a problem and did get only shown by the refactoring?

@Sysix Sysix force-pushed the 03-07-refactor_linter_resolve_globals_from_env_on_build branch from 78e61c4 to c2d4fb9 Compare March 7, 2026 19:01
@Sysix Sysix marked this pull request as ready for review March 7, 2026 19:18
@Sysix Sysix force-pushed the 03-07-refactor_linter_resolve_globals_from_env_on_build branch from c2d4fb9 to ccf8d18 Compare March 9, 2026 19:25
@Sysix Sysix force-pushed the 03-07-fix_linter_check_globals_entry_for_no-undef_no-extend-native_and_no-constant-binary-expression_ branch from ea41224 to 7269cc0 Compare March 9, 2026 19:25
@Sysix Sysix force-pushed the 03-07-refactor_linter_resolve_globals_from_env_on_build branch from ccf8d18 to f75f83c Compare March 9, 2026 19:44
@graphite-app graphite-app bot changed the base branch from 03-07-fix_linter_check_globals_entry_for_no-undef_no-extend-native_and_no-constant-binary-expression_ to graphite-base/20100 March 9, 2026 22:25
@graphite-app graphite-app bot force-pushed the 03-07-refactor_linter_resolve_globals_from_env_on_build branch from f75f83c to 58a1a9c Compare March 9, 2026 22:30
@graphite-app graphite-app bot force-pushed the graphite-base/20100 branch from 7269cc0 to a9acb2b Compare March 9, 2026 22:30
@graphite-app graphite-app bot changed the base branch from graphite-base/20100 to main March 9, 2026 22:31
@graphite-app graphite-app bot force-pushed the 03-07-refactor_linter_resolve_globals_from_env_on_build branch from 58a1a9c to 7f99530 Compare March 9, 2026 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast-tools Area - AST tools A-cli Area - CLI A-linter Area - Linter A-linter-plugins Area - Linter JS plugins C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants