Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint-plugin): [prefer-optional-chain] handle cases where the first operands are unrelated to the rest of the chain and add type info #6397

Merged
merged 32 commits into from
Jul 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
a657697
feat(eslint-plugin): [prefer-optional-chain] support more cases
bradzacher Jan 31, 2023
0eaad01
wip
bradzacher Jan 31, 2023
bbf5788
WIP
bradzacher Feb 4, 2023
a519c00
WIP
bradzacher Feb 5, 2023
a354d0a
temp
bradzacher Feb 7, 2023
e7efa91
Merge branch 'main' into 6332-refactor-prefer-optional-chain
bradzacher Feb 7, 2023
b6fcab3
tests are passing without fixers
bradzacher Feb 7, 2023
e51128a
add more test
bradzacher Feb 7, 2023
34671ec
fix lint errors across codebase revealed by changes
bradzacher Feb 7, 2023
e1c5eee
more test cases
bradzacher Feb 7, 2023
3c387a9
more test cases
bradzacher Feb 7, 2023
cb28981
Merge branch 'v6' into 6332-refactor-prefer-optional-chain
bradzacher Apr 17, 2023
74d0d0b
lint after rebase
bradzacher Apr 17, 2023
74ff6d0
make it type-aware
bradzacher Apr 17, 2023
cff21e5
Merge branch 'v6' into 6332-refactor-prefer-optional-chain
bradzacher Apr 27, 2023
2f66664
update configs
bradzacher Apr 27, 2023
1b17c93
update docs
bradzacher Apr 27, 2023
f45a30f
schema snap
bradzacher Apr 27, 2023
0a3a548
Merge branch 'v6' into 6332-refactor-prefer-optional-chain
bradzacher Apr 28, 2023
d90a7c5
WIP add fixer back
bradzacher May 6, 2023
d0a67ae
algorithm improvements and some test fixes
bradzacher May 9, 2023
3071db7
modularise the rule
bradzacher May 23, 2023
709939d
more work
bradzacher May 25, 2023
1a04ff7
Merge branch 'v6' into 6332-refactor-prefer-optional-chain
bradzacher Jun 24, 2023
a66a4a5
finished tests
bradzacher Jun 24, 2023
52ca2e1
few more tests to ensure coverage
bradzacher Jun 24, 2023
c58118c
remove name cos spelling
bradzacher Jun 24, 2023
6e558a4
add docs for the new flag
bradzacher Jun 24, 2023
7d08e25
update schema snapshot
bradzacher Jun 24, 2023
d37c271
Merge branch 'v6' into 6332-refactor-prefer-optional-chain
bradzacher Jul 7, 2023
3e22d5f
review comments
bradzacher Jul 7, 2023
4c36891
configs
bradzacher Jul 7, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import type { SyntaxKind } from 'typescript';

// the members of ts.BinaryOperator
export interface BinaryOperatorToText {
[SyntaxKind.InstanceOfKeyword]: 'instanceof';
[SyntaxKind.InKeyword]: 'in';

// math
[SyntaxKind.AsteriskAsteriskToken]: '**';
[SyntaxKind.AsteriskToken]: '*';
[SyntaxKind.SlashToken]: '/';
[SyntaxKind.PercentToken]: '%';
[SyntaxKind.PlusToken]: '+';
[SyntaxKind.MinusToken]: '-';

// bitwise
[SyntaxKind.AmpersandToken]: '&';
[SyntaxKind.BarToken]: '|';
[SyntaxKind.CaretToken]: '^';
[SyntaxKind.LessThanLessThanToken]: '<<';
[SyntaxKind.GreaterThanGreaterThanToken]: '>>';
[SyntaxKind.GreaterThanGreaterThanGreaterThanToken]: '>>>';

// logical
[SyntaxKind.AmpersandAmpersandToken]: '&&';
[SyntaxKind.BarBarToken]: '||';
[SyntaxKind.LessThanToken]: '<';
[SyntaxKind.LessThanEqualsToken]: '<=';
[SyntaxKind.GreaterThanToken]: '>';
[SyntaxKind.GreaterThanEqualsToken]: '>=';
[SyntaxKind.EqualsEqualsToken]: '==';
[SyntaxKind.EqualsEqualsEqualsToken]: '===';
[SyntaxKind.ExclamationEqualsEqualsToken]: '!==';
[SyntaxKind.ExclamationEqualsToken]: '!=';
}
6 changes: 5 additions & 1 deletion packages/ast-spec/src/expression/BinaryExpression/spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@ import type { AST_NODE_TYPES } from '../../ast-node-types';
import type { BaseNode } from '../../base/BaseNode';
import type { PrivateIdentifier } from '../../special/PrivateIdentifier/spec';
import type { Expression } from '../../unions/Expression';
import type { ValueOf } from '../../utils';
import type { BinaryOperatorToText } from './BinaryOperatorToText';

export * from './BinaryOperatorToText';

export interface BinaryExpression extends BaseNode {
type: AST_NODE_TYPES.BinaryExpression;
operator: string;
operator: ValueOf<BinaryOperatorToText>;
left: Expression | PrivateIdentifier;
right: Expression;
}
20 changes: 20 additions & 0 deletions packages/ast-spec/tests/BinaryOperatorToText.type-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import type {
AssignmentOperator,
BinaryOperator,
SyntaxKind,
} from 'typescript';

import type { BinaryOperatorToText } from '../src';

type BinaryOperatorWithoutInvalidTypes = Exclude<
BinaryOperator,
| AssignmentOperator // --> AssignmentExpression
| SyntaxKind.CommaToken // -> SequenceExpression
| SyntaxKind.QuestionQuestionToken // -> LogicalExpression
>;
type _Test = {
readonly [T in BinaryOperatorWithoutInvalidTypes]: BinaryOperatorToText[T];
// If there are any BinaryOperator members that don't have a corresponding
// BinaryOperatorToText, then this line will error with "Type 'T' cannot
// be used to index type 'BinaryOperatorToText'."
};
199 changes: 195 additions & 4 deletions packages/eslint-plugin/docs/rules/prefer-optional-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,204 @@ foo?.a?.b?.c?.d?.e;

<!--/tabs-->

:::note
There are a few edge cases where this rule will false positive. Use your best judgement when evaluating reported errors.
:::
## Options

In the context of the descriptions below a "loose boolean" operand is any operand that implicitly coerces the value to a boolean.
Specifically the argument of the not operator (`!loose`) or a bare value in a logical expression (`loose && looser`).

### `allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing`

When this option is `true`, the rule will not provide an auto-fixer for cases where the return type of the expression would change. For example for the expression `!foo || foo.bar` the return type of the expression is `true | T`, however for the equivalent optional chain `foo?.bar` the return type of the expression is `undefined | T`. Thus changing the code from a logical expression to an optional chain expression has altered the type of the expression.

In some cases this distinction _may_ matter - which is why these fixers are considered unsafe - they may break the build! For example in the following code:

```ts
declare const foo: { bar: boolean } | null | undefined;
declare function acceptsBoolean(arg: boolean): void;

// ✅ typechecks succesfully as the expression only returns `boolean`
acceptsBoolean(foo != null && foo.bar);

// ❌ typechecks UNSUCCESSFULLY as the expression returns `boolean | undefined`
acceptsBoolean(foo != null && foo.bar);
```

This style of code isn't super common - which means having this option set to `true` _should_ be safe in most codebases. However we default it to `false` due to its unsafe nature. We have provided this option for convenience because it increases the autofix cases covered by the rule. If you set option to `true` the onus is entirely on you and your team to ensure that each fix is correct and safe and that it does not break the build.

When this option is `false` unsafe cases will have suggestion fixers provided instead of auto-fixers - meaning you can manually apply the fix using your IDE tooling.

### `checkAny`

When this option is `true` the rule will check operands that are typed as `any` when inspecting "loose boolean" operands.

<!--tabs-->

#### ❌ Incorrect for `checkAny: true`

```ts
declare const thing: any;

thing && thing.toString();
```

#### ✅ Correct for `checkAny: false`

```ts
declare const thing: any;

thing && thing.toString();
```

<!--/tabs-->

### `checkUnknown`

When this option is `true` the rule will check operands that are typed as `unknown` when inspecting "loose boolean" operands.

<!--tabs-->

#### ❌ Incorrect for `checkUnknown: true`

```ts
declare const thing: unknown;

thing && thing.toString();
```

#### ✅ Correct for `checkUnknown: false`

```ts
declare const thing: unknown;

thing && thing.toString();
```

<!--/tabs-->

### `checkString`

When this option is `true` the rule will check operands that are typed as `string` when inspecting "loose boolean" operands.

<!--tabs-->

#### ❌ Incorrect for `checkString: true`

```ts
declare const thing: string;

thing && thing.toString();
```

#### ✅ Correct for `checkString: false`

```ts
declare const thing: string;

thing && thing.toString();
```

<!--/tabs-->

### `checkNumber`

When this option is `true` the rule will check operands that are typed as `number` when inspecting "loose boolean" operands.

<!--tabs-->

#### ❌ Incorrect for `checkNumber: true`

```ts
declare const thing: number;

thing && thing.toString();
```

#### ✅ Correct for `checkNumber: false`

```ts
declare const thing: number;

thing && thing.toString();
```

<!--/tabs-->

### `checkBoolean`

When this option is `true` the rule will check operands that are typed as `boolean` when inspecting "loose boolean" operands.

<!--tabs-->

#### ❌ Incorrect for `checkBoolean: true`

```ts
declare const thing: boolean;

thing && thing.toString();
```

#### ✅ Correct for `checkBoolean: false`

```ts
declare const thing: boolean;

thing && thing.toString();
```

<!--/tabs-->

### `checkBigInt`

When this option is `true` the rule will check operands that are typed as `bigint` when inspecting "loose boolean" operands.

<!--tabs-->

#### ❌ Incorrect for `checkBigInt: true`

```ts
declare const thing: bigint;

thing && thing.toString();
```

#### ✅ Correct for `checkBigInt: false`

```ts
declare const thing: bigint;

thing && thing.toString();
```

<!--/tabs-->

### `requireNullish`

When this option is `true` the rule will skip operands that are not typed with `null` and/or `undefined` when inspecting "loose boolean" operands.

<!--tabs-->

#### ❌ Incorrect for `requireNullish: true`

```ts
declare const thing1: string | null;
thing1 && thing1.toString();
```

#### ✅ Correct for `requireNullish: true`

```ts
declare const thing1: string | null;
thing1?.toString();

declare const thing2: string;
thing2 && thing2.toString();
```

<!--/tabs-->

## When Not To Use It

If you don't mind using more explicit `&&`s, you don't need this rule.
If you don't mind using more explicit `&&`s/`||`s, you don't need this rule.

## Further Reading

Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,15 @@
"generate:configs": "yarn tsx tools/generate-configs.ts",
"lint": "nx lint",
"test": "jest --coverage",
"test-single": "jest --no-coverage",
"typecheck": "tsc -p tsconfig.json --noEmit"
},
"dependencies": {
"@eslint-community/regexpp": "^4.5.0",
"@typescript-eslint/scope-manager": "5.61.0",
"@typescript-eslint/type-utils": "5.61.0",
"@typescript-eslint/utils": "5.61.0",
"@typescript-eslint/visitor-keys": "5.61.0",
"debug": "^4.3.4",
"grapheme-splitter": "^1.0.4",
"graphemer": "^1.4.0",
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/disable-type-checked.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export = {
'@typescript-eslint/non-nullable-type-assertion-style': 'off',
'@typescript-eslint/prefer-includes': 'off',
'@typescript-eslint/prefer-nullish-coalescing': 'off',
'@typescript-eslint/prefer-optional-chain': 'off',
'@typescript-eslint/prefer-readonly': 'off',
'@typescript-eslint/prefer-readonly-parameter-types': 'off',
'@typescript-eslint/prefer-reduce-type-parameter': 'off',
Expand Down
1 change: 0 additions & 1 deletion packages/eslint-plugin/src/configs/stylistic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,5 @@ export = {
'@typescript-eslint/prefer-for-of': 'error',
'@typescript-eslint/prefer-function-type': 'error',
'@typescript-eslint/prefer-namespace-keyword': 'error',
'@typescript-eslint/prefer-optional-chain': 'error',
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export default util.createRule<Options, MessageIds>({
if (
node.kind !== 'get' ||
!node.value.body ||
!node.value.body.body.length
node.value.body.body.length === 0
) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ export default util.createRule<Options, MessageIds>({
* Checks if a function name is allowed and should not be checked.
*/
function isAllowedName(node: TSESTree.Node | undefined): boolean {
if (!node || !options.allowedNames || !options.allowedNames.length) {
if (!node || !options.allowedNames || options.allowedNames.length === 0) {
return false;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/eslint-plugin/src/rules/indent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ export default util.createRule<Options, MessageIds>({
// transform it to a BinaryExpression
return rules['BinaryExpression, LogicalExpression']({
type: AST_NODE_TYPES.BinaryExpression,
operator: 'as',
operator: 'as' as any,
left: node.expression,
// the first typeAnnotation includes the as token
right: node.typeAnnotation as any,
Expand All @@ -217,7 +217,7 @@ export default util.createRule<Options, MessageIds>({
test: {
parent: node,
type: AST_NODE_TYPES.BinaryExpression,
operator: 'extends',
operator: 'extends' as any,
left: node.checkType as any,
right: node.extendsType as any,

Expand Down
5 changes: 2 additions & 3 deletions packages/eslint-plugin/src/rules/no-extraneous-class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,8 @@ export default util.createRule<Options, MessageIds>({
): boolean => {
return !!(
allowWithDecorator &&
node &&
node.decorators &&
node.decorators.length
node?.decorators &&
node.decorators.length !== 0
);
};

Expand Down
3 changes: 1 addition & 2 deletions packages/eslint-plugin/src/rules/prefer-includes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,7 @@ export default createRule({
.getProperty('includes')
?.getDeclarations();
if (
includesMethodDecl == null ||
!includesMethodDecl.some(includesMethodDecl =>
!includesMethodDecl?.some(includesMethodDecl =>
hasSameParameters(includesMethodDecl, instanceofMethodDecl),
)
) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export type PreferOptionalChainMessageIds =
| 'preferOptionalChain'
| 'optionalChainSuggest';

export interface PreferOptionalChainOptions {
checkAny?: boolean;
checkUnknown?: boolean;
checkString?: boolean;
checkNumber?: boolean;
checkBoolean?: boolean;
checkBigInt?: boolean;
requireNullish?: boolean;
allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean;
}