enable @typescript-eslint/no-unused-vars

[Abraham Guo (abrahamguo)]

Resolves the TODO comment regarding enabling the ESLint rule @typescript-eslint/no-unused-vars

Fixes#57124

[TypeScript Bot (typescript-bot)]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[Abraham Guo (abrahamguo)]

@microsoft-github-policy-service agree

[Jake Bailey (jakebailey)]

You should see #55139 for context. There are some unfortunate caveats, I'm afraid.

[Jake Bailey (jakebailey)]

Well, we could of course ignore lines for JSDoc, like you've done here. Maybe that's enough and we can live with it to avoid compiler errors.

[Abraham Guo (abrahamguo)]

Yes, I think that (at least for now) it seems like an easy task to suppress the <5 places with JSDoc issues in order to get the benefit of this rule applying throughout the whole codebase.

[Jake Bailey (jakebailey)]

Generally speaking, I prefer the settings and changes in #55139, besides the JSDoc plugin thing.

That being said, the regex should probably be changed to ^_[^_] as your PR has pointed out that __String is treated as used which isn't intentional.

[Jake Bailey (jakebailey)]

The scripts are currently inconsistent as to their use of typedefs or import type nodes; can you switch them all to be import type nodes? At least for the ones that aren't compound like CallOrNewExpression?

[Abraham Guo (abrahamguo)]

Sure — my thinking was to create typedefs for ones that are used more than once, since the imports are long, but I can inline the typedefs if you like that 👍🏻

[Jake Bailey (jakebailey)]

That makes sense, though I think having the code be more easily copy-paste-able between the rules is probably worthwhile.

Eventually we'll have #57207, though. That'll be awesome.

[Jake Bailey (jakebailey)]

Looks good to me, thanks for bearing with all of my comments.

[Abraham Guo (abrahamguo)]

No worries! Thanks for the detailed review.

Would you be open to future PRs turning on other ESLint rules for the codebase?
In particular, I've been looking at prefer-destructuring or logical-assignment-operators which are both autofixable, as well as some of the rules from @typescript-eslint/recommended-requiring-type-checking.
I wasn't sure if there was any bigger vision one way or another.

[Jake Bailey (jakebailey)]

Enabling the rules that use type checking is a bit of a perf hole; I wouldn't attempt to enable any of those unless there are some that look super valuable.

Last I checked logical-assignment-operators it had a load of false positives; I tried to enable it and fix everything but I had to sift through hundreds of changes to make sure none caused a behavior change.

There may be others that are valuable but I think it really depends.

[Abraham Guo (abrahamguo)]

logical-assignment-operators looks to just change a = a || b to a ||= b (and same with &&??), so it looks to be guaranteed correctly autofixable.

I'll look into the type-checked lint rules and maybe do some performance monitoring.

[Jake Bailey (jakebailey)]

Ah, I was thinking of prefer-nullish-coalescing.

But generally, know that it isn't a goal to enable every lint rule; the codebase is huge and it slows down linting and editor performance. Most on the team don't even run eslint in the editor for that reason. We really only want cheap rules / rules that have high value.