feat(eslint-plugin): add strict-type-predicates #738

bradleyayers · 2019-07-21T23:28:26Z

This is a direct port of the TSLint version, with as few changes as possible.

Notable differences:

No warning emitted when strictNullChecks is not enabled.
Shorter rule description
Only a single set of tests (instead of split unknown, strictNullChecks, noStrictNullChecks)

bradzacher

I haven't read the code yet, but I noticed a few things:

you forgot a rule readme
there needs to be an entry in the root eslint-plugin readme
there should be an entry in configs/all.json

run:

yarn generate:configs to add it to all.json
yarn check:docs to validate your docs setup

bradleyayers · 2019-09-25T22:50:32Z

Hey @bradzacher any more thoughts on this? I'm thinking that I'll use a local fork for my own project to give it some real-world testing. Are there any guides on how to do this (or is it as simple as npm pack)?

bradzacher · 2019-09-26T00:12:56Z

It's on my todo list. I've had family visiting for a few weeks so my spare time has been sparse.

I'm about to start getting back into the review queue.

I've never done a local fork, sorry, so I don't have any advice for you. I'd assume it should be pretty simple because the publish step is literally just publish each package's directory.

ark120202 · 2019-09-30T11:36:18Z

Looks like it has some intersection with no-unnecessary-condition, at least in that case:

const number = 1;
if (number === null) {}
if (number === undefined) {}

It doesn't handle that though:

const numberOrNull: number | null = 0;
if (numberOrNull != null) {}

const number: number = 0;
if (typeof number === 'string') {}

I wonder if there's really a need for multiple rules being so similar (conceptually)?
Would it maybe be better to improve no-unnecessary-condition instead?

bradleyayers · 2019-10-03T05:17:40Z

I like the idea of merging it into no-unnecessary-condition, that seems like it could be a good fit. I think the name is an improvement too. I feel like it might need some options to enable the new behaviour though, as it's quite strict and might be undesirable for some people.

bradzacher · 2019-10-07T16:07:07Z

I'm curious - how much value is there for you in these checks?

const numberOrNull: number | null = 0;
// Implicitly checks for `!== undefined`, which is always false.
if (numberOrNull != null) {
  return;
}
const numberOrUndefined: number | undefined = 0;
// Implicitly checks for `!== null`, which is always false.
if (numberOrNull != undefined) {
  return;
}

I've always held the opinion that null checks should always be made [!=]= null, because it catches the undefined case - treating null and undefined differently seems like a path to bugs IMO.

This does seem like it has value for obvious reasons:

const number: number = 0;
// Always false.
if (typeof number === 'string') {
  return;
}

bradleyayers · 2019-10-07T21:34:45Z

In general I've found the rule really valuable for killing stale code, increasing confidence in refactors, and readability (you know exactly what types are being dealt with). The trade-off of having more verbosity in exchange for more clarity has been one we've been happy with.

Specifically regarding null/undefined comparisons, they're good for refactors where a null might be swapped for an undefined, but a !== null is left unchanged in the code. Like you say though, another option is to ban !== for null/undefined, and force null/undefined comparisons to use !=. I think that's a reasonable direction to go, and encourage consistency with optional chaining.

What rule do you use to enforce that?

bradzacher · 2019-10-07T22:01:22Z

What rule do you use to enforce that?

The base eslint eqeqeq rule will cover it, if you pass the second argument:
["error", "always", {"null": "never"}] - eslint demo

I'd be happy to have both in no-unnecessary-condition:

the typeof check can probably be straight up built into the rule
== null can be added as an option to the rule.
- I think that it is ultimately a stylistic choice as to whether you use x == null or x === null || x === undefined, so I think having an option to be super strict and trust types completely is 100% okay!

bradzacher

thanks for being patient.
a few comments about the place

mostly looks good.
thanks for doing this.

I think it'd be great if the (==|!=|===|!===) (null|undefined) checks had a flag to disable it.
It might just be me being selfish, because I like everything but that check :)

bradzacher · 2020-01-16T02:44:42Z

packages/eslint-plugin/docs/rules/strict-type-predicates.md

+const number: number = 0;
+// Always false.
+if (typeof number === 'number') {
+  return;
+}


Wouldn't this be reported by the rule, because it's always true?

This should probably follow the above tests

const numberOrUndefined: number | undefined = 0;

bradzacher · 2020-01-16T02:47:25Z

packages/eslint-plugin/docs/rules/strict-type-predicates.md

+const numberOrNull: number | null = 0;
+if (numberOrNull !== null) {
+  return;
+}
+
+const numberOrUndefined: number | undefined = 0;
+if (numberOrNull !== undefined) {
+  return;
+}
+
+const number: number = 0;
+// Always false.
+if (typeof number === 'number') {
+  return;
+}


nit with these - just for correctness, typescript narrows the type of the const variables to just be number, because it knows they can't change, so they can't be undefined.

A good way around this is to either declare the variables:

declare const numberOrUndefined: number | undefined; if (numberOrNull !== undefined) { return; }

Or wrap your examples in a function:

function ex1(numberOrUndefined: number | undefined) { if (numberOrNull !== undefined) { return; } }

bradzacher · 2020-01-16T02:49:21Z