feat(eslint-plugin): [no-unnecessary-condition][strict-boolean-expressions] add option to make the rules error on files without strictNullChecks turned on

[bradzacher]

I'm very much sick of closing issues about these rules by users not using strictNullChecks.
I wish that TS would just make this true by default...

This will make the rules report an unsilenceable error for every file that is not using strictNullChecks.

cc rule authors @phaux and @Retsam as you're both passionate about these rules.
Do you think this is a valid change for both rules?

[typescript-eslint]

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[codecov]

Codecov Report

Merging #2345 into v4 will increase coverage by 0.00%.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##               v4    #2345   +/-   ##
=======================================
  Coverage   92.86%   92.86%           
=======================================
  Files         286      286           
  Lines        9064     9073    +9     
  Branches     2517     2521    +4     
=======================================
+ Hits         8417     8426    +9     
  Misses        319      319           
  Partials      328      328           

Flag	Coverage Δ
#unittest	92.86% <90.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...slint-plugin/src/rules/no-unnecessary-condition.ts	95.73% <80.00%> (+0.10%)	⬆️
...int-plugin/src/rules/strict-boolean-expressions.ts	97.89% <100.00%> (+0.11%)	⬆️

[phaux]

If it requires manual enabling, then an error is okay. If we ever want to add strict-bool-exprs to the recommended set (it's not recommended IIRC), then it would make more sense to make it do nothing, to not everwhelm the new users with errors.

So the question is whether are there any plans to include strict-bool-exprs in default config. I actually wanted to propose that in #1423 but I forgot 😅

[bradzacher]

are there any plans to include strict-bool-exprs in default config

I would love to - but I think that it's kind of stylistic (many people don't like being strict...), so no doubt we'd get a lot of push-back about it.

no-unnecessary-condition isn't in recommended either - many users have weakly typed codebases or practice defensive programming, so the rule causes noise for them.

I'm thinking maybe we should create a 'recommended-strict' ruleset which gives us another level of recommendation for people?

[phaux]

@bradzacher We had this talk about strict-boolean-expressions before in #1423 (comment) , but that was before the big refactor. Now we only report nullable strings, numbers and booleans in this rule by default. Previously, it were all the non-boolean values.

I tried enabling this rule (the 3.0 version) on a few projects at work, and guess what. It reported a lot of errors, and upon looking at them, about half of them looked like an actual error or oversight, something that could potentially cause a bug in the runtime.

I think we should make this rule even more user friendly: suggest using nullish coalescing in the message, provide suggestion or auto fix for the common cases, etc, and then add it to the recommended set.

As for no-unnecessary-condition, I don't care. I'm indifferent if it reports an error, or doesn't work at all with strictNullChecks. I never use it.

[bradzacher]

I think we should make this rule even more user friendly: suggest using nullish coalescing in the message, provide suggestion or auto fix for the common cases, etc, and then add it to the recommended set.

Definitely! strict-bool-expr could 100% benefit from suggestion fixers for all cases.

I could see offering a whole suite of suggestions, such as:

declare const foo: string | null | undefined;
declare const bar: string | null;
declare const baz: string | undefined;

// input
if (foo) {}
if (bar) {}
if (baz) {}

// suggestion 1 - strict equality
if (foo !== null && foo !== undefined && foo !== '') {}
if (bar !== null && bar !== '') {}
if (baz !== undefined && baz !== '') {}

// suggestion 2 - loose nullish equality 
if (foo != null && foo !== '') {}
if (bar != null && bar !== '') {}
if (baz != null && baz !== '') {}

// suggestion 3 - just strict nullish
if (foo !== null && foo !== undefined) {}
if (bar !== null) {}
if (baz !== undefined) {}

// suggestion 4 - just loose nullish
if (foo != null) {}
if (bar != null) {}
if (baz != null) {}

// suggestion 5 - just string equality
if (foo !== '') {}
if (bar !== '') {}
if (baz !== '') {}

We could also add a configurable autofixer with two options eg 'eqeqeq' | 'eqeq' | 'none', which adds an auto fixer which fixes to suggestion 1, suggestion 2, or no autofixer, respectively.

---

My concern with adding it to recommended is that there's a lot of users who would rather do if (foo) {} over if (foo !== null && foo !== undefined && foo !== '') {} or if (foo != null && foo !== '') {}. Some people (esp JS "purists" working in TS) just don't want to be verbose, and see this sort of linting as unhelpful cruft.

I 100% think there's value in the rule, and since working with flow (where this is a forced part of the typechecker itself), I've come to see tangible value in it - but I know a lot of people will hate it, and I don't know if I can deal with the very vocal minority fighting our recommended set again.

[phaux]

@bradzacher

If the default options are used, then simple nullish coalescing could be suggested:

if (foo) {}
// fixed
if (foo ?? "") {}
if (foo ?? 0) {}
if (foo ?? false) {}

I like how this makes it explicit what's happening, so that the developer has to think "do I really want that behavior?". If the developer is not very familiar with JS it will force them to find out what ?? means, which is also good. It could even be an autofix since it doesn't even change the behavior during runtime.

---

I could see offering a whole suite of suggestions, such as

I don't see any value in suggesting anything other than foo != null. I've never seen anybody write foo !== null && foo !== undefined etc. From my experience at work, most developers use the original (which is often wrong and needs to be fixed), or != null which is fine. I've also seen people using lodash isNil which is exactly like == null, but.. why?

---

I'm in favor of the following solution:

• Show two suggestions: use nullish coalescing to falsy value, or compare with null.
  • Alternatively make the nullish coalescing the autofix.
• Come up with a better error message which explains why it's wrong and what's the solution. I think the current one is good enough and very concise, but I wouldn't mind a longer one.

---

I really really hate pointing out these errors to colleagues. They are like a plague. Sometimes I feel like a bad person for pointing them out everytime. Even senior developers sometimes make them.

[bradzacher]

Some codebases don't use == null, and instead always explicitly check both null and undefined. I know a lot of developers don't realise that == null matches both (I personally didn't know this until ~2 years ago!).

If the codebase is using the eqeqeq rule with the default option, then fixing to == null will trigger that lint error, and probably confuse the user.

Side note - some codebases also go as far as to do typeof foo === 'undefined' so they explicitly guard against `undefined` being reassigned (because in JS, you're ridiculously allowed to do that).

var undefined = 1;
console.log(typeof undefined);
// > number
console.log(typeof window.undefined);
// > undefined

const x = {};
console.log(x.notDefined === undefined);
// > false

At FB, we use == null + === falsey in all our conditionals (as I've mentioned before - flow has this lint rule built into it). Worth noting that I just did a quick search of the codebase, and there are 0 matches for the regex if \(.+? \?\? .+?\) in the JS codebase.

If we went down the route of adding all of the suggestion fixers, we could probably offer a config option to choose the style of suggestions here instead of always giving every single option to the user ('eqeq' | 'eqeqeq' | 'typeof' | 'nullish-coalesce').

[phaux]

Some codebases don't use == null, and instead always explicitly check both null and undefined

I've never worked with such codebase. I know that transpilers generate code like this, because some weird obsolete DOM properties behave differently otherwise (document.all?).

It could be supported as a rule option though.

If the codebase is using the eqeqeq rule with the default option, then fixing to == null will trigger that lint error, and probably confuse the user.

Yeah, but that's normal that rules can collide with each other. Even typescript-eslint has colliding rules (eg. require-await and require-async, I don't remember the names exactly). As long as they're not colliding with other recommended sets it should be okay to include them as recommended IMO. eqeqeq is not in the recommended eslint config. I do use it but I use the "smart" option which is compatible.

some codebases also go as far as to do typeof foo === 'undefined' so they explicitly guard against undefined being reassigned (because in JS, you're ridiculously allowed to do that).

TypeScript already guards against it. Also I think it's a runtime error to reassign undefined in module context (they get "use strict" by default), but I might be wrong.

It could also be supported as an option, but not the default.

At FB, we use == null + === falsey in all our conditionals

I don't like how this changes the semantics when the types are wrong. If you're writing a library, but the user doesn't use TS, then foo != null && foo !== "" will behave in unexpected way if the user gives any other falsy value, eg false. That's a small detail though.

Worth noting that I just did a quick search of the codebase, and there are 0 matches for the regex if \(.+? \?\? .+?\) in the JS codebase.

That's probably because this syntax didn't exist until recently. I still like it the most for the reasons I already said.

If we went down the route of adding all of the suggestion fixers, we could probably offer a config option to choose the style of suggestions here instead of always giving every single option to the user ('eqeq' | 'eqeqeq' | 'typeof' | 'nullish-coalesce').

That would be fine. The only question remaining is which should be in the recommended set. I vote for "nullish-coalescing". The other option is to not autofix, but show all the possibilities as suggestions by default.

---

Also note that the "nullish-coalescing" fix/suggestion is the only one which doesn't change the runtime semantics. The other ones change the condition from checking for falsy to checking for nullish. Autofixing to nullish-coalescing doesn't change anything. It only makes it explicit what's happening.