Skip to content

Commit 00df6e5

Browse files
committedOct 8, 2024·
fix(linter): friendly diagnostic messages for no-else-return (#6349)
### Before ``` ⚠ eslint(no-else-return): Unnecessary 'else' after 'return'. ╭─[no_else_return.tsx:1:48] 1 │ function foo1() { if (true) { return x; } else { return y; } } · ───────────── ╰──── help: Replace ` else { return y; }` with ` return y; `. ``` ### After ``` ⚠ eslint(no-else-return): Unnecessary 'else' after 'return'. ╭─[no_else_return.tsx:1:31] 1 │ function foo1() { if (true) { return x; } else { return y; } } · ────┬──── ───┬── · │ ╰── Making this `else` block unnecessary. · ╰── This consequent block always returns, ╰──── help: Remove the `else` block, moving its contents outside of the `if` statement. ```
1 parent 40932f7 commit 00df6e5

File tree

2 files changed

+468
-274
lines changed

2 files changed

+468
-274
lines changed
 

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

+41-26
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,13 @@ declare_oxc_lint!(
165165
conditional_fix
166166
);
167167

168-
fn no_else_return_diagnostic(else_stmt: &Statement) -> OxcDiagnostic {
169-
OxcDiagnostic::warn("Unnecessary 'else' after 'return'.").with_label(else_stmt.span())
168+
fn no_else_return_diagnostic(else_keyword: Span, last_return: Span) -> OxcDiagnostic {
169+
OxcDiagnostic::warn("Unnecessary 'else' after 'return'.")
170+
.with_labels([
171+
last_return.label("This consequent block always returns,"),
172+
else_keyword.label("Making this `else` block unnecessary."),
173+
])
174+
.with_help("Remove the `else` block, moving its contents outside of the `if` statement.")
170175
}
171176

172177
fn is_safe_from_name_collisions(
@@ -201,20 +206,22 @@ fn is_safe_from_name_collisions(
201206

202207
fn no_else_return_diagnostic_fix(
203208
ctx: &LintContext,
209+
last_return_span: Span,
204210
else_stmt_prev: &Statement,
205211
else_stmt: &Statement,
206212
if_block_node: &AstNode,
207213
) {
208-
let diagnostic = no_else_return_diagnostic(else_stmt);
214+
let prev_span = else_stmt_prev.span();
215+
let else_content_span = else_stmt.span();
216+
let else_keyword_span = Span::new(prev_span.end, else_content_span.start);
217+
let diagnostic = no_else_return_diagnostic(else_keyword_span, last_return_span);
209218
let parent_scope_id = if_block_node.scope_id();
210219

211220
if !is_safe_from_name_collisions(ctx, else_stmt, parent_scope_id) {
212221
ctx.diagnostic(diagnostic);
213222
return;
214223
}
215224
ctx.diagnostic_with_fix(diagnostic, |fixer| {
216-
let prev_span = else_stmt_prev.span();
217-
let else_content_span = else_stmt.span();
218225
let target_span = Span::new(prev_span.end, else_content_span.end);
219226

220227
// Capture the contents of the `else` statement, removing curly braces
@@ -262,35 +269,41 @@ fn left_offset_for_whitespace(ctx: &LintContext, position: u32) -> u32 {
262269
offset as u32
263270
}
264271

265-
fn naive_has_return(node: &Statement) -> bool {
272+
fn naive_has_return(node: &Statement) -> Option<Span> {
266273
match node {
267274
Statement::BlockStatement(block) => {
268-
let Some(last_child) = block.body.last() else {
269-
return false;
270-
};
271-
matches!(last_child, Statement::ReturnStatement(_))
275+
let last_child = block.body.last()?;
276+
if let Statement::ReturnStatement(r) = last_child {
277+
Some(r.span)
278+
} else {
279+
None
280+
}
272281
}
273-
Statement::ReturnStatement(_) => true,
274-
_ => false,
282+
Statement::ReturnStatement(r) => Some(r.span),
283+
_ => None,
275284
}
276285
}
277286

278-
fn check_for_return_or_if(node: &Statement) -> bool {
287+
fn check_for_return_or_if(node: &Statement) -> Option<Span> {
279288
match node {
280-
Statement::ReturnStatement(_) => true,
289+
Statement::ReturnStatement(r) => Some(r.span),
281290
Statement::IfStatement(if_stmt) => {
282-
let Some(alternate) = &if_stmt.alternate else {
283-
return false;
284-
};
285-
naive_has_return(alternate) && naive_has_return(&if_stmt.consequent)
291+
let alternate = if_stmt.alternate.as_ref()?;
292+
if let (Some(_), Some(ret_span)) =
293+
(naive_has_return(alternate), naive_has_return(&if_stmt.consequent))
294+
{
295+
Some(ret_span)
296+
} else {
297+
None
298+
}
286299
}
287-
_ => false,
300+
_ => None,
288301
}
289302
}
290303

291-
fn always_returns(stmt: &Statement) -> bool {
304+
fn always_returns(stmt: &Statement) -> Option<Span> {
292305
match stmt {
293-
Statement::BlockStatement(block) => block.body.iter().any(check_for_return_or_if),
306+
Statement::BlockStatement(block) => block.body.iter().find_map(check_for_return_or_if),
294307
node => check_for_return_or_if(node),
295308
}
296309
}
@@ -303,8 +316,8 @@ fn check_if_with_else(ctx: &LintContext, node: &AstNode) {
303316
return;
304317
};
305318

306-
if always_returns(&if_stmt.consequent) {
307-
no_else_return_diagnostic_fix(ctx, &if_stmt.consequent, alternate, node);
319+
if let Some(last_return_span) = always_returns(&if_stmt.consequent) {
320+
no_else_return_diagnostic_fix(ctx, last_return_span, &if_stmt.consequent, alternate, node);
308321
}
309322
}
310323

@@ -315,16 +328,18 @@ fn check_if_without_else(ctx: &LintContext, node: &AstNode) {
315328
let mut current_node = if_stmt;
316329
let mut last_alternate;
317330
let mut last_alternate_prev;
331+
let mut last_return_span;
318332

319333
loop {
320334
let Some(alternate) = &current_node.alternate else {
321335
return;
322336
};
323-
if !always_returns(&current_node.consequent) {
337+
let Some(ret_span) = always_returns(&current_node.consequent) else {
324338
return;
325-
}
339+
};
326340
last_alternate_prev = &current_node.consequent;
327341
last_alternate = alternate;
342+
last_return_span = ret_span;
328343
match alternate {
329344
Statement::IfStatement(if_stmt) => {
330345
current_node = if_stmt;
@@ -333,7 +348,7 @@ fn check_if_without_else(ctx: &LintContext, node: &AstNode) {
333348
}
334349
}
335350

336-
no_else_return_diagnostic_fix(ctx, last_alternate_prev, last_alternate, node);
351+
no_else_return_diagnostic_fix(ctx, last_return_span, last_alternate_prev, last_alternate, node);
337352
}
338353

339354
impl Rule for NoElseReturn {

‎crates/oxc_linter/src/snapshots/no_else_return.snap

+427-248
Large diffs are not rendered by default.

0 commit comments

Comments
 (0)
Please sign in to comment.