Closed Bug 1436575 Opened 2 years ago Closed 2 years ago

eslint: Prevent ==/!= comparisons with booleans as they aren't necessary

Categories

(Firefox Build System :: Lint and Formatting, enhancement)

enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: MattN, Assigned: jaws)

References

()

Details

Attachments

(6 files)

The 2nd rule for JS in the Coding Style doc[1] isn't currently enforced:

> Do not compare x == true or x == false. Use (x) or (!x) instead.

This can be enforced easily thanks to an example[2] from the eslint issue tracker:

    "no-restricted-syntax": [
      "error",
      {
        "selector": "BinaryExpression[operator=/(==|!=)/][left.raw=/^(true|false)$/], BinaryExpression[operator=/(==|!=)/][right.raw=/^(true|false)$/]",
        "message": "Don't compare for inexact equality against boolean literals"
      }
    ],

I ran this on browser/ and only 25 files violated this so it's clear that there is agreement on this (at least within browser/).

We should enable this on the whole tree since it's in the coding style guide.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices
[2] https://github.com/eslint/eslint/issues/9743#issuecomment-353418510
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment on attachment 8949487 [details]
Bug 1436575 - Prevent ==/!= comparisons with booleans as they aren't necessary.

https://reviewboard.mozilla.org/r/218806/#review225172

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-compare-against-boolean-literals.js:2
(Diff revision 1)
>  /**
> - * @fileoverview Use .includes instead of .indexOf
> + * @fileoverview Restrict comparing against `true` or `false`.

Please can you add docs for this in tools/lint/docs/linters/eslint-plugin-mozilla.rst

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-compare-against-boolean-literals.js:22
(Diff revision 1)
>    // ---------------------------------------------------------------------------
>    // Public
>    //  --------------------------------------------------------------------------
>  
>    return {
>      "BinaryExpression": function(node) {

Please can you add a test for this.

See https://firefox-source-docs.mozilla.org/tools/lint/linters/eslint-plugin-mozilla.html#tests for more info.
Attachment #8949487 - Flags: review?(standard8) → review-
Comment on attachment 8949488 [details]
Bug 1436575 - Autofix errors from no-compare-against-boolean-literal.

https://reviewboard.mozilla.org/r/218808/#review225178
Comment on attachment 8949488 [details]
Bug 1436575 - Autofix errors from no-compare-against-boolean-literal.

https://reviewboard.mozilla.org/r/218808/#review225180
Attachment #8949488 - Flags: review?(standard8) → review+
Comment on attachment 8949489 [details]
Bug 1436575 - Clean up some of the autofix changes from no-compare-against-boolean-literal.

https://reviewboard.mozilla.org/r/218810/#review225182
Attachment #8949489 - Flags: review?(standard8) → review+
Comment on attachment 8949490 [details]
Bug 1436575 - Manually fix the errors from no-compare-against-boolean-literal that the autofix couldn't change.

https://reviewboard.mozilla.org/r/218812/#review225184
Attachment #8949490 - Flags: review?(standard8) → review+
Comment on attachment 8949767 [details]
Bug 1436575 - Add exceptions for cases that cannot be simplified by negation or !! coercion.

https://reviewboard.mozilla.org/r/219070/#review225186
Attachment #8949767 - Flags: review?(standard8) → review+
Comment on attachment 8949524 [details]
Bug 1436575 - Remove autofix from rule since it is not reliable in all cases.

https://reviewboard.mozilla.org/r/218878/#review225188
Attachment #8949524 - Flags: review?(standard8) → review+
Comment on attachment 8949487 [details]
Bug 1436575 - Prevent ==/!= comparisons with booleans as they aren't necessary.

https://reviewboard.mozilla.org/r/218806/#review225190

::: commit-message-0ac95:1
(Diff revision 1)
> +Bug 1436575 - Prevent ==/!= comparisons with booleans as they aren't necessary. r?standard8

Looks like we also need to fix the issues referenced on the try server...

::: tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js:173
(Diff revision 1)
>      "max-nested-callbacks": ["error", 10],
>  
>      "mozilla/avoid-removeChild": "error",
>      "mozilla/import-browser-window-globals": "error",
>      "mozilla/import-globals": "error",
> +    "mozilla/no-compare-against-boolean-literals": "error",

Please can you also bump the version numbers in tools/lint/eslint/eslint-plugin-mozilla/package.json & package-lock.json - that will let me publish this easily once this lands.
Comment on attachment 8949487 [details]
Bug 1436575 - Prevent ==/!= comparisons with booleans as they aren't necessary.

https://reviewboard.mozilla.org/r/218806/#review225190

> Looks like we also need to fix the issues referenced on the try server...

Tests are green on my most recent push,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0c840d40bb4f76c87b1471eea87217a30c8c89f

There are failures in Linux x64 Stylo Disabled debug bc14 on browser/base/content/test/general/browser_bug386835.js but that test passes for me locally in my Ubuntu 64-bit debug stylo-disabled build so I think this is intermittent and an existing issue.
Comment on attachment 8949487 [details]
Bug 1436575 - Prevent ==/!= comparisons with booleans as they aren't necessary.

https://reviewboard.mozilla.org/r/218806/#review226014
Attachment #8949487 - Flags: review?(standard8) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fe9d1e9a56192d89268f5b182dcc7d5733f1ed97 -d 8eb7b1aa7dde: rebasing 447434:fe9d1e9a5619 "Bug 1436575 - Prevent ==/!= comparisons with booleans as they aren't necessary. r=standard8"
rebasing 447435:9e15cde0eef8 "Bug 1436575 - Autofix errors from no-compare-against-boolean-literal. r=standard8"
merging browser/components/enterprisepolicies/Policies.jsm
merging browser/components/preferences/in-content/main.js
rebasing 447436:75e27f88f50c "Bug 1436575 - Clean up some of the autofix changes from no-compare-against-boolean-literal. r=standard8"
rebasing 447437:2672df4067fd "Bug 1436575 - Manually fix the errors from no-compare-against-boolean-literal that the autofix couldn't change. r=standard8"
merging browser/components/enterprisepolicies/Policies.jsm
merging security/sandbox/test/browser_content_sandbox_fs.js
merging toolkit/content/widgets/dialog.xml
warning: conflicts while merging security/sandbox/test/browser_content_sandbox_fs.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ee5aabe89ef
Prevent ==/!= comparisons with booleans as they aren't necessary. r=standard8
https://hg.mozilla.org/integration/autoland/rev/89f7592e2b46
Autofix errors from no-compare-against-boolean-literal. r=standard8
https://hg.mozilla.org/integration/autoland/rev/7234fe36aac2
Clean up some of the autofix changes from no-compare-against-boolean-literal. r=standard8
https://hg.mozilla.org/integration/autoland/rev/a40adad0e66d
Manually fix the errors from no-compare-against-boolean-literal that the autofix couldn't change. r=standard8
https://hg.mozilla.org/integration/autoland/rev/9439baa123a9
Add exceptions for cases that cannot be simplified by negation or !! coercion. r=standard8
https://hg.mozilla.org/integration/autoland/rev/2d1f497a1fff
Remove autofix from rule since it is not reliable in all cases. r=standard8
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.