Skip to content

Commit c9cb2a4

Browse files
antoinezanardicamc314
antoinezanardi
and
camc314
authored
refactor(eslint/arrow-body-style): clean up implementation and improve documentation (#13389)
This is my **first contribution to Oxc**, aimed at improving code maintainability and standardizing documentation across rules. This PR serves as a **draft proposal** for how I can contribute to rules, focusing on both code and documentation. Comments and suggestions are very welcome to refine the style in a way that satisfies everyone. > [!NOTE] > This PR is focused solely on refactoring and documentation; no test behavior is modified. ## 🔩 Refactoring Goals when refactoring rules: * Make the code more **idiomatic Rust** (use `?`, `match`, `if let`, `Option`/`Result`, `Iterator`, etc. naturally). * Improve **readability and maintainability** through better structuring, naming, and relevant refactoring. * Follow **Rust best practices** (borrowing management, clarity of signatures, use of `&`/`Cow`/`From`/`Into` where appropriate). * **Avoid adding complexity**: prioritize clarity and conciseness. * Prefer **early returns** to reduce nesting and improve flow. * Prefer the **functional programming** style. ## 📚 Documentation Main changes in the documentation: * Add relevant content from the original ESLint rule for context. * Move up the **Options** section and clarify all possibilities. * Clarify the examples with **detailed sections for each option**, including correct and incorrect usage. **Next steps (if accepted):** * Apply this documentation format and refactoring approach to other rules in future PRs for consistency across the project. ## 📖 Rule documentation skeleton ``` # Rule Name ## What it does _A concise description of what the rule enforces or prevents. Briefly mention the relevant language construct(s) and what aspects are controlled._ --- ## Why is this bad? _Explain why not following this rule can lead to issues such as readability, maintainability, or bugs._ --- ## Options **First option:** - Type: `string` - Enum: `"<option1>"`, `"<option2>"`, ... - Default: `"<defaultOption>"` _Possible values (add descriptions for each value):_ - `<option1>`: _Explain what this enforces or prevents._ - `<option2>`: _Explain what this enforces or prevents._ - ... **Second option (if applicable):** - Type: `object` - Properties: - `<propertyName>`: `<type>` (default: `<defaultValue>`) - _Explanation of what this property controls._ _Note: Add any relevant caveats, such as when an option is only used with a particular mode/value._ **Example configuration:** ```json { "<rule-name>": ["error", "<option>", { "<propertyName>": <value> }] } `` --- ## Examples _Organize examples under sections for each main option._ ### `"<option>"` #### Incorrect ```js // Code that violates the rule with this option `` #### Correct ```js // Compliant code with this rule/option `` _Repeat the above for each relevant option, such as:_ ### `"<option>" with { "<propertyName>": true }` #### Incorrect ```js // Code sample `` #### Correct ```js // Code sample `` ``` --------- Co-authored-by: Cameron Clark <cameron.clark@hey.com>
1 parent fd3233c commit c9cb2a4

File tree

1 file changed

+156
-99
lines changed

1 file changed

+156
-99
lines changed

‎crates/oxc_linter/src/rules/eslint/arrow_body_style.rs‎

Lines changed: 156 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,28 @@
1+
use serde_json::Value;
2+
3+
use oxc_allocator::Box as OxcBox;
14
use oxc_ast::{
25
AstKind,
6+
ast::{ArrowFunctionExpression, FunctionBody, ReturnStatement},
37
ast::{Expression, Statement},
48
};
59
use oxc_diagnostics::OxcDiagnostic;
610
use oxc_macros::declare_oxc_lint;
711
use oxc_span::Span;
8-
use serde_json::Value;
912

1013
use crate::{AstNode, context::LintContext, rule::Rule};
1114

1215
fn arrow_body_style_diagnostic(span: Span, msg: &str) -> OxcDiagnostic {
1316
OxcDiagnostic::warn(msg.to_string()).with_label(span)
1417
}
1518

19+
fn diagnostic_expected_block(ctx: &LintContext, span: Span) {
20+
ctx.diagnostic(arrow_body_style_diagnostic(
21+
span,
22+
"Expected block statement surrounding arrow body.",
23+
));
24+
}
25+
1626
#[derive(Debug, Default, PartialEq, Clone)]
1727
enum Mode {
1828
#[default]
@@ -29,18 +39,6 @@ impl Mode {
2939
_ => Self::AsNeeded,
3040
}
3141
}
32-
33-
pub fn is_always(&self) -> bool {
34-
matches!(self, Self::Always)
35-
}
36-
37-
pub fn is_never(&self) -> bool {
38-
matches!(self, Self::Never)
39-
}
40-
41-
pub fn is_as_needed(&self) -> bool {
42-
matches!(self, Self::AsNeeded)
43-
}
4442
}
4543

4644
#[derive(Debug, Default, Clone)]
@@ -53,89 +51,132 @@ declare_oxc_lint!(
5351
/// ### What it does
5452
///
5553
/// This rule can enforce or disallow the use of braces around arrow function body.
54+
/// Arrow functions can use either:
55+
/// - a block body `() => { ... }`
56+
/// - or a concise body `() => expression` with an implicit return.
5657
///
5758
/// ### Why is this bad?
5859
///
59-
/// Arrow functions have two syntactic forms for their function bodies.
60-
/// They may be defined with a block body (denoted by curly braces) () => { ... }
61-
/// or with a single expression () => ..., whose value is implicitly returned.
60+
/// Inconsistent use of block vs. concise bodies makes code harder to read.
61+
/// Concise bodies are limited to a single expression, whose value is implicitly returned.
62+
///
63+
/// ### Options
64+
///
65+
/// First option:
66+
/// - Type: `string`
67+
/// - Enum: `"always"`, `"as-needed"`, `"never"`
68+
/// - Default: `"never"`
69+
///
70+
/// Possible values:
71+
/// * `never` enforces no braces where they can be omitted (default)
72+
/// * `always` enforces braces around the function body
73+
/// * `as-needed` enforces no braces around the function body (constrains arrow functions to the role of returning an expression)
74+
///
75+
/// Second option:
76+
/// - Type: `object`
77+
/// - Properties:
78+
/// - `requireReturnForObjectLiteral`: `boolean` (default: `false`) - requires braces and an explicit return for object literals.
79+
///
80+
/// Note: This option only applies when the first option is `"as-needed"`.
81+
///
82+
/// Example configuration:
83+
/// ```json
84+
/// {
85+
/// "arrow-body-style": ["error", "as-needed", { "requireReturnForObjectLiteral": true }]
86+
/// }
87+
/// ```
6288
///
6389
/// ### Examples
6490
///
91+
/// #### `"never"` (default)
92+
///
93+
/// Examples of **incorrect** code for this rule with the `never` option:
94+
/// ```js
95+
/// /* arrow-body-style: ["error", "never"] */
96+
///
97+
/// /* ✘ Bad: */
98+
/// const foo = () => {
99+
/// return 0;
100+
/// };
101+
/// ```
102+
///
103+
/// Examples of **correct** code for this rule with the `never` option:
104+
/// ```js
105+
/// /* arrow-body-style: ["error", "never"] */
106+
///
107+
/// /* ✔ Good: */
108+
/// const foo = () => 0;
109+
/// const bar = () => ({ foo: 0 });
110+
/// ```
111+
///
112+
/// #### `"always"`
113+
///
65114
/// Examples of **incorrect** code for this rule with the `always` option:
66115
/// ```js
116+
/// /* arrow-body-style: ["error", "always"] */
117+
///
118+
/// /* ✘ Bad: */
67119
/// const foo = () => 0;
68120
/// ```
69121
///
70122
/// Examples of **correct** code for this rule with the `always` option:
71123
/// ```js
124+
/// /* arrow-body-style: ["error", "always"] */
125+
///
126+
/// /* ✔ Good: */
72127
/// const foo = () => {
73128
/// return 0;
74129
/// };
75130
/// ```
76131
///
132+
/// #### `"as-needed"`
133+
///
77134
/// Examples of **incorrect** code for this rule with the `as-needed` option:
78135
/// ```js
136+
/// /* arrow-body-style: ["error", "as-needed"] */
137+
///
138+
/// /* ✘ Bad: */
79139
/// const foo = () => {
80140
/// return 0;
81141
/// };
82142
/// ```
83143
///
84144
/// Examples of **correct** code for this rule with the `as-needed` option:
85145
/// ```js
146+
/// /* arrow-body-style: ["error", "as-needed"] */
147+
///
148+
/// /* ✔ Good: */
86149
/// const foo1 = () => 0;
87150
///
88151
/// const foo2 = (retv, name) => {
89152
/// retv[name] = true;
90153
/// return retv;
91154
/// };
155+
///
156+
/// const foo3 = () => {
157+
/// bar();
158+
/// };
92159
/// ```
93160
///
94-
/// Examples of **incorrect** code for this rule with the { "requireReturnForObjectLiteral": true } option:
161+
/// #### `"as-needed"` with `requireReturnForObjectLiteral`
162+
///
163+
/// Examples of **incorrect** code for this rule with the `{ "requireReturnForObjectLiteral": true }` option:
95164
/// ```js
96165
/// /* arrow-body-style: ["error", "as-needed", { "requireReturnForObjectLiteral": true }]*/
166+
///
167+
/// /* ✘ Bad: */
97168
/// const foo = () => ({});
98169
/// const bar = () => ({ bar: 0 });
99170
/// ```
100171
///
101-
/// Examples of **correct** code for this rule with the { "requireReturnForObjectLiteral": true } option:
172+
/// Examples of **correct** code for this rule with the `{ "requireReturnForObjectLiteral": true }` option:
102173
/// ```js
103174
/// /* arrow-body-style: ["error", "as-needed", { "requireReturnForObjectLiteral": true }]*/
175+
///
176+
/// /* ✔ Good: */
104177
/// const foo = () => {};
105178
/// const bar = () => { return { bar: 0 }; };
106179
/// ```
107-
///
108-
/// Examples of **incorrect** code for this rule with the `never` option:
109-
/// ```js
110-
/// const foo = () => {
111-
/// return 0;
112-
/// };
113-
/// ```
114-
///
115-
/// Examples of **correct** code for this rule with the `never` option:
116-
/// ```js
117-
/// const foo = () => 0;
118-
/// const bar = () => ({ foo: 0 });
119-
/// ```
120-
///
121-
/// ### Options
122-
///
123-
/// The rule takes one or two options. The first is a string, which can be:
124-
///
125-
/// * `always` enforces braces around the function body
126-
/// * `never` enforces no braces where they can be omitted (default)
127-
/// * `as-needed` enforces no braces around the function body (constrains arrow functions to the role of returning an expression)
128-
///
129-
/// The second one is an object for more fine-grained configuration
130-
/// when the first option is "as-needed". Currently,
131-
/// the only available option is requireReturnForObjectLiteral, a boolean property.
132-
/// It’s false by default. If set to true, it requires braces and an explicit return for object literals.
133-
///
134-
/// ```json
135-
/// {
136-
/// "arrow-body-style": ["error", "as-needed", { "requireReturnForObjectLiteral": true }]
137-
/// }
138-
/// ```
139180
ArrowBodyStyle,
140181
eslint,
141182
style,
@@ -144,69 +185,85 @@ declare_oxc_lint!(
144185

145186
impl Rule for ArrowBodyStyle {
146187
fn from_configuration(value: Value) -> Self {
147-
let obj1 = value.get(0);
148-
let obj2 = value.get(1);
188+
let mode = value.get(0).and_then(Value::as_str).map(Mode::from).unwrap_or_default();
149189

150-
Self {
151-
mode: obj1.and_then(Value::as_str).map(Mode::from).unwrap_or_default(),
152-
require_return_for_object_literal: obj2
153-
.and_then(|v| v.get("requireReturnForObjectLiteral"))
154-
.and_then(Value::as_bool)
155-
.unwrap_or(false),
156-
}
190+
let require_return_for_object_literal = value
191+
.get(1)
192+
.and_then(|v| v.get("requireReturnForObjectLiteral"))
193+
.and_then(Value::as_bool)
194+
.unwrap_or(false);
195+
196+
Self { mode, require_return_for_object_literal }
157197
}
158-
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
198+
199+
fn run(&self, node: &AstNode, ctx: &LintContext) {
159200
let AstKind::ArrowFunctionExpression(arrow_func_expr) = node.kind() else {
160201
return;
161202
};
162-
let body = &arrow_func_expr.body;
163-
let statements = &body.statements;
164203

165204
if arrow_func_expr.expression {
166-
if self.mode.is_always() {
167-
ctx.diagnostic(arrow_body_style_diagnostic(
168-
body.span,
169-
"Expected block statement surrounding arrow body.",
170-
));
171-
}
172-
if self.mode.is_as_needed() && self.require_return_for_object_literal {
173-
if let Some(Expression::ObjectExpression(_)) =
174-
arrow_func_expr.get_expression().map(Expression::get_inner_expression)
175-
{
176-
ctx.diagnostic(arrow_body_style_diagnostic(
177-
body.span,
178-
"Expected block statement surrounding arrow body.",
179-
));
180-
}
181-
}
205+
self.run_for_arrow_expression(arrow_func_expr, ctx);
182206
} else {
183-
if self.mode.is_never() {
184-
let msg = if statements.is_empty() {
207+
self.run_for_arrow_block(&arrow_func_expr.body, ctx);
208+
}
209+
}
210+
}
211+
212+
impl ArrowBodyStyle {
213+
fn run_for_arrow_expression(
214+
&self,
215+
arrow_func_expr: &ArrowFunctionExpression,
216+
ctx: &LintContext,
217+
) {
218+
let body = &arrow_func_expr.body;
219+
220+
match (
221+
&self.mode,
222+
&self.require_return_for_object_literal,
223+
arrow_func_expr.get_expression().map(Expression::get_inner_expression),
224+
) {
225+
(Mode::Always, _, _) => diagnostic_expected_block(ctx, body.span),
226+
(Mode::AsNeeded, true, Some(Expression::ObjectExpression(_))) => {
227+
diagnostic_expected_block(ctx, body.span);
228+
}
229+
_ => {}
230+
}
231+
}
232+
233+
fn run_for_arrow_block_return_statement(
234+
&self,
235+
return_statement: &OxcBox<ReturnStatement>,
236+
body: &FunctionBody,
237+
ctx: &LintContext,
238+
) {
239+
if self.require_return_for_object_literal
240+
&& matches!(return_statement.argument, Some(Expression::ObjectExpression(_)))
241+
{
242+
return;
243+
}
244+
245+
ctx.diagnostic(arrow_body_style_diagnostic(
246+
body.span,
247+
"Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`.",
248+
));
249+
}
250+
251+
fn run_for_arrow_block(&self, body: &FunctionBody, ctx: &LintContext) {
252+
match self.mode {
253+
Mode::Never => {
254+
let msg = if body.statements.is_empty() {
185255
"Unexpected block statement surrounding arrow body; put a value of `undefined` immediately after the `=>`."
186256
} else {
187257
"Unexpected block statement surrounding arrow body."
188258
};
189259
ctx.diagnostic(arrow_body_style_diagnostic(body.span, msg));
190260
}
191-
if self.mode.is_as_needed() {
192-
// check is there only one `ReturnStatement`
193-
if statements.len() != 1 {
194-
return;
195-
}
196-
let inner_statement = &statements[0];
197-
198-
if let Statement::ReturnStatement(return_statement) = inner_statement {
199-
let return_val = &return_statement.argument;
200-
if self.require_return_for_object_literal
201-
&& return_val
202-
.as_ref()
203-
.is_some_and(|v| matches!(v, Expression::ObjectExpression(_)))
204-
{
205-
return;
206-
}
207-
ctx.diagnostic(arrow_body_style_diagnostic(body.span, "Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`."));
261+
Mode::AsNeeded if body.statements.len() == 1 => {
262+
if let Statement::ReturnStatement(return_statement) = &body.statements[0] {
263+
self.run_for_arrow_block_return_statement(return_statement, body, ctx);
208264
}
209265
}
266+
_ => {}
210267
}
211268
}
212269
}
@@ -222,7 +279,7 @@ fn test() {
222279
("var foo = () => { /* do nothing */ };", None),
223280
(
224281
"var foo = () => {
225-
/* do nothing */
282+
/* do nothing */
226283
};",
227284
None,
228285
),
@@ -260,7 +317,7 @@ fn test() {
260317
),
261318
(
262319
"var foo = () => {
263-
/* do nothing */
320+
/* do nothing */
264321
};",
265322
Some(serde_json::json!(["as-needed", { "requireReturnForObjectLiteral": true }])),
266323
),

0 commit comments

Comments
 (0)