Skip to content

Commit 7f434d3

Browse files
DunqingDunqing
committed
feat(formatter): correctly handle semicolon
1 parent 2436cab commit 7f434d3

File tree

9 files changed

+200
-109
lines changed

9 files changed

+200
-109
lines changed

‎crates/oxc_formatter/examples/formatter.rs‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,19 +8,21 @@
88
//! Create a `test.js` file and run:
99
//! ```bash
1010
//! cargo run -p oxc_formatter --example formatter [filename]
11+
//! cargo run -p oxc_formatter --example formatter -- --no-semi [filename]
1112
//! ```
1213
1314
use std::{fs, path::Path};
1415

1516
use oxc_allocator::Allocator;
16-
use oxc_formatter::{BracketSameLine, FormatOptions, Formatter};
17+
use oxc_formatter::{BracketSameLine, FormatOptions, Formatter, Semicolons};
1718
use oxc_parser::{ParseOptions, Parser};
1819
use oxc_span::SourceType;
1920
use pico_args::Arguments;
2021

2122
/// Format a JavaScript or TypeScript file
2223
fn main() -> Result<(), String> {
2324
let mut args = Arguments::from_env();
25+
let no_semi = args.contains("--no-semi");
2426
let name = args.free_from_str().unwrap_or_else(|_| "test.js".to_string());
2527

2628
// Read source file
@@ -46,8 +48,12 @@ fn main() -> Result<(), String> {
4648
}
4749

4850
// Format the parsed code
49-
let options =
50-
FormatOptions { bracket_same_line: BracketSameLine::from(true), ..Default::default() };
51+
let semicolons = if no_semi { Semicolons::AsNeeded } else { Semicolons::Always };
52+
let options = FormatOptions {
53+
bracket_same_line: BracketSameLine::from(true),
54+
semicolons,
55+
..Default::default()
56+
};
5157
let code = Formatter::new(&allocator, options).build(&ret.program);
5258

5359
println!("{code}");

‎crates/oxc_formatter/src/parentheses/expression.rs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -173,17 +173,16 @@ impl<'a> NeedsParentheses<'a> for AstNode<'a, CallExpression<'a>> {
173173
AstNodes::NewExpression(_) => true,
174174
AstNodes::Decorator(_) => !is_identifier_or_static_member_only(&self.callee),
175175
AstNodes::ExportDefaultDeclaration(_) => {
176-
let callee = &self.callee;
176+
let callee = &self.callee();
177177
let callee_span = callee.span();
178178
let leftmost = ExpressionLeftSide::leftmost(callee);
179179
// require parens for iife and
180180
// when the leftmost expression is not a class expression or a function expression
181181
callee_span != leftmost.span()
182182
&& matches!(
183183
leftmost,
184-
ExpressionLeftSide::Expression(
185-
Expression::ClassExpression(_) | Expression::FunctionExpression(_)
186-
)
184+
ExpressionLeftSide::Expression(e)
185+
if matches!(e.as_ref(), Expression::ClassExpression(_) | Expression::FunctionExpression(_))
187186
)
188187
}
189188
_ => false,

‎crates/oxc_formatter/src/write/arrow_function_expression.rs‎

Lines changed: 63 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -687,31 +687,31 @@ impl<'a> Format<'a> for ArrowChain<'a, '_> {
687687

688688
#[derive(Debug)]
689689
pub enum ExpressionLeftSide<'a, 'b> {
690-
Expression(&'b Expression<'a>),
691-
AssignmentTarget(&'b AssignmentTarget<'a>),
692-
SimpleAssignmentTarget(&'b SimpleAssignmentTarget<'a>),
690+
Expression(&'b AstNode<'a, Expression<'a>>),
691+
AssignmentTarget(&'b AstNode<'a, AssignmentTarget<'a>>),
692+
SimpleAssignmentTarget(&'b AstNode<'a, SimpleAssignmentTarget<'a>>),
693693
}
694694

695-
impl<'a, 'b> From<&'b Expression<'a>> for ExpressionLeftSide<'a, 'b> {
696-
fn from(value: &'b Expression<'a>) -> Self {
695+
impl<'a, 'b> From<&'b AstNode<'a, Expression<'a>>> for ExpressionLeftSide<'a, 'b> {
696+
fn from(value: &'b AstNode<'a, Expression<'a>>) -> Self {
697697
Self::Expression(value)
698698
}
699699
}
700700

701-
impl<'a, 'b> From<&'b AssignmentTarget<'a>> for ExpressionLeftSide<'a, 'b> {
702-
fn from(value: &'b AssignmentTarget<'a>) -> Self {
701+
impl<'a, 'b> From<&'b AstNode<'a, AssignmentTarget<'a>>> for ExpressionLeftSide<'a, 'b> {
702+
fn from(value: &'b AstNode<'a, AssignmentTarget<'a>>) -> Self {
703703
Self::AssignmentTarget(value)
704704
}
705705
}
706706

707-
impl<'a, 'b> From<&'b SimpleAssignmentTarget<'a>> for ExpressionLeftSide<'a, 'b> {
708-
fn from(value: &'b SimpleAssignmentTarget<'a>) -> Self {
707+
impl<'a, 'b> From<&'b AstNode<'a, SimpleAssignmentTarget<'a>>> for ExpressionLeftSide<'a, 'b> {
708+
fn from(value: &'b AstNode<'a, SimpleAssignmentTarget<'a>>) -> Self {
709709
Self::SimpleAssignmentTarget(value)
710710
}
711711
}
712712

713713
impl<'a, 'b> ExpressionLeftSide<'a, 'b> {
714-
pub fn leftmost(expression: &'b Expression<'a>) -> Self {
714+
pub fn leftmost(expression: &'b AstNode<'a, Expression<'a>>) -> Self {
715715
let mut current: Self = expression.into();
716716
loop {
717717
match current.left_expression() {
@@ -729,60 +729,46 @@ impl<'a, 'b> ExpressionLeftSide<'a, 'b> {
729729
/// if the expression has no left side.
730730
pub fn left_expression(&self) -> Option<Self> {
731731
match self {
732-
Self::Expression(expression) => match expression {
733-
Expression::SequenceExpression(expr) => expr.expressions.first().map(Into::into),
734-
Expression::StaticMemberExpression(expr) => Some((&expr.object).into()),
735-
Expression::ComputedMemberExpression(expr) => Some((&expr.object).into()),
736-
Expression::PrivateFieldExpression(expr) => Some((&expr.object).into()),
737-
Expression::TaggedTemplateExpression(expr) => Some((&expr.tag).into()),
738-
Expression::NewExpression(expr) => Some((&expr.callee).into()),
739-
Expression::CallExpression(expr) => Some((&expr.callee).into()),
740-
Expression::ConditionalExpression(expr) => Some((&expr.test).into()),
741-
Expression::TSAsExpression(expr) => Some((&expr.expression).into()),
742-
Expression::TSSatisfiesExpression(expr) => Some((&expr.expression).into()),
743-
Expression::TSNonNullExpression(expr) => Some((&expr.expression).into()),
744-
Expression::AssignmentExpression(expr) => Some(Self::AssignmentTarget(&expr.left)),
745-
Expression::UpdateExpression(expr) => {
732+
Self::Expression(expression) => match expression.as_ast_nodes() {
733+
AstNodes::SequenceExpression(expr) => expr.expressions().first().map(Into::into),
734+
AstNodes::StaticMemberExpression(expr) => Some(expr.object().into()),
735+
AstNodes::ComputedMemberExpression(expr) => Some(expr.object().into()),
736+
AstNodes::PrivateFieldExpression(expr) => Some(expr.object().into()),
737+
AstNodes::TaggedTemplateExpression(expr) => Some(expr.tag().into()),
738+
AstNodes::NewExpression(expr) => Some(expr.callee().into()),
739+
AstNodes::CallExpression(expr) => Some(expr.callee().into()),
740+
AstNodes::ConditionalExpression(expr) => Some(expr.test().into()),
741+
AstNodes::TSAsExpression(expr) => Some(expr.expression().into()),
742+
AstNodes::TSSatisfiesExpression(expr) => Some(expr.expression().into()),
743+
AstNodes::TSNonNullExpression(expr) => Some(expr.expression().into()),
744+
AstNodes::AssignmentExpression(expr) => Some(Self::AssignmentTarget(expr.left())),
745+
AstNodes::UpdateExpression(expr) => {
746746
if expr.prefix {
747747
None
748748
} else {
749-
Some(Self::SimpleAssignmentTarget(&expr.argument))
749+
Some(Self::SimpleAssignmentTarget(expr.argument()))
750750
}
751751
}
752-
Expression::BinaryExpression(binary) => Some((&binary.left).into()),
753-
Expression::LogicalExpression(logical) => Some((&logical.left).into()),
754-
Expression::ChainExpression(chain) => match &chain.expression {
755-
ChainElement::CallExpression(expr) => Some((&expr.callee).into()),
756-
ChainElement::TSNonNullExpression(expr) => Some((&expr.expression).into()),
757-
ChainElement::ComputedMemberExpression(expr) => Some((&expr.object).into()),
758-
ChainElement::StaticMemberExpression(expr) => Some((&expr.object).into()),
759-
ChainElement::PrivateFieldExpression(expr) => Some((&expr.object).into()),
752+
AstNodes::BinaryExpression(binary) => Some(binary.left().into()),
753+
AstNodes::LogicalExpression(logical) => Some(logical.left().into()),
754+
AstNodes::ChainExpression(chain) => match &chain.expression().as_ast_nodes() {
755+
AstNodes::CallExpression(expr) => Some(expr.callee().into()),
756+
AstNodes::TSNonNullExpression(expr) => Some(expr.expression().into()),
757+
AstNodes::ComputedMemberExpression(expr) => Some(expr.object().into()),
758+
AstNodes::StaticMemberExpression(expr) => Some(expr.object().into()),
759+
AstNodes::PrivateFieldExpression(expr) => Some(expr.object().into()),
760+
_ => {
761+
unreachable!()
762+
}
760763
},
761764
_ => None,
762765
},
763-
Self::AssignmentTarget(target) => match target {
764-
match_simple_assignment_target!(AssignmentTarget) => {
765-
Self::SimpleAssignmentTarget(target.to_simple_assignment_target())
766-
.left_expression()
767-
}
768-
_ => None,
769-
},
770-
Self::SimpleAssignmentTarget(target) => match target {
771-
SimpleAssignmentTarget::TSAsExpression(expr) => Some((&expr.expression).into()),
772-
SimpleAssignmentTarget::TSSatisfiesExpression(expr) => {
773-
Some((&expr.expression).into())
774-
}
775-
SimpleAssignmentTarget::TSNonNullExpression(expr) => {
776-
Some((&expr.expression).into())
777-
}
778-
SimpleAssignmentTarget::TSTypeAssertion(expr) => Some((&expr.expression).into()),
779-
SimpleAssignmentTarget::ComputedMemberExpression(expr) => {
780-
Some((&expr.object).into())
781-
}
782-
SimpleAssignmentTarget::StaticMemberExpression(expr) => Some((&expr.object).into()),
783-
SimpleAssignmentTarget::PrivateFieldExpression(expr) => Some((&expr.object).into()),
784-
SimpleAssignmentTarget::AssignmentTargetIdentifier(identifier_reference) => None,
785-
},
766+
Self::AssignmentTarget(target) => {
767+
Self::get_left_side_of_assignment(target.as_ast_nodes())
768+
}
769+
Self::SimpleAssignmentTarget(target) => {
770+
Self::get_left_side_of_assignment(target.as_ast_nodes())
771+
}
786772
}
787773
}
788774

@@ -793,10 +779,24 @@ impl<'a, 'b> ExpressionLeftSide<'a, 'b> {
793779
ExpressionLeftSide::SimpleAssignmentTarget(target) => target.span(),
794780
}
795781
}
782+
783+
fn get_left_side_of_assignment(node: &'b AstNodes<'a>) -> Option<ExpressionLeftSide<'a, 'b>> {
784+
match node {
785+
AstNodes::TSAsExpression(expr) => Some(expr.expression().into()),
786+
AstNodes::TSSatisfiesExpression(expr) => Some(expr.expression().into()),
787+
AstNodes::TSNonNullExpression(expr) => Some(expr.expression().into()),
788+
AstNodes::TSTypeAssertion(expr) => Some(expr.expression().into()),
789+
AstNodes::ComputedMemberExpression(expr) => Some(expr.object().into()),
790+
AstNodes::StaticMemberExpression(expr) => Some(expr.object().into()),
791+
AstNodes::PrivateFieldExpression(expr) => Some(expr.object().into()),
792+
_ => None,
793+
}
794+
}
796795
}
797796

798-
fn should_add_parens(body: &FunctionBody) -> bool {
799-
let Statement::ExpressionStatement(stmt) = body.statements.first().unwrap() else {
797+
fn should_add_parens(body: &AstNode<'_, FunctionBody<'_>>) -> bool {
798+
let AstNodes::ExpressionStatement(stmt) = body.statements().first().unwrap().as_ast_nodes()
799+
else {
800800
unreachable!()
801801
};
802802

@@ -805,11 +805,13 @@ fn should_add_parens(body: &FunctionBody) -> bool {
805805
// case and added by the object expression itself
806806
if matches!(&stmt.expression, Expression::ConditionalExpression(_)) {
807807
!matches!(
808-
ExpressionLeftSide::leftmost(&stmt.expression),
808+
ExpressionLeftSide::leftmost(stmt.expression()),
809809
ExpressionLeftSide::Expression(
810+
e
811+
) if matches!(e.as_ref(),
810812
Expression::ObjectExpression(_)
811-
| Expression::FunctionExpression(_)
812-
| Expression::ClassExpression(_)
813+
| Expression::FunctionExpression(_)
814+
| Expression::ClassExpression(_)
813815
)
814816
)
815817
} else {

‎crates/oxc_formatter/src/write/mod.rs‎

Lines changed: 92 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ use crate::{
5353
},
5454
},
5555
generated::ast_nodes::{AstNode, AstNodes},
56-
options::{FormatTrailingCommas, QuoteProperties, TrailingSeparator},
56+
options::{FormatTrailingCommas, QuoteProperties, Semicolons, TrailingSeparator},
5757
parentheses::NeedsParentheses,
5858
utils::{
5959
assignment_like::AssignmentLike,
@@ -382,7 +382,7 @@ impl<'a> FormatWrite<'a> for AstNode<'a, ArrayAssignmentTarget<'a>> {
382382
group(&soft_block_indent(&format_once(|f| {
383383
if !self.elements.is_empty() {
384384
write_array_node(
385-
self.elements.len(),
385+
self.elements.len() + usize::from(self.rest.is_some()),
386386
self.elements().iter().map(AstNode::as_ref),
387387
f,
388388
)?;
@@ -589,8 +589,97 @@ impl<'a> FormatWrite<'a> for AstNode<'a, EmptyStatement> {
589589
}
590590
}
591591

592+
/// Returns `true` if the expression needs a leading semicolon to prevent ASI issues
593+
fn expression_statement_needs_semicolon<'a>(
594+
stmt: &AstNode<'a, ExpressionStatement<'a>>,
595+
f: &mut Formatter<'_, 'a>,
596+
) -> bool {
597+
if matches!(
598+
stmt.parent,
599+
// `if (true) (() => {})`
600+
AstNodes::IfStatement(_)
601+
// `do ({} => {}) while (true)`
602+
| AstNodes::DoWhileStatement(_)
603+
// `while (true) (() => {})`
604+
| AstNodes::WhileStatement(_)
605+
// `for (;;) (() => {})`
606+
| AstNodes::ForStatement(_)
607+
// `for (i in o) (() => {})`
608+
| AstNodes::ForInStatement(_)
609+
// `for (i of o) (() => {})`
610+
| AstNodes::ForOfStatement(_)
611+
// `with(true) (() => {})`
612+
| AstNodes::WithStatement(_)
613+
// `label: (() => {})`
614+
| AstNodes::LabeledStatement(_)
615+
) {
616+
return false;
617+
}
618+
// Arrow functions need semicolon only if they will have parentheses
619+
// e.g., `(a) => {}` needs `;(a) => {}` but `a => {}` doesn't need semicolon
620+
if let Expression::ArrowFunctionExpression(arrow) = &stmt.expression {
621+
return !can_avoid_parentheses(arrow, f);
622+
}
623+
624+
// First check if the expression itself needs protection
625+
let expr = stmt.expression();
626+
627+
// Get the leftmost expression to check what the line starts with
628+
let mut current = ExpressionLeftSide::Expression(expr);
629+
loop {
630+
let needs_semi = match current {
631+
ExpressionLeftSide::Expression(expr) => {
632+
expr.needs_parentheses(f)
633+
|| match expr.as_ref() {
634+
Expression::ArrayExpression(_)
635+
| Expression::RegExpLiteral(_)
636+
| Expression::TSTypeAssertion(_)
637+
| Expression::ArrowFunctionExpression(_)
638+
| Expression::JSXElement(_) => true,
639+
640+
Expression::TemplateLiteral(template) => true,
641+
Expression::UnaryExpression(unary) => {
642+
matches!(
643+
unary.operator,
644+
UnaryOperator::UnaryPlus | UnaryOperator::UnaryNegation
645+
)
646+
}
647+
_ => false,
648+
}
649+
}
650+
ExpressionLeftSide::AssignmentTarget(assignment) => {
651+
matches!(
652+
assignment.as_ref(),
653+
AssignmentTarget::ArrayAssignmentTarget(_)
654+
| AssignmentTarget::TSTypeAssertion(_)
655+
)
656+
}
657+
ExpressionLeftSide::SimpleAssignmentTarget(assignment) => {
658+
matches!(
659+
assignment.as_ref(),
660+
| SimpleAssignmentTarget::TSTypeAssertion(_)
661+
)
662+
}
663+
_ => false,
664+
};
665+
666+
if needs_semi {
667+
return true;
668+
}
669+
670+
if let Some(next) = current.left_expression() { current = next } else { return false }
671+
}
672+
}
673+
592674
impl<'a> FormatWrite<'a> for AstNode<'a, ExpressionStatement<'a>> {
593675
fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> {
676+
// Check if we need a leading semicolon to prevent ASI issues
677+
if f.options().semicolons == Semicolons::AsNeeded
678+
&& expression_statement_needs_semicolon(self, f)
679+
{
680+
write!(f, ";")?;
681+
}
682+
594683
write!(f, [self.expression(), OptionalSemicolon])
595684
}
596685
}
@@ -932,7 +1021,7 @@ impl<'a> FormatWrite<'a> for AstNode<'a, ArrayPattern<'a>> {
9321021
group(&soft_block_indent(&format_once(|f| {
9331022
if !self.elements.is_empty() {
9341023
write_array_node(
935-
self.elements.len(),
1024+
self.elements.len() + usize::from(self.rest.is_some()),
9361025
self.elements().iter().map(AstNode::as_ref),
9371026
f,
9381027
)?;

‎crates/oxc_formatter/src/write/return_or_throw_statement.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ impl<'a> Format<'a> for FormatReturnOrThrowArgument<'a, '_> {
122122
///
123123
/// Traversing the left nodes is necessary in case the first node is parenthesized because
124124
/// parentheses will be removed (and be re-added by the return statement, but only if the argument breaks)
125-
fn has_argument_leading_comments(argument: &Expression, f: &Formatter<'_, '_>) -> bool {
125+
fn has_argument_leading_comments(argument: &AstNode<Expression>, f: &Formatter<'_, '_>) -> bool {
126126
let source_text = f.source_text();
127127

128128
let mut current = Some(ExpressionLeftSide::from(argument));
@@ -147,7 +147,7 @@ fn has_argument_leading_comments(argument: &Expression, f: &Formatter<'_, '_>) -
147147
// This check is based on
148148
// <https://github.com/prettier/prettier/blob/7584432401a47a26943dd7a9ca9a8e032ead7285/src/language-js/comments/handle-comments.js#L335-L349>
149149
if let ExpressionLeftSide::Expression(left_side) = left_side {
150-
let has_leading_own_line_comment = match left_side {
150+
let has_leading_own_line_comment = match left_side.as_ref() {
151151
Expression::ChainExpression(chain) => {
152152
if let ChainElement::StaticMemberExpression(member) = &chain.expression {
153153
is_line_comment_or_multi_line_comment(

0 commit comments

Comments
 (0)