feat(rome_js_semantic): semantic read events for self invoking functions

[xunilrj]

trafficstars

Summary

Expands https://github.com/rome/tools/issues/2979 with correctly not flagging self-invoking functions as unused.

I don't consider this implementation complete yet. But we need to think about how to deal with complex expressions in these cases.

Test Plan

> cargo test -p rome_js_analyze -- no_unused_variables

[cloudflare-workers-and-pages[bot]]

Deploying with    Cloudflare Pages

Latest commit:	b045137
Status:	✅  Deploy successful!
Preview URL:	https://8bc6fa60.tools-8rn.pages.dev
Branch Preview URL:	https://feature-no-unused-variable-s.tools-8rn.pages.dev

View logs

[github-actions[bot]]

Playground for commit 45c40d4

[MichaReiser]

Have you consider implementing such methods on JsAnyExpression and all expression types? I can see this to be useful for many other analysis too

("FOO") == "FOO"

[xunilrj]

Have you consider implementing such methods on JsAnyExpression and all expression types? I can see this to be useful for many other analysis too

Yes. But I don´t consider this implementation mature yet. I am still going to improve this a little bit inside the noUnusedVariables until I feel more confident with it.

For example, not sure what to do with the ternary operator yet.

(true?function f(){}:function g(){})();

f and g are used in this scenario. Currently the semantic model don´t have a way to model this.

[MichaReiser]

Have you consider implementing such methods on JsAnyExpression and all expression types? I can see this to be useful for many other analysis too

Yes. But I don´t consider this implementation mature yet. I am still going to improve this a little bit inside the noUnusedVariables until I feel more confident with it.

For example, not sure what to do with the ternary operator yet.

(true?function f(){}:function g(){})();

f and g are used in this scenario. Currently the semantic model don´t have a way to model this.

In my view, this is a slightly different use case that also comes at a higher cost which is why I would model this as a resolve_value function that tries to resolve the expression to a value if it can or yields an Err if it isn't possible to resolve the value.

[MichaReiser]

So I've been thinking about this, and in the end I'm not really sure this is actually a false positive. Conceptually the noUnusedVariable rule (and the idea of variable usage in the semantic model in general) is about the usage of names and symbols, not about their concrete value. So in the case of (function f(){})() the rule is actually correctly raising a diagnostic because the name f is never used in the entire document, what's incorrect is how the error is explained to the user. I think it would help to have a suggested fix that removes the id of the function expression, but that's not a safe change to apply since it has visible side-effects (it changes the name property of the resulting function, which might be used for debugging purpose like in React.memo(function Component() {}) for instance)

This is an excellent point and, in my view, reason to remove FunctionExpression and ClassExpression from the noUnusedVariable rule because these expressions not only bind a name but also have a value.

Unused expressions should instead be reported by a noUnreachable rule (which likely need to handle FunctionExpression and ClassExpressions with names differently.

[ematipico]

We actually have an example in our codebase

https://github.com/rome/tools/blob/main/website/playground/src/MermaidGraph.tsx#L7-L20