[prefer-string-starts-ends-with] False positives when specific length is required

[pgsandstrom]

Repro

const test = (s1: string, s2: string) => s1.substring(2) === s2

This method is suggested to be rewritten with .endsWith(). However, that is not feasible. This function requires s1 to be exactly 2 characters longer than s2. That cannot be replicated with .endsWith().

Expected Result

There should be no error reported on the above function.

Actual Result

An error is reported.

Additional Info

I think one could say that the rule fails whenever there is a length requirement hidden in the code. For example, it makes the same error for this function, which requires s2 to be exactly two characters long to ever return true. Simply replacing it with endsWith() would remove all length requirements.

const anotherTest = (s1: string, s2: string) => s1.substring(s1.length - 2) === s2

Versions

package	version
@typescript-eslint/eslint-plugin	2.0.0
@typescript-eslint/parser	2.0.0
TypeScript	3.5.3
ESLint	6.2.1
node	12.0.0
npm	6.9.0

[bradzacher]

This is by design for the rule.
I think if you're intentionally not using endsWith, you should disable the lint rule with a comment explaining why you are using the implicit length check.

Personally, if I was reading the rule and saw s1.substring(2) === s2, my first thought wouldn't be "s2 needs to be exactly two characters longer than s1", it would instead be "s1 has a 2 character long prefix we don't care about".
From that IMO you should do an explicit length check with endsWith, so that your intention is clear.

[pgsandstrom]

While the function could be written in many (perhaps better) ways, I don't think my way of writing it is unreasonable or very uncommon, which is what should matter for the purpose of linting.

I think it's problematic that the rule makes a suggestion that would change the behaviour of the code. In some cases, a subtle bug will be introduced into the code. Wouldn't be reasonable that rules that are part of the recommended set is more conservative about things like this?

Of course all of this is subjective, I just wanted to give my two cents.

[bradzacher]

I do get what you're saying, but the rule is about the standardisation and performance benefits of using startsWith/endsWith.

If you're choosing not to use it for a specific reason, then that is why disable comments exist, so you can explain why you're violating the rule.

I don't think my way of writing it is unreasonable

Don't get me wrong, it's certainly not unreasonable.

In the interest of clarity, I would argue that it is not the best choice of code, because it relies upon an implicit assertion made by the method, instead of an explicit one. Implicit assertions aren't as clear as you might feel - I would consider myself a JS expert (~8yr experience with the language), yet I wouldn't have picked up on that implicit logic, because I've never seen it used that way before.

As an engineer, I much prefer explicit code. A few keystrokes can save a world of headaches later.

or very uncommon

I would contest this, if only because this is the first time someone has raised an issue with the rule's functionality in 4 months. We have ~1.4mil weekly downloads for the plugin, but this is the first time someone's brought up this usage. Though this could be very much a poor assumption on my behalf - the majority of our users could very well not this style of substring, without using the rule.

---

That being said, happy to accept a PR to add an option to this rule to disable this specific check, if you're so motivated.

[pgsandstrom]

Fixed in #1920.