[restrict-template-expressions] option to ignore for console.log and similar methods

[justingrant]

I just upgraded typescript-eslint and got many new restrict-template-expressions warnings, almost all of which were in console.log messages or other similar contexts where the warning was a false positive. In logging code, logging undefined/null as "undefined" or "null" is almost always the expected result, but in normal code the reverse is true.

Therefore, I think this is a useful rule for "normal" code where strings are being constructed for showing to users, but if 99% of the time it's a false positive on log statements then it's hard to justify leaving it active in my projects.

Could typescript-eslint offer a way to automatically ignore this rule when the template strings are being constructed as parameters to console.log? Ideally, I could add a whitelist of other methods (e.g. custom loggers) whose parameters would also be ignored by this rule.

I assume that the hard part would be uniquely identifying methods. IMHO I think it's OK to use dumb regex matching by the name used in the code, e.g. methodName: [/\.log$/, /myCoolLogger/] which puts the onus on developers to add any aliases. A default could be [/\.log$/, [/\.warn$/, [/\.info$/] or maybe just [/console\.$/].

Repro

{
  "rules": {
    "@typescript-eslint/restrict-template-expressions": ["warn", { "allowNumber": true }]
  }
}

const foo = Math.random() < 0.5 ? 1 : undefined;
console.log(`foo is: ${foo}`);

Expected Result
There's some way for me to tell ESLint that some functions like console.log should allow undefined or null values-- if only because a big reason for logging is to report on unusual/unexpected values!

Actual Result

Invalid type "1 | undefined" of template literal expression.
eslint@typescript-eslint/restrict-template-expressions

Additional Info
n/a

Versions

package	version
@typescript-eslint/eslint-plugin	3.4.0
@typescript-eslint/parser	3.4.0
TypeScript	3.9.5
ESLint	7.3.1
node	14.4.0
npm	6.14.5

[leethree]

This would be very helpful!

[OmgImAlexis]

Actually I think this comes down to more of how console should be used.
I don't actually see much of a reason to pass in a template sting when format specifiers exist.

Your example can be changed to this.

const foo = Math.random() < 0.5 ? 1 : undefined;
console.log('foo is: %s', foo);

[bradzacher]

I would agree.
Looking at this now - I don't think this is worthwhile when you can either use the format string version of console.log like you mentioned, or you can just add each as a separate argument and it'll log each one with a space.

the benefit of the latter is that each arg will be printed using the native formatter without worring about % commands.

[justingrant]

@bradzacher - Do you think that using template strings to log possibly-undefined values is a bad practice and should be avoided? Or do you think this pattern is fine, but it's not worth accommodating in ESLint because developers can work around false positive eslint warnings by changing their code to use to the multi-argument syntax?

IMHO this rule is useful to catch subtle bugs in template strings, but if this (or any!) ESLint rule overwhelmingly reports false positive warnings, many developers will probably turn it off. This deprives the ecosystem of the benefit of the rule being enabled on most code. This seems like a bad outcome for a good rule.

Actually I think this comes down to more of how console should be used.
I don't actually see much of a reason to pass in a template sting when format specifiers exist.

@OmgImAlexis - My understanding is that template strings have a few advantages over multiple-argument usage:

• %s is fragile because writing and editing expressions using %s requires making changes in two places, so it's easy for them to get out of sync and also harder to understand what the code is doing. This brought up a flashback from my C days where mismatched sprintf arguments were a painfully common bug. ;-)
• The non-%s multiple-argument form doesn't have that mismatching problem which is good, but it's also not a very well-known pattern either. So it may not be present in existing code or in the minds of existing developers. Also, if you want to use a non-space delimiter (or no delimiters) for a particular log message, then you're out of luck.
• Using concatenation patterns that are unique to logging code makes it harder to copy/paste code between logging and non-logging code. Template strings don't have this problem.
• (This one is more of a personal preference) I find template strings easier to read and write than having to mentally string together multiple arguments. Perhaps this is because most modern code that does string concat will use template strings, so it's more familiar?

[leethree]

template strings are easy to read and understand. it's not just used in console.log but also custom loggers. Besides, I don't think there is an industrial standard for "how console should be used."

This deprives the ecosystem of the benefit of the rule being enabled on most code. This seems like a bad outcome for a good rule.

fully agreed.

[bradzacher]

The problem is that block lists are a bad way of configuring type-aware lint rules.

I've mentioned this countless times in issues in this repo.

A free text blocklist lacks any context, so it's easy to make it too wide.

For example, if you declare something like methodName: ['log'], then this will match any method called log.

Which means it'll match Math.log, which we all know is NOT a logging function. But the lack of context is what causes an incorrect match. Obviously Math.log is not a method that accepts a string, but this is just a simple example.

A blocklist that falsely matches creates false negatives.
False negatives are an insidious thing. They erode a user's trust in linting tools.
They're much worse than false positives. False positives create noise that's annoying. Noise that's seen. Noise that can be grasped and understood. Noise that can be silenced.

False negatives are invisible. They lie in wait to cause a bug in production. Once discovered, the user asks "why didn't the linter catch this case?" followed by "what other cases have been missed?"
This makes the user second guess the linter, and trust it much less.

Sure - it's possible for us to build support in here specifically for console methods (it's a standard part of TS types, so a rule can interrogate the types and thus not false match).

But as mentioned - template strings will very easily cause you issues with many logging cases, so template strings are not a good way to do your logging. Which is why I would say that it's not a a false positive in the rule.

Would you rather have [object Object] in your production logs when you're investigating a bug all because you decided to ignore logging methods?
Or would you rather have a properly logged object because the rule reported correctly and you ensured it was stringified?

From my experience doing log investigations - you 110% want the latter.

[justingrant]

The problem is that block lists are a bad way of configuring type-aware lint rules.
(snip)
Sure - it's possible for us to build support in here specifically for console methods (it's a standard part of TS types, so a rule can interrogate the types and thus not false match).
(snip)
Would you rather have [object Object] in your production logs when you're investigating a bug all because you decided to ignore logging methods?
Or would you rather have a properly logged object because the rule reported correctly and you ensured it was stringified?

These concerns make sense. I'd be OK with override options that applied to console methods only, e.g.

{
  allowNumber: true,
  allowBoolean: false,
  allowAny: false,
  allowNullish: false,
  console: {
    allowBoolean: true,
    allowNullish: true,
  }
}

It isn't a perfect solution (e.g. won't work for custom loggers) but it'd get rid of 95%+ of my false positives which in turn would let me justify leaving this rule enabled for all code.

[bradzacher]

happy to accept a PR for that sort of very limited scope option.

probably something like this in case we decide to add more overrides in later:

{
  allowNumber: true,
  allowBoolean: false,
  allowAny: false,
  allowNullish: false,
  overrides: {
    console: {
      allowBoolean: true,
      allowNullish: true,
    }
  }
}

[monfera]

Linting rules toward structural typing become more and more elaborate, as they have a fractal nature. There's always more and more intricacies to consider. It's a misnomer if restrict-template-expressions has jurisdiction over console.log anyway. Also, it's perfectly OK for numbers etc. to appear in template strings.

It feels there's a growing imbalance between ever more granular ways of constraining legal, standardized JavaScript by non-standards informal efforts. Is it good to add a runtime ${String(myNumber)} call into the template to placate some static type checker? That's the default semantics. If this is deemed super useful, we ought to use a TS, compile-time annotation to indicate, "yes I know it is / can be a number and it's okay".

Meanwhile, assuming no elaborate branding setup, TS and its linters don't bat an eyelid when the code adds an angle in radians to an angle in degrees 🤷‍♂️