1870205 - Enable ESLint rule no-use-before-define across the tree

Status: NEW

(Developer Infrastructure :: Lint and Formatting, task, P3)

Description

[Tomislav Jovanovic :zombie]

https://eslint.org/docs/latest/rules/no-use-before-define

We should switch to variables: false as the default config for no-use-before-define. This setting still makes it an error to use variables from the same scope before they're defined, as well variables from a synchronous parent scope (while example below).

The only thing it allows is accessing a variable from inside the function, even when that function is defined before the variable. This is rarely a real error, and when it is, it's very obviously broken, because it means a function is being called (synchronously, directly) from the same scope it is defined in, but before the variables are defined.

/* eslint no-use-before-define: ["error", { "variables": false } ] */

let foo;

function f() {
  alert(foo + bar + baz);      // allowed
}

while (Math.random() < 0.5) {
  alert(foo + bar + baz);      // error
}

alert(foo + bar + baz);        // error

foo = 1;
var bar = 2;
let baz = 3;

f();

ESLint playground link.

The reason for changing: the workaround we use when we actually need to use a global variable from a function defined earlier is: just stick let foo; at the top of the file, which shuts up the linter, but provides false sense that this is somehow "safe". The natural way to do this is to declare and initialize a variable later in the file, which makes it more obvious that it might not be initialized if the function is called before the variable declaration (and using the var, which is fairly uncommon today, only reinforces that point).

I've never liked this pattern, but now we also have a good reason to get rid of it: TypeScript type inference works much better with the natural way of writing code. Object literals are fully typed when initialized during definition all at once.

(and btw, while we're changing the defaults, we should also use function: false, as functions are actually hoisted, and there's nothing wrong or unsafe with calling a function declared later in the file)

Comment 1

[Tomislav Jovanovic :zombie]

I forgot to emphasize, setting some or even all of the flags to false isn't equivalent to turning off the rule completely. This still reports errors for using foo and Baz, though obviously not for bar because that's always safe:

/* eslint no-use-before-define: ["error", { variables: false, functions: false, classes: false } ] */

alert(foo + bar() + new Baz());        // 2 errors

var foo = 1;

function bar() {}

class Baz {}

Playground link.

Comment 2

[Mark Banner (:standard8)]

no-use-before-define is only enabled in a few places. We can't change the defaults on the global scale, as we can't define them without defining the rule. That said, it looks like someone thought about enabling it at some time when we were setting up ESLint.

I had a brief play around last week with the definition that you'd put into this patch. There were a reasonable amount of cases across the tree. I'm fairly sure there were quite a few items that I'd refer to as false positives, especially relating to class usage.

However, I think this would be worth investigating more, to see if it is worth enabling the rule or not.

Comment 3

[Tomislav Jovanovic :zombie]

Oh I didn't realize this wasn't globally enable, thanks.

I guess since this is already a specialization, I'll go ahead and change it in extensions code for this experiment. My hunch is these proposed defaults are more likely to be viable for rollout across the whole tree anyway..

But in case a different global config is chosen, I'll be on the hook to fix extensions code to work with it, one way or the other.

Comment 4

[Mark Banner (:standard8)]

Discussing with :Gijs and :Mossop we think we should go with the rule definition that extensions have:

    "no-use-before-define": [
      "error",
      {
        allowNamedExports: true,
        classes: true,
        // The next two being false allows idiomatic patterns which are more
        // type-inference friendly.  Functions are hoisted, so this is safe.
        functions: false,
        // This flag is only meaningful for `var` declarations.
        // When false, it still disallows use-before-define in the same scope.
        // Since we only allow `var` at the global scope, this is no worse than
        // how we currently declare an uninitialized `let` at the top of file.
        variables: false,
      },
    ],

We agreed that it'd be good to roll this out across the tree, we could probably do it with good-first-bugs.