Add rules that require type information #250

Mrtenz · 2022-11-11T18:32:14Z

Closes #2.

This adds rules that require type information to the TypeScript ESLint config. I've set it to extend the default configuration, and added some other rules that are not included in the default.

Mrtenz

Below is an explanation of the rules I've added, and the reasoning behind some of them.

Mrtenz · 2022-11-11T18:33:02Z

packages/typescript/src/index.js

+    '@typescript-eslint/no-unsafe-argument': 'off',
+    '@typescript-eslint/no-unsafe-assignment': 'off',
+    '@typescript-eslint/no-unsafe-call': 'off',
+    '@typescript-eslint/no-unsafe-member-access': 'off',
+    '@typescript-eslint/no-unsafe-return': 'off',


All of these have to do with the use of any. While I strongly prefer not using it, we do use it in some areas, so these rules are turned off.

I would really like to enable these rules, but saving them for a future release might be a good idea anyway, to reduce the disruption these other rules will cause. I've created an issue to track this: #251

Mrtenz · 2022-11-11T18:33:44Z

packages/typescript/src/index.js

+    '@typescript-eslint/no-unsafe-call': 'off',
+    '@typescript-eslint/no-unsafe-member-access': 'off',
+    '@typescript-eslint/no-unsafe-return': 'off',
+    '@typescript-eslint/restrict-template-expressions': 'off',


This rule prevents the use of non-string types in template expressions. We make use of this in certain places, like key-tree using bip32:${number} for derivation paths.

Hmm, interesting. I like the idea of requiring explicit type casting (e.g. bip32:{String(number)}). It's not too inconvenient, and can catch cases where you don't realize it's not a string.

The thing is that we only want to accept numbers in those cases.

type PathSegment = `bip32:${number}`; const foo: PathSegment = 'bip32:foo'; // Invalid const bar: PathSegment = 'bip32:123'; // Valid

Ohhh I misunderstood, that was a string template type not an actual string template with a variable called number.

Yeah that's a great point. Agreed with turning this off.

Is it actually used for complex types, or only primitives? If the latter, maybe consider allow-number etc so we can eat the cake and have it without [object Object]s?

Primitives and other types like this:

type Hex = `0x${string}`; type Foo = `foo-${Hex}`;

I'm not sure how this rule handles the second case, I'll give it a try.

Ah, I misunderstood the rule slightly. I think it's fine if we use allowNumber, allowBoolean, maybe allowAny?

Hmm. Would we benefit of allowAny? The other two make sense.

Though either way, having this rule enabled with allowAny would still be an improvement compared to today, so I don't feel strongly about us not using allowAny.

I vote for allowNumber and allowBoolean. Those make the most sense to me.

Removed allowAny.

Mrtenz · 2022-11-11T18:36:00Z

packages/typescript/src/index.js

+    '@typescript-eslint/restrict-template-expressions': 'off',
+
+    // Our rules that require type information
+    '@typescript-eslint/consistent-type-exports': 'error',


Up for debate if we want to enable this one. It enforces import type { ... } when importing types. If there are multiple imports from the same location, the imports would be split up in two, e.g.:

// foo.ts export type Foo = string; export const bar = 'baz'; // bar.ts import type { Foo } from './foo'; import { bar } from './foo';

I think personally it makes it more clear if something is just a type or not.

I agree, this seems great to me.

Mrtenz · 2022-11-11T18:40:19Z

packages/typescript/src/index.js

+    '@typescript-eslint/naming-convention': [
+      'error',
+      {
+        selector: 'default',
+        format: ['camelCase'],
+        leadingUnderscore: 'forbid',
+        trailingUnderscore: 'forbid',
+      },
+      {
+        selector: 'enumMember',
+        format: ['PascalCase'],
+      },
+      {
+        selector: 'variable',
+        format: ['camelCase', 'UPPER_CASE', 'PascalCase'],
+      },
+      {
+        selector: 'typeLike',
+        format: ['PascalCase'],
+      },
+      {
+        selector: 'interface',
+        format: ['PascalCase'],
+        custom: {
+          regex: '^I[A-Z]',
+          match: false,
+        },
+      },
+    ],


Also up for debate. This enforces consistent naming for different kinds of identifiers. Currently it's configured ilke:

By default we use camelCase. This includes things like function names, args, etc.

Enum members use PascalCase, e.g.:
enum Foo { Bar, Baz }

Variables can use camelCase, UPPER_CASE, or PascalCase. Reasoning for this is that in certain cases we want to use UPPER_CASE, like for constants, and in other cases we want to use PascalCase, like for structs.

"Type like"-identifiers use PascalCase.

Interfaces use PascalCase, and cannot start with I.

@brad-decker You might have an opinion on this too.

I've added object literal properties and methods, which can be camelCase, UPPER_CASE, and PascalCase as well. This is useful in situations where we don't control the naming (i.e., external libraries).

Also changed it so we allow leading underscores in front of variables. We use this occasionally to signal that a variable is unused.

I feel like using PascalCase for variables is not very common. You mentioned that utils uses it for structs, but is that the only case? If so I wonder whether that should be an override.

We do it in snaps-monorepo as well, also for structs. I don't know if there's an easy way to override these rules without copying the entire rule?

I think if the convention is Enums will use PascalCase, then I am in favor of using that. The rules as stated make sense to me.

Okay, no problem.

Mrtenz · 2022-11-11T18:40:51Z

packages/typescript/src/index.js

+        },
+      },
+    ],
+    '@typescript-eslint/no-confusing-void-expression': 'error',


This can catch some bugs, like trying to log a function that returns void.

This is causing a lot of errors in snaps-monorepo, especially in functions that do something like return fn() where fn returns void. I think we should not enable this rule.

Ah, that makes sense.

I can see how this rule might improve readability, but it would be more verbose, and might make it easier to forget to return.

Mrtenz · 2022-11-11T18:49:39Z

packages/typescript/src/index.js

+    '@typescript-eslint/prefer-includes': 'error',
+    '@typescript-eslint/prefer-nullish-coalescing': 'error',
+    '@typescript-eslint/prefer-readonly': 'error',
+    '@typescript-eslint/prefer-reduce-type-parameter': 'error',


This makes sure we use the generic parameter of Array.prototype.reduce, rather than casting the initial value to something.

const foo = []; foo.reduce<Bar>(...); // Valid foo.reduce(reducer, defaultValue as Bar); // Invalid

Mrtenz · 2022-11-11T18:50:26Z

packages/typescript/src/index.js

+    '@typescript-eslint/prefer-nullish-coalescing': 'error',
+    '@typescript-eslint/prefer-readonly': 'error',
+    '@typescript-eslint/prefer-reduce-type-parameter': 'error',
+    '@typescript-eslint/prefer-string-starts-ends-with': 'error',


This prefers using String.prototype.startsWith and String.prototype.endsWith over other alternatives, like checking a substring from index 0 to n.

Mrtenz · 2022-11-11T18:51:19Z

packages/typescript/src/index.js

+    '@typescript-eslint/prefer-readonly': 'error',
+    '@typescript-eslint/prefer-reduce-type-parameter': 'error',
+    '@typescript-eslint/prefer-string-starts-ends-with': 'error',
+    '@typescript-eslint/promise-function-async': 'error',


This makes sure that functions that return a Promise result are always marked as async functions. That way we cannot throw both regular errors and Promise rejections from the same function.

This might be my favorite rule

Mrtenz · 2022-11-11T18:51:41Z

packages/typescript/src/index.js

+    '@typescript-eslint/prefer-reduce-type-parameter': 'error',
+    '@typescript-eslint/prefer-string-starts-ends-with': 'error',
+    '@typescript-eslint/promise-function-async': 'error',
+    '@typescript-eslint/switch-exhaustiveness-check': 'error',


This makes sure that all the possible values of a union are covered in a switch statement that does not have a default case.

Mrtenz · 2022-11-11T18:52:30Z

packages/typescript/src/index.js

+    // This enables support for linting rules that require type information. We
+    // assume that the project has a `tsconfig.json` file in the directory where
+    // ESLint is being run.
+    tsconfigRootDir: process.cwd(),


I assume that we always run eslint from the root project here, but that may not always be the case, especially in monorepos. We may have to configure this on a per-project basis.

Tested this in snaps-monorepo, and it does not work out of the box with this option. All monorepo packages need to have their own .eslintrc, and specify the tsconfigRootDir. Otherwise ESLint will complain that the files are not included in tsconfig.json (since they aren't in the root config).

Mrtenz · 2022-11-11T20:18:53Z

I did some quick testing in snaps-monorepo, and this is causing a lot of errors that are not automatically fixable. Will have to look into this further.

Gudahtt

LGTM! Very excited to see this land.

Can we leave this open a few days to allow others to comment? Since there are a few potentially controversial rules here. i.e. maybe we can merge it on Tuesday.

Missed comment about testing; I will help test these rules out before approving again.

Mrtenz · 2022-11-12T08:23:09Z

packages/typescript/src/index.js

+    '@typescript-eslint/no-unsafe-member-access': 'off',
+    '@typescript-eslint/no-unsafe-return': 'off',
+    '@typescript-eslint/restrict-template-expressions': 'off',
+    '@typescript-eslint/unbound-method': 'off',


This rule is causing a lot of confusing errors like this:

14:36 error Avoid referencing unbound methods which may cause unintentional scoping of `this`. If your function does not access `this`, you can annotate it with `this: void`, or consider using an arrow function instead @typescript-eslint/unbound-method

I don't think this is actually an issue, so I have disabled this rule.

You may be aware but it's an easy enough mistake to do:

class Foo { public logThis() { console.log(this); } } const foo = new Foo(); foo.logThis(); // logs foo window.setTimeout(foo.logThis); // logs global window.setTimeout(foo.logThis.bind()); // do this window.setTimeout(() => foo.logThis()); // or this

Is it worth silencing individually where it's actually desired?

The problem is that it causes errors in situations like this:

expect(Math.abs).toStrictEqual(rootRealmGlobal.Math.abs);

Which we happen to do quite a bit in snaps-monorepo. We could just disable it for that repo, but I would guess that there are others with a similar problem.

Hmm. Are examples like that widespread? We can disable the rule for specific files/lines. It does seem like what the rule is suggesting is what we are already doing in most places.

It seemed to happen quite a bit in snaps-monorepo. Not sure about other repos.

I guess that this can be revisited if/when at least Typescript stdlib is annotating consistently to make those functions work. Or it can be enabled by default and disabled on a per-need (lines or repo) basis.

https://www.typescriptlang.org/docs/handbook/2/functions.html#declaring-this-in-a-function

https://github.com/microsoft/TypeScript/blob/c0f8d1cf75f85132ed46950de884d97ec5312ae8/lib/lib.es5.d.ts#L665

I've re-enabled it. If it becomes an issue for certain repos, we can disable it for those repos.

mcmire · 2022-11-14T16:07:55Z

Looks good to me, minus the bit about getting this to work with snaps-monorepo.

packages/typescript/rules-snapshot.json

Gudahtt

LGTM!

Mrtenz commented Nov 11, 2022

View reviewed changes

Mrtenz marked this pull request as ready for review November 11, 2022 18:55

Mrtenz requested a review from a team as a code owner November 11, 2022 18:55

Gudahtt previously approved these changes Nov 12, 2022

View reviewed changes

Mrtenz commented Nov 12, 2022

View reviewed changes

Mrtenz force-pushed the mrtenz/type-rules branch from a12f8a7 to e3829a7 Compare November 14, 2022 14:31

brad-decker previously approved these changes Nov 14, 2022

View reviewed changes

Mrtenz dismissed brad-decker’s stale review via cecf1b8 November 14, 2022 20:28

legobeat reviewed Nov 15, 2022

View reviewed changes

packages/typescript/rules-snapshot.json Outdated Show resolved Hide resolved

Mrtenz added 14 commits November 15, 2022 12:56

Add rules that require type information

e2308b1

Run lint:fix

8dd9361

Disable no-confusing-void-expression

2e5514a

Fix rule typo

474fc93

Allow camelCase, PascalCase and UPPER_CASE for object literal properties

6ea2d6e

Allow camelCase, PascalCase and UPPER_CASE for object literal methods

89d4a65

Disable unbound-method

215381b

Allow leading underscore in front of variables

783e84f

Enable no-throw-literal

013eaae

Enable restrict-template-expression

7305131

Run lint:fix

c1621fd

Fix tests

ed5d5a8

Remove allowAny from restrict-template-expressions

441ed90

Re-enable unbound-method

2d0011d

Mrtenz force-pushed the mrtenz/type-rules branch from 3911d8e to 2d0011d Compare November 15, 2022 11:56

Fix rebase issue

4321730

Gudahtt approved these changes Nov 15, 2022

View reviewed changes

Mrtenz merged commit 32abef3 into main Nov 15, 2022

Mrtenz deleted the mrtenz/type-rules branch November 15, 2022 15:47

Add rules that require type information #250

Add rules that require type information #250

Conversation

Mrtenz commented Nov 11, 2022 • edited

Mrtenz left a 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

Gudahtt Nov 11, 2022 • 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

Gudahtt Nov 14, 2022 • 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

Mrtenz Nov 12, 2022 • 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

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

Mrtenz commented Nov 11, 2022

Gudahtt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legobeat Nov 14, 2022 • 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

legobeat Nov 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire commented Nov 14, 2022

Gudahtt left a comment

Choose a reason for hiding this comment

Mrtenz commented Nov 11, 2022 •

edited

Gudahtt Nov 11, 2022 •

edited

Gudahtt Nov 14, 2022 •

edited

Mrtenz Nov 12, 2022 •

edited

legobeat Nov 14, 2022 •

edited

legobeat Nov 15, 2022 •

edited