Skip to content

feat(no-undef, no-global-assign): support host-supplied globals - #1517

Open
bartlomieju wants to merge 2 commits into
mainfrom
no-undef-lib-globals
Open

feat(no-undef, no-global-assign): support host-supplied globals#1517
bartlomieju wants to merge 2 commits into
mainfrom
no-undef-lib-globals

Conversation

@bartlomieju

@bartlomieju bartlomieju commented Jun 29, 2026

Copy link
Copy Markdown
Member

The no-undef and no-global-assign rules consult a single hand-maintained
GLOBALS list, so DOM globals such as document and HTMLElement are reported
as "not defined" even when a project's compilerOptions.lib includes "dom",
and there is no way to make either rule reflect the environment the project
actually configured.

This adds an optional globals field to LintConfig: a name -> writable map
that, when present, fully replaces the built-in GLOBALS list for both rules.
The intent is that the host (deno) derives this set from the resolved
TypeScript lib and passes it in, so deno_lint itself carries no hardcoded
per-lib tables and never drifts against a TypeScript version. When the field
is absent the built-in list is used, so default behavior is unchanged. Both
rules now resolve globals through Context::global_with_writable /
Context::is_global, which preserves the writable flag that no-global-assign
relies on (onmessage may be reassigned, Object may not).

A follow-up commit converts the built-in GLOBALS list from a linear-scanned
slice to a compile-time phf::Map, so both the built-in and host-supplied
lookups are O(1) per identifier instead of a scan over ~200 entries. The list
is module-private, so this is not a public API change. It also drops a
duplicate TransformStream entry that was harmless for the linear scan but
that phf rejects as a duplicate key.

This is the deno_lint half of the change; a deno-side follow-up will compute
the set from compilerOptions.lib and wire it through LintConfig.globals. It
supersedes #1493, which instead adds a second, unconditional DOM_GLOBALS
list -- that approach weakens no-undef for every project regardless of its
lib configuration, whereas this keeps the recognized globals tied to what
the project configured.

Refs #622, #590, #555, #1287, denoland/deno#27379

`no-undef` and `no-global-assign` previously consulted a single
hand-maintained `GLOBALS` static, so DOM globals such as `document`
were reported as "not defined" even when a project's
`compilerOptions.lib` includes `"dom"` (denoland/deno#27379, #622,
#590), and there was no way to reflect the configured environment.

Add an optional `globals` field to `LintConfig`: a name -> writable map
(`ConfiguredGlobals`) that, when present, fully replaces the built-in
`GLOBALS` for both rules. The host (deno) derives it from the resolved
TypeScript `lib`, keeping deno_lint free of any hardcoded per-lib
tables. When absent, the built-in list is used, so default behavior is
unchanged.

Both rules now go through `Context::global_with_writable` /
`Context::is_global` rather than reading `GLOBALS` directly, so the
writable flag (e.g. `onmessage` may be reassigned, `Object` may not)
keeps driving `no-global-assign`.
The None branch of Context::global_with_writable did an O(n) linear scan
over the ~210-entry GLOBALS slice for every identifier. Convert GLOBALS to
a compile-time phf::Map so both the built-in and host-supplied lookups are
O(1). GLOBALS is private (mod globals is not pub), so the type change is
not a public API break.

Also removes a duplicate TransformStream entry that was harmless for the
linear scan but rejected by phf::phf_map! as a duplicate key.
@bartlomieju

Copy link
Copy Markdown
Member Author

Verdict: LGTM — correct, well-tested, good design + a perf win. Recommend merge (public API + behavior change → maintainer sign-off; mergeStateStatus: BLOCKED, so not auto-merging).

This is the principled version of the DOM-globals problem: the host derives the recognized set from compilerOptions.lib and passes it via LintConfig::globals, instead of hardcoding an unconditional DOM list. It correctly supersedes #1493 (which I reviewed earlier and flagged for exactly the false-negative-in-non-DOM-projects issue and the linear-scan perf — both addressed here). Recommend closing #1493 as superseded.

Correctness — verified

  • Context::global_with_writable resolves host globals when present, else built-in GLOBALS, preserving the writability bit that no-global-assign depends on. is_global = .is_some(). Clean.
  • no_global_assign faithfully keeps the semantics: if let Some(writable) = ...; if !writable { flag } — read-only globals can't be reassigned, writable ones can. The new no_global_assign_configured_globals test covers read-only (document), writable (onmessage), and full-replacement (Object not in the supplied set → allowed).
  • The GLOBALSphf::Map conversion is behavior-preserving; dropping the duplicate TransformStream key is correct (harmless under linear scan, required by phf). Module-private, so no public API change from that part.

Performance — improvement

  • O(1) phf lookup per identifier replaces the ~200-entry linear scan for both built-in and host-supplied paths. This is the hot-path win I suggested on fix(no-undef): recognize DOM globals #1493 — nice to see it here for real.

One caution for the deno-side follow-up (out of scope here)

  • Since a host-supplied set fully replaces the built-in list, the deno side must include core JS globals (Object, Array, globalThis, …) in addition to lib-derived ones, or no-undef will false-positive on them. Intentional per the design, just worth guarding in the CLI wiring.

Housekeeping: public LintConfig::globals addition + behavior change → sign-off. mergeable: MERGEABLE but BLOCKED (likely awaiting review/CI).

No changes requested. Solid; recommend merging (and closing #1493).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant