Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

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

Already on GitHub?Sign in to your account

Closed
Show file tree
Hide file tree
Changes fromall commits
Commits
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 deletionspackages/eslint-plugin/src/rules/no-unnecessary-condition.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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. forconst foo: [T1, ...T2[]] -foo[1] is possiblyundefined

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 thenever 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.
EGtype 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 handlesymbol indexes as well.
They're super rare but it should just be one extraif

declareconstobj:{[k:symbol]:string};declareconstkey:symbol;constres=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 DownExpand 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 DownExpand 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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -318,15 +318,41 @@ if (returnsArr?.()[42]) {
}
returnsArr?.()[42]?.toUpperCase();
`,
// nullish +arrayindex
// 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 DownExpand 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 }];
declareconst x: [{ foo: string }];
if (x[0]) {
}
if (x[0]?.foo) {
}
`,
output: `
const x = [{}] as [{ foo: string }];
declareconst x: [{ foo: string }];
if (x[0]) {
}
if (x[0].foo) {
Expand DownExpand 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
MemberAuthor

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.

armano2 reacted with thumbs up emoji
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 usenoUncheckedIndexAccess!

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

Copy link
MemberAuthor

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 asdictionary[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 ifnoUncheckedIndexAccess is turned on then TS will report the type asT | undefimed to us.

However inx ?? y, TS will report the write type, meaning it reports justT - no matter whether or notnoUncheckedIndexAccess is on or not.

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

Copy link
MemberAuthor

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 withoutnoUncheckedIndexAccess. See this test case:

declareconstarr:{foo:number}[];constbar=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 toRecord<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 inundefined because most usecases for reading is an index signature - the direct iteration methods (Object.entries /Object.values) are both rarer usecases so theundefined doesn't get in the way as much.

Now that we havenoUncheckedIndexAccess - 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
[8]ページ先頭
©2009-2025 Movatter.jp