enable @typescript-eslint/no-unused-vars

[abrahamguo]

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

Fixes #57124

[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.

[abrahamguo]

@microsoft-github-policy-service agree

[jakebailey]

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

[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.

[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.

[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.

[jakebailey]

I also think we could remove allowUnusedLabels too, we're already getting no-unused-labels from eslint.

[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?

[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 👍🏻

[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.

[jakebailey]

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

[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.

[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.

[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.

[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.