Implement most of the Eslint plugin promise #4252

jelly · 2024-07-14T14:33:00Z

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.

graphite-app · 2024-07-14T14:33:05Z

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.

codspeed-hq · 2024-07-14T14:41:49Z

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 · 2024-07-14T16:00:16Z

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 · 2024-07-14T16:16:15Z

crates/oxc_linter/src/rules/promise/avoid_new.rs

+            return;
+        };
+
+        if ident.name == "Promise" && ctx.semantic().is_reference_to_global_variable(ident) {


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?

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

jelly · 2024-07-14T16:19:52Z

crates/oxc_linter/src/rules/promise/valid_params.rs

+        return true;
+    }
+
+    // foo().then(foo => {}), needed?


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

jelly · 2024-07-14T16:20:13Z

crates/oxc_linter/src/rules/promise/valid_params.rs

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


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

jelly · 2024-07-14T16:20:46Z

crates/oxc_linter/src/ast_util.rs

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


Move to utils::promise instead.

jelly · 2024-07-14T16:21:45Z

crates/oxc_linter/src/rules/promise/param_names.rs

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


is_empty() instead?

jelly · 2024-07-14T21:19:04Z

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 · 2024-07-15T01:21:21Z

@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 · 2024-07-15T01:22:32Z

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

rzvxa · 2024-07-15T01:54:49Z

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.

Rule Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/avoid-new.md)

Rule Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-new-statics.md)

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/param-names.md)

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/valid-params.md)

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-return-in-finally.md)

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-return-wrap.md)

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-promise-in-callback.md)

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/prefer-await-to-then.md)

Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-callback-in-promise.md)

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/catch-or-return.md)

jelly · 2024-07-16T10:21:23Z

Split off the easy part in #4293

…ics, params-names (#4293) This introduces the `eslint-plugin-promise` plugin and implements three relatively simple rules. Split off from #4252

…prefer-await-to-then` rule (#4318) Part of #4252 --------- Co-authored-by: wenzhe <mysteryven@gmail.com>

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

jelly force-pushed the eslint-plugin-promise branch from a3e0f0a to 45b57c0 Compare July 14, 2024 14:36

Boshen requested review from mysteryven and DonIsaac July 14, 2024 14:54

jelly force-pushed the eslint-plugin-promise branch from 99c861c to 8975004 Compare July 14, 2024 15:59

jelly marked this pull request as draft July 14, 2024 16:00

jelly force-pushed the eslint-plugin-promise branch from 8975004 to dec5ba5 Compare July 14, 2024 16:08

jelly commented Jul 14, 2024

View reviewed changes

jelly added 10 commits July 16, 2024 12:10

feat(tasks/rulegen): support creating eslint-plugin-promise rules

aaecb33

feat(linter/eslint-plugin-promise): implement avoid-new

f7b004f

Rule Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/avoid-new.md)

feat(linter/eslint-plugin-promise): implement no-new-statics

0d4d3a4

Rule Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-new-statics.md)

feat(linter/eslint-plugin-promise): implement param-names

38883c0

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/param-names.md)

feat(linter/eslint-plugin-promise): implement valid-params

9d0ba51

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/valid-params.md)

refactor(linter): make is_promise re-usable for other future rules

271bbec

feat(linter/eslint-plugin-promise): implement no-return-in-finally

8245726

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-return-in-finally.md)

feat(linter/eslint-plugin-promise): implement no-return-wrap

65cf0ed

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-return-wrap.md)

feat(linter/eslint-plugin-promise): implement no-promise-in-callback

86c8dfa

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-promise-in-callback.md)

feat(eslint-plugin-promise): implement prefer-await-to-then

ffa143c

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/prefer-await-to-then.md)

jelly added 3 commits July 16, 2024 12:13

refactor(linter): introduce promise utility module

13510f3

feat(linter/eslint-plugin-promise): implement no-callback-in-promise

1ffd302

Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-callback-in-promise.md)

feat(linter/eslint-plugin-promise): implement catch-or-return

c01a85d

Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/catch-or-return.md)

jelly force-pushed the eslint-plugin-promise branch from 44af4c5 to c01a85d Compare July 16, 2024 10:13

jelly mentioned this pull request Jul 16, 2024

feat(linter): Add eslint-plugin-promise rules: avoid-new, no-new-statics, params-names #4293

Merged

jelly mentioned this pull request Jul 17, 2024

feat(linter): implement eslint-plugin-promise/no-return-in-finally, prefer-await-to-then rule #4318

Merged

mysteryven added a commit that referenced this pull request Aug 10, 2024

feat(linter): implement `eslint-plugin-promise/no-return-in-finally, …

5992b75

…prefer-await-to-then` rule (#4318) Part of #4252 --------- Co-authored-by: wenzhe <mysteryven@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement most of the Eslint plugin promise #4252

Implement most of the Eslint plugin promise #4252

jelly commented Jul 14, 2024 •

edited

Loading

graphite-app bot commented Jul 14, 2024

codspeed-hq bot commented Jul 14, 2024 •

edited

Loading

jelly commented Jul 14, 2024

jelly Jul 14, 2024

jelly Jul 14, 2024

jelly Jul 14, 2024

jelly Jul 14, 2024

jelly Jul 14, 2024

jelly Jul 14, 2024

jelly commented Jul 14, 2024

DonIsaac commented Jul 15, 2024

DonIsaac commented Jul 15, 2024

rzvxa commented Jul 15, 2024

jelly commented Jul 16, 2024

Implement most of the Eslint plugin promise #4252

Are you sure you want to change the base?

Implement most of the Eslint plugin promise #4252

Conversation

jelly commented Jul 14, 2024 • edited Loading

graphite-app bot commented Jul 14, 2024

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

codspeed-hq bot commented Jul 14, 2024 • edited Loading

CodSpeed Performance Report

Merging #4252 will degrade performances by 3.97%

Summary

Benchmarks breakdown

jelly commented Jul 14, 2024

jelly Jul 14, 2024

Choose a reason for hiding this comment

jelly Jul 14, 2024

Choose a reason for hiding this comment

jelly Jul 14, 2024

Choose a reason for hiding this comment

jelly Jul 14, 2024

Choose a reason for hiding this comment

jelly Jul 14, 2024

Choose a reason for hiding this comment

jelly Jul 14, 2024

Choose a reason for hiding this comment

jelly commented Jul 14, 2024

DonIsaac commented Jul 15, 2024

DonIsaac commented Jul 15, 2024

rzvxa commented Jul 15, 2024

jelly commented Jul 16, 2024

jelly commented Jul 14, 2024 •

edited

Loading

codspeed-hq bot commented Jul 14, 2024 •

edited

Loading