Bug: use-isnan doesn't report NaN in indexOf or lastIndexOf when fromIndex is given

[Tanujkanti4441]

Environment

Node version: latest
npm version: latest
Local ESLint version: v8.57.0
Global ESLint version:
Operating System: windows

What parser are you using?

Default (Espree)

What did you do?

i tried it in playground

/*eslint use-isnan: ["error", {"enforceForIndexOf": true}]*/

var foo = myArray.indexOf((doStuff(), NaN)); // output will be -1
var foo = myArray.indexOf(NaN, 2) // output will be -1
var foo = myArray.lastIndexOf((doStuff(), NaN)); // output will be -1
var foo = myArray.lastIndexOf(NaN, 2) // output will be -1

What did you expect to happen?

I expected that use-isnan rule will report these cases as all have the output -1.

What actually happened?

the rule doesn't report these cases.

Link to Minimal Reproducible Example

https://eslint.org/play/#eyJ0ZXh0IjoiLyplc2xpbnQgdXNlLWlzbmFuOiBbXCJlcnJvclwiLCB7XCJlbmZvcmNlRm9ySW5kZXhPZlwiOiB0cnVlfV0qL1xuXG52YXIgZm9vID0gbXlBcnJheS5pbmRleE9mKChkb1N0dWZmKCksIE5hTikpOyAvLyBvdXRwdXQgd2lsbCBiZSAtMVxudmFyIGZvbyA9IG15QXJyYXkuaW5kZXhPZihOYU4sIDIpIC8vIG91dHB1dCB3aWxsIGJlIC0xXG52YXIgZm9vID0gbXlBcnJheS5sYXN0SW5kZXhPZigoZG9TdHVmZigpLCBOYU4pKTsgLy8gb3V0cHV0IHdpbGwgYmUgLTFcbnZhciBmb28gPSBteUFycmF5Lmxhc3RJbmRleE9mKE5hTiwgMikgLy8gb3V0cHV0IHdpbGwgYmUgLTEiLCJvcHRpb25zIjp7ImVudiI6e30sInJ1bGVzIjp7fSwicGFyc2VyT3B0aW9ucyI6eyJlY21hRmVhdHVyZXMiOnt9LCJlY21hVmVyc2lvbiI6ImxhdGVzdCIsInNvdXJjZVR5cGUiOiJtb2R1bGUifX19

Participation

• I am willing to submit a pull request for this issue.

Additional comments

No response

[mdjermanovic]

/*eslint use-isnan: ["error", {"enforceForIndexOf": true}]*/

var foo = myArray.indexOf((doStuff(), NaN)); // output will be -1
var foo = myArray.indexOf(NaN, 2) // output will be -1
var foo = myArray.lastIndexOf((doStuff(), NaN)); // output will be -1
var foo = myArray.lastIndexOf(NaN, 2) // output will be -1

The rule does report myArray.indexOf((doStuff(), NaN)) and var foo = myArray.lastIndexOf((doStuff(), NaN)) in ESLint v9.0.0-beta.1. Handling of sequence expressions was added in #18059.

Note that the Playground is using ESLint v8.57.0.

[Tanujkanti4441]

got it! but following tests are valid in ruleTester are they valid?

{
            code: "foo.lastIndexOf(NaN, 2)",
            options: [{ enforceForIndexOf: true }]
        },
        {
            code: "foo.indexOf(NaN, 4)",
            options: [{ enforceForIndexOf: true }]
        },

[mdjermanovic]

As for not reporting cases with multiple arguments, it seems to be intentional:

eslint/lib/rules/use-isnan.js

Line 185 in e37153f

node.arguments.length === 1 &&

eslint/tests/lib/rules/use-isnan.js

Lines 265 to 268 in e37153f

	{
	code: "foo.lastIndexOf(NaN, b)",
	options: [{ enforceForIndexOf: true }]
	},

I believe this is because we don't know for sure if .indexOf()/.lastIndexOf() are really calls to Array.prototype methods, and an unexpected number of arguments is an indication that they're not. However, we may have missed the fact that these methods have two parameters.

I think it's fine to update this rule to report cases with two arguments, and treat this as a semver-minor bug fix. Curious what others think.

[mdjermanovic]

So, the question is whether these cases should be reported:

/*eslint use-isnan: ["error", {"enforceForIndexOf": true}]*/

var foo = myArray.indexOf(NaN, 2) // output will be -1
var foo = myArray.lastIndexOf(NaN, 2) // output will be -1

[fasttime]

I think it's fine to update this rule to report cases with two arguments, and treat this as a semver-minor bug fix. Curious what others think.

Makes sense to me 👍🏻 The logic to check indexOf and lastIndexOf calls was added in #12379, but I cannot find a discussion about restricting the applicability of the check to calls with one argument. If the two arguments case was not properly considered, it seems fine to treat this change as a bugfix.

[Tanujkanti4441]

@eslint/eslint-team are we up for implementing this?

[mdjermanovic]

Yes, PR is welcome.