Updated typescript-eslint to v6 #54693

JoshuaKGoldberg · 2023-06-18T14:43:43Z

Applies the following strategy for updating the linter:

Bumping the version to the latest beta (technically rc, but there's no meaningful difference here)
Extended from our two recommended starting non-type-checked configs: recommended and stylistic
Removed any existing rule configurations that are redundant when extending those configs
Disabled any rules with existing violations that seemed intentional, under a "not applicable here" comment
Disabled any rules with existing violations that I wasn't sure the intentionality behind, under a "Todo: do we want these?" comment
Manually fixed up remaining lint complaints

Explanations for the changes are inline. I realize the mild hubris on my end of posting lint recommendations on the TypeScript codebase itself - seeking input as to whether we're going the wrong way on any of them. 😄

For the Todo rules, I'm interested in your opinions on whether you'd want each of them or not in the TypeScript codebase.

Fixes #54692

src/compiler/binder.ts

JoshuaKGoldberg · 2023-06-18T14:47:13Z

src/compiler/checker.ts

@@ -5732,7 +5732,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
        return rightMeaning === SymbolFlags.Value ? SymbolFlags.Value : SymbolFlags.Namespace;
    }

-    function getAccessibleSymbolChain(symbol: Symbol | undefined, enclosingDeclaration: Node | undefined, meaning: SymbolFlags, useOnlyExternalAliasing: boolean, visitedSymbolTablesMap: Map<SymbolId, SymbolTable[]> = new Map()): Symbol[] | undefined {
+    function getAccessibleSymbolChain(symbol: Symbol | undefined, enclosingDeclaration: Node | undefined, meaning: SymbolFlags, useOnlyExternalAliasing: boolean, visitedSymbolTablesMap = new Map<SymbolId, SymbolTable[]>()): Symbol[] | undefined {


@typescript-eslint/consistent-generic-constructors: "The generic type arguments should be specified as part of the constructor type arguments."

This comes from the stylistic config - it's a little more succinct to put the <...> type parameters after the new Map. The rule is auto-fixable.

scripts/eslint/rules/argument-trivia.cjs

sandersn · 2023-06-26T22:13:15Z

mild hubris on my end of posting lint recommendations on the TypeScript codebase itself

On the contrary; we really really don't want our codebase to be the basis for good Typescript style. eslint is absolutely the right provider of those defaults.

sandersn

Comments on the TODO-commented rules.

👍🏼 means that I like the value given in the PR; I commented on the others.

src/compiler/checker.ts

.eslintrc.json

sandersn · 2023-06-26T22:41:27Z

.eslintrc.json

+        "@typescript-eslint/ban-types": "off",
+        "@typescript-eslint/no-empty-function": "off",
+        "@typescript-eslint/no-empty-interface": "off",
+        "@typescript-eslint/no-unused-vars": "off",


Is this off because the tsconfig has the equivalent Typescript setting on? If not, what kind and how many unused variables do we have?

Yes, it's this; honestly I would much prefer if we didn't use TS for this at all and instead made it a lint. So annoying to have to ensure my variables are all used just to run tests or something. At least debug skips typecheck.

Only 18 failures at the moment. Not bad!

However, typescript-eslint/typescript-eslint#5017 -> gajus/eslint-plugin-jsdoc#858 is impacting the codebase here. Probably best to set up the workarounds in a separate PR IMO.

sandersn · 2023-06-26T22:43:22Z

.eslintrc.json

+        "@typescript-eslint/sort-type-constituents": "off",
+
+        // Pending https://github.com/typescript-eslint/typescript-eslint/issues/4820
+        "@typescript-eslint/prefer-optional-chain": "off",


Is this off because it causes lots of churn? If so, I'd like to see it turned on, but maybe in a separate PR.

Yeah, and also because the rule has more false positives right now than I'm comfortable with - especially in a codebase like TypeScript's that does a lot of funky stuff with scoping.

I'm honestly shocked we aren't getting lints from prefer-nullish-coalescing too... I was restructuring the lint config of hereby and hit some false positives there too on code like:

declare const a: string | undefined declare const b: string const foo = a || b;

Where the actual intent is that the empty string goes away. But maybe that's not a false positive.

sandersn · 2023-06-26T22:43:41Z

.eslintrc.json

+        "@typescript-eslint/no-empty-function": "off",
+        "@typescript-eslint/no-empty-interface": "off",
+        "@typescript-eslint/no-unused-vars": "off",
+        "@typescript-eslint/sort-type-constituents": "off",


sandersn · 2023-06-30T23:51:45Z

@JoshuaKGoldberg I read your replies. It sounds to me like this PR is in a good state, and then needs 3 or 4 follow ups to turn on even more lint rules (and also turn off the tsconfig setting for checking unused variables). Does that sound about right to you?

JoshuaKGoldberg · 2023-06-30T23:56:09Z

@sandersn that's correct!

sandersn · 2023-07-05T20:21:14Z

I forgot to ask: do you want to merge this with v6 beta or wait for the full release?

I'll ask the rest of the team to see if anybody there cares one way or the other as well.
Edit: Yep, we're OK with the beta if you are.

JoshuaKGoldberg · 2023-07-06T00:09:36Z

Beta works for me! We've scheduled the v6 announcement blog post to go out on Monday (🤞, knock on wood, etc.). It's no trouble at all. And, bonus, anything that bumps up my TypeScript PR count makes me happy 😄

sandersn · 2023-07-06T20:29:02Z

Looks like there's some weirdness with failing tests (jsdocThisType), so probably needs a merge from main.

jakebailey · 2023-07-06T20:31:39Z

If we're going to merge this, probably it's worth merging in proximity to #54835 to reduce the speedbump of the dep change?

jakebailey · 2023-07-06T20:37:15Z

Also, this PR is still pinned back to an old alpha, does that need to be bumped? Or are we just waiting for a "more real" release?

JoshuaKGoldberg · 2023-07-06T22:00:56Z

The alphas aren't changing much release-to-release at this point. I bumped to the latest in 0fde8d4 and the only relevant change was a couple rule changes in the used configs (typescript-eslint/typescript-eslint#7157).

Edit: so, actually answering: no, this doesn't need to bump to new alphas.

JoshuaKGoldberg · 2023-07-11T20:49:55Z

Btw, I just updated to typescript-eslint@v6.0.0 stable. 🥳

jakebailey · 2023-07-11T20:59:48Z

Updated the title to reflect that.

package.json

.eslintrc.json

jakebailey

This seems good to me now, though given how many lints we disable, I half wonder if we should just disable the : Map lint then send them all as follow-ups.

Here's a diff for the eslint rules that can be dropped as they are part of the eslint recommended set:

diff --git a/.eslintrc.json b/.eslintrc.json
index 56d271a3f4..69ec74d564 100644
--- a/.eslintrc.json
+++ b/.eslintrc.json
@@ -109,18 +109,18 @@
         "no-null/no-null": "error",
 
         // eslint
-        "constructor-super": "error",
+        // "constructor-super": "error",
         "curly": ["error", "multi-line"],
         "dot-notation": "error",
         "eqeqeq": "error",
         "linebreak-style": ["error", "windows"],
         "new-parens": "error",
         "no-caller": "error",
-        "no-duplicate-case": "error",
-        "no-empty": "error",
+        // "no-duplicate-case": "error",
+        // "no-empty": "error",
         "no-eval": "error",
         "no-extra-bind": "error",
-        "no-fallthrough": "error",
+        // "no-fallthrough": "error",
         "no-new-func": "error",
         "no-new-wrappers": "error",
         "no-return-await": "error",
@@ -132,24 +132,24 @@
             { "name": "setImmediate" },
             { "name": "clearImmediate" }
         ],
-        "no-sparse-arrays": "error",
+        // "no-sparse-arrays": "error",
         "no-template-curly-in-string": "error",
         "no-throw-literal": "error",
         "no-trailing-spaces": "error",
         "no-undef-init": "error",
-        "no-unsafe-finally": "error",
-        "no-unused-labels": "error",
+        // "no-unsafe-finally": "error",
+        // "no-unused-labels": "error",
         "no-var": "error",
         "object-shorthand": "error",
         "prefer-const": "error",
         "prefer-object-spread": "error",
         "quote-props": ["error", "consistent-as-needed"],
         "space-in-parens": "error",
-        "unicode-bom": ["error", "never"],
-        "use-isnan": "error",
-        "no-prototype-builtins": "error",
-        "no-self-assign": "error",
-        "no-dupe-else-if": "error"
+        "unicode-bom": ["error", "never"]
+        // "use-isnan": "error",
+        // "no-prototype-builtins": "error",
+        // "no-self-assign": "error",
+        // "no-dupe-else-if": "error"
     },
     "overrides": [
         // By default, the ESLint CLI only looks at .js files. But, it will also look at

I also want to (not in this PR) reorganize this eslint config to distinguish which rules are effectively formatting rules and which are not; I feel like the current ordering by plugin is not really very helpful now that we are using extends, and I already know which rules are going to go away when we dprint.

jakebailey · 2023-07-14T18:35:32Z

@JoshuaKGoldberg You may have missed it, but the code box above has a scrollbar and there are 8 other ones to also remove.

JoshuaKGoldberg · 2023-07-15T18:33:03Z

Ah, great - thanks for finishing this up!

...I did miss the scrollbar 🙈

Updated typescript-eslint to v6 beta

a6f12dd

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 18, 2023

JoshuaKGoldberg commented Jun 18, 2023

View reviewed changes

src/compiler/binder.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg commented Jun 18, 2023

View reviewed changes

JoshuaKGoldberg mentioned this pull request Jun 18, 2023

Try out v6 beta on various important community repos typescript-eslint/typescript-eslint#6760

Closed

JoshuaKGoldberg added 3 commits June 18, 2023 11:41

Corrected non-null chains

b001345

Removed unnecessary rule meta.docs.recommended properties

4951fc2

Fix build complaint for argument-triva.cjs

f810a22

jakebailey reviewed Jun 20, 2023

View reviewed changes

scripts/eslint/rules/argument-trivia.cjs Show resolved Hide resolved

JoshuaKGoldberg mentioned this pull request Jun 20, 2023

Consider keeping parserServices.hasFullTypeInformation for another major version? typescript-eslint/typescript-eslint#7124

Closed

JoshuaKGoldberg marked this pull request as ready for review June 22, 2023 15:06

typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 22, 2023

Merge branch 'main'

70caa0c

sandersn added this to Not started in PR Backlog Jun 26, 2023

sandersn assigned jakebailey Jun 26, 2023

sandersn moved this from Not started to Waiting on author in PR Backlog Jun 26, 2023

sandersn self-requested a review June 26, 2023 22:09

sandersn reviewed Jun 26, 2023

View reviewed changes

Updated messaging (comment) for todo rules

052f6ed

sandersn approved these changes Jul 6, 2023

View reviewed changes

PR Backlog automation moved this from Waiting on author to Needs merge Jul 6, 2023

JoshuaKGoldberg added 2 commits July 6, 2023 17:58

Merge branch 'main'

ee035f7

Bumped to latest alpha

0fde8d4

JoshuaKGoldberg added 2 commits July 11, 2023 16:18

Merge branch 'main' into typescript-eslint-v6

170c4e8

typescript-eslint@v6 stable 🥳

2b56c5a

jakebailey changed the title ~~Updated typescript-eslint to v6 beta~~ Updated typescript-eslint to v6 Jul 11, 2023

jakebailey reviewed Jul 11, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

jakebailey reviewed Jul 13, 2023

View reviewed changes

.eslintrc.json Show resolved Hide resolved

.eslintrc.json Outdated Show resolved Hide resolved

Review feedback: ^versions, eslint:recommended, config phrasing

880768e

JoshuaKGoldberg requested a review from jakebailey July 14, 2023 15:36

jakebailey approved these changes Jul 14, 2023

View reviewed changes

A couple more disables

53e2ae1

Remove last eslint rules in their recommended set

0b72601

jakebailey merged commit 0ae8ca1 into microsoft:main Jul 14, 2023
18 checks passed

PR Backlog automation moved this from Needs merge to Done Jul 14, 2023

JoshuaKGoldberg deleted the typescript-eslint-v6 branch July 15, 2023 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated typescript-eslint to v6 #54693

Updated typescript-eslint to v6 #54693

JoshuaKGoldberg commented Jun 18, 2023 •

edited

JoshuaKGoldberg Jun 18, 2023

sandersn commented Jun 26, 2023

sandersn left a comment •

edited

sandersn Jun 26, 2023

jakebailey Jun 26, 2023

JoshuaKGoldberg Jun 26, 2023

sandersn Jun 26, 2023

JoshuaKGoldberg Jun 26, 2023

jakebailey Jul 14, 2023

sandersn Jun 26, 2023

sandersn commented Jun 30, 2023

JoshuaKGoldberg commented Jun 30, 2023

sandersn commented Jul 5, 2023 •

edited

JoshuaKGoldberg commented Jul 6, 2023

sandersn commented Jul 6, 2023

jakebailey commented Jul 6, 2023

jakebailey commented Jul 6, 2023

JoshuaKGoldberg commented Jul 6, 2023 •

edited

JoshuaKGoldberg commented Jul 11, 2023

jakebailey commented Jul 11, 2023

jakebailey left a comment

jakebailey commented Jul 14, 2023

JoshuaKGoldberg commented Jul 15, 2023

Updated typescript-eslint to v6 #54693

Updated typescript-eslint to v6 #54693

Conversation

JoshuaKGoldberg commented Jun 18, 2023 • edited

Choose a reason for hiding this comment

sandersn commented Jun 26, 2023

sandersn left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Jun 30, 2023

JoshuaKGoldberg commented Jun 30, 2023

sandersn commented Jul 5, 2023 • edited

JoshuaKGoldberg commented Jul 6, 2023

sandersn commented Jul 6, 2023

jakebailey commented Jul 6, 2023

jakebailey commented Jul 6, 2023

JoshuaKGoldberg commented Jul 6, 2023 • edited

JoshuaKGoldberg commented Jul 11, 2023

jakebailey commented Jul 11, 2023

jakebailey left a comment

Choose a reason for hiding this comment

jakebailey commented Jul 14, 2023

JoshuaKGoldberg commented Jul 15, 2023

JoshuaKGoldberg commented Jun 18, 2023 •

edited

sandersn left a comment •

edited

sandersn commented Jul 5, 2023 •

edited

JoshuaKGoldberg commented Jul 6, 2023 •

edited