no-useless-undefined - should not be fixable

[mmkal]

As it stands, the rule corrects console.log(undefined) to console.log(). These have different runtime behaviour, so it isn't safe to autofix. The fixer should be downgraded to a suggestion.

[sindresorhus]

I suggest we just ignore console.* methods.

[fisker]

Why console.log(undefined)? I can't image a scenario.

[mmkal]

There are many functions that behave differently when they receive an explicit undefined. console.log was just a simple way of showing that. Here's a contrived example of how this could break a program:

const logResultsAndExit = (...args: unknown[]) => {
  if (args.length === 0) {
    throw Error('missing args')
  }
  console.log(...args)
  process.exit()
}

// Before fix
logResultsAndExit(undefined) // works

// After fix
logResultsAndExit() // throws at runtime

A more likely example, with jest:

expect(someValue).toEqual(undefined)

The above works ok without the rule. With it, it'll be fixed to expect(someValue).toEqual() which is a compiler error. If the fix becomes becomes a suggestion, I can look at it, and decide for myself it's better to change it to expect(someValue).toBeUndefined() , but there's no way the no-useless-undefined could know about that particular jest feature.

I accept that it's usually not a good design, but 1) it's not always in our control - maybe we didn't write the function, we're just using it and 2) that's why there's a lint rule for this. It's ok to discourage accidental reliance on bad design, and even to suggest a fix, but it's not ok to change the runtime behaviour with an auto-fix.

[fisker]

The jest one already fixed in #758.

The arguments length case need discuss.

[mmkal]

#758 looks like a fine workaround for jest. But it won't be possible to whitelist all legitimate/necessary usages of explicit undefined.

The simplest way I can put it is - the rule is currently going against this guidance from the eslint docs:

1. Avoid any fixes that could change the runtime behavior of code and cause it to stop working.

[fisker]

As far as I can tell, almost every auto-fix breaks some edge cases.

[mmkal]

I don't think that's true. And if they do, they should be reported as bugs and fixed! This is exactly what the suggest API is for, so I don't really understand the resistance. A styleguide rule should not be able to silently introduce subtle rutime-only bugs into a program.

[fisker]

Simple, any code changes inside function effect Function#toString(). Maybe you'll say nobody use Function#toString().

Take some of our rules,

• better-regex breaks use of regex.source
• escape-case breaks use of String.raw
• prefer-spread breaks foo.concat(bar), if bar has Symbol.isConcatSpreadable

[mmkal]

Fair enough - I suppose there is a threshold of what an edge case is. Relevant xkcd:

I still think this rule is making too many assumptions about the runtime implementation of the function. It relies on the function being called not doing something strange, which to me is dangerous.

[fisker]

The arguments length case need discuss.

[papb]

But it won't be possible to whitelist all legitimate/necessary usages of explicit undefined.

I agree with this, I think using a whitelist is risky. Since I can't think of a better option, I am also in favor of downgrading to a suggestion.

Of course, the whitelist could be kept, in order to not even suggest in the known situations and improve developer experience.

[sindresorhus]

but it's not ok to change the runtime behaviour with an auto-fix.

That's just unrealistic in practice. Yes, it's something to strive for, but if we were to do this 100%, I don't think any rule could have an auto-fix as there are always edge-cases that are impossible to statically detect.

[mmkal]

Fair enough that we can't expect to succeed in never making runtime behaviour changes. But in this case, I think there's a pretty reasonable chance that automatically deleting a parameter will have a real side-effect.

Which is related to another reason this shouldn't auto-fix IMO: unlike most autofix rules like prettier/prettier, indent, no-semi, etc., if I write doSomething(undefined), it's fairly likely that I did it on purpose, specifically because I didn't want doSomething() for whatever reason. The rule is autofixing to the more obvious overload. The fact that I wrote doSomething(undefined) is a signal that doSomething is a weird function that behaves differently based on the number of args - at least the probability is much higher than for the average function call.

So while it's an edge case for the set of all function calls that undefined is "useful", when this rule is actually triggered, it's much more likely to be significant. It's a good thing that the linter is flagging unusual and potentially dangerous code, but it's a bad thing that it's blindly fixing it, potentially without the developer noticing. In my case, I updated this package in a medium sized repo, and there were a bunch of lint changes. Included were a small number of false positive fixes for this rule that got silently changed by yarn lint --fix. They might have slipped in unnoticed if there wasn't proper test coverage. I would have preferred to handle them manually, since then I could have given proper thought to whether some refactoring/redesign was needed.

Anyway, I'll shut up now - it's not that big a deal 🙃

[papb]

I think @mmkal is right... I think it is very likely that someone wrote foo(undefined) in purpose because foo() would have different behavior.

[sindresorhus]

I think it is very likely that someone wrote foo(undefined) in purpose because foo() would have different behavior.

In this specific example, I agree too.

[fisker]

What do you prefer?

• an option to ignore CallExpression
• a configurable list

Or better solution?

---

original post in #807 (comment)

[sindresorhus]

Yes, an option to ignore function parameters.