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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): [no-unnecessary-condition] improve optional chain handling 2 - electric boogaloo #2138

Merged
merged 9 commits into from Jun 1, 2020
Merged

fix(eslint-plugin): [no-unnecessary-condition] improve optional chain handling 2 - electric boogaloo #2138

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f7b727b
fix(eslint-plugin): [no-unnecessary-condition] improve optional chain…
yeonjuan May 31, 2020
8ab4cb5
fix formatting
yeonjuan May 31, 2020
ef4ff94
refactor
yeonjuan May 31, 2020
2a2e9ec
add test case
yeonjuan May 31, 2020
378cae0
fix lint
yeonjuan May 31, 2020
2c3c383
trailing comma
yeonjuan May 31, 2020
77721a6
delete not occurred case
yeonjuan May 31, 2020
07fde54
refactor
yeonjuan May 31, 2020
16e5a14
fix false neg
yeonjuan Jun 1, 2020
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
57 changes: 48 additions & 9 deletions packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ import {
nullThrows,
NullThrowsReasons,
isMemberOrOptionalMemberExpression,
isIdentifier,
} from '../util';

// Truthiness utilities
Expand Down Expand Up @@ -426,6 +427,48 @@ export default createRule<Options, MessageId>({
return false;
}

// Checks whether a member expression is nullable or not regardless of it's previous node.
// Example:
// ```
// // 'bar' is nullable if 'foo' is null.
// // but this function checks regardless of 'foo' type, so returns 'true'.
// declare const foo: { bar : { baz: string } } | null
// foo?.bar;
// ```
function isNullableOriginFromPrev(
node: TSESTree.MemberExpression | TSESTree.OptionalMemberExpression,
): boolean {
const prevType = getNodeType(node.object);
const property = node.property;
if (prevType.isUnion() && isIdentifier(property)) {
const ownPropertyType = prevType.types
.map(type => checker.getTypeOfPropertyOfType(type, property.name))
.find(t => t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mapping over the types in the union isn't quite right, and will cause issues in the following case. The checker API is smart enough to analyse unions to get the type of a property on that union.

interface Foo {
    a: number;
}

interface Bar {
    a: string | null;
}

declare const x: Foo | Bar;
# type = type of x;
$ type.types.map(t => checker.typeToString(checker.getTypeOfPropertyOfType(t, 'a')))

> ["number", "string | null"]

Eg with your current logic, this case will just check the number case, and will thus false-positive on the property.

If you instead just ask for the property on the union type itself:

# type = type of x;
$ checker.typeToString(checker.getTypeOfPropertyOfType(type, 'a'))

> "string | number | null"

It will work as expected. However if for some reason the base type is nullish, you need to also scrub the null from the union first:

interface Foo {
    a: number;
}

interface Bar {
    a: string | null;
}

declare const x: {t: Foo | Bar} | null;

x?.t.a;
# type = type of x?.t;
$ nonNullType = checker.getNonNullableType(type)
$ checker.typeToString(checker.getTypeOfPropertyOfType(nonNullType, 'a'))

> "string | number | null"

Copy link
Contributor Author

@yeonjuan yeonjuan Jun 1, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradzacher
Thanks for the review. I'll change it soon :)


if (ownPropertyType) {
return (
!isNullableType(ownPropertyType, { allowUndefined: true }) &&
isNullableType(prevType, { allowUndefined: true })
);
}
}
return false;
}

function isOptionableExpression(
node: TSESTree.LeftHandSideExpression,
): boolean {
const type = getNodeType(node);
const isOwnNullable = isMemberOrOptionalMemberExpression(node)
? !isNullableOriginFromPrev(node)
: true;
return (
isTypeFlagSet(type, ts.TypeFlags.Any) ||
isTypeFlagSet(type, ts.TypeFlags.Unknown) ||
(isNullableType(type, { allowUndefined: true }) && isOwnNullable)
);
}

function checkOptionalChain(
node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression,
beforeOperator: TSESTree.Node,
Expand All @@ -444,16 +487,12 @@ export default createRule<Options, MessageId>({
return;
}

const nodeToCheck = isMemberOrOptionalMemberExpression(node)
? node.object
: node;
const type = getNodeType(nodeToCheck);
const nodeToCheck =
node.type === AST_NODE_TYPES.OptionalCallExpression
? node.callee
: node.object;

if (
isTypeFlagSet(type, ts.TypeFlags.Any) ||
isTypeFlagSet(type, ts.TypeFlags.Unknown) ||
isNullableType(type, { allowUndefined: true })
) {
if (isOptionableExpression(nodeToCheck)) {
return;
}

Expand Down
199 changes: 199 additions & 0 deletions packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
View file
Open in desktop
Expand Up @@ -329,6 +329,17 @@ let unknownValue: unknown;
unknownValue?.();
`,
'const foo = [1, 2, 3][0];',
`
declare const foo: { bar?: { baz: { c: string } } } | null;
foo?.bar?.baz;
`,
`
foo?.bar?.baz?.qux;
`,
`
declare const foo: { bar: { baz: string } };
foo.bar.qux?.();
`,
],
invalid: [
// Ensure that it's checking in all the right places
Expand Down Expand Up @@ -836,5 +847,193 @@ x.a;
},
],
},
{
code: `
declare const foo: { bar: { baz: { c: string } } } | null;
foo?.bar?.baz;
`,
output: `
declare const foo: { bar: { baz: { c: string } } } | null;
foo?.bar.baz;
`,
errors: [
{
messageId: 'neverOptionalChain',
line: 3,
endLine: 3,
column: 9,
endColumn: 11,
},
],
},
{
code: `
declare const foo: { bar?: { baz: { qux: string } } } | null;
foo?.bar?.baz?.qux;
`,
output: `
declare const foo: { bar?: { baz: { qux: string } } } | null;
foo?.bar?.baz.qux;
`,
errors: [
{
messageId: 'neverOptionalChain',
line: 3,
endLine: 3,
column: 14,
endColumn: 16,
},
],
},
{
code: `
declare const foo: { bar: { baz: { qux?: () => {} } } } | null;
foo?.bar?.baz?.qux?.();
`,
output: `
declare const foo: { bar: { baz: { qux?: () => {} } } } | null;
foo?.bar.baz.qux?.();
`,
errors: [
{
messageId: 'neverOptionalChain',
line: 3,
endLine: 3,
column: 9,
endColumn: 11,
},
{
messageId: 'neverOptionalChain',
line: 3,
endLine: 3,
column: 14,
endColumn: 16,
},
],
},
{
code: `
declare const foo: { bar: { baz: { qux: () => {} } } } | null;
foo?.bar?.baz?.qux?.();
`,
output: `
declare const foo: { bar: { baz: { qux: () => {} } } } | null;
foo?.bar.baz.qux();
`,
errors: [
{
messageId: 'neverOptionalChain',
line: 3,
endLine: 3,
column: 9,
endColumn: 11,
},
{
messageId: 'neverOptionalChain',
line: 3,
endLine: 3,
column: 14,
endColumn: 16,
},
{
messageId: 'neverOptionalChain',
line: 3,
endLine: 3,
column: 19,
endColumn: 21,
},
],
},
{
code: `
type baz = () => { qux: () => {} };
declare const foo: { bar: { baz: baz } } | null;
foo?.bar?.baz?.().qux?.();
`,
output: `
type baz = () => { qux: () => {} };
declare const foo: { bar: { baz: baz } } | null;
foo?.bar.baz().qux();
`,
errors: [
{
messageId: 'neverOptionalChain',
line: 4,
endLine: 4,
column: 9,
endColumn: 11,
},
{
messageId: 'neverOptionalChain',
line: 4,
endLine: 4,
column: 14,
endColumn: 16,
},
{
messageId: 'neverOptionalChain',
line: 4,
endLine: 4,
column: 22,
endColumn: 24,
},
],
},
{
code: `
type baz = null | (() => { qux: () => {} });
declare const foo: { bar: { baz: baz } } | null;
foo?.bar?.baz?.().qux?.();
`,
output: `
type baz = null | (() => { qux: () => {} });
declare const foo: { bar: { baz: baz } } | null;
foo?.bar.baz?.().qux();
`,
errors: [
{
messageId: 'neverOptionalChain',
line: 4,
endLine: 4,
column: 9,
endColumn: 11,
},
{
messageId: 'neverOptionalChain',
line: 4,
endLine: 4,
column: 22,
endColumn: 24,
},
],
},
{
code: `
type baz = null | (() => { qux: () => {} } | null);
declare const foo: { bar: { baz: baz } } | null;
foo?.bar?.baz?.()?.qux?.();
`,
output: `
type baz = null | (() => { qux: () => {} } | null);
declare const foo: { bar: { baz: baz } } | null;
foo?.bar.baz?.()?.qux();
`,
errors: [
{
messageId: 'neverOptionalChain',
line: 4,
endLine: 4,
column: 9,
endColumn: 11,
},
{
messageId: 'neverOptionalChain',
line: 4,
endLine: 4,
column: 23,
endColumn: 25,
},
],
},
],
});