prefer-string-starts-ends-with: incorrect autofix when string does not exist

[bmish]

Problem

If the string variable is null or undefined, the autofix will cause an exception.

Before autofix:

/^en/.test(lang)
// Returns `false` if `lang` is `undefined` / `null`

After autofix:

lang.startsWith('en')
// Uncaught TypeError: Cannot read property 'startsWith' of undefined

Fix

A year from now when we drop support for Node 12, we can use optional chaining to autofix this safely:

lang?.startsWith('en')

Until then, we have a few options:

1. Do nothing since the autofixer has been around or a year (prefer-starts-ends-with: Add auto-fix #711) and no one else has complained
2. Temporarily turn the autofixer into an automated suggestion until we can use the optional chaining autofix
3. Warn about this situation in the rule doc

[fisker]

I assume you mean lang.startsWith?.('en')

[bmish]

@fisker no actually. The situation here is that the string (lang) does not exist, not that the function does not exist.

[fisker]

Oh right. but if function didn't exists lang?.startsWith() still error (undefined is not a function).

[fisker]

Correct fix for this case should be

String(lang).startsWith('en')

[bmish]

@fisker that fix should work, although it's not the most elegant fix, since most of the time the string casting will be unnecessary and potentially cause the reader confusion (it's not clear that it's there to solve the problem of undefined/null). So we could use that but we should definitely follow-up to use optional chaining in a year.

[bmish]

Using (lang || '').startsWith('en') would actually make the fix's intent more clear. I think I may prefer this one.

[fisker]

No, you are wrong

/^un/.test(undefined)
true

String(undefined).startsWith('un')
true

(undefined || '').startsWith('un')
false

undefined?.startsWith('en')
undefined

[bmish]

Oh wow. I am surprised by the behavior with /^un/.test(undefined). Not intuitive, and probably not the intention of the original code, but technically your fix is correct.

[fisker]

Yap, if it's not string, it convert to string first, we can remove auto fix for it or use suggestions.

[fisker]

What if it's unknown value? /^x/.test(foo), people may don't want wrap it with String(), I think we can still assume it's string?

[bmish]

I'm kind of torn on this. Ideally, an autofix should be both correct and match the intent of the original code.

1. lang.startsWith('en') (today's autofix) -- matches intent, crashes on undefined, boolean result, not fully correct
2. lang?.startsWith('en') -- matches intent, doesn't crash on undefined, not necessarily a boolean result, not fully correct
3. (lang || '').startsWith('en') -- matches intent, doesn't crash on undefined, boolean result, not fully correct
4. String(lang).startsWith('en') -- doesn't match intent, redundant most of the time, boolean result, but fully correct

There's no perfect choice. I could see arguments for each.

One argument for keeping today's autofix is that it's the nicest fix and it will also fail-fast if someone uses undefined with it, so any issue should be detected quickly.

[fisker]

I prefer keep the current fix, but suggest String(foo).startsWith() for known non-string cases. Or suggest all those codes.

[fisker]

Like suggestions:

1. Fix to String(...).startsWith(...)
2. Fix to ...?.startsWith(...)
3. Fix to (... || '').startsWith(...)

[bmish]

Yeah I think we're agreed on keeping the current autofix and then providing all of these as suggestions.

[fisker]

Want work on it?

[bmish]

Sure I'll work on it.