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

feat(linter): implement prefer-await-to-callbacks #6153

Merged
merged 2 commits into from
Sep 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ mod promise {
pub mod no_new_statics;
pub mod no_return_in_finally;
pub mod param_names;
pub mod prefer_await_to_callbacks;
pub mod prefer_await_to_then;
pub mod spec_only;
pub mod valid_params;
Expand Down Expand Up @@ -760,6 +761,7 @@ oxc_macros::declare_all_lint_rules! {
promise::no_new_statics,
promise::no_return_in_finally,
promise::param_names,
promise::prefer_await_to_callbacks,
promise::prefer_await_to_then,
promise::spec_only,
promise::valid_params,
Expand Down
185 changes: 185 additions & 0 deletions crates/oxc_linter/src/rules/promise/prefer_await_to_callbacks.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
use oxc_ast::{
ast::{Argument, Expression, FormalParameters, MemberExpression},
AstKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_semantic::NodeId;
use oxc_span::{GetSpan, Span};

use crate::{context::LintContext, rule::Rule, AstNode};

fn prefer_await_to_callbacks(span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Prefer `async`/`await` to the callback pattern").with_label(span)
}

#[derive(Debug, Default, Clone)]
pub struct PreferAwaitToCallbacks;

declare_oxc_lint!(
/// ### What it does
///
/// The rule encourages the use of `async/await` for handling asynchronous code
/// instead of traditional callback functions. `async/await`, introduced in ES2017,
/// provides a clearer and more concise syntax for writing asynchronous code,
/// making it easier to read and maintain.
///
/// ### Why is this bad?
///
/// Using callbacks can lead to complex, nested structures known as "callback hell,"
/// which make code difficult to read and maintain. Additionally, error handling can
/// become cumbersome with callbacks, whereas `async/await` allows for more straightforward
/// try/catch blocks for managing errors.
///
/// ### Examples
///
/// Examples of **incorrect** code for this rule:
/// ```js
/// cb()
/// callback()
/// doSomething(arg, (err) => {})
/// function doSomethingElse(cb) {}
/// ```
///
/// Examples of **correct** code for this rule:
/// ```js
/// await doSomething(arg)
/// async function doSomethingElse() {}
/// function* generator() {
/// yield yieldValue(err => {})
/// }
/// eventEmitter.on('error', err => {})
/// ```
PreferAwaitToCallbacks,
style,
);

impl Rule for PreferAwaitToCallbacks {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
match node.kind() {
AstKind::CallExpression(expr) => {
let callee_name = expr.callee.get_identifier_reference().map(|id| id.name.as_str());
if matches!(callee_name, Some("callback" | "cb")) {
ctx.diagnostic(prefer_await_to_callbacks(expr.span));
return;
}

if let Some(last_arg) = expr.arguments.last() {
let args = match last_arg {
Argument::FunctionExpression(expr) => &expr.params,
Argument::ArrowFunctionExpression(expr) => &expr.params,
_ => return,
};

let callee_property_name = expr
.callee
.as_member_expression()
.and_then(MemberExpression::static_property_name);

if matches!(callee_property_name, Some("on" | "once")) {
return;
}

let is_lodash = expr.callee.as_member_expression().map_or(false, |mem_expr| {
matches!(mem_expr.object(), Expression::Identifier(id) if matches!(id.name.as_str(), "_" | "lodash" | "underscore"))
});

let calls_array_method = callee_property_name
.is_some_and(Self::is_array_method)
&& (expr.arguments.len() == 1 || (expr.arguments.len() == 2 && is_lodash));

let is_array_method =
callee_name.is_some_and(Self::is_array_method) && expr.arguments.len() == 2;

if calls_array_method || is_array_method {
return;
}

let Some(param) = args.items.first() else {
return;
};

if matches!(param.pattern.get_identifier().as_deref(), Some("err" | "error"))
&& !Self::is_inside_yield_or_await(node.id(), ctx)
{
ctx.diagnostic(prefer_await_to_callbacks(last_arg.span()));
}
}
}
AstKind::Function(func) => {
Self::check_last_params_for_callback(&func.params, ctx);
}
AstKind::ArrowFunctionExpression(expr) => {
Self::check_last_params_for_callback(&expr.params, ctx);
}
_ => {}
}
}
}

impl PreferAwaitToCallbacks {
fn check_last_params_for_callback(params: &FormalParameters, ctx: &LintContext) {
let Some(param) = params.items.last() else {
return;
};

let id = param.pattern.get_identifier();
if matches!(id.as_deref(), Some("callback" | "cb")) {
ctx.diagnostic(prefer_await_to_callbacks(param.span));
}
}

fn is_inside_yield_or_await(id: NodeId, ctx: &LintContext) -> bool {
ctx.nodes().iter_parents(id).skip(1).any(|parent| {
matches!(parent.kind(), AstKind::AwaitExpression(_) | AstKind::YieldExpression(_))
})
}

fn is_array_method(name: &str) -> bool {
["map", "every", "forEach", "some", "find", "filter"].contains(&name)
}
}

#[test]
fn test() {
use crate::tester::Tester;

let pass = vec![
"async function hi() { await thing().catch(err => console.log(err)) }",
"async function hi() { await thing().then() }",
"async function hi() { await thing().catch() }",
r#"dbConn.on("error", err => { console.error(err) })"#,
r#"dbConn.once("error", err => { console.error(err) })"#,
"heart(something => {})",
"getErrors().map(error => responseTo(error))",
"errors.filter(err => err.status === 402)",
r#"errors.some(err => err.message.includes("Yo"))"#,
"errors.every(err => err.status === 402)",
"errors.filter(err => console.log(err))",
r#"const error = errors.find(err => err.stack.includes("file.js"))"#,
"this.myErrors.forEach(function(error) { log(error); })",
r#"find(errors, function(err) { return err.type === "CoolError" })"#,
r#"map(errors, function(error) { return err.type === "CoolError" })"#,
r#"_.find(errors, function(error) { return err.type === "CoolError" })"#,
r#"_.map(errors, function(err) { return err.type === "CoolError" })"#,
];

let fail = vec![
"heart(function(err) {})",
"heart(err => {})",
r#"heart("ball", function(err) {})"#,
"function getData(id, callback) {}",
"const getData = (cb) => {}",
"var x = function (x, cb) {}",
"cb()",
"callback()",
"heart(error => {})",
"async.map(files, fs.stat, function(err, results) { if (err) throw err; });",
"_.map(files, fs.stat, function(err, results) { if (err) throw err; });",
"map(files, fs.stat, function(err, results) { if (err) throw err; });",
"map(function(err, results) { if (err) throw err; });",
"customMap(errors, (err) => err.message)",
];

Tester::new(PreferAwaitToCallbacks::NAME, pass, fail).test_and_snapshot();
}
86 changes: 86 additions & 0 deletions crates/oxc_linter/src/snapshots/prefer_await_to_callbacks.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
---
source: crates/oxc_linter/src/tester.rs
---
⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:7]
1 │ heart(function(err) {})
· ────────────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:7]
1 │ heart(err => {})
· ─────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:15]
1 │ heart("ball", function(err) {})
· ────────────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:22]
1 │ function getData(id, callback) {}
· ────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:18]
1 │ const getData = (cb) => {}
· ──
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:22]
1 │ var x = function (x, cb) {}
· ──
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:1]
1 │ cb()
· ────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:1]
1 │ callback()
· ──────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:7]
1 │ heart(error => {})
· ───────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:27]
1 │ async.map(files, fs.stat, function(err, results) { if (err) throw err; });
· ──────────────────────────────────────────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:23]
1 │ _.map(files, fs.stat, function(err, results) { if (err) throw err; });
· ──────────────────────────────────────────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:21]
1 │ map(files, fs.stat, function(err, results) { if (err) throw err; });
· ──────────────────────────────────────────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:5]
1 │ map(function(err, results) { if (err) throw err; });
· ──────────────────────────────────────────────
╰────

⚠ eslint-plugin-promise(prefer-await-to-callbacks): Prefer `async`/`await` to the callback pattern
╭─[prefer_await_to_callbacks.tsx:1:19]
1 │ customMap(errors, (err) => err.message)
· ────────────────────
╰────