feat(lint): add rule noUselessLengthCheck

[GunseiKPaseri]

Summary

Implement noUselessLengthCheck(unicorn/no-useless-length-check)

closes #3941

Test Plan

• Added tests and snapshots

[codspeed-hq]

CodSpeed Performance Report

Merging #4060 will not alter performance

_{Comparing GunseiKPaseri:no-useless-length-check (b8e7493) with main (6c209ef)}

Summary

✅ 99 untouched benchmarks

[dyc3]

Looks good. The merge conflict can be fixed by rerunning codegen.

[Conaclos]

@GunseiKPaseri Could you add a changelog entry and resolve the conflicts?

[unvalley]

We can use <Emphasis> tag here to Array#every(), true, Array#some(), false

[unvalley]

looks good to me overall, left nits suggestions

[GunseiKPaseri]

It was difficult to determine whether it was an unsafe fix, but is it okay to leave it as an unsafe fix? Is there any way to make it a safe fix?

[ematipico]

I left an early review

[ematipico]

Two things to change in all the code:

• don't allocate a string, so don't use to_string there's no need. You can use syntax().text_trimmed()
• don't check the binary operations using the raw characters, use this utility https://github.com/biomejs/biome/blob/main/crates%2Fbiome_js_syntax%2Fsrc%2Fexpr_ext.rs#L259-L260

[ematipico]

Thank you for your contribution @GunseiKPaseri

This rule is quite complex, so we wouldn't want to urge the need to merge. There are a few concerns that should be addressed before:

• recursions: we should remove it and find an algorithm that puts less memory pressure
• complexity: I believe the code of this rule is too complex for what it should do, and it can be simplified. For example, I can't understand why we use a Vec<> for signals.
• documentation: since this rule requires a lot of code, you should strive to provide as much documentation as possible. We are picky on this front

[ematipico]

JsParenthesizedExpression can be tough to deal with; for example, I don't think we are able to catch these cases:

if (((((array.length === 0 || array.every(Boolean))))));

The type AnyJsExpression has a method called omit_parentheses, which returns the expression inside all the parenthesis. You should use that, and see if you can catch cases like the one I shared

[GunseiKPaseri]

This function returns whether the parent is a JsLogicalExpression. When the parent is a JsParenthesizedExpression, we want to continue searching up to its parent as well, so this function is called to check for that. Therefore, even with the current implementation, such cases can be detected. (However, since I have learned about the convenient omit_parentheses function, I would like to rewrite some parts using it. Thank you.)

[ematipico]

I find it difficult to understand the documentation; maybe you should provide an example. Perhaps because you call the second argument "error", you should see a better name, like the one I suggested.

Also, it would be nice to provide a code example in the docs.

[ematipico]

This solution isn't maintainable, because if one day we add a new variant to ErrorType, we need to tweak business logic.

The thing is, the rule knows it always needs to check for Array.every() and Array.some(), so I don't understand why we need to pass err_type as a parameter.

[GunseiKPaseri]

The naming might have been misleading. This actually refers to both &&・||and Some・Every, and it is intended to specify searching only one of them in the recursion. Since most of the code is the same, I consolidated it.

[ematipico]

This is a recursion, and when you want to avoid it. Usually, recursions are fine, but they aren't fit for working with trees, like an AST/CST.

You should refactor this code and try to use a simple loop, or maybe you could evaluate an iterator.

[GunseiKPaseri]

I want to search through the entire logical expression. However, I couldn't think of a method other than recursion to extract A, B, C from A || B || C or A, B, C, D from (A || B) || (C || D). If it encounters something other than a logical expression or a different operator, the implementation is designed to terminate the process. Therefore, I believe the search will not become heavier than the logical expression being passed. If there is a good utility for this, I would like to use it.

[ematipico]

There are too many arguments here, I think. But it's fine. However, you should document them and explain what they are. What's any_exp, for example? It's difficult to understand what it is, especially in relation to the name to search_local_exp. Maybe you want to give a better name, like expression_to_search.

Also, replacer is a bit cryptic. Usually, you should try to give your parameters a name based on its business logic, not on its type.

[ematipico]

I believe it's best to use a struct for the state, so you can document every field. As it is, it's difficult to understand what is it.

[ematipico]

Here's a generic comment based on the responses I had from the other comments.

I believe I understand more or less what you're trying to do; here's some advice based on my understanding of what you're trying to do.

---

When you submit a PR that has some heavy business logic, you should at least provide a technical explanation of how you solve a problem. This is important because it reduces the cognitive load of the reviewers. I understand that the solution is in your head, but the solution isn't in the head of the reviewers, so you need to explain your solution to the others. This happened before, so please try to follow it.

---

I still believe you don't need multiple signals, and still believe this rule can be simplified.

1. You're querying a JsBinaryExpression, and that's why it gets very complex. I think we should query JsCallExpression, check if it's a known function (Array.some and Array.every). Once we do that, we know in advance which operator we need to check. && -> Array.every and || -> Array.some. This removes the for loop we use at the very beginning. Plus, it will remove the recursion and the vector of signals.

   if (array.length === 0 && array.some(Boolean)) {}
   //	                      ^^^^^^^^^^^^^^^^ query this instead

   Then, navigate the ancestors, maybe using the semantic model to check that the array used inside a branch of the logical expression is the same as the one used in the other branch.

2. You assume that we need to search every logical expresssion branch and notify all violations, however I haven't managed to find a test where this is needed. If there is, please help me by providing a good description of the PR and maybe a good code example. Please don't use things like A || B || C because they don't help without an actual code example. This isn't what a rule needs to do. A rule, in the vast majority of cases, needs to find the first violation, and that's it. If you still believe we need to emit multiple signals from this one rule, then provide a code example that requires it.

3. The recursion can be fixed with an iterator. An iterator is better because it's lazy, so it doesn't put too much pressure on memory. You can just create a type that holds to the any_expr, which gets updated every time you call next().