typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star16k

fix(scope-manager): reconcile type-value distinction for implicit globals across libs#11715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

lilnasy wants to merge2 commits intotypescript-eslint:mainfromlilnasy:main

Closed

fix(scope-manager): reconcile type-value distinction for implicit globals across libs#11715

lilnasy wants to merge2 commits intotypescript-eslint:mainfromlilnasy:main

+42 −2

Conversation

Copy link

lilnasy commentedOct 23, 2025

PR Checklist

Addresses an existing open issue:fixesScope Manager fails to pick up references to implicit globals #11714
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

To fix the issue, implicit variables are mutated as the global scope is being populated.

I admittedly did not spend much time thinking of a proper solution. Though it was tricky to pinpoint, the fix is simple. I imagine the "correct" location for the fix is a matter of maintainer preference.

Direct commits from maintainers welcome.

Copy link

Contributor

typescript-eslintbot commentedOct 23, 2025

Thanks for the PR,@lilnasy!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlifybot commentedOct 23, 2025•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`381f374`
🔍 Latest deploy log	https://app.netlify.com/projects/typescript-eslint/deploys/691cc5cb2055780008dd5de6
😎 Deploy Preview	https://deploy-preview-11715--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 2 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 92 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify project configuration.

lilnasy changed the title~~fix(scope manager): reconcile type-value distinction for implicit globals across libs~~fix(scope-manager): reconcile type-value distinction for implicit globals across libs

Oct 23, 2025

Copy link

nx-cloudbot commentedOct 23, 2025•
edited
Loading

🤖 Nx Cloud AI Fix Eligible

An automatically generated fix could have helped fix failing tasks for this run, but Self-healing CI is disabled for this workspace. Visit workspace settings to enable it and get automatic fixes in future runs.

_{To disable these notifications, a workspace admin can disable themin workspace settings.}

View yourCI Pipeline Execution ↗ for commit381f374

Command	Status	Duration	Result
`nx run-many -t lint`	❌ Failed	3m 21s	View ↗
`nx test eslint-plugin --coverage=false`	✅ Succeeded	5m 23s	View ↗
`nx run-many -t typecheck`	✅ Succeeded	1m 59s	View ↗
`nx test eslint-plugin-internal --coverage=false`	✅ Succeeded	12s	View ↗
`nx test typescript-estree --coverage=false`	✅ Succeeded	2s	View ↗
`nx run integration-tests:test`	✅ Succeeded	4s	View ↗
`nx run types:build`	✅ Succeeded	5s	View ↗
`nx run generate-configs`	✅ Succeeded	7s	View ↗
`Additional runs (29)`	✅ Succeeded	...	View ↗

☁️Nx Cloud last updated this comment at2025-11-18 19:27:48 UTC

Copy link

codecovbot commentedOct 23, 2025•
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.55%. Comparing base (28cf803) to head (381f374).
⚠️ Report is 26 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@##             main   #11715   +/-   ##=======================================  Coverage   90.55%   90.55%           =======================================  Files         522      522             Lines       53063    53073   +10       Branches     8838     8842    +4     =======================================+ Hits        48049    48059   +10- Misses       4999     5000    +1+ Partials       15       14    -1

Flag	Coverage Δ
unittest	`90.55% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

Files with missing lines	Coverage Δ
packages/scope-manager/src/scope/ScopeBase.ts	`96.04% <100.00%> (+0.12%)`	⬆️
.../scope-manager/src/variable/ImplicitLibVariable.ts	`87.50% <100.00%> (ø)`

🚀 New features to boost your workflow:

❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lilnasy force-pushed themain branch fromdfc9368 tocae9f5aCompare

November 1, 2025 21:28

lilnasy mentioned this pull request

Nov 1, 2025

fix(linter): patch@typescript-eslint/scope manageroxc-project/oxc#15214

Merged

bradzacher requested changes

Nov 4, 2025

View reviewed changes

Copy link

Member

bradzacher left a comment

Choose a reason for hiding this comment

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

instead of mutating the variable here -- could we instead do it higher.

eg:

In theGlobalScope class (which callsScopeBase.defineVariable):

typescript-eslint/packages/scope-manager/src/scope/GlobalScope.ts

Lines 68 to 79 inea2ee6b

	publicdefineImplicitVariable(
	name:string,
	options:ImplicitLibVariableOptions,
	):void{
	this.defineVariable(
	newImplicitLibVariable(this,name,options),
	this.set,
	this.variables,
	null,
	null,
	);
	}

Or in theReferencer class (which callsGlobalScope.defineImplicitVariable):

typescript-eslint/packages/scope-manager/src/referencer/Referencer.ts

Lines 72 to 75 inea2ee6b

	// This loop is guaranteed to see each included lib exactly once
	for(const[name,variable]oflib.variables){
	globalScope.defineImplicitVariable(name,variable);
	}

I think the latter would be better as we could pre-aggregate the variable definitions to deduplicate and merge them before even callingdefineImplicitVariable.

lilnasy force-pushed themain branch from191576b tob3876a9Compare

November 8, 2025 02:20

Copy link

Author

lilnasy commentedNov 8, 2025

@bradzacher I'm thinking it would be even better to go one level higher.

The first year-wise occurrence of the globals I glimpsed at seems to always beTYPE_VALUE. If that holds true for all, we could simply ensure the iteration is in chronological order at runtime and it would work. The test could be made more comprehensive to prevent regressions.

lilnasy force-pushed themain branch fromb3876a9 tod78359dCompare

November 10, 2025 18:22

Copy link

Author

lilnasy commentedNov 10, 2025•
edited
Loading

I have a branch where Iflatten the lib references at codegen time instead of at runtime, fixing the bug right there. It increases the package size a little but reducesScopeManager instantiation time.

I can move forward with whichever approach maintainers prefer.

overlookmotel pushed a commit to oxc-project/oxc that referenced this pull request

Nov 17, 2025

fix(linter): patch@typescript-eslint/scope manager(#15214)

fba31fa

- Part of#14827- Fixes the bug noticed in a review in#14890 (comment)- Originally reported astypescript-eslint/typescript-eslint#11714- Originally fixed intypescript-eslint/typescript-eslint#11715- Patching in oxlint because neither the bug nor the fix has beenreviewed in the `typescript-eslint` org.

lilnasy added2 commits

November 19, 2025 00:45

fix(scope manager): reconcile type-value distinction for implicit glo…

7577887

…bals across libs

fix test failures

381f374

lilnasy force-pushed themain branch fromd78359d to381f374Compare

November 18, 2025 19:15

JoshuaKGoldberg requested a review frombradzacher

November 24, 2025 13:58

Copy link

Member

bradzacher commentedNov 24, 2025

I believe the behaviour that we will need to add as part of the eslint v10 upgrades will provide you the behaviour you want.

This behaviour will resolve all references to globals as part of the analysis rather than leaving them all open.

Copy link

Member

JoshuaKGoldberg commentedNov 28, 2025•
edited
Loading

@bradzacher so just confirming, is there any change we should make here? Is#11714 a no-op/wontfix given the upcoming ESLint v10 changes?

Linking for visibility:#11762 tracks updating scope-manager for compatibility with ESLint v10.

JoshuaKGoldberg added the triageWaiting for team members to take a look label

Nov 28, 2025

Copy link

Author

lilnasy commentedNov 28, 2025

I believe the behaviour that we will need to add as part of the eslint v10 upgrades will provide you the behaviour you want.

I agree.

Is#11714 a no-op/wontfix given the upcoming ESLint v10 changes?

@JoshuaKGoldberg I think that depends on what you meant in the comment here:#11762 (comment). Could you clarify? Can@typescript-eslint/scope-manager remain compatible with both ESLint v8 and v10?

Copy link

Member

JoshuaKGoldberg commentedNov 28, 2025

We haven't looked deeply into it yet, but I really hope so. Dropping support for v8 and v9 would be a breaking change that we're not ready to release a new major version for anytime soon.

Copy link

Member

bradzacher commentedNov 29, 2025

Well technically by being compatible with v10 we're also compatible with v8/v9 - cos eslint will just have nothing to do.

Copy link

Author

lilnasy commentedNov 29, 2025

Thanks, I thought that might be the case. Fixing this as part of ESLint 10 compat seems appropriate. Oxlint will be able to remove the patch we made to@typescript-eslint/scope-manager then (if it happens before our rust reimplementation).

lilnasy closed this

Nov 29, 2025

Labels

triage

Waiting for team members to take a look

Movatterモバイル変換

Uh oh!

fix(scope-manager): reconcile type-value distinction for implicit globals across libs#11715

fix(scope-manager): reconcile type-value distinction for implicit globals across libs#11715

Conversation

lilnasy commentedOct 23, 2025

PR Checklist

Overview

Uh oh!

typescript-eslintbot commentedOct 23, 2025

Uh oh!

netlifybot commentedOct 23, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

✅ Deploy Preview fortypescript-eslint ready!

Uh oh!

nx-cloudbot commentedOct 23, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

🤖 Nx Cloud AI Fix Eligible

Uh oh!

codecovbot commentedOct 23, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

lilnasy commentedNov 8, 2025

Uh oh!

lilnasy commentedNov 10, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

bradzacher commentedNov 24, 2025

Uh oh!

JoshuaKGoldberg commentedNov 28, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

lilnasy commentedNov 28, 2025

Uh oh!

JoshuaKGoldberg commentedNov 28, 2025

Uh oh!

bradzacher commentedNov 29, 2025

Uh oh!

lilnasy commentedNov 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlifybot commentedOct 23, 2025•
edited
Loading

nx-cloudbot commentedOct 23, 2025•
edited
Loading

codecovbot commentedOct 23, 2025•
edited
Loading

lilnasy commentedNov 10, 2025•
edited
Loading

JoshuaKGoldberg commentedNov 28, 2025•
edited
Loading