This modifies the unused variable analysis to ignore declarations that are "implicitly" exported. Roughly, this means declaration files which do not contain any exports, recursively into namespaces, excludingexport import ... = ... which doesn't count.

This PR is not yet complete; I'm missing:

Look forexport {} blocks, including empty ones (which probably don't show up in scope)
Check other ambient contexts like globals (need to query for that, and figure out the rules)
Consider whether I should actually move this into the rule and do things like those which useambientDeclarationSelector
Consider whether this analysis can actually replaceambientDeclarationSelector entirely (if not vice versa)

I'm mainly having trouble at the moment debugging a single test; addingonly to the test doesn't seem to really work and one of my tests fails for a reason I haven't figured out yet.

That and my continued issues withnx, where it always tells me that the tests pass, so I have to disable it, which can only be done via the checked in config file☹️

jakebailey added3 commits

March 2, 2024 20:41

WIP

7117fcc

WIP

3d2fc3b

revert nx

f9f18c5

Copy link

Contributor

typescript-eslintbot commentedMar 6, 2024

Thanks for the PR,@jakebailey!

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 commentedMar 6, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`f9f18c5`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/65e8f850e46f9400093786ac
😎 Deploy Preview	https://deploy-preview-8611--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 9 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (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 site configuration.

Copy link

nx-cloudbot commentedMar 6, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitf9f18c5. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

🟥 Failed Commands
`nx test eslint-plugin --coverage=false`

✅ Successfully ran 28 targets

Sent with 💌 fromNxCloud.

jakebailey changed the title~~feat(eslint-plugin): [no-unused-var] Handle implicit exports in declaration files~~feat(eslint-plugin): [no-unused-var] handle implicit exports in declaration files

Mar 6, 2024

bradzacher reviewed

Mar 6, 2024

View reviewed changes

packages/eslint-plugin/src/util/collectUnusedVariables.ts

		}

		for (const variable of scope.variables) {
		if (isExported(variable) && !isExportImportEquals(variable)) {

Copy link

Member

bradzacherMar 6, 2024

Choose a reason for hiding this comment

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

declarenamespaceFoo{constx=1;exportconsty=3;}Foo.x;Foo.y;

Theexport doesn't always restrict other implicit, ambient exports, right?

Copy link

CollaboratorAuthor

jakebaileyMar 6, 2024

Choose a reason for hiding this comment

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

Hm, the rules for this are super annoying. What I have currently may just be true only for declaration files, but "ambient" ones with declare (in ts files only?) may have different rules?

Copy link

Member

bradzacherNov 10, 2024•
edited
Loading

Choose a reason for hiding this comment

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

It looks like these rules apply to both ts files and dts files as well
playground? Or did I misunderstand you?

It looks likeexport = is invalid in anamespace but I think that's the only case that invalidates this logic ondeclared namespaces.

So I think this entire function can just be updated to also short-circuit onif (namespace.declare) { return namespace_has_export_equals }

Copy link

CollaboratorAuthor

jakebaileyNov 10, 2024

Choose a reason for hiding this comment

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

There are more issues than just this one IIRC; I've been so busy with other stuff that I haven't been able to sit down and actually fix this PR, sorry.

I'm also not 100% sure what works and doesn't work in the playground when it comes to d.ts files; not really something we often play with; I've been meaning to figure out something better for testing this

If it's bugging you all to stay open, I can close it, though if someone else attempts it too I'd love to review it and double check everything (but I don't think anyone's working on it besides me)

Copy link

Member

bradzacherNov 10, 2024

Choose a reason for hiding this comment

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

Nah Josh just pinged me cos I didn't respond :P
No rush at all.

jakebailey mentioned this pull request

Mar 19, 2024

local types are considered exported in.d.ts files unlike local types in.ts filesmicrosoft/TypeScript#57764

Closed

Copy link

Member

JoshuaKGoldberg commentedJun 2, 2024

👋 just checking in@jakebailey, are you waiting on us for anything?

JoshuaKGoldberg added awaiting response

Issues waiting for a reply from the OP or another party

stalePRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period labels

Jun 2, 2024

Copy link

CollaboratorAuthor

jakebailey commentedJun 2, 2024

I honestly don't remember, I think I was blocked on "man the rules are complicated to implement" and finding time to figure them out to try again here.

Copy link

Member

JoshuaKGoldberg commentedJun 2, 2024

👍 should we close this out then? (do you know if you'll have time + priority soon?)

Copy link

CollaboratorAuthor

jakebailey commentedJun 2, 2024

5.5 just closed so I'll probably have more time now, but I guess I don't know why closing it is helpful... Only been 2 months and I only stopped because I had some very high priority work elsewhere.