Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement most of the Eslint plugin promise #4252

Closed
wants to merge 13 commits into from

Conversation

jelly
Copy link
Contributor

@jelly jelly commented Jul 14, 2024

One rule promise/param-names is part of the eslint-config-standard so would be useful for oxlint to support. But only supporting one rule seems a bit unreasonable so I looked at at least implementing a few of the rules which got out of hand fast....

So this PR implements 10 out of the 15 rules. Some rules lack the explanation why this is bad because such documentation does not exists, there are multiple issues about it eslint-community/eslint-plugin-promise#214 and eslint-community/eslint-plugin-promise#109

Some of the leftover rules are a bit debatable if they are useful such as no-native, disallowing native Promise implementations. And most folks seem to disable the rule.

Leftover rules:

  • always-return
  • no-multiple-resolved
  • no-nesting
  • prefer-await-to-callbacks
  • no-native

I am happy to split up this PR into multiple smaller PR's if this is too big to review.

Copy link

graphite-app bot commented Jul 14, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI labels Jul 14, 2024
Copy link

codspeed-hq bot commented Jul 14, 2024

CodSpeed Performance Report

Merging #4252 will degrade performances by 3.97%

Comparing jelly:eslint-plugin-promise (c01a85d) with main (c5731a5)

Summary

❌ 1 regressions
✅ 31 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main jelly:eslint-plugin-promise Change
linter[checker.ts] 1.4 s 1.5 s -3.97%

@jelly
Copy link
Contributor Author

jelly commented Jul 14, 2024

Ok, this PR is clearly way too big and I just discovered the ability to make a util::promise module which seems to be good to refactor a bunch of rules. Moving this into a draft and I'll split up some commits.

@jelly jelly marked this pull request as draft July 14, 2024 16:00
return;
};

if ident.name == "Promise" && ctx.semantic().is_reference_to_global_variable(ident) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering what is common in oxlint here, there is:

        let AstKind::NewExpression(new_expr) = node.kind() else {
            return;
        };
        if !new_expr.callee.is_specific_id("Array") {
            return;
        }

Which would also run for:

class Array {};

const list = new Array(5).map(_ => {});

So that seems like it should be refactored?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe we should get is_global_id("Array", ctx) which also checks if the global reference.

return true;
}

// foo().then(foo => {}), needed?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have no idea what case this handles :-)

if member_expr.object().is_specific_id("Promise")
&& matches!(
prop_name,
"resolve" | "reject" | "all" | "allSettled" | "race" | "any" | "withResolvers"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved to a utility, see previous rule which checks literally the same thing.

@@ -403,3 +403,34 @@ pub fn is_function_node(node: &AstNode) -> bool {
_ => false,
}
}

pub fn is_promise(call_expr: &CallExpression) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to utils::promise instead.


impl ParamNames {
fn check_parameter_names(&self, params: &FormalParameters, ctx: &LintContext) {
if params.items.len() < 1 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_empty() instead?

@jelly
Copy link
Contributor Author

jelly commented Jul 14, 2024

Two rules need to be branch aware: always-return no-multiple-resolved, which I am not sure if already implemented in oxint.

no-native doesn't feel very useful to implement, also mostly disabled from a github code search.

no-nesting prefer-await-to-callbacks, seem a bit complicated.

@DonIsaac
Copy link
Collaborator

@jelly we do, there's a control flow graph in Semantic. I'm not super familiar with the code though, @rzvxa is more familiar with it.

@DonIsaac
Copy link
Collaborator

@jelly I'll save my review for when you've split this PR up. Feel free to request one when you're ready.

@rzvxa
Copy link
Contributor

rzvxa commented Jul 15, 2024

Two rules need to be branch aware: always-return no-multiple-resolved, which I am not sure if already implemented in oxint.

Yes, we have a CFG. While it isn't documented, It's already used in some rules that you can study; Some of these rules are pretty similar to what you want for example getter_return and require_render_return(I suggest reading this one first since the former is in nursery and is a tad bit more complicated) should be really close to the always_return and can be used as a skeleton to build no_multiple_resolved on top of it.

To find all rules relying on CFG use the find-reference command of your LSP client on the LintContext::cfg method.

The graph itself uses petgraph and you can use its depth-first/breadth-first-search among other walk strategies, But for small subgraphs, I suggest our in-house search methods(which are more efficient on small subgraphs), You'd find them at crates/oxc_cfg/src/visit.rs.

Since we don't have a CFG visualization on our website(yet) you can create a test.js file in the root of the project - this is an ignored file - and run cargo run -p oxc_semantic --example cfg to generate semantic/CFG details as RON and DOT.

Then you can copy the content of the test.dot file into this webpage(or the DOT visualizer of your choice) to get a visualization of it.

Keep in mind the output of this dotfile is really dirty and is meant to be used for deep debugging purposes. To get a more simple dot output you should use DisplayDot(located at crates/oxc_cfg/src/dot.rs) and DebugDot(located at crates/oxc_semantic/src/dot.rs) traits. Those don't have an example that you can run on demand but it should be pretty trivial to set one up.

Since as of now I'm the only one who used our new CFG, I'm looking forward to your feedback and bug reports😅
Let me know if there is anything that I can help with, I'm always happy to chat. Just hit me up here or on Discord.

jelly added 10 commits July 16, 2024 12:10
@jelly
Copy link
Contributor Author

jelly commented Jul 16, 2024

Split off the easy part in #4293

mysteryven pushed a commit that referenced this pull request Jul 17, 2024
…ics, params-names (#4293)

This introduces the `eslint-plugin-promise` plugin and implements three
relatively simple rules.

Split off from #4252
mysteryven added a commit that referenced this pull request Aug 10, 2024
…prefer-await-to-then` rule (#4318)

Part of #4252

---------

Co-authored-by: wenzhe <[email protected]>
@mysteryven
Copy link
Contributor

mysteryven commented Oct 16, 2024

Seems there are only two rules not implemented(promise/no-promise-in-callback, promise/no-return-wrap), can we close this PR as a placeholder?

@jelly
Copy link
Contributor Author

jelly commented Oct 16, 2024

Sure!

@jelly jelly closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area - CLI A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants