1409693 - [eslint] Investigate turning on no-unused-expressions

Status: NEW

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

Description

[Mark Banner (:standard8)]

no-unused-expressions can catch a few issues in the code, that generally are bugs or could lead to strict warnings.

Although we do invoke various properties for side-effects intentionally, I think it is probably worth turning this on anyway, as it will highlight those.

As an initial stab, it seems we'll want this in recommended.js:

    "no-unused-expressions": ["error", {
      "allowShortCircuit": true,
      "allowTernary": true
    }],

Comment 1

[Mark Banner (:standard8)]

I took another look at this today. First an update config:

+    "no-unused-expressions": ["error", {
+      "allowShortCircuit": true,
+      "allowTaggedTemplates": true,
+      "allowTernary": true,
+    }],

With this, there's about 200 errors. The majority of these are false positives. There's a few actual issues which I'll file as blockers as I get time, though the more worrying bugs are already filed.

Of the false positives, there's a lot of places where we do `element.clientTop` or similar to force a layout flush or hooking up of xbl. For those we probably want to think about if we could have a utility function, e.g. forceFlush(element), so that this is explicit in the code, and then ESLint doesn't warn about it.

For other intentional things we could just do // eslint-disable-next-line no-unused-expressions or similar.

The few things we have caught make me think it is worth moving on this at some stage.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Recent example found by coverity: https://bugzilla.mozilla.org/show_bug.cgi?id=1662769
Probably saved some time for devs