fix(rome_js_analyze): only emit control flow side-effects for variable declarators with an initializer

[leops]

Summary

This PR modifies how control flow instructions are emitted for variable statements: instead of considering the entire statement as a side effect, each initializer expression now creates it own instruction. This means variable declarations without an initializer are not considered to have any side effects in terms of control flow, checking whether the variable itself it unused will be part of the semantic analysis

Fixes #2869

Test Plan

I've added additional test cases in JsVariableStatement.js to check the correct warnings are being emitted

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	82fef77
Status:	✅  Deploy successful!
Preview URL:	https://ac2b8165.tools-8rn.pages.dev
Branch Preview URL:	https://fix-variable-control-flow.tools-8rn.pages.dev

View logs

[github-actions]

Playground for commit 22c9cdd

[ematipico]

This case should throw a diagnostic, even when it gets assigned. Same for const.

Examples

[leops]

As I mentioned in the PR description, I expect a diagnostic for this case will be emitted by a future noUnusedVariable lint rule based on the semantic analysis instead. Any access to the variable before the return statement is invalid and will emit an error (since reading or writing a let variable before its declared is not permitted), while any access to the variable coming after the declaration will also be marked as unreachable and should be ignored by the noUnusedVariable rule. Since the variable will be marked as unused by the semantic analysis I wanted to avoid a duplicate diagnostic flagging it as unreachable again in the control flow analysis, so the control flow diagnostic only covers the initializer expression

[ematipico]

I thought you were just covering the var declarations, because also the let don't need an initializers. Thank you.

Although also const would be affected by the noUnusedVariable, because it requires a initializer, but they are not hoisted to the gloabl scope. Which means that we could have double diagnostics on the same line.

[xunilrj]

while any access to the variable coming after the declaration will also be marked as unreachable and should be ignored by the noUnusedVariable rule.

Do we want that? This would imply a rule that needs the SemanticModeland the CFG. This means that we need to check if every reference is reachable or not.

[leops]

Which means that we could have double diagnostics on the same line.

There would indeed be two diagnostics on the same line but they would not necessarily overlap: noDeadCode will only highlight the initializer expression, and noUnusedVariable could highlight only the identifier in the declaration:

const UNUSED = "value";
      ------   ------- noDeadCode
      |
      noUnusedVariable

Do we want that? This would imply a rule that needs the SemanticModeland the CFG. This means that we need to check if every reference is reachable or not.

I don't think it's a requirement for the initial version of noUnusedVariable but it does seem like something we'll want to have eventually (and an good use case to exercise the capability of the analyzer infrastructure to orchestrate the execution of rules to fulfill complex query requirements)

[xunilrj]

I was expecting this PR to do something like:

warning[noDeadCode]: This code is unreachable
   ┌─ JsVariableStatement.js:29:21
   │
27 │     return;
   │     ------- This statement will return from the function ...
28 │     var variable1 = initializer(),
   │                     ------------- ... before it can reach this code
             variable2,
29 │         variable3 = initializer();
   │                     ------------- ... before it can reach this code

[leops]

This is due to a limitation in the Language Server Protocol: when the diagnostic is displayed in the editor only the primary range will be rendered as "unnecessary", and since diagnostics can only have a single primary label the linter needs to emit a new diagnostic for each non-contiguous range of code that's unreachable