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.

Sign up for GitHub

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

Jump to bottom

fix(linter): no-else-return fixer fails when else has no trailing whitespace #6348

Merged
merged 1 commit into from
Oct 7, 2024
Merged

fix(linter): no-else-return fixer fails when else has no trailing whitespace #6348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
126 changes: 75 additions & 51 deletions crates/oxc_linter/src/rules/eslint/no_else_return.rs
View file
Open in desktop
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