1359350 - Add no-eval rule to eslint config

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Frederik Braun [:freddy]]

+++ This bug was initially created as a clone of Bug #1358050 +++

We discourage the use of eval.
string arguments to setInterval/setTimeout are disallowed with 1358050.

I suggest we also add no-eval

Comment 1

[Frederik Braun [:freddy]]

FWIW, I misremembered that no-implied-eval included eval. but it was only setTimeout, setInterval, so here's the second round. Patch incoming.

Comment 2

[Frederik Braun [:freddy]]

Attachment: Bug 1359350: Add no-eval rule to eslint config (and whitelist failures in tests)

Review commit: https://reviewboard.mozilla.org/r/137092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137092/

Comment 3

[Frederik Braun [:freddy]]

Standard8 suggested I file bugs for the whitelisted failures and he will take some of them as mentored bugs.
Still looking into how to do that fast and automatically.

Comment 4

[Frederik Braun [:freddy]]

Comment on attachment 8865424 [details]
Bug 1359350: Add no-eval rule to eslint config (and whitelist failures in tests)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137092/diff/1-2/

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8865424 [details]
Bug 1359350: Add no-eval rule to eslint config (and whitelist failures in tests)

https://reviewboard.mozilla.org/r/137092/#review140152

Looks good. r=Standard8

For filing the bugs, I think it might be worth looking at grouping issues into various areas, e.g. just one bug for dom/indexedDB to cover fixing all the tests there as they have the same issue, etc.

::: .eslintrc.js:22
(Diff revision 2)
> -
>      // No (!foo in bar) or (!object instanceof Class)
>      "no-unsafe-negation": "error",
>      // No eval() and no strings in the first param of setTimeout or setInterval
>      "no-implied-eval": "error",
> +    "no-eval": "error",

Please can you also add this to tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js so that we have it there for when we apply it globally (and also for our github projects that use the plugin).

Whilst you're there, could you also bump the version number in tools/lint/eslint/eslint-plugin-mozilla/package.json to 0.2.47 - I really ought to do another release for that soon (generally it doesn't need bumping on each update of the plugin files now, but I've not done a release for a while).

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8865424 [details]
Bug 1359350: Add no-eval rule to eslint config (and whitelist failures in tests)

https://reviewboard.mozilla.org/r/137092/#review140154

Comment 7

[Frederik Braun [:freddy]]

Comment on attachment 8865424 [details]
Bug 1359350: Add no-eval rule to eslint config (and whitelist failures in tests)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137092/diff/2-3/

Comment 8

[Frederik Braun [:freddy]]

Thanks for the speedy review.
Waiting for try results before asking for checkin-needed: https://reviewboard.mozilla.org/r/137092/#comment181908

Comment 9

[Frederik Braun [:freddy]]

All green.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f71cd863be6e
Add no-eval rule to eslint config (and whitelist failures in tests) r=standard8

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f71cd863be6e