valid-typeof does not report invalid comparisons to undefined

[not-an-aardvark]

What version of ESLint are you using? 3.1.0

What parser (default, Babel-ESLint, etc.) are you using? default

Please show your full configuration:

{
  "rules": {
    "valid-typeof": 2
  }
}

What did you do? Please include the actual source code causing the issue.

// oops, this was probably supposed to be the string 'undefined'
if (typeof window === undefined) {
  foo();
}

What did you expect to happen?

I expect ESLint to report an error, since the global undefined value is not a valid typeof comparison.

What actually happened? Please include the actual, raw output from ESLint.

ESLint did not detect any errors.

[ilyavolodin]

How should we handle the case like this:

var undefined = "undefined";
if (typeof window === undefined) {
  foo();
}

ESLint is a static analysis tool. We can't track values of variables, and undefined can be reassigned when using ES3.

[ljharb]

undefined can be redefined everywhere, except in the global scope in ES5+.

That said, eslint certainly should be able to track the values of variables in the same file, and couldn't var undefined, let undefined, or const undefined being present cause the requested warning to be skipped?

[not-an-aardvark]

Thanks for the explanation @ilyavolodin; I had forgotten that it was possible to redeclare undefined as a local variable.

Based on this table, it seems like all modern browsers don't allow undefined to be redeclared in the global scope. I'm unsure of whether ESLint supports any of the environments that do allow redeclaration. (If I'm understanding it correctly, @ljharb's solution only works if the global undefined value is immutable.)

[ilyavolodin]

Unfortunately, tracking value in JS is not quite that easy:

eval("var und" + "efined = \"undefined\""); //or a million other ways to set a value of a variable
if (typeof window === undefined) {
  foo();
}

We currently don't have a way to reliably track variable values. It's something we want to look into in the future, but nothing is currently available for this.

@not-an-aardvark We don't set requirements for JS support, there are plenty of sites that still have to support IE8 (or even IE6 in some cases) out there, so we can't assume that everyone is writing ES5+ (I know I don't, most of the time).

[ljharb]

@ilyavolodin would it not be reasonable to say that if a user is doing something like that, they are welcome to turn off the linter rule?

[not-an-aardvark]

@ilyavolodin Fair enough. While I understand your reasoning, it's unfortunate that we have to account for that, since I'm guessing statements such as typeof x === undefined are errors much more often than not.

[ilyavolodin]

@ljharb That's one way to go, but there are much simpler cases too:

var undefined = require('./my-file');

In any case, while we can track variable declarations currently through Escope, it doesn't track variable values. As I said, that's something we want to look into in the future. For now we just don't have the tools required to do this.

[ljharb]

I think this would still be useful, even if there exists some edge cases it would be a false positive on. Anyone doing var undefined = is violating (what should be) another linter rule anyways :-p

eslint isn't just to catch errors, and most rules don't apply to every scenario, which is why they're configurable. If this was an option - especially defaulted to off - then any users who wish to redefine undefined would certainly want to leave that option disabled, and the rest of us can still benefit from it.

[ilyavolodin]

Very well, that's a valid argument. I marked this issue as evaluating and enhancement. Let's see what the team thinks.

[platinumazure]

I definitely think valid-typeof should enforce comparisons against string literals, since most of the time that is the intent. If this needs to be behind an option to avoid breaking existing behavior, I would be fine with that, but I strongly believe it should be possible for users to restrict valid-typeof to compare against only literals containing the strings that the typeof operator can return.

And I would argue that this concern (wanting to compare against string) has nothing whatsoever to do with the value of undefined. I consider this equal to wanting to flag typeof foo === string (except that no-undef should catch the bad string variable). The whole point is that typeof returns a string and so users should compare it to a string literal (and ideally one of the known return values).

[nzakas]

I think the only way we can possibly do something that makes sense is to add an option like requireStringLiterals that ensures the RHS is always a string. Anything else is just guessing and not appropriate for this rule.

[not-an-aardvark]

Ping on this issue; is there enough support for it that a PR to add the requireStringLiterals option would be accepted?

[platinumazure]

I can get behind that option. 👍 from me.

[ilyavolodin]

I'll support an option for comparing to literals only. 👍

[platinumazure]

@ilyavolodin Literals === string literals, or any literals? Not much point in allowing comparisons to boolean or numeric literals, no?

[ilyavolodin]

Sorry, I meant string literals.

[vitorbal]

I'll 👍 a requireStringLiterals option too, but we still need a champion!

[btmills]

👍 I'll champion, marking as accepted! I'll review #6923 later today.