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

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

Split off from https://github.com/oxc-project/oxc/pull/4252
This commit is contained in:
Jelle van der Waa 2024-07-17 03:21:20 +02:00 committed by GitHub
parent fc0b17d5a0
commit 1f8968a521
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
15 changed files with 512 additions and 1 deletions

View file

@ -223,6 +223,10 @@ pub struct EnablePlugins {
/// Enable the React performance plugin and detect rendering performance problems
#[bpaf(switch, hide_usage)]
pub react_perf_plugin: bool,
/// Enable the promise plugin and detect promise usage problems
#[bpaf(switch, hide_usage)]
pub promise_plugin: bool,
}
#[cfg(test)]

View file

@ -104,7 +104,8 @@ impl Runner for LintRunner {
.with_vitest_plugin(enable_plugins.vitest_plugin)
.with_jsx_a11y_plugin(enable_plugins.jsx_a11y_plugin)
.with_nextjs_plugin(enable_plugins.nextjs_plugin)
.with_react_perf_plugin(enable_plugins.react_perf_plugin);
.with_react_perf_plugin(enable_plugins.react_perf_plugin)
.with_promise_plugin(enable_plugins.promise_plugin);
let linter = match Linter::from_options(lint_options) {
Ok(lint_service) => lint_service,

View file

@ -29,6 +29,7 @@ pub struct LintOptions {
pub jsx_a11y_plugin: bool,
pub nextjs_plugin: bool,
pub react_perf_plugin: bool,
pub promise_plugin: bool,
}
impl Default for LintOptions {
@ -48,6 +49,7 @@ impl Default for LintOptions {
jsx_a11y_plugin: false,
nextjs_plugin: false,
react_perf_plugin: false,
promise_plugin: false,
}
}
}
@ -138,6 +140,12 @@ impl LintOptions {
self.react_perf_plugin = yes;
self
}
#[must_use]
pub fn with_promise_plugin(mut self, yes: bool) -> Self {
self.promise_plugin = yes;
self
}
}
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
@ -342,6 +350,7 @@ impl LintOptions {
"react_perf" => self.react_perf_plugin,
"oxc" => self.oxc_plugin,
"eslint" | "tree_shaking" => true,
"promise" => self.promise_plugin,
name => panic!("Unhandled plugin: {name}"),
})
.cloned()

View file

@ -432,6 +432,12 @@ mod tree_shaking {
pub mod no_side_effects_in_initialization;
}
mod promise {
pub mod avoid_new;
pub mod no_new_statics;
pub mod param_names;
}
oxc_macros::declare_all_lint_rules! {
eslint::array_callback_return,
eslint::constructor_super,
@ -822,4 +828,7 @@ oxc_macros::declare_all_lint_rules! {
jsdoc::require_returns_type,
jsdoc::require_yields,
tree_shaking::no_side_effects_in_initialization,
promise::avoid_new,
promise::no_new_statics,
promise::param_names,
}

View file

@ -0,0 +1,69 @@
use oxc_ast::{ast::Expression, AstKind};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};
fn avoid_new_promise_diagnostic(span0: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("eslint-plugin-promise(avoid-new): Avoid creating new promises")
.with_label(span0)
}
#[derive(Debug, Default, Clone)]
pub struct AvoidNew;
declare_oxc_lint!(
/// ### What it does
///
/// Disallow creating new promises outside of utility libs.
///
/// ### Why is this bad?
///
/// If you dislike the new promise style promises.
///
/// ### Example
/// ```javascript
/// new Promise((resolve, reject) => { ... });
/// ```
AvoidNew,
restriction,
);
impl Rule for AvoidNew {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
let AstKind::NewExpression(expr) = node.kind() else {
return;
};
let Expression::Identifier(ident) = &expr.callee else {
return;
};
if ident.name == "Promise" && ctx.semantic().is_reference_to_global_variable(ident) {
ctx.diagnostic(avoid_new_promise_diagnostic(expr.span));
}
}
}
#[test]
fn test() {
use crate::tester::Tester;
let pass = vec![
"Promise.resolve()",
"Promise.reject()",
"Promise.all()",
"new Horse()",
"new PromiseLikeThing()",
"new Promise.resolve()",
];
let fail = vec![
"var x = new Promise(function (x, y) {})",
"new Promise()",
"Thing(new Promise(() => {}))",
];
Tester::new(AvoidNew::NAME, pass, fail).test_and_snapshot();
}

View file

@ -0,0 +1,110 @@
use oxc_ast::{ast::Expression, AstKind};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;
use crate::{context::LintContext, rule::Rule, AstNode};
fn static_promise_diagnostic(x0: &str, span0: Span) -> OxcDiagnostic {
OxcDiagnostic::warn(format!(
"eslint-plugin-promise(no-new-statics): Disallow calling `new` on a `Promise.{x0}`"
))
.with_label(span0)
}
#[derive(Debug, Default, Clone)]
pub struct NoNewStatics;
declare_oxc_lint!(
/// ### What it does
///
/// Disallow calling new on a Promise static method.
///
/// ### Why is this bad?
///
/// Calling a Promise static method with new is invalid, resulting in a TypeError at runtime.
///
/// ### Example
/// ```javascript
/// new Promise.resolve(value);
/// ```
NoNewStatics,
correctness
);
impl Rule for NoNewStatics {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
let AstKind::NewExpression(new_expr) = node.kind() else {
return;
};
let Some(member_expr) = &new_expr.callee.get_member_expr() else {
return;
};
let Expression::Identifier(ident) = &member_expr.object() else {
return;
};
if ident.name != "Promise" || !ctx.semantic().is_reference_to_global_variable(ident) {
return;
}
let Some(prop_name) = member_expr.static_property_name() else {
return;
};
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
if matches!(
prop_name,
"resolve" | "reject" | "all" | "allSettled" | "race" | "any" | "withResolvers"
) {
ctx.diagnostic_with_fix(
static_promise_diagnostic(
prop_name,
Span::new(new_expr.span.start, ident.span.start - 1),
),
|fixer| fixer.delete_range(Span::new(new_expr.span.start, ident.span.start)),
);
}
}
}
#[test]
fn test() {
use crate::tester::Tester;
let pass = vec![
"Promise.resolve()",
"Promise.reject()",
"Promise.all()",
"Promise.race()",
"new Promise(function (resolve, reject) {})",
"new SomeClass()",
"SomeClass.resolve()",
"new SomeClass.resolve()",
];
let fail = vec![
"new Promise.resolve()",
"new Promise.reject()",
"new Promise.all()",
"new Promise.allSettled()",
"new Promise.any()",
"new Promise.race()",
"function foo() {
var a = getA()
return new Promise.resolve(a)
}",
];
let fix = vec![
("new Promise.resolve()", "Promise.resolve()", None),
("new Promise.reject()", "Promise.reject()", None),
("new Promise.all()", "Promise.all()", None),
("new Promise.allSettled()", "Promise.allSettled()", None),
("new Promise.any()", "Promise.any()", None),
("new Promise.race()", "Promise.race()", None),
];
Tester::new(NoNewStatics::NAME, pass, fail).expect_fix(fix).test_and_snapshot();
}

View file

@ -0,0 +1,179 @@
use oxc_ast::{
ast::{BindingPatternKind, Expression, FormalParameter, FormalParameters},
AstKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;
use regex::Regex;
use crate::{context::LintContext, rule::Rule, AstNode};
fn param_names_diagnostic(span0: Span, x0: &str) -> OxcDiagnostic {
OxcDiagnostic::warn(format!("eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `{x0}`")).with_label(span0)
}
#[derive(Debug, Default, Clone)]
pub struct ParamNames(Box<ParamNamesConfig>);
#[derive(Debug, Default, Clone)]
pub struct ParamNamesConfig {
resolve_pattern: Option<Regex>,
reject_pattern: Option<Regex>,
}
impl std::ops::Deref for ParamNames {
type Target = ParamNamesConfig;
fn deref(&self) -> &Self::Target {
&self.0
}
}
enum ParamType {
Resolve,
Reject,
}
declare_oxc_lint!(
/// ### What it does
///
/// Enforce standard parameter names for Promise constructors.
///
/// ### Why is this bad?
///
/// Ensures that new Promise() is instantiated with the parameter names resolve, reject to
/// avoid confusion with order such as reject, resolve. The Promise constructor uses the
/// RevealingConstructor pattern. Using the same parameter names as the language specification
/// makes code more uniform and easier to understand.
///
/// ### Example
/// ```javascript
/// new Promise(function (reject, resolve) { ... }) // incorrect order
/// new Promise(function (ok, fail) { ... }) // non-standard parameter names
/// ```
ParamNames,
style,
);
impl Rule for ParamNames {
fn from_configuration(value: serde_json::Value) -> Self {
let mut cfg = ParamNamesConfig::default();
if let Some(config) = value.get(0) {
if let Some(val) = config.get("resolvePattern").and_then(serde_json::Value::as_str) {
cfg.resolve_pattern = Regex::new(val).ok();
}
if let Some(val) = config.get("rejectPattern").and_then(serde_json::Value::as_str) {
cfg.reject_pattern = Regex::new(val).ok();
}
}
Self(Box::new(cfg))
}
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
let AstKind::NewExpression(new_expr) = node.kind() else {
return;
};
if !new_expr.callee.is_specific_id("Promise") || new_expr.arguments.len() != 1 {
return;
}
for argument in &new_expr.arguments {
let Some(arg_expr) = argument.as_expression() else {
continue;
};
match arg_expr {
Expression::ArrowFunctionExpression(arrow_expr) => {
self.check_parameter_names(&arrow_expr.params, ctx);
}
Expression::FunctionExpression(func_expr) => {
self.check_parameter_names(&func_expr.params, ctx);
}
_ => continue,
}
}
}
}
impl ParamNames {
fn check_parameter_names(&self, params: &FormalParameters, ctx: &LintContext) {
if params.items.is_empty() {
return;
}
self.check_parameter(&params.items[0], &ParamType::Resolve, ctx);
if params.items.len() > 1 {
self.check_parameter(&params.items[1], &ParamType::Reject, ctx);
}
}
fn check_parameter(&self, param: &FormalParameter, param_type: &ParamType, ctx: &LintContext) {
let BindingPatternKind::BindingIdentifier(param_ident) = &param.pattern.kind else {
return;
};
let param_pattern = if matches!(param_type, ParamType::Reject) {
&self.reject_pattern
} else {
&self.resolve_pattern
};
match param_pattern {
Some(pattern) => {
if !pattern.is_match(param_ident.name.as_str()) {
ctx.diagnostic(param_names_diagnostic(param_ident.span, pattern.as_str()));
}
}
None => {
if matches!(param_type, ParamType::Resolve)
&& !matches!(param_ident.name.as_str(), "_resolve" | "resolve")
{
ctx.diagnostic(param_names_diagnostic(param_ident.span, "^_?resolve$"));
} else if matches!(param_type, ParamType::Reject)
&& !matches!(param_ident.name.as_str(), "_reject" | "reject")
{
ctx.diagnostic(param_names_diagnostic(param_ident.span, "^_?reject$"));
}
}
}
}
}
#[test]
fn test() {
use crate::tester::Tester;
let pass = vec![
("new Promise(function(resolve, reject) {})", None),
("new Promise(function(resolve, _reject) {})", None),
("new Promise(function(_resolve, reject) {})", None),
("new Promise(function(_resolve, _reject) {})", None),
("new Promise(function(resolve) {})", None),
("new Promise(function(_resolve) {})", None),
("new Promise(resolve => {})", None),
("new Promise((resolve, reject) => {})", None),
("new Promise(() => {})", None),
("new NonPromise()", None),
(
"new Promise((yes, no) => {})",
Some(serde_json::json!([{ "resolvePattern": "^yes$", "rejectPattern": "^no$" }])),
),
];
let fail = vec![
("new Promise(function(reject, resolve) {})", None),
("new Promise(function(resolve, rej) {})", None),
("new Promise(yes => {})", None),
("new Promise((yes, no) => {})", None),
(
"new Promise(function(resolve, reject) { config(); })",
Some(serde_json::json!([{ "resolvePattern": "^yes$", "rejectPattern": "^no$" }])),
),
];
Tester::new(ParamNames::NAME, pass, fail).test_and_snapshot();
}

View file

@ -0,0 +1,20 @@
---
source: crates/oxc_linter/src/tester.rs
---
⚠ eslint-plugin-promise(avoid-new): Avoid creating new promises
╭─[avoid_new.tsx:1:9]
1 │ var x = new Promise(function (x, y) {})
· ───────────────────────────────
╰────
⚠ eslint-plugin-promise(avoid-new): Avoid creating new promises
╭─[avoid_new.tsx:1:1]
1 │ new Promise()
· ─────────────
╰────
⚠ eslint-plugin-promise(avoid-new): Avoid creating new promises
╭─[avoid_new.tsx:1:7]
1 │ Thing(new Promise(() => {}))
· ─────────────────────
╰────

View file

@ -0,0 +1,46 @@
---
source: crates/oxc_linter/src/tester.rs
---
⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a `Promise.resolve`
╭─[no_new_statics.tsx:1:1]
1 │ new Promise.resolve()
· ───
╰────
⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a `Promise.reject`
╭─[no_new_statics.tsx:1:1]
1 │ new Promise.reject()
· ───
╰────
⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a `Promise.all`
╭─[no_new_statics.tsx:1:1]
1 │ new Promise.all()
· ───
╰────
⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a `Promise.allSettled`
╭─[no_new_statics.tsx:1:1]
1 │ new Promise.allSettled()
· ───
╰────
⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a `Promise.any`
╭─[no_new_statics.tsx:1:1]
1 │ new Promise.any()
· ───
╰────
⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a `Promise.race`
╭─[no_new_statics.tsx:1:1]
1 │ new Promise.race()
· ───
╰────
⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a `Promise.resolve`
╭─[no_new_statics.tsx:3:13]
2 │ var a = getA()
3 │ return new Promise.resolve(a)
· ───
4 │ }
╰────

View file

@ -0,0 +1,50 @@
---
source: crates/oxc_linter/src/tester.rs
---
⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?resolve$`
╭─[param_names.tsx:1:22]
1 │ new Promise(function(reject, resolve) {})
· ──────
╰────
⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?reject$`
╭─[param_names.tsx:1:30]
1 │ new Promise(function(reject, resolve) {})
· ───────
╰────
⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?reject$`
╭─[param_names.tsx:1:31]
1 │ new Promise(function(resolve, rej) {})
· ───
╰────
⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?resolve$`
╭─[param_names.tsx:1:13]
1 │ new Promise(yes => {})
· ───
╰────
⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?resolve$`
╭─[param_names.tsx:1:14]
1 │ new Promise((yes, no) => {})
· ───
╰────
⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?reject$`
╭─[param_names.tsx:1:19]
1 │ new Promise((yes, no) => {})
· ──
╰────
⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^yes$`
╭─[param_names.tsx:1:22]
1 │ new Promise(function(resolve, reject) { config(); })
· ───────
╰────
⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^no$`
╭─[param_names.tsx:1:31]
1 │ new Promise(function(resolve, reject) { config(); })
· ──────
╰────

View file

@ -145,6 +145,9 @@ new-react-perf-rule name:
new-n-rule name:
cargo run -p rulegen {{name}} n
new-promise-rule name:
cargo run -p rulegen {{name}} promise
clone-submodule dir url sha:
git clone --depth=1 {{url}} {{dir}} || true
cd {{dir}} && git fetch origin {{sha}} && git reset --hard {{sha}}

View file

@ -55,6 +55,9 @@ const NODE_TEST_PATH: &str =
const TREE_SHAKING_PATH: &str =
"https://raw.githubusercontent.com/lukastaegert/eslint-plugin-tree-shaking/master/src/rules";
const PROMISE_TEST_PATH: &str =
"https://raw.githubusercontent.com/eslint-community/eslint-plugin-promise/main/__tests__";
struct TestCase {
source_text: String,
code: Option<String>,
@ -570,6 +573,7 @@ pub enum RuleKind {
JSDoc,
Node,
TreeShaking,
Promise,
}
impl RuleKind {
@ -586,6 +590,7 @@ impl RuleKind {
"jsdoc" => Self::JSDoc,
"n" => Self::Node,
"tree-shaking" => Self::TreeShaking,
"promise" => Self::Promise,
_ => Self::ESLint,
}
}
@ -606,6 +611,7 @@ impl Display for RuleKind {
Self::JSDoc => write!(f, "eslint-plugin-jsdoc"),
Self::Node => write!(f, "eslint-plugin-n"),
Self::TreeShaking => write!(f, "eslint-plugin-tree-shaking"),
Self::Promise => write!(f, "eslint-plugin-promise"),
}
}
}
@ -632,6 +638,7 @@ fn main() {
RuleKind::JSDoc => format!("{JSDOC_TEST_PATH}/{camel_rule_name}.js"),
RuleKind::Node => format!("{NODE_TEST_PATH}/{kebab_rule_name}.js"),
RuleKind::TreeShaking => format!("{TREE_SHAKING_PATH}/{kebab_rule_name}.test.ts"),
RuleKind::Promise => format!("{PROMISE_TEST_PATH}/{kebab_rule_name}.js"),
RuleKind::Oxc => String::new(),
};

View file

@ -42,6 +42,7 @@ impl<'a> Template<'a> {
RuleKind::JSDoc => Path::new("crates/oxc_linter/src/rules/jsdoc"),
RuleKind::Node => Path::new("crates/oxc_linter/src/rules/node"),
RuleKind::TreeShaking => Path::new("crates/oxc_linter/src/rules/tree_shaking"),
RuleKind::Promise => Path::new("crates/oxc_linter/src/rules/promise"),
};
std::fs::create_dir_all(path)?;

View file

@ -66,6 +66,8 @@ Arguments:
Enable the Next.js plugin and detect Next.js problems
- **` --react-perf-plugin`** &mdash;
Enable the React performance plugin and detect rendering performance problems
- **` --promise-plugin`** &mdash;
Enable the promise plugin and detect promise usage problems

View file

@ -40,6 +40,7 @@ Enable Plugins
--nextjs-plugin Enable the Next.js plugin and detect Next.js problems
--react-perf-plugin Enable the React performance plugin and detect rendering performance
problems
--promise-plugin Enable the promise plugin and detect promise usage problems
Fix Problems
--fix Fix as many issues as possible. Only unfixed issues are reported in