Skip to content

fix(eslint-plugin): [no-unnecessary-condition] ignore optionally accessing index signatures #6762

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

Closed
wants to merge 1 commit into from
Closed

fix(eslint-plugin): [no-unnecessary-condition] ignore optionally accessing index signatures #6762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 28 additions & 13 deletions packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,17 +182,32 @@ export default createRule<Options, MessageId>({
return checker.isTupleType(nodeType);
}

function isArrayIndexExpression(node: TSESTree.Expression): boolean {
// Index accesses are exempt from certain checks about nullish-ness, because
// the type returned by the checker is not the strictest
function isIndexAccessExpression(node: TSESTree.Expression): boolean {
if (node.type !== AST_NODE_TYPES.MemberExpression) {
return false;
}
const objType = getNodeType(node.object);
if (checker.isTupleType(objType)) {
// If indexing a tuple with a literal, the type will be sound
return node.property.type !== AST_NODE_TYPES.Literal;
}
Comment on lines +192 to +195
Copy link
Member

Choose a reason for hiding this comment

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

This check is actually no longer sound with modern TS.

Tuples can be variadic now - (eg [T1, ...T2[]], [...T1[], T2], [T1, ...T2[], T3], [...T1[]]) - so they're no more safe than an array.

I.e. for const foo: [T1, ...T2[]] - foo[1] is possibly undefined

playground

if (node.computed) {
const indexType = getNodeType(node.property);
if (isTypeAnyType(indexType)) {
Copy link
Member

Choose a reason for hiding this comment

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

question: do we want to handle the never case here?
I think it's okay not to just figured I'd flag it for completeness sake

// No need to check anything about any
return true;
}
if (isTypeFlagSet(indexType, ts.TypeFlags.NumberLike)) {
Copy link
Member

Choose a reason for hiding this comment

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

This doens't handle "nominal" number types here.
EG type T = number & { __brand: unknown } would work in the signature but we would not match it here.

I think it's okay that we ignore it here - it's a super niche usecase.
Just commenting for completeness's sake.

return (
checker.getIndexTypeOfType(objType, ts.IndexKind.Number) !==
undefined
);
}
}
Comment on lines +202 to +208
Copy link
Member

Choose a reason for hiding this comment

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

we should probably also handle symbol indexes as well.
They're super rare but it should just be one extra if

declare const obj: {[k: symbol]: string};
declare const key: symbol;
const res = obj[key];
res?.length // should not report

return (
// Is an index signature
node.type === AST_NODE_TYPES.MemberExpression &&
node.computed &&
// ...into an array type
(nodeIsArrayType(node.object) ||
// ... or a tuple type
(nodeIsTupleType(node.object) &&
// Exception: literal index into a tuple - will have a sound type
node.property.type !== AST_NODE_TYPES.Literal))
checker.getIndexTypeOfType(objType, ts.IndexKind.String) !== undefined
);
}

Expand All @@ -215,7 +230,7 @@ export default createRule<Options, MessageId>({
// Since typescript array index signature types don't represent the
// possibility of out-of-bounds access, if we're indexing into an array
// just skip the check, to avoid false positives
if (isArrayIndexExpression(node)) {
if (isIndexAccessExpression(node)) {
return;
}

Expand Down Expand Up @@ -276,7 +291,7 @@ export default createRule<Options, MessageId>({
// possibility of out-of-bounds access, if we're indexing into an array
// just skip the check, to avoid false positives
if (
!isArrayIndexExpression(node) &&
!isIndexAccessExpression(node) &&
!(
node.type === AST_NODE_TYPES.ChainExpression &&
node.expression.type !== AST_NODE_TYPES.TSNonNullExpression &&
Expand Down Expand Up @@ -490,7 +505,7 @@ export default createRule<Options, MessageId>({
): boolean {
const lhsNode =
node.type === AST_NODE_TYPES.CallExpression ? node.callee : node.object;
if (node.optional && isArrayIndexExpression(lhsNode)) {
if (node.optional && isIndexAccessExpression(lhsNode)) {
return true;
}
if (
Expand Down
64 changes: 30 additions & 34 deletions packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,15 +318,41 @@ if (returnsArr?.()[42]) {
}
returnsArr?.()[42]?.toUpperCase();
`,
// nullish + array index
// nullish + index signature
`
declare const arr: string[][];
declare const x: any;
arr[x] ?? [];
`,
// nullish + optional array index
`
declare const arr: { foo: number }[];
const bar = arr[42]?.foo ?? 0;
`,
`
declare const foo: Record<string, number>;
declare const bar: string;

foo[bar] ??= 0;
`,
`
declare const foo: Record<string, number>;
declare const bar: string;

foo[bar] ?? 0;
`,
`
function getElem(dict: Record<string, { foo: string }>, key: string) {
if (dict[key]) {
return dict[key].foo;
} else {
return '';
}
}
`,
`
declare const dict: Record<string, object>;
if (dict['mightNotExist']) {
}
`,
// Doesn't check the right-hand side of a logical expression
// in a non-conditional context
Expand Down Expand Up @@ -895,28 +921,18 @@ function nothing3(x: [string, string]) {
],
},
// Indexing cases
{
// This is an error because 'dict' doesn't represent
// the potential for undefined in its types
code: `
declare const dict: Record<string, object>;
if (dict['mightNotExist']) {
}
`,
errors: [ruleError(3, 5, 'alwaysTruthy')],
},
{
// Should still check tuples when accessed with literal numbers, since they don't have
// unsound index signatures
code: `
const x = [{}] as [{ foo: string }];
declare const x: [{ foo: string }];
if (x[0]) {
}
if (x[0]?.foo) {
}
`,
output: `
const x = [{}] as [{ foo: string }];
declare const x: [{ foo: string }];
if (x[0]) {
}
if (x[0].foo) {
Expand Down Expand Up @@ -1670,26 +1686,6 @@ pick({ foo: 1, bar: 2 }, 'bar');
},
{
code: `
function getElem(dict: Record<string, { foo: string }>, key: string) {
if (dict[key]) {
return dict[key].foo;
} else {
return '';
}
}
Comment on lines -1673 to -1679
Copy link
Member Author

Choose a reason for hiding this comment

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

This was arguably a bug fix in the existing test case, because it shouldn't be reported either.

Copy link
Member

Choose a reason for hiding this comment

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

This actually should be reported because the fixture config doesn't use noUncheckedIndexAccess!

I think we should probably guard the new code around whether or not the flag is on.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bradzacher This code is the exact same principle as dictionary[key] ??= defaultVal. If we want to ignore that, we have to ignore others as well.

Copy link
Member

Choose a reason for hiding this comment

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

They are and they aren't. You're correct in that they're both record index access in a logical position - so they're semantically the same case. However TS reports the types differently for the two cases.

In this instance TS will report the read type, meaning that if noUncheckedIndexAccess is turned on then TS will report the type as T | undefimed to us.

However in x ?? y, TS will report the write type, meaning it reports just T - no matter whether or not noUncheckedIndexAccess is on or not.

This is the fundamental difference between the two cases and why we are false positiving on the assignment operators, but not on this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

However, we already ignore array index access, even without noUncheckedIndexAccess. See this test case:

declare const arr: { foo: number }[];
const bar = arr[42]?.foo ?? 0;

I find it quite natural to extend the scope to all index signatures, since there isn't something special about array indices.

Copy link
Member

Choose a reason for hiding this comment

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

The reason we did it for arrays and not objects is because defining arrays as (T | undefined)[] is similar to Record<string, T | undefined>, but far, far less ergonomic syntactically and is arguably worse given how common it is to iterate arrays directly using an iterator method or for/of.

For objects it's pretty natural to union in undefined because most usecases for reading is an index signature - the direct iteration methods (Object.entries / Object.values) are both rarer usecases so the undefined doesn't get in the way as much.

Now that we have noUncheckedIndexAccess - we don't really need the logic at all, but I understand that some people think the compiler option goes too far and can get in the way (which is true to a degree).

The motivation behind this change isn't bringing the two sets up to parity (that's a much larger change that we should discuss as a group to decide if we think we should treat objects the same as arrays in this regard) - instead it's just fixing the current false positives we have due to the logical operator handling we added in #6594

`,
errors: [
{
messageId: 'alwaysTruthy',
line: 3,
endLine: 3,
column: 7,
endColumn: 16,
},
],
},
{
code: `
declare let foo: {};
foo ??= 1;
`,
Expand Down