Skip to content

Commit

Permalink
feat(linter/node): implement no-exports-assign (#5370)
Browse files Browse the repository at this point in the history
  • Loading branch information
shulaoda authored Sep 3, 2024
1 parent 0a5780d commit 4473779
Show file tree
Hide file tree
Showing 11 changed files with 175 additions and 3 deletions.
4 changes: 4 additions & 0 deletions apps/oxlint/src/command/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
3 changes: 2 additions & 1 deletion apps/oxlint/src/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions crates/oxc_linter/src/options/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down
7 changes: 7 additions & 0 deletions crates/oxc_linter/src/options/plugins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub struct LintPluginOptions {
pub nextjs: bool,
pub react_perf: bool,
pub promise: bool,
pub node: bool,
}

impl Default for LintPluginOptions {
Expand All @@ -34,6 +35,7 @@ impl Default for LintPluginOptions {
nextjs: false,
react_perf: false,
promise: false,
node: false,
}
}
}
Expand All @@ -55,6 +57,7 @@ impl LintPluginOptions {
nextjs: false,
react_perf: false,
promise: false,
node: false,
}
}

Expand All @@ -74,6 +77,7 @@ impl LintPluginOptions {
nextjs: true,
react_perf: true,
promise: true,
node: true,
}
}
}
Expand All @@ -99,6 +103,7 @@ impl<S: AsRef<str>> FromIterator<(S, bool)> for LintPluginOptions {
"nextjs" => options.nextjs = enabled,
"react-perf" => options.react_perf = enabled,
"promise" => options.promise = enabled,
"node" => options.node = enabled,
_ => { /* ignored */ }
}
}
Expand Down Expand Up @@ -129,6 +134,7 @@ mod test {
&& self.nextjs == other.nextjs
&& self.react_perf == other.react_perf
&& self.promise == other.promise
&& self.node == other.node
}
}

Expand Down Expand Up @@ -160,6 +166,7 @@ mod test {
nextjs: false,
react_perf: false,
promise: false,
node: false,
};
assert_eq!(plugins, expected);
}
Expand Down
5 changes: 5 additions & 0 deletions crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
}
129 changes: 129 additions & 0 deletions crates/oxc_linter/src/rules/node/no_exports_assign.rs
Original file line number Diff line number Diff line change
@@ -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();
}
9 changes: 9 additions & 0 deletions crates/oxc_linter/src/snapshots/no_exports_assign.snap
Original file line number Diff line number Diff line change
@@ -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]
1exports = {}
· ───────
╰────
help: Unexpected assignment to 'exports' variable. Use 'module.exports' instead.
8 changes: 7 additions & 1 deletion crates/oxc_linter/src/tester.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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());
Expand Down
3 changes: 2 additions & 1 deletion tasks/benchmark/benches/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Expand Down
2 changes: 2 additions & 0 deletions tasks/website/src/linter/snapshots/cli.snap
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ Arguments:
Enable the React performance plugin and detect rendering performance problems
- **` --promise-plugin`** &mdash;
Enable the promise plugin and detect promise usage problems
- **` --node-plugin`** &mdash;
Enable the node plugin and detect node usage problems



Expand Down
1 change: 1 addition & 0 deletions tasks/website/src/linter/snapshots/cli_terminal.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 4473779

Please sign in to comment.