Skip to content

Conversation

dbarabashh
Copy link
Contributor

@dbarabashh dbarabashh commented Aug 8, 2025

Fix #11334

PR Checklist

Overview

This PR improves type safety in the scope-manager package by preventing TSESTree.Program nodes from being used as definition nodes.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @dbarabashh!

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

netlify bot commented Aug 8, 2025

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 382a149
🔍 Latest deploy log https://app.netlify.com/projects/typescript-eslint/deploys/68b0b4fce94a83000803763e
😎 Deploy Preview https://deploy-preview-11469--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 77 (🔴 down 20 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 your Netlify project configuration.

Copy link

nx-cloud bot commented Aug 8, 2025

View your CI Pipeline Execution ↗ for commit 382a149

Command Status Duration Result
nx test eslint-plugin --coverage=false ✅ Succeeded 5m 16s View ↗
nx run-many -t lint ✅ Succeeded 3m 13s View ↗
nx run-many -t typecheck ✅ Succeeded 2m 22s View ↗
nx test typescript-estree --coverage=false ✅ Succeeded 21s View ↗
nx test eslint-plugin-internal --coverage=false ✅ Succeeded 9s View ↗
nx run integration-tests:test ✅ Succeeded 6s View ↗
nx run types:build ✅ Succeeded 5s View ↗
nx run generate-configs ✅ Succeeded 9s View ↗
Additional runs (27) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-08-28 20:11:12 UTC

@dbarabashh
Copy link
Contributor Author

I am not sure that the linter failed now exactly because of my changes, because locally this command was successful

@JoshuaKGoldberg
Copy link
Member

Try a yarn build first, then a yarn lint? It looks like the types are better informed because of this change (nice!), so lint rules are pointing out previously missed unnecessary ?.s and the like.

Copy link

codecov bot commented Aug 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.87%. Comparing base (31a7336) to head (382a149).
⚠️ Report is 28 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11469      +/-   ##
==========================================
- Coverage   90.87%   90.87%   -0.01%     
==========================================
  Files         505      505              
  Lines       51146    51143       -3     
  Branches     8424     8424              
==========================================
- Hits        46480    46477       -3     
  Misses       4652     4652              
  Partials       14       14              
Flag Coverage Δ
unittest 90.87% <100.00%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
...t-plugin/src/rules/no-confusing-void-expression.ts 100.00% <100.00%> (ø)
...s/eslint-plugin/src/util/collectUnusedVariables.ts 96.19% <100.00%> (ø)
...ges/scope-manager/src/definition/DefinitionBase.ts 100.00% <ø> (ø)
...src/definition/ImplicitGlobalVariableDefinition.ts 100.00% <ø> (ø)
packages/scope-manager/src/referencer/Reference.ts 93.44% <ø> (ø)
🚀 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.

@dbarabashh
Copy link
Contributor Author

@JoshuaKGoldberg thank you, done

@kirkwaiblinger
Copy link
Member

@dbarabashh Would you mind also auditing for uses of nullThrows that can be removed? For example,

const block = nullThrows(node.parent, NullThrowsReasons.MissingParent);

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

+1 to what Kirk said. I also think the new type is good enough that it should be moved to @typescript-eslint/types altogether. Thanks for starting this! 🙌

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Aug 27, 2025
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 28, 2025
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🤤 look at all those removed ?~s and nullThrows()`s! Absolutely lovely. Thanks!

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Sep 3, 2025
Copy link
Member

@kirkwaiblinger kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

🎉🎉

@JoshuaKGoldberg JoshuaKGoldberg merged commit c068a4f into typescript-eslint:main Sep 3, 2025
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: DefinitionBase shouldn't allow its Node type to be TSESTree.Program
3 participants