no-array-callback-reference rule is not compatible with TypeScript type guards

[moltar]

function isDefined<T>(argument: T | undefined): argument is T {
  return typeof argument !== 'undefined'
}

const items = [1, 2, undefined]

items.filter(isDefined).map((number) => {
  // number is now type guarded and undefined is eliminated
  console.log(number)
})

items.filter((num) => isDefined(num)).map((number) => {
  // however, this way it does not work, and "number" can still be undefined
  console.log(number)
})

I am not sure why TypeScript is not inferring the results in the second way.

[fisker]

Console shows the same result on typescript playground

[moltar]

That's not true:

---

number is not undefined:

Screenshot:

---

number is: number | undefined type.

Screenshot:

---

I think my example was poor, here is a better one:

https://www.typescriptlang.org/play/index.html?target=99#code/GYVwdgxgLglg9mABDAzgEQKbBmDATAHgBUA+ACgEMAnAcxAFsMwoAuRIxAH0XDyx3wBKNtTqNmyFO0QBvAFCJEVDFBBUkUAJ4AHDHGCJRDJlEQBCALwXEAcl79ceG3IC+cuRAQpTMKBnpS1gDaAIwANIgATBH22I4Auu6+-igAdNgANn5UZKiYcUKp9BTaZGRgDABGGFSCiBYksgqIAPQtiBX01VSSHXAA7ohauoh01Hx4hmCTsQKTqIgYGTD0OBR+eM3KquodVTWugkl+AekwWTVlnXUNkvlz5QyCgkUlV-u19Y3yim2IABYDDAANxqESg-wW-QommQpjwcAwUjAcFM-TgVAA1hEKNNEAAiTrdfGICC4xDec4ZRDVHjTBz4LYqNRIIk1RAAakQIUOQA

[fisker]

Sorry, I mean complied js code result.

[moltar]

But the code being validated is the TypeScript one. So I’m not sure how the result is relevant?

[fisker]

Expect

items.filter(((num) => isDefined(num)) as typeof isDefined).map((number) => {
  // however, this way it does not work, and "number" can still be undefined
  return number + 1
})

?

[moltar]

I am not sure what is going on there. I am getting confused.

Both of the code snippets I posted are valid.

My only concern is that the rule is being triggered in a case where there is no other option. The only option is to do filter(isDefined), as that is how TypeScript works/infers.

[fisker]

It's not only about .filter, all function references lost types.

[oss6]

Hi, the issue is described here: microsoft/TypeScript#10734 and here: microsoft/TypeScript#16069.
Would it be sensible to add an exception for functions with type guards (either by default or through an option)?

[moltar]

@oss6 thanks for finding the proper references!

[moltar]

I think the rule is renamed to unicorn/no-array-callback-reference now as I understand.

[osdiab]

I agree that this particular scenario is one where array callbacks is actually nice to use. I'd be interested if the unicorn/no-array-callback-reference rule was configurable to allow it specifically for filter calls, since wrapping a function in TypeScript loses the type guard, making this non-ergonomic

[Vages]

I could try my hand at this. My initial plan is to add an option with a name like ignoreTypeGuardFilters to the rule. If this is set to true, the linter will ignore any function considered a type guard.

This should be trivial for cases where the function exists in the same file, such as this: https://astexplorer.net/#/gist/6cb8b6a6683042ce444d2033901e43c5/95af3ecc1cb391cc9b94a37fe455880ab94c0bc9
I'm not sure about what to do when the function is imported, though.

Does anyone know of any existing rules that rely on the Typescript return type of something that's imported? (in unicorn or anywhere else). I've tried thinking about this for 15 minutes without coming up with someone – but I'm relatively new to writing/reading eslint rules.

[fisker]

Does anyone know of any existing rules that rely on the Typescript return type of something that's imported?

I don't know about this, but you can try check https://github.com/typescript-eslint/typescript-eslint, maybe they do this.

[robeady]

I met this problem too. I'd like to keep using the rule but I can see no convenient workaround. Would a more practical solution be to let the user configure the list of allowed function names, either for filter callbacks or any callback? Introducing type sensitive lints seems pretty drastic for such an isolated issue.

[RebeccaStevens]

The typeguard info is lost because the wrapper function is not a typeguard. If you simply make your wrapper function a typeguard then problem fixed.

e.g.

items.filter((num): num is number => isDefined(num)).map((number) => {
  console.log(number + 1);
});

playground link

Though this being said, typeguards probably should be ignored by this rule (probably via an option like @Vages suggests (I would suggest the option be named ignoreTypeGuards or allowTypeGuards)) as typeguards are highly unlikely to change their parameter signature.

This should be trivial for cases where the function exists in the same file
I'm not sure about what to do when the function is imported, though.

It shouldn't make any difference if the function is defined in the same file or not; all the information needed is in the function's type information.

If people want this, I can make a PR (I'm the main contributor for eslint-plugin-functional so I'm pretty experienced with working with eslint rules).

[Vages]

With an experienced eslint-rulemaker like you, @RebeccaStevens, making the extension, I'm quite certain that the lint-rule should just ignore type-guards by default (in hindsight, I think I only suggested it due to a lack of confidence in my lint-rule abilities 😅). Having an option for it seems bloated, and if this actually affects someone negatively (which I really doubt), we could add the option in later.

I'm only an ordinary contributor myself, but if you make a PR, I could test it and give some feedback. Just tag me when you add it. PS: I use eslint-plugin-functional; good work!

[RebeccaStevens]

I haven't looked too closely at the rule but I believe currently the rule doesn't use type information.
It might be best to use an option to ignore typeguards so that the rule still works out of the box without needing type information; thus also ensuring JS users can still use the rule.
I'm not familiar with the internals of this project however so I don't know whether that would be a concern.

[Vages]

I'm not familiar with the internals of this project however so I don't know whether that would be a concern.

I'm not either. Do you have an opinion, @fisker?

[tetarchus]

Was there ever any update on this? Currently can work around with disable comment, but would be nice to be able to remove them.

[briavicenti]

+1, I'd love to see a fix to this rule so I can stop disabling it on all inline typeguards.