From 447377907432e6f5a2881785d6d1db0c86f5fdcf Mon Sep 17 00:00:00 2001 From: dalaoshu Date: Tue, 3 Sep 2024 19:53:14 +0800 Subject: [PATCH] feat(linter/node): implement no-exports-assign (#5370) --- apps/oxlint/src/command/lint.rs | 4 + apps/oxlint/src/lint/mod.rs | 3 +- crates/oxc_linter/src/options/mod.rs | 7 + crates/oxc_linter/src/options/plugins.rs | 7 + crates/oxc_linter/src/rules.rs | 5 + .../src/rules/node/no_exports_assign.rs | 129 ++++++++++++++++++ .../src/snapshots/no_exports_assign.snap | 9 ++ crates/oxc_linter/src/tester.rs | 8 +- tasks/benchmark/benches/linter.rs | 3 +- tasks/website/src/linter/snapshots/cli.snap | 2 + .../src/linter/snapshots/cli_terminal.snap | 1 + 11 files changed, 175 insertions(+), 3 deletions(-) create mode 100644 crates/oxc_linter/src/rules/node/no_exports_assign.rs create mode 100644 crates/oxc_linter/src/snapshots/no_exports_assign.snap diff --git a/apps/oxlint/src/command/lint.rs b/apps/oxlint/src/command/lint.rs index 93a8ca7d02e6e..9e39d1f1afb06 100644 --- a/apps/oxlint/src/command/lint.rs +++ b/apps/oxlint/src/command/lint.rs @@ -261,6 +261,10 @@ pub struct EnablePlugins { /// Enable the promise plugin and detect promise usage problems #[bpaf(switch, hide_usage)] pub promise_plugin: bool, + + /// Enable the node plugin and detect node usage problems + #[bpaf(switch, hide_usage)] + pub node_plugin: bool, } #[cfg(test)] diff --git a/apps/oxlint/src/lint/mod.rs b/apps/oxlint/src/lint/mod.rs index e19d40bbbac73..8e7055824cea9 100644 --- a/apps/oxlint/src/lint/mod.rs +++ b/apps/oxlint/src/lint/mod.rs @@ -105,7 +105,8 @@ impl Runner for LintRunner { .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_promise_plugin(enable_plugins.promise_plugin); + .with_promise_plugin(enable_plugins.promise_plugin) + .with_node_plugin(enable_plugins.node_plugin); let linter = match Linter::from_options(lint_options) { Ok(lint_service) => lint_service, diff --git a/crates/oxc_linter/src/options/mod.rs b/crates/oxc_linter/src/options/mod.rs index e2d6e72ec4a37..348949bb70838 100644 --- a/crates/oxc_linter/src/options/mod.rs +++ b/crates/oxc_linter/src/options/mod.rs @@ -144,6 +144,12 @@ impl LintOptions { self.plugins.promise = yes; self } + + #[must_use] + pub fn with_node_plugin(mut self, yes: bool) -> Self { + self.plugins.node = yes; + self + } } impl LintOptions { @@ -240,6 +246,7 @@ impl LintOptions { "oxc" => self.plugins.oxc, "eslint" | "tree_shaking" => true, "promise" => self.plugins.promise, + "node" => self.plugins.node, name => panic!("Unhandled plugin: {name}"), }) .cloned() diff --git a/crates/oxc_linter/src/options/plugins.rs b/crates/oxc_linter/src/options/plugins.rs index 3c4cab9ab855b..97d464676ade4 100644 --- a/crates/oxc_linter/src/options/plugins.rs +++ b/crates/oxc_linter/src/options/plugins.rs @@ -17,6 +17,7 @@ pub struct LintPluginOptions { pub nextjs: bool, pub react_perf: bool, pub promise: bool, + pub node: bool, } impl Default for LintPluginOptions { @@ -34,6 +35,7 @@ impl Default for LintPluginOptions { nextjs: false, react_perf: false, promise: false, + node: false, } } } @@ -55,6 +57,7 @@ impl LintPluginOptions { nextjs: false, react_perf: false, promise: false, + node: false, } } @@ -74,6 +77,7 @@ impl LintPluginOptions { nextjs: true, react_perf: true, promise: true, + node: true, } } } @@ -99,6 +103,7 @@ impl> FromIterator<(S, bool)> for LintPluginOptions { "nextjs" => options.nextjs = enabled, "react-perf" => options.react_perf = enabled, "promise" => options.promise = enabled, + "node" => options.node = enabled, _ => { /* ignored */ } } } @@ -129,6 +134,7 @@ mod test { && self.nextjs == other.nextjs && self.react_perf == other.react_perf && self.promise == other.promise + && self.node == other.node } } @@ -160,6 +166,7 @@ mod test { nextjs: false, react_perf: false, promise: false, + node: false, }; assert_eq!(plugins, expected); } diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index a2925b0e03e7d..89fe952614681 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -470,6 +470,10 @@ mod vitest { pub mod require_local_test_context_for_concurrent_snapshots; } +mod node { + pub mod no_exports_assign; +} + oxc_macros::declare_all_lint_rules! { eslint::array_callback_return, eslint::constructor_super, @@ -892,4 +896,5 @@ oxc_macros::declare_all_lint_rules! { vitest::prefer_to_be_truthy, vitest::no_conditional_tests, vitest::require_local_test_context_for_concurrent_snapshots, + node::no_exports_assign, } diff --git a/crates/oxc_linter/src/rules/node/no_exports_assign.rs b/crates/oxc_linter/src/rules/node/no_exports_assign.rs new file mode 100644 index 0000000000000..ebdec46cc9c86 --- /dev/null +++ b/crates/oxc_linter/src/rules/node/no_exports_assign.rs @@ -0,0 +1,129 @@ +use oxc_ast::{ + ast::{AssignmentTarget, Expression, IdentifierReference, MemberExpression}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::{GetSpan, Span}; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn no_exports_assign(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Disallow the assignment to `exports`.") + .with_label(span) + .with_help("Unexpected assignment to 'exports' variable. Use 'module.exports' instead.") +} + +fn is_global_reference(ctx: &LintContext, id: &IdentifierReference, name: &str) -> bool { + if let Some(reference_id) = id.reference_id() { + return id.name == name && ctx.symbols().is_global_reference(reference_id); + } + false +} + +fn is_exports(node: &AssignmentTarget, ctx: &LintContext) -> bool { + let AssignmentTarget::AssignmentTargetIdentifier(id) = node else { + return false; + }; + + is_global_reference(ctx, id, "exports") +} + +fn is_module_exports(expr: Option<&MemberExpression>, ctx: &LintContext) -> bool { + let Some(mem_expr) = expr else { + return false; + }; + + let Some(obj_id) = mem_expr.object().get_identifier_reference() else { + return false; + }; + + return mem_expr.static_property_name() == Some("exports") + && is_global_reference(ctx, obj_id, "module"); +} + +#[derive(Debug, Default, Clone)] +pub struct NoExportsAssign; + +declare_oxc_lint!( + /// ### What it does + /// + /// This rule is aimed at disallowing `exports = {}`, but allows `module.exports = exports = {}` to avoid conflict with `n/exports-style` rule's `allowBatchAssign` option. + /// + /// ### Why is this bad? + /// + /// Directly using `exports = {}` can lead to confusion and potential bugs because it reassigns the `exports` object, which may break module exports. It is more predictable and clearer to use `module.exports` directly or in conjunction with `exports`. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```js + /// exports = {} + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```js + /// module.exports.foo = 1 + /// exports.bar = 2 + /// module.exports = {} + /// + /// // allows `exports = {}` if along with `module.exports =` + /// module.exports = exports = {} + /// exports = module.exports = {} + /// ``` + NoExportsAssign, + style, + fix +); + +impl Rule for NoExportsAssign { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::AssignmentExpression(assign_expr) = node.kind() else { + return; + }; + + if !is_exports(&assign_expr.left, ctx) { + return; + } + + if let Expression::AssignmentExpression(assign_expr) = &assign_expr.right { + if is_module_exports(assign_expr.left.as_member_expression(), ctx) { + return; + }; + } + + let parent = ctx.nodes().parent_node(node.id()); + if let Some(AstKind::AssignmentExpression(assign_expr)) = parent.map(AstNode::kind) { + if is_module_exports(assign_expr.left.as_member_expression(), ctx) { + return; + } + } + + ctx.diagnostic_with_fix(no_exports_assign(assign_expr.left.span()), |fixer| { + fixer.replace(assign_expr.left.span(), "module.exports") + }); + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "module.exports.foo = 1", + "exports.bar = 1", + "module.exports = exports = {}", + "exports = module.exports = {}", + "function f(exports) { exports = {} }", + "let exports; exports = {}", + ]; + + let fail = vec!["exports = {}"]; + + let fix = vec![("exports = {}", "module.exports = {}")]; + + Tester::new(NoExportsAssign::NAME, pass, fail) + .expect_fix(fix) + .with_node_plugin(true) + .test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_exports_assign.snap b/crates/oxc_linter/src/snapshots/no_exports_assign.snap new file mode 100644 index 0000000000000..717ae8354d976 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_exports_assign.snap @@ -0,0 +1,9 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ node(no-exports-assign): Disallow the assignment to `exports`. + ╭─[no_exports_assign.tsx:1:1] + 1 │ exports = {} + · ─────── + ╰──── + help: Unexpected assignment to 'exports' variable. Use 'module.exports' instead. diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index eab1e29ab95da..0b683b4ccdafe 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -250,6 +250,11 @@ impl Tester { self } + pub fn with_node_plugin(mut self, yes: bool) -> Self { + self.plugins.node = yes; + self + } + /// Add cases that should fix problems found in the source code. /// /// These cases will fail if no fixes are produced or if the fixed source @@ -351,7 +356,8 @@ impl Tester { .with_vitest_plugin(self.plugins.vitest) .with_jsx_a11y_plugin(self.plugins.jsx_a11y) .with_nextjs_plugin(self.plugins.nextjs) - .with_react_perf_plugin(self.plugins.react_perf); + .with_react_perf_plugin(self.plugins.react_perf) + .with_node_plugin(self.plugins.node); let eslint_config = eslint_config .as_ref() .map_or_else(OxlintConfig::default, |v| OxlintConfig::deserialize(v).unwrap()); diff --git a/tasks/benchmark/benches/linter.rs b/tasks/benchmark/benches/linter.rs index 137b44cf43d14..03a5fe7ebe755 100644 --- a/tasks/benchmark/benches/linter.rs +++ b/tasks/benchmark/benches/linter.rs @@ -46,7 +46,8 @@ fn bench_linter(criterion: &mut Criterion) { .with_jsx_a11y_plugin(true) .with_nextjs_plugin(true) .with_react_perf_plugin(true) - .with_vitest_plugin(true); + .with_vitest_plugin(true) + .with_node_plugin(true); let linter = Linter::from_options(lint_options).unwrap(); let semantic = Rc::new(semantic_ret.semantic); b.iter(|| linter.run(Path::new(std::ffi::OsStr::new("")), Rc::clone(&semantic))); diff --git a/tasks/website/src/linter/snapshots/cli.snap b/tasks/website/src/linter/snapshots/cli.snap index 6fc70907bfd65..e67de10780933 100644 --- a/tasks/website/src/linter/snapshots/cli.snap +++ b/tasks/website/src/linter/snapshots/cli.snap @@ -65,6 +65,8 @@ Arguments: Enable the React performance plugin and detect rendering performance problems - **` --promise-plugin`** — Enable the promise plugin and detect promise usage problems +- **` --node-plugin`** — + Enable the node plugin and detect node usage problems diff --git a/tasks/website/src/linter/snapshots/cli_terminal.snap b/tasks/website/src/linter/snapshots/cli_terminal.snap index 193dfb931d734..ff249ddb94136 100644 --- a/tasks/website/src/linter/snapshots/cli_terminal.snap +++ b/tasks/website/src/linter/snapshots/cli_terminal.snap @@ -42,6 +42,7 @@ Enable Plugins --react-perf-plugin Enable the React performance plugin and detect rendering performance problems --promise-plugin Enable the promise plugin and detect promise usage problems + --node-plugin Enable the node plugin and detect node usage problems Fix Problems --fix Fix as many issues as possible. Only unfixed issues are reported in