-
Notifications
You must be signed in to change notification settings - Fork 653
feat(rome_js_semantic): semantic read events for self invoking functions #3071
Conversation
Deploying with
|
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 |
Some(JsAnyExpression::JsSequenceExpression(e)) => { | ||
expr = e.right().ok(); | ||
} | ||
Some(JsAnyExpression::JsAssignmentExpression(e)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to add TsAsExpression
, TsTypeAssertionExpression
, TsNonNullAssertionExpression
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment below. It is still missing a lot of cases. Will try to improve it in other PRs.
Have you consider implementing such methods on
|
Yes. But I don´t consider this implementation mature yet. I am still going to improve this a little bit inside the For example, not sure what to do with the ternary operator yet. (true?function f(){}:function g(){})();
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Unused expressions should instead be reported by a |
We actually have an example in our codebase https://github.com/rome/tools/blob/main/website/playground/src/MermaidGraph.tsx#L7-L20 |
3e4c260
to
b045137
Compare
Summary
Expands #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