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/node): implement no-exports-assign #5370

Merged
merged 4 commits into from
Sep 3, 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
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]
1 │ exports = {}
· ───────
╰────
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