Skip to content

Commit

Permalink
fix(linter): no-else-return fixer fails when else has no trailing…
Browse files Browse the repository at this point in the history 
… whitespace (#6348)

Fixes a bug in `eslint/no_else_return`'s fixer where fixes were not being
property applied when `else` had no whitespace immediately after it. For
example:
```js
if(x){ return x }else{ return y }
```

I also refactored the rule's fixer to avoid string allocations as much as
possible.
  • Loading branch information
@DonIsaac
DonIsaac committed Oct 7, 2024
1 parent a1e0d30 commit 71ad5d3
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 96 deletions.
126 changes: 75 additions & 51 deletions crates/oxc_linter/src/rules/eslint/no_else_return.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::{context::LintContext, rule::Rule, AstNode};
use cow_utils::CowUtils;
use oxc_ast::{ast::Statement, AstKind};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
Expand Down Expand Up @@ -163,7 +162,7 @@ declare_oxc_lint!(
/// ```
NoElseReturn,
pedantic,
fix
conditional_fix
);

fn no_else_return_diagnostic(else_stmt: &Statement) -> OxcDiagnostic {
Expand Down Expand Up @@ -200,42 +199,69 @@ fn is_safe_from_name_collisions(
}
}

fn replace_block(s: &str) -> String {
if s.starts_with('{') && s.ends_with('}') && s.len() > 1 {
s[1..s.len() - 1].to_string()
} else {
s.to_string()
}
}

fn no_else_return_diagnostic_fix(
ctx: &LintContext,
else_stmt_prev: &Statement,
else_stmt: &Statement,
if_block_node: &AstNode,
) {
let diagnostic = no_else_return_diagnostic(else_stmt);
let parent_scope_id = if_block_node.scope_id();

if !is_safe_from_name_collisions(ctx, else_stmt, parent_scope_id) {
return ctx.diagnostic(no_else_return_diagnostic(else_stmt));
};
ctx.diagnostic(diagnostic);
return;
}
ctx.diagnostic_with_fix(diagnostic, |fixer| {
let prev_span = else_stmt_prev.span();
let else_content_span = else_stmt.span();
let target_span = Span::new(prev_span.end, else_content_span.end);

let prev_span = else_stmt_prev.span();
let span = else_stmt.span();
// Capture the contents of the `else` statement, removing curly braces
// for block statements
let mut replacement_span = if let Statement::BlockStatement(block) = else_stmt {
let first_stmt_start = block.body.first().map(|stmt| stmt.span().start);
let last_stmt_end = block.body.last().map(|stmt| stmt.span().end);
let (Some(start), Some(end)) = (first_stmt_start, last_stmt_end) else {
return fixer.noop();
};
Span::new(start, end)
} else {
else_content_span
};

let else_code = ctx.source_range(span);
let else_code_prev_token = ctx
.source_range(Span::new(prev_span.end, span.end))
.cow_replacen("else ", "", 1)
.cow_replace(else_code, "")
.to_string();
let fix_else_code = else_code_prev_token + &replace_block(else_code);
// expand the span start leftwards to include any leading whitespace
replacement_span =
replacement_span.expand_left(left_offset_for_whitespace(ctx, replacement_span.start));

ctx.diagnostic_with_fix(no_else_return_diagnostic(else_stmt), |fixer| {
fixer.replace(Span::new(prev_span.end, span.end), fix_else_code)
// Check if if statement's consequent block could introduce an ASI
// hazard when `else` is removed.
let needs_newline = match else_stmt_prev {
Statement::ExpressionStatement(s) => !ctx.source_range(s.span).ends_with(';'),
Statement::ReturnStatement(s) => !ctx.source_range(s.span).ends_with(';'),
_ => false,
};
if needs_newline {
let replacement = ctx.source_range(replacement_span);
fixer.replace(target_span, "\n".to_string() + replacement)
} else {
fixer.replace_with(&target_span, &replacement_span)
}
});
}

#[allow(clippy::cast_possible_truncation)]
fn left_offset_for_whitespace(ctx: &LintContext, position: u32) -> u32 {
if position == 0 {
return position;
}

let chars = ctx.source_text()[..(position as usize)].chars().rev();
let offset = chars.take_while(|c| c.is_whitespace()).count();
debug_assert!(offset < u32::MAX as usize);
offset as u32
}

fn naive_has_return(node: &Statement) -> bool {
match node {
Statement::BlockStatement(block) => {
Expand Down Expand Up @@ -473,48 +499,46 @@ fn test() {
];

let fix = vec![
("function foo1() { if (true) { return x; } else { return y; } }", "function foo1() { if (true) { return x; } return y; }", None),
("function foo2() { if (true) { var x = bar; return x; } else { var y = baz; return y; } }", "function foo2() { if (true) { var x = bar; return x; } var y = baz; return y; }", None),
("function foo1() { if (true) { return x; } else { return y; } }", "function foo1() { if (true) { return x; } return y; }", None),
("function foo1() { if(true){ return x; }else{ return y; } }", "function foo1() { if(true){ return x; } return y; }", None),
("function foo2() { if (true) { var x = bar; return x; } else { var y = baz; return y; } }", "function foo2() { if (true) { var x = bar; return x; } var y = baz; return y; }", None),
("function foo3() { if (true) return x; else return y; }", "function foo3() { if (true) return x; return y; }", None),
("function foo4() { if (true) { if (false) return x; else return y; } else { return z; } }", "function foo4() { if (true) { if (false) return x; return y; } return z; }", None),
("function foo5() { if (true) { if (false) { if (true) return x; else { w = y; } } else { w = x; } } else { return z; } }", "function foo5() { if (true) { if (false) { if (true) return x; w = y; } else { w = x; } } else { return z; } }", None),
("function foo4() { if (true) { if (false) return x; else return y; } else { return z; } }", "function foo4() { if (true) { if (false) return x; return y; } return z; }", None),
("function foo5() { if (true) { if (false) { if (true) return x; else { w = y; } } else { w = x; } } else { return z; } }", "function foo5() { if (true) { if (false) { if (true) return x; w = y; } else { w = x; } } else { return z; } }", None),
("function foo6() { if (true) { if (false) { if (true) return x; else return y; } } else { return z; } }", "function foo6() { if (true) { if (false) { if (true) return x; return y; } } else { return z; } }", None),
("function foo7() { if (true) { if (false) { if (true) return x; else return y; } return w; } else { return z; } }", "function foo7() { if (true) { if (false) { if (true) return x; return y; } return w; } return z; }", None),
("function foo8() { if (true) { if (false) { if (true) return x; else return y; } else { w = x; } } else { return z; } }", "function foo8() { if (true) { if (false) { if (true) return x; return y; } w = x; } else { return z; } }", None),
("function foo9() {if (x) { return true; } else if (y) { return true; } else { notAReturn(); } }", "function foo9() {if (x) { return true; } else if (y) { return true; } notAReturn(); }", None),
("function foo7() { if (true) { if (false) { if (true) return x; else return y; } return w; } else { return z; } }", "function foo7() { if (true) { if (false) { if (true) return x; return y; } return w; } return z; }", None),
("function foo8() { if (true) { if (false) { if (true) return x; else return y; } else { w = x; } } else { return z; } }", "function foo8() { if (true) { if (false) { if (true) return x; return y; } w = x; } else { return z; } }", None),
("function foo9() {if (x) { return true; } else if (y) { return true; } else { notAReturn(); } }", "function foo9() {if (x) { return true; } else if (y) { return true; } notAReturn(); }", None),
("function foo9a() {if (x) { return true; } else if (y) { return true; } else { notAReturn(); } }", "function foo9a() {if (x) { return true; } if (y) { return true; } else { notAReturn(); } }", Some(serde_json::json!([{ "allowElseIf": false }]))),
("function foo9b() {if (x) { return true; } if (y) { return true; } else { notAReturn(); } }", "function foo9b() {if (x) { return true; } if (y) { return true; } notAReturn(); }", Some(serde_json::json!([{ "allowElseIf": false }]))),
("function foo9b() {if (x) { return true; } if (y) { return true; } else { notAReturn(); } }", "function foo9b() {if (x) { return true; } if (y) { return true; } notAReturn(); }", Some(serde_json::json!([{ "allowElseIf": false }]))),
("function foo10() { if (foo) return bar; else (foo).bar(); }", "function foo10() { if (foo) return bar; (foo).bar(); }", None),
("function foo13() { if (foo) return bar;
else { [1, 2, 3].map(foo) } }", "function foo13() { if (foo) return bar;
[1, 2, 3].map(foo) }", None),
else { [1, 2, 3].map(foo) } }", "function foo13() { if (foo) return bar; [1, 2, 3].map(foo) }", None),
("function foo14() { if (foo) return bar
else { baz(); }
[1, 2, 3].map(foo) }", "function foo14() { if (foo) return bar
baz();
[1, 2, 3].map(foo) }", "function foo14() { if (foo) return bar\n baz();
[1, 2, 3].map(foo) }", None),
("function foo17() { if (foo) return bar
else { baz() }
qaz() }", "function foo17() { if (foo) return bar
baz()
qaz() }", "function foo17() { if (foo) return bar\n baz()
qaz() }", None),
("function foo19() { if (true) { return x; } else if (false) { return y; } }", "function foo19() { if (true) { return x; } if (false) { return y; } }", Some(serde_json::json!([{ "allowElseIf": false }]))),
("function foo20() {if (x) { return true; } else if (y) { notAReturn() } else { notAReturn(); } }", "function foo20() {if (x) { return true; } if (y) { notAReturn() } else { notAReturn(); } }", Some(serde_json::json!([{ "allowElseIf": false }]))),
("function foo21() { var x = true; if (x) { return x; } else if (x === false) { return false; } }", "function foo21() { var x = true; if (x) { return x; } if (x === false) { return false; } }", Some(serde_json::json!([{ "allowElseIf": false }]))),
("function foo() { var a; if (bar) { return true; } else { var a; } }", "function foo() { var a; if (bar) { return true; } var a; }", None),
("function foo() { if (bar) { var a; if (baz) { return true; } else { var a; } } }", "function foo() { if (bar) { var a; if (baz) { return true; } var a; } }", None),
("function foo() { var a; if (bar) { return true; } else { var a; } }", "function foo() { var a; if (bar) { return true; } var a; }", None),
("function foo() { if (bar) { var a; if (baz) { return true; } else { var a; } } }", "function foo() { if (bar) { var a; if (baz) { return true; } var a; } }", None),
("function foo() {let a; if (bar) { if (baz) { return true; } else { let a; } } }", "function foo() {let a; if (bar) { if (baz) { return true; } let a; } }", None),
("function foo() { try {} catch (a) { if (bar) { if (baz) { return true; } else { let a; } } } }", "function foo() { try {} catch (a) { if (bar) { if (baz) { return true; } let a; } } }", None),
("function foo() { if (bar) { return true; } else { let arguments; } }", "function foo() { if (bar) { return true; } let arguments; }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let arguments; } } }", "function foo() { if (bar) { if (baz) { return true; } let arguments; } }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let a; } } a; }", "function foo() { if (bar) { if (baz) { return true; } let a; } a; }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let a; } } if (quux) { var a; } }", "function foo() { if (bar) { if (baz) { return true; } let a; } if (quux) { var a; } }", None),
("function foo() { if (quux) { var a; } if (bar) { if (baz) { return true; } else { let a; } } }", "function foo() { if (quux) { var a; } if (bar) { if (baz) { return true; } let a; } }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let a; } } if (quux) { function a(){} } }", "function foo() { if (bar) { if (baz) { return true; } let a; } if (quux) { function a(){} } }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let a; } } function a(){} }", "function foo() { if (bar) { if (baz) { return true; } let a; } function a(){} }", None),
("if (foo) { return true; } else { let a; }", "if (foo) { return true; } let a; ", None)
("function foo() { var a; if (bar) { return true; } else { var a; } }", "function foo() { var a; if (bar) { return true; } var a; }", None),
("function foo() { if (bar) { var a; if (baz) { return true; } else { var a; } } }", "function foo() { if (bar) { var a; if (baz) { return true; } var a; } }", None),
("function foo() { var a; if (bar) { return true; } else { var a; } }", "function foo() { var a; if (bar) { return true; } var a; }", None),
("function foo() { if (bar) { var a; if (baz) { return true; } else { var a; } } }", "function foo() { if (bar) { var a; if (baz) { return true; } var a; } }", None),
("function foo() {let a; if (bar) { if (baz) { return true; } else { let a; } } }", "function foo() {let a; if (bar) { if (baz) { return true; } let a; } }", None),
("function foo() { try {} catch (a) { if (bar) { if (baz) { return true; } else { let a; } } } }", "function foo() { try {} catch (a) { if (bar) { if (baz) { return true; } let a; } } }", None),
("function foo() { if (bar) { return true; } else { let arguments; } }", "function foo() { if (bar) { return true; } let arguments; }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let arguments; } } }", "function foo() { if (bar) { if (baz) { return true; } let arguments; } }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let a; } } a; }", "function foo() { if (bar) { if (baz) { return true; } let a; } a; }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let a; } } if (quux) { var a; } }", "function foo() { if (bar) { if (baz) { return true; } let a; } if (quux) { var a; } }", None),
("function foo() { if (quux) { var a; } if (bar) { if (baz) { return true; } else { let a; } } }", "function foo() { if (quux) { var a; } if (bar) { if (baz) { return true; } let a; } }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let a; } } if (quux) { function a(){} } }", "function foo() { if (bar) { if (baz) { return true; } let a; } if (quux) { function a(){} } }", None),
("function foo() { if (bar) { if (baz) { return true; } else { let a; } } function a(){} }", "function foo() { if (bar) { if (baz) { return true; } let a; } function a(){} }", None),
("if (foo) { return true; } else { let a; }", "if (foo) { return true; } let a;", None)
];
Tester::new(NoElseReturn::NAME, pass, fail).expect_fix(fix).test_and_snapshot();
}
Loading

0 comments on commit 71ad5d3

Please sign in to comment.