diff --git a/crates/oxc_linter/src/rules/unicorn/catch_error_name.rs b/crates/oxc_linter/src/rules/unicorn/catch_error_name.rs index 6a02c8ae4c26e..9c1fde7afc258 100644 --- a/crates/oxc_linter/src/rules/unicorn/catch_error_name.rs +++ b/crates/oxc_linter/src/rules/unicorn/catch_error_name.rs @@ -4,8 +4,8 @@ use oxc_ast::{ }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_semantic::SymbolId; use oxc_span::{CompactStr, Span}; +use oxc_syntax::identifier::is_identifier_name; use crate::{context::LintContext, rule::Rule, AstNode}; @@ -120,7 +120,7 @@ declare_oxc_lint!( CatchErrorName, unicorn, style, - pending + fix ); impl Rule for CatchErrorName { @@ -141,6 +141,10 @@ impl Rule for CatchErrorName { .get(0) .and_then(|v| v.get("name")) .and_then(serde_json::Value::as_str) + .and_then(|name| { + let name = name.trim(); + is_identifier_name(name).then_some(name) + }) .unwrap_or("error"), ); @@ -149,47 +153,20 @@ impl Rule for CatchErrorName { fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { if let AstKind::CatchParameter(catch_param) = node.kind() { - if let oxc_ast::ast::BindingPatternKind::BindingIdentifier(binding_ident) = - &catch_param.pattern.kind - { - if self.is_name_allowed(&binding_ident.name) { - return; - } - - if binding_ident.name.starts_with('_') { - if symbol_has_references(binding_ident.symbol_id(), ctx) { - ctx.diagnostic(catch_error_name_diagnostic( - binding_ident.name.as_str(), - &self.name, - binding_ident.span, - )); - } - return; - } - - ctx.diagnostic(catch_error_name_diagnostic( - binding_ident.name.as_str(), - &self.name, - binding_ident.span, - )); - } + self.check_binding_identifier(ctx, &catch_param.pattern.kind); } if let AstKind::CallExpression(call_expr) = node.kind() { if let Some(member_expr) = call_expr.callee.as_member_expression() { if member_expr.static_property_name() == Some("catch") { - if let Some(arg0) = call_expr.arguments.first() { - if let Some(diagnostic) = self.check_function_arguments(arg0, ctx) { - ctx.diagnostic(diagnostic); - } + if let Some(arg) = call_expr.arguments.first() { + self.check_function_arguments(arg, ctx); } } if member_expr.static_property_name() == Some("then") { - if let Some(arg0) = call_expr.arguments.get(1) { - if let Some(diagnostic) = self.check_function_arguments(arg0, ctx) { - ctx.diagnostic(diagnostic); - } + if let Some(arg) = call_expr.arguments.get(1) { + self.check_function_arguments(arg, ctx); } } } @@ -202,74 +179,74 @@ impl CatchErrorName { self.name == name || self.ignore.iter().any(|s| s.as_str() == name) } - fn check_function_arguments( - &self, - arg0: &Argument, - ctx: &LintContext, - ) -> Option { - let expr = arg0.as_expression()?; - let expr = expr.without_parentheses(); - - if let Expression::ArrowFunctionExpression(arrow_expr) = expr { - if let Some(arg0) = arrow_expr.params.items.first() { - if let BindingPatternKind::BindingIdentifier(v) = &arg0.pattern.kind { - if self.is_name_allowed(&v.name) { - return None; - } + fn check_function_arguments(&self, arg: &Argument, ctx: &LintContext) { + let Some(expr) = arg.as_expression() else { return }; - if v.name.starts_with('_') { - if symbol_has_references(v.symbol_id(), ctx) { - ctx.diagnostic(catch_error_name_diagnostic( - v.name.as_str(), - &self.name, - v.span, - )); - } + let first_arg = match expr.without_parentheses() { + Expression::ArrowFunctionExpression(arrow_expr) => arrow_expr.params.items.first(), + Expression::FunctionExpression(fn_expr) => fn_expr.params.items.first(), + _ => return, + }; - return None; - } + let Some(arg) = first_arg else { return }; + self.check_binding_identifier(ctx, &arg.pattern.kind); + } - return Some(catch_error_name_diagnostic(v.name.as_str(), &self.name, v.span)); - } + fn check_binding_identifier( + &self, + ctx: &LintContext, + binding_pattern_kind: &BindingPatternKind, + ) { + if let BindingPatternKind::BindingIdentifier(binding_ident) = binding_pattern_kind { + if self.is_name_allowed(&binding_ident.name) { + return; } - } - if let Expression::FunctionExpression(fn_expr) = expr { - if let Some(arg0) = fn_expr.params.items.first() { - if let BindingPatternKind::BindingIdentifier(binding_ident) = &arg0.pattern.kind { - if self.is_name_allowed(&binding_ident.name) { - return None; - } + if binding_ident.name.starts_with('_') { + let mut iter = + ctx.semantic().symbol_references(binding_ident.symbol_id()).peekable(); - if binding_ident.name.starts_with('_') { - if symbol_has_references(binding_ident.symbol_id(), ctx) { - ctx.diagnostic(catch_error_name_diagnostic( - binding_ident.name.as_str(), - &self.name, - binding_ident.span, - )); - } + if iter.peek().is_some() { + ctx.diagnostic_with_fix( + catch_error_name_diagnostic( + binding_ident.name.as_str(), + &self.name, + binding_ident.span, + ), + |fixer| { + let fixer = fixer.for_multifix(); + let mut declaration_fix = fixer.new_fix_with_capacity(2); - return None; - } + declaration_fix + .push(fixer.replace(binding_ident.span, self.name.clone())); + + for reference in iter { + let node = ctx.nodes().get_node(reference.node_id()).kind(); + let Some(id) = node.as_identifier_reference() else { continue }; - return Some(catch_error_name_diagnostic( - binding_ident.name.as_str(), - &self.name, - binding_ident.span, - )); + declaration_fix.push(fixer.replace(id.span, self.name.clone())); + } + + declaration_fix + }, + ); } + + return; } - } - None + ctx.diagnostic_with_fix( + catch_error_name_diagnostic( + binding_ident.name.as_str(), + &self.name, + binding_ident.span, + ), + |fixer| fixer.replace(binding_ident.span, self.name.clone()), + ); + } } } -fn symbol_has_references(symbol_id: SymbolId, ctx: &LintContext) -> bool { - ctx.semantic().symbol_references(symbol_id).next().is_some() -} - #[test] fn test() { use crate::tester::Tester; @@ -338,5 +315,69 @@ fn test() { ("promise.then(undefined, (foo) => { })", None), ]; - Tester::new(CatchErrorName::NAME, CatchErrorName::PLUGIN, pass, fail).test_and_snapshot(); + let fix = vec![ + ( + "try { } catch (descriptiveError) { }", + "try { } catch (exception) { }", + Some(serde_json::json!([{"name": "exception"}])), + ), + ( + "try { } catch (e) { }", + "try { } catch (has_space_after) { }", + Some(serde_json::json!([{"name": "has_space_after "}])), + ), + ( + "try { } catch (e) { }", + "try { } catch (error) { }", + Some(serde_json::json!([{"name": "1_start_with_a_number"}])), + ), + ( + "try { } catch (e) { }", + "try { } catch (error) { }", + Some(serde_json::json!([{"name": "_){ } evilCode; if(false"}])), + ), + ( + "try { } catch (notMatching) { }", + "try { } catch (error) { }", + Some(serde_json::json!([{"ignore": []}])), + ), + ( + "try { } catch (notMatching) { }", + "try { } catch (error) { }", + Some(serde_json::json!([{"ignore": ["unicorn"]}])), + ), + ( + "try { } catch (notMatching) { }", + "try { } catch (error) { }", + Some(serde_json::json!([{"ignore": ["unicorn"]}])), + ), + ( + "try { } catch (_) { console.log(_) }", + "try { } catch (error) { console.log(error) }", + None, + ), + ( + "promise.catch(notMatching => { })", + "promise.catch(error => { })", + Some(serde_json::json!([{"ignore": ["unicorn"]}])), + ), + ("promise.catch((foo) => { })", "promise.catch((error) => { })", None), + ("promise.catch(function (foo) { })", "promise.catch(function (error) { })", None), + ("promise.catch((function (foo) { }))", "promise.catch((function (error) { }))", None), + ( + "promise.then(function (foo) { }).catch((foo) => { })", + "promise.then(function (foo) { }).catch((error) => { })", + None, + ), + ( + "promise.then(undefined, function (foo) { })", + "promise.then(undefined, function (error) { })", + None, + ), + ("promise.then(undefined, (foo) => { })", "promise.then(undefined, (error) => { })", None), + ]; + + Tester::new(CatchErrorName::NAME, CatchErrorName::PLUGIN, pass, fail) + .expect_fix(fix) + .test_and_snapshot(); } diff --git a/crates/oxc_linter/src/snapshots/unicorn_catch_error_name.snap b/crates/oxc_linter/src/snapshots/unicorn_catch_error_name.snap index 8d20046cae372..7e273242335c5 100644 --- a/crates/oxc_linter/src/snapshots/unicorn_catch_error_name.snap +++ b/crates/oxc_linter/src/snapshots/unicorn_catch_error_name.snap @@ -6,42 +6,49 @@ source: crates/oxc_linter/src/tester.rs 1 │ try { } catch (descriptiveError) { } · ──────────────── ╰──── + help: Replace `descriptiveError` with `exception`. - ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "e" should be named "has_space_after " + ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "e" should be named "has_space_after" ╭─[catch_error_name.tsx:1:16] 1 │ try { } catch (e) { } · ─ ╰──── + help: Replace `e` with `has_space_after`. - ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "e" should be named "1_start_with_a_number" + ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "e" should be named "error" ╭─[catch_error_name.tsx:1:16] 1 │ try { } catch (e) { } · ─ ╰──── + help: Replace `e` with `error`. - ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "e" should be named "_){ } evilCode; if(false" + ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "e" should be named "error" ╭─[catch_error_name.tsx:1:16] 1 │ try { } catch (e) { } · ─ ╰──── + help: Replace `e` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "notMatching" should be named "error" ╭─[catch_error_name.tsx:1:16] 1 │ try { } catch (notMatching) { } · ─────────── ╰──── + help: Replace `notMatching` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "notMatching" should be named "error" ╭─[catch_error_name.tsx:1:16] 1 │ try { } catch (notMatching) { } · ─────────── ╰──── + help: Replace `notMatching` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "notMatching" should be named "error" ╭─[catch_error_name.tsx:1:16] 1 │ try { } catch (notMatching) { } · ─────────── ╰──── + help: Replace `notMatching` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "_" should be named "error" ╭─[catch_error_name.tsx:1:16] @@ -54,39 +61,46 @@ source: crates/oxc_linter/src/tester.rs 1 │ promise.catch(notMatching => { }) · ─────────── ╰──── + help: Replace `notMatching` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "foo" should be named "error" ╭─[catch_error_name.tsx:1:16] 1 │ promise.catch((foo) => { }) · ─── ╰──── + help: Replace `foo` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "foo" should be named "error" ╭─[catch_error_name.tsx:1:25] 1 │ promise.catch(function (foo) { }) · ─── ╰──── + help: Replace `foo` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "foo" should be named "error" ╭─[catch_error_name.tsx:1:26] 1 │ promise.catch((function (foo) { })) · ─── ╰──── + help: Replace `foo` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "foo" should be named "error" ╭─[catch_error_name.tsx:1:41] 1 │ promise.then(function (foo) { }).catch((foo) => { }) · ─── ╰──── + help: Replace `foo` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "foo" should be named "error" ╭─[catch_error_name.tsx:1:35] 1 │ promise.then(undefined, function (foo) { }) · ─── ╰──── + help: Replace `foo` with `error`. ⚠ eslint-plugin-unicorn(catch-error-name): The catch parameter "foo" should be named "error" ╭─[catch_error_name.tsx:1:26] 1 │ promise.then(undefined, (foo) => { }) · ─── ╰──── + help: Replace `foo` with `error`.