+
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

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

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Aug 16, 2022

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

> cargo test -p rome_js_analyze -- no_unused_variables

@xunilrj xunilrj requested a review from leops as a code owner August 16, 2022 21:43
@xunilrj xunilrj temporarily deployed to aws August 16, 2022 21:43 Inactive
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 16, 2022

Deploying with  Cloudflare Pages  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
Copy link

github-actions bot commented Aug 16, 2022

Some(JsAnyExpression::JsSequenceExpression(e)) => {
expr = e.right().ok();
}
Some(JsAnyExpression::JsAssignmentExpression(e)) => {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@MichaReiser
Copy link
Contributor

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
Copy link
Contributor Author

xunilrj commented Aug 17, 2022

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.

@xunilrj xunilrj temporarily deployed to aws August 17, 2022 07:49 Inactive
@MichaReiser
Copy link
Contributor

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.

Copy link
Contributor

@leops leops left a 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)

@MichaReiser
Copy link
Contributor

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
Copy link
Contributor

We actually have an example in our codebase

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

@xunilrj xunilrj mentioned this pull request Aug 19, 2022
@xunilrj xunilrj force-pushed the feature/no-unused-variable-self-invoking branch from 3e4c260 to b045137 Compare August 22, 2022 12:31
@xunilrj xunilrj temporarily deployed to aws August 22, 2022 12:31 Inactive
@xunilrj xunilrj merged commit f61d542 into main Aug 22, 2022
@xunilrj xunilrj deleted the feature/no-unused-variable-self-invoking branch August 22, 2022 14:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载