Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Aug 16, 2020

Conversation

@bradzacher
Copy link
Member

@bradzacher bradzacher commented Jul 30, 2020

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?

@bradzacher bradzacher added this to the 4.0.0 milestone Jul 30, 2020
@typescript-eslint
Copy link

@typescript-eslint typescript-eslint bot commented Jul 30, 2020

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
Copy link

@codecov codecov bot commented Jul 30, 2020

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
Copy link
Contributor

@phaux phaux commented Jul 31, 2020

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
Copy link
Member Author

@bradzacher bradzacher commented Jul 31, 2020

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
Copy link
Contributor

@phaux phaux commented Jul 31, 2020

@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
Copy link
Member Author

@bradzacher bradzacher commented Aug 8, 2020

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.

@bradzacher bradzacher force-pushed the v4 branch 2 times, most recently from cf29440 to 12b9c8a Aug 9, 2020
…essions] add option to make the rules error on files without `strictNullChecks` turned on
@bradzacher bradzacher force-pushed the strict-null-rules branch from c40023e to 0a9eeed Aug 10, 2020
@phaux
Copy link
Contributor

@phaux phaux commented Aug 11, 2020

@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
Copy link
Member Author

@bradzacher bradzacher commented Aug 11, 2020

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
Copy link
Contributor

@phaux phaux commented Aug 12, 2020

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.

@bradzacher bradzacher merged commit ee5b194 into v4 Aug 16, 2020
11 checks passed
11 checks passed
Typecheck
Details
Unit tests Unit tests
Details
Code style and lint
Details
Run integration tests on primary Node.js version
Details
Run unit tests on other Node.js versions (10.x)
Details
Run unit tests on other Node.js versions (14.x)
Details
Publish the latest code as a canary version
Details
Publish the latest code as a v4 prerelease version
Details
Semantic Pull Request ready to be squashed
Details
codecov/patch 90.00% of diff hit (target 90.00%)
Details
codecov/project 92.86% (+0.00%) compared to 12b9c8a
Details
@bradzacher bradzacher deleted the strict-null-rules branch Aug 16, 2020
@bradzacher bradzacher linked an issue that may be closed by this pull request Aug 16, 2020
bradzacher added a commit that referenced this pull request Aug 19, 2020
…sions] add option to make the rules error on files without `strictNullChecks` turned on (#2345)
bradzacher added a commit that referenced this pull request Aug 29, 2020
…sions] add option to make the rules error on files without `strictNullChecks` turned on (#2345)
bradzacher added a commit that referenced this pull request Aug 29, 2020
…sions] add option to make the rules error on files without `strictNullChecks` turned on (#2345)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants