feat(rome_js_analyze): improvements to noUnusedVariables to consider catch, typescript and export

[xunilrj]

Summary

This expands #2979 with two extra cases:

Now we recommend removing the whole catch declaration:

warning[js/noUnusedVariables]: This variable is unused.
   ┌─ noUnusedVariables.js:39:10
   │
39 │ } catch (e) {}
   │          -
Suggested fix: Remove this variable.
      | @@ -36,4 +36,4 @@
35 35 |   };
36 36 |   
37 37 |   try {
38    | - } catch (e) {}
   38 | + } catch {}

and we don´t generate false positives to interfaces and abstract methods on Typescript and exported functions.

Test Plan

cargo run -p rome_cli -- check crates/rome_js_analyze/tests/specs/js/noUnusedVariablesTypescript.ts

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	1e50f87
Status:	✅  Deploy successful!
Preview URL:	https://a66bd4b0.tools-8rn.pages.dev
Branch Preview URL:	https://feature-no-unused-variable-i.tools-8rn.pages.dev

View logs

[ematipico]

Now that we have multiple files for the same rule, I think it will work better for to create a folder called noUnusedVariables/ and have there the files? We already do this for some other rule

[github-actions]

Playground for commit a524444

[xunilrj]

Now that we have multiple files for the same rule, I think it will work better for to create a folder called noUnusedVariables/ and have there the files? We already do this for some other rule

Done. But now we strangely have a "noUnusedVariables" group. Not so sure I like that.

[ematipico]

Now that we have multiple files for the same rule, I think it will work better for to create a folder called noUnusedVariables/ and have there the files? We already do this for some other rule

Done. But now we strangely have a "noUnusedVariables" group. Not so sure I like that.

Ah, I am not sure how things are set up, but I was referring to something like this:
https://github.com/rome/tools/tree/main/crates/rome_js_analyze/tests/specs/js/noDeadCode

[leops]

Should this check be moved to the semantic model builder ? It could make sense to consider that identifiers in TypeScript declarations do not create a new binding in the semantic model

[xunilrj]

Yes. It will as an "is_used" query. Or something like that.

[ematipico]

@xunilrj could you please re-open the issue or create new issues to list what we are missing in order to make the rule stable?

[ematipico]

We should either choose Always or MaybeIncorrect. Using Unspecified means that its applicability is unknown, which is not the case. Since the rule is still unstable, we should choose MaybeIncorrect

[xunilrj]

Done.

[ematipico]

Out of curiosity, is this expected? The no_unused_variables/ part. (it should be js)

[xunilrj]

hum... I think this is part of the confusion with "groups". I will fix it.

[xunilrj]

Done

[ematipico]

A suggestion: maybe we should not recommend this rule for the time being?

[leops]

A suggestion: maybe we should not recommend this rule for the time being?

This is actually a very interesting issue: our lint rules are unstable, and some of them have a number of false positive issues due to the complexity of statically analyzing JavaScript code.
On one hand it's tempting to remove these rules from the recommended set since it largely degrades the initial experience of people trying out the toolchain, even if we advertise the linter as unstable.
But people encountering these bugs also generates valuable feedback on bugs that we actually need to fix, and by disabling these rules by default I think we risk having these rules be indefinitely unstable because they're only actually enabled by a handful of users, and we do not have enough information on how the rule actually performs "in the wild" to decide that a rule is stable enough to be enabled by default.

[ematipico]

Fortunately people can turn off rules via configuration now, so there's a good workaround in case of errors, and people won't be blocked. Less pressure for fixes. So yeah, I think it's worth turn them on and fix possible errors.