From 9b3ba60f256a56af706a5d34e54cec8ffa13bd65 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 2 Sep 2022 09:31:17 +0200 Subject: [PATCH 1/3] feat(rome_js_formatter): Conditional JSX Chain layout This PR implements Prettier's conditional JSX chain layout. ``` abcdefgh ? ( ) : ( ); ``` That parenthesizes the `consequent` and `alternate` except for `null`, `undefined`, and nested conditionals in the alternate. This PR further removes the `Default` implementation from `JsFormatOptions` because passing a `SourceType` that matches with the type of the AST is mandatory to get correct formatting results. ## Tests I verified the prettier snapshots and added our own tests to cover the exceptions of `null`, `undefined` and nested alternates that don't get parenthesized. --- .../js/expressions/conditional_expression.rs | 21 +- .../src/ts/types/conditional_type.rs | 2 +- .../src/utils/conditional.rs | 728 ++++++++++++------ crates/rome_js_formatter/src/utils/mod.rs | 2 +- .../tests/specs/jsx/conditional.jsx | 49 ++ .../tests/specs/jsx/conditional.jsx.snap | 118 +++ .../break-parent.js.snap | 106 --- .../prettier/js/ternaries/binary.js.snap | 19 +- .../js/ternaries/nested-in-condition.js.snap | 132 ---- .../prettier/js/ternaries/nested.js.snap | 77 +- crates/rome_js_syntax/src/expr_ext.rs | 22 +- 11 files changed, 708 insertions(+), 568 deletions(-) create mode 100644 crates/rome_js_formatter/tests/specs/jsx/conditional.jsx create mode 100644 crates/rome_js_formatter/tests/specs/jsx/conditional.jsx.snap delete mode 100644 crates/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/break-parent.js.snap delete mode 100644 crates/rome_js_formatter/tests/specs/prettier/js/ternaries/nested-in-condition.js.snap diff --git a/crates/rome_js_formatter/src/js/expressions/conditional_expression.rs b/crates/rome_js_formatter/src/js/expressions/conditional_expression.rs index 9dfe44eae5e..631e4da49f3 100644 --- a/crates/rome_js_formatter/src/js/expressions/conditional_expression.rs +++ b/crates/rome_js_formatter/src/js/expressions/conditional_expression.rs @@ -1,5 +1,6 @@ use crate::prelude::*; -use crate::utils::JsAnyConditional; +use crate::utils::{ConditionalJsxChain, JsAnyConditional}; +use rome_formatter::FormatRuleWithOptions; use crate::parentheses::{ is_binary_like_left_or_right, is_conditional_test, is_spread, @@ -8,7 +9,18 @@ use crate::parentheses::{ use rome_js_syntax::{JsConditionalExpression, JsSyntaxKind, JsSyntaxNode}; #[derive(Debug, Clone, Default)] -pub struct FormatJsConditionalExpression; +pub struct FormatJsConditionalExpression { + jsx_chain: ConditionalJsxChain, +} + +impl FormatRuleWithOptions for FormatJsConditionalExpression { + type Options = ConditionalJsxChain; + + fn with_options(mut self, options: Self::Options) -> Self { + self.jsx_chain = options; + self + } +} impl FormatNodeRule for FormatJsConditionalExpression { fn fmt_fields( @@ -16,7 +28,10 @@ impl FormatNodeRule for FormatJsConditionalExpression { node: &JsConditionalExpression, formatter: &mut JsFormatter, ) -> FormatResult<()> { - JsAnyConditional::from(node.clone()).fmt(formatter) + JsAnyConditional::from(node.clone()) + .format() + .with_options(self.jsx_chain) + .fmt(formatter) } fn needs_parentheses(&self, item: &JsConditionalExpression) -> bool { diff --git a/crates/rome_js_formatter/src/ts/types/conditional_type.rs b/crates/rome_js_formatter/src/ts/types/conditional_type.rs index a56b90c423e..88650860167 100644 --- a/crates/rome_js_formatter/src/ts/types/conditional_type.rs +++ b/crates/rome_js_formatter/src/ts/types/conditional_type.rs @@ -17,7 +17,7 @@ impl FormatNodeRule for FormatTsConditionalType { node: &TsConditionalType, formatter: &mut JsFormatter, ) -> FormatResult<()> { - JsAnyConditional::from(node.clone()).fmt(formatter) + JsAnyConditional::from(node.clone()).format().fmt(formatter) } fn needs_parentheses(&self, item: &TsConditionalType) -> bool { diff --git a/crates/rome_js_formatter/src/utils/conditional.rs b/crates/rome_js_formatter/src/utils/conditional.rs index e242c45bdf8..b793d239f4b 100644 --- a/crates/rome_js_formatter/src/utils/conditional.rs +++ b/crates/rome_js_formatter/src/utils/conditional.rs @@ -1,11 +1,14 @@ use crate::prelude::*; -use rome_formatter::write; +use rome_formatter::{ + write, FormatContext, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions, +}; +use crate::{AsFormat, IntoFormat}; use rome_js_syntax::{ JsAnyExpression, JsAssignmentExpression, JsCallExpression, JsComputedMemberExpression, JsConditionalExpression, JsInitializerClause, JsNewExpression, JsReturnStatement, JsStaticMemberExpression, JsSyntaxKind, JsSyntaxNode, JsSyntaxToken, JsThrowStatement, - JsUnaryExpression, JsYieldArgument, TsAsExpression, TsConditionalType, + JsUnaryExpression, JsYieldArgument, SourceType, TsAsExpression, TsConditionalType, TsNonNullAssertionExpression, TsType, }; use rome_rowan::{declare_node_union, match_ast, AstNode, SyntaxResult}; @@ -14,20 +17,61 @@ declare_node_union! { pub JsAnyConditional = JsConditionalExpression | TsConditionalType } -impl Format for JsAnyConditional { - fn fmt(&self, f: &mut Formatter) -> FormatResult<()> { - let syntax = self.syntax(); - let consequent = self.consequent()?; - let alternate = self.alternate()?; +impl<'a> AsFormat<'a> for JsAnyConditional { + type Format = FormatRefWithRule<'a, JsAnyConditional, FormatJsAnyConditionalRule>; + + fn format(&'a self) -> Self::Format { + FormatRefWithRule::new(self, FormatJsAnyConditionalRule::default()) + } +} + +impl IntoFormat for JsAnyConditional { + type Format = FormatOwnedWithRule; - let layout = self.layout(); + fn into_format(self) -> Self::Format { + FormatOwnedWithRule::new(self, FormatJsAnyConditionalRule::default()) + } +} + +#[derive(Debug, Copy, Clone, Default)] +pub struct FormatJsAnyConditionalRule { + /// Whether the parent is a jsx conditional chain. + /// Gets passed through from the root to the consequent and alternate of [JsConditionalExpression]s. + /// + /// Doesn't apply for [TsConditionalType]. + jsx_chain: ConditionalJsxChain, +} + +impl FormatRuleWithOptions for FormatJsAnyConditionalRule { + type Options = ConditionalJsxChain; + + fn with_options(mut self, options: Self::Options) -> Self { + self.jsx_chain = options; + self + } +} + +impl FormatRule for FormatJsAnyConditionalRule { + type Context = JsFormatContext; + + fn fmt( + &self, + conditional: &JsAnyConditional, + f: &mut Formatter, + ) -> FormatResult<()> { + let syntax = conditional.syntax(); + let consequent = conditional.consequent()?; + let alternate = conditional.alternate()?; + + let layout = self.layout(conditional, f.context().options().source_type()); + let jsx_chain = layout.jsx_chain().unwrap_or(self.jsx_chain); let format_consequent_and_alternate = format_with(|f| { write!( f, [ soft_line_break_or_space(), - self.question_mark_token().format(), + conditional.question_mark_token().format(), space() ] )?; @@ -54,7 +98,7 @@ impl Format for JsAnyConditional { f, [ soft_line_break_or_space(), - self.colon_token().format(), + conditional.colon_token().format(), space() ] )?; @@ -78,40 +122,60 @@ impl Format for JsAnyConditional { }); let format_tail_with_indent = format_with(|f: &mut JsFormatter| { - // Add an extra level of indent to nested consequences. - if layout.is_nested_consequent() { - // if f.context().indent_style().is_tab() { - // This may look silly but the `dedent` is to remove the outer `align` added by the parent's formatting of the consequent. - // The `indent` is necessary to indent the content by one level with a tab. - // Adding an `indent` without the `dedent` would result in the `outer` align being converted - // into a `indent` + the `indent` added here, ultimately resulting in a two-level indention. - write!(f, [dedent(&indent(&format_consequent_and_alternate))]) - } else { - format_consequent_and_alternate.fmt(f) + match conditional { + JsAnyConditional::JsConditionalExpression(conditional) if jsx_chain.is_chain() => { + write!( + f, + [ + space(), + conditional.question_mark_token().format(), + space(), + format_jsx_chain_consequent(consequent.as_expression().unwrap()), + space(), + conditional.colon_token().format(), + space(), + format_jsx_chain_alternate(alternate.as_expression().unwrap()) + ] + ) + } + _ => { + // Add an extra level of indent to nested consequences. + if layout.is_nested_consequent() { + // if f.context().indent_style().is_tab() { + // This may look silly but the `dedent` is to remove the outer `align` added by the parent's formatting of the consequent. + // The `indent` is necessary to indent the content by one level with a tab. + // Adding an `indent` without the `dedent` would result in the `outer` align being converted + // into a `indent` + the `indent` added here, ultimately resulting in a two-level indention. + write!(f, [dedent(&indent(&format_consequent_and_alternate))]) + } else { + format_consequent_and_alternate.fmt(f) + } + } } }); - let should_extra_indent = self.should_extra_indent(&layout); + let should_extra_indent = self.should_extra_indent(conditional, &layout); let format_inner = format_with(|f| { write!( f, [FormatConditionalTest { - conditional: self, + conditional, layout: &layout, }] )?; // Indent the `consequent` and `alternate` **only if** this is the root conditional expression // OR this is the `test` of a conditional expression. - if layout.is_root() || layout.is_nested_test() { + if jsx_chain.is_no_chain() && (layout.is_root() || layout.is_nested_test()) { write!(f, [indent(&format_tail_with_indent)])?; } else { // Don't indent for nested `alternate`s or `consequence`s write!(f, [format_tail_with_indent])?; } - let break_closing_parentheses = self.is_parent_static_member_expression(&layout); + let break_closing_parentheses = jsx_chain.is_no_chain() + && self.is_parent_static_member_expression(conditional, &layout); // Add a soft line break in front of the closing `)` in case the parent is a static member expression // ``` @@ -144,6 +208,246 @@ impl Format for JsAnyConditional { } } +impl FormatJsAnyConditionalRule { + fn layout(&self, conditional: &JsAnyConditional, source_type: SourceType) -> ConditionalLayout { + match conditional.syntax().parent() { + Some(parent) if parent.kind() == conditional.syntax().kind() => { + let conditional_parent = JsAnyConditional::unwrap_cast(parent); + + if conditional_parent.is_test(conditional.syntax()) { + ConditionalLayout::NestedTest { + parent: conditional_parent, + } + } else if conditional_parent.is_alternate(conditional.syntax()) { + ConditionalLayout::NestedAlternate { + parent: conditional_parent, + } + } else { + ConditionalLayout::NestedConsequent { + parent: conditional_parent, + } + } + } + parent => { + let is_jsx_chain = match conditional { + JsAnyConditional::JsConditionalExpression(conditional) + if source_type.variant().is_jsx() => + { + is_jsx_conditional_chain(conditional) + } + _ => false, + }; + + ConditionalLayout::Root { + parent, + jsx_chain: is_jsx_chain.into(), + } + } + } + } + + /// It is desired to add an extra indent if this conditional is a [JsConditionalExpression] and is directly inside + /// of a member chain: + /// + /// ```javascript + /// // Input + /// return (a ? b : c).member + /// + /// // Default + /// return (a + /// ? b + /// : c + /// ).member + /// + /// // Preferred + /// return ( + /// a + /// ? b + /// : c + /// ).member + /// ``` + fn should_extra_indent( + &self, + conditional: &JsAnyConditional, + layout: &ConditionalLayout, + ) -> bool { + enum Ancestor { + MemberChain(JsAnyExpression), + Root(JsSyntaxNode), + } + + let conditional = match conditional { + JsAnyConditional::JsConditionalExpression(conditional) => conditional, + JsAnyConditional::TsConditionalType(_) => { + return false; + } + }; + + let ancestors = layout + .parent() + .into_iter() + .flat_map(|parent| parent.ancestors()); + let mut parent = None; + let mut expression = JsAnyExpression::from(conditional.clone()); + + // This tries to find the start of a member chain by iterating over all ancestors of the conditional. + // The iteration "breaks" as soon as a non-member-chain node is found. + for ancestor in ancestors { + let ancestor = match_ast! { + match &ancestor { + JsCallExpression(call_expression) => { + if call_expression + .callee() + .as_ref() + == Ok(&expression) + { + Ancestor::MemberChain(call_expression.into()) + } else { + Ancestor::Root(call_expression.into_syntax()) + } + }, + + JsStaticMemberExpression(member_expression) => { + if member_expression + .object() + .as_ref() + == Ok(&expression) + { + Ancestor::MemberChain(member_expression.into()) + } else { + Ancestor::Root(member_expression.into_syntax()) + } + }, + JsComputedMemberExpression(member_expression) => { + if member_expression + .object() + .as_ref() + == Ok(&expression) + { + Ancestor::MemberChain(member_expression.into()) + } else { + Ancestor::Root(member_expression.into_syntax()) + } + }, + TsNonNullAssertionExpression(non_null_assertion) => { + if non_null_assertion + .expression() + .as_ref() + == Ok(&expression) + { + Ancestor::MemberChain(non_null_assertion.into()) + } else { + Ancestor::Root(non_null_assertion.into_syntax()) + } + }, + JsNewExpression(new_expression) => { + // Skip over new expressions + if new_expression + .callee() + .as_ref() + == Ok(&expression) + { + parent = new_expression.syntax().parent(); + expression = new_expression.into(); + break; + } + + Ancestor::Root(new_expression.into_syntax()) + }, + TsAsExpression(as_expression) => { + if as_expression + .expression() + .as_ref() + == Ok(&expression) + { + parent = as_expression.syntax().parent(); + expression = as_expression.into(); + break; + } + + Ancestor::Root(as_expression.into_syntax()) + }, + _ => Ancestor::Root(ancestor), + } + }; + + match ancestor { + Ancestor::MemberChain(left) => { + // Store the node that is highest in the member chain + expression = left; + } + Ancestor::Root(root) => { + parent = Some(root); + break; + } + } + } + + // Don't indent if this conditional isn't part of a member chain. + // e.g. don't indent for `return a ? b : c`, only for `return (a ? b : c).member` + if expression.syntax() == conditional.syntax() { + return false; + } + + match parent { + None => false, + Some(parent) => { + let argument = match parent.kind() { + JsSyntaxKind::JS_INITIALIZER_CLAUSE => { + let initializer = JsInitializerClause::unwrap_cast(parent); + initializer.expression().ok().map(JsAnyExpression::from) + } + JsSyntaxKind::JS_RETURN_STATEMENT => { + let return_statement = JsReturnStatement::unwrap_cast(parent); + return_statement.argument().map(JsAnyExpression::from) + } + JsSyntaxKind::JS_THROW_STATEMENT => { + let throw_statement = JsThrowStatement::unwrap_cast(parent); + throw_statement.argument().ok().map(JsAnyExpression::from) + } + JsSyntaxKind::JS_UNARY_EXPRESSION => { + let unary_expression = JsUnaryExpression::unwrap_cast(parent); + unary_expression.argument().ok().map(JsAnyExpression::from) + } + JsSyntaxKind::JS_YIELD_ARGUMENT => { + let yield_argument = JsYieldArgument::unwrap_cast(parent); + yield_argument.expression().ok().map(JsAnyExpression::from) + } + JsSyntaxKind::JS_ASSIGNMENT_EXPRESSION => { + let assignment_expression = JsAssignmentExpression::unwrap_cast(parent); + assignment_expression + .right() + .ok() + .map(JsAnyExpression::from) + } + _ => None, + }; + + argument.map_or(false, |argument| argument == expression) + } + } + } + + /// Returns `true` if this is the root conditional expression and the parent is a [JsStaticMemberExpression]. + fn is_parent_static_member_expression( + &self, + conditional: &JsAnyConditional, + layout: &ConditionalLayout, + ) -> bool { + if !conditional.is_conditional_expression() { + return false; + } + + match layout { + ConditionalLayout::Root { + parent: Some(parent), + .. + } => JsStaticMemberExpression::can_cast(parent.kind()), + _ => false, + } + } +} + /// Formats the test conditional of a conditional expression. struct FormatConditionalTest<'a> { conditional: &'a JsAnyConditional, @@ -182,6 +486,15 @@ declare_node_union! { ExpressionOrType = JsAnyExpression | TsType } +impl ExpressionOrType { + fn as_expression(&self) -> Option<&JsAnyExpression> { + match self { + ExpressionOrType::JsAnyExpression(expression) => Some(expression), + ExpressionOrType::TsType(_) => None, + } + } +} + impl Format for ExpressionOrType { fn fmt(&self, f: &mut Formatter) -> FormatResult<()> { match self { @@ -199,12 +512,12 @@ enum ConditionalLayout { /// /// ```javascript /// outerCondition - // ? consequent - // : nestedAlternate + - // binary + // <- notice how the content is aligned to the `: ` - // ? consequentOfnestedAlternate - // : alternateOfNestedAlternate; - // ``` + /// ? consequent + /// : nestedAlternate + + /// binary + // <- notice how the content is aligned to the `: ` + /// ? consequentOfnestedAlternate + /// : alternateOfNestedAlternate; + /// ``` NestedAlternate { parent: JsAnyConditional }, /// Conditional that is the `test` of another conditional. @@ -226,10 +539,10 @@ enum ConditionalLayout { /// /// ```javascript /// condition1 - // ? condition2 - // ? consequent2 // <-- consequent and alternate gets indented - // : alternate2 - // : alternate1; + /// ? condition2 + /// ? consequent2 // <-- consequent and alternate gets indented + /// : alternate2 + /// : alternate1; /// ``` NestedConsequent { parent: JsAnyConditional }, @@ -241,10 +554,67 @@ enum ConditionalLayout { Root { /// The closest ancestor that isn't a parenthesized node. parent: Option, + + jsx_chain: ConditionalJsxChain, }, } +/// A [JsConditionalExpression] that itself or any of its parent's [JsConditionalExpression] have a a [JsxTagExpression] +/// as its [`test`](JsConditionalExpression::test), [`consequent`](JsConditionalExpression::consequent) or [`alternate`](JsConditionalExpression::alternate). +/// +/// Parenthesizes the `consequent` and `alternate` if it the group breaks except if the expressions are +/// * `null` +/// * `undefined` +/// * or a nested [JsConditionalExpression] in the alternate branch +/// +/// ```javascript +/// abcdefgh? ( +/// +/// +/// +/// +/// ) : ( +/// +/// +/// +/// +/// ); +/// ``` +#[derive(Copy, Clone, Eq, PartialEq, Debug, Default)] +pub enum ConditionalJsxChain { + Chain, + #[default] + NoChain, +} + +impl ConditionalJsxChain { + pub const fn is_chain(&self) -> bool { + matches!(self, ConditionalJsxChain::Chain) + } + pub const fn is_no_chain(&self) -> bool { + matches!(self, ConditionalJsxChain::NoChain) + } +} + +impl From for ConditionalJsxChain { + fn from(value: bool) -> Self { + match value { + true => ConditionalJsxChain::Chain, + false => ConditionalJsxChain::NoChain, + } + } +} + impl ConditionalLayout { + const fn jsx_chain(&self) -> Option { + match self { + ConditionalLayout::NestedAlternate { .. } + | ConditionalLayout::NestedTest { .. } + | ConditionalLayout::NestedConsequent { .. } => None, + ConditionalLayout::Root { jsx_chain, .. } => Some(*jsx_chain), + } + } + const fn is_root(&self) -> bool { matches!(self, ConditionalLayout::Root { .. }) } @@ -252,10 +622,10 @@ impl ConditionalLayout { /// Returns the parent node, if any fn parent(&self) -> Option<&JsSyntaxNode> { match self { - ConditionalLayout::NestedAlternate { parent } - | ConditionalLayout::NestedTest { parent } - | ConditionalLayout::NestedConsequent { parent } => Some(parent.syntax()), - ConditionalLayout::Root { parent } => parent.as_ref(), + ConditionalLayout::NestedAlternate { parent, .. } + | ConditionalLayout::NestedTest { parent, .. } + | ConditionalLayout::NestedConsequent { parent, .. } => Some(parent.syntax()), + ConditionalLayout::Root { parent, .. } => parent.as_ref(), } } @@ -273,40 +643,6 @@ impl ConditionalLayout { } impl JsAnyConditional { - fn layout(&self) -> ConditionalLayout { - let resolved_parent = match self { - JsAnyConditional::JsConditionalExpression(conditional) => conditional.syntax().parent(), - JsAnyConditional::TsConditionalType(ty) => ty.syntax().parent(), - }; - - let parent = match resolved_parent { - None => return ConditionalLayout::Root { parent: None }, - Some(parent) => parent, - }; - - if parent.kind() == self.syntax().kind() { - let conditional_parent = JsAnyConditional::unwrap_cast(parent); - - if conditional_parent.is_test(self.syntax()) { - ConditionalLayout::NestedTest { - parent: conditional_parent, - } - } else if conditional_parent.is_alternate(self.syntax()) { - ConditionalLayout::NestedAlternate { - parent: conditional_parent, - } - } else { - ConditionalLayout::NestedConsequent { - parent: conditional_parent, - } - } - } else { - ConditionalLayout::Root { - parent: Some(parent), - } - } - } - /// Returns `true` if `node` is the `test` of this conditional. fn is_test(&self, node: &JsSyntaxNode) -> bool { match self { @@ -380,197 +716,85 @@ impl JsAnyConditional { const fn is_conditional_expression(&self) -> bool { matches!(self, JsAnyConditional::JsConditionalExpression(_)) } +} - /// It is desired to add an extra indent if this conditional is a [JsConditionalExpression] and is directly inside - /// of a member chain: - /// - /// ```javascript - /// // Input - /// return (a ? b : c).member - /// - /// // Default - /// return (a - /// ? b - /// : c - /// ).member - /// - /// // Preferred - /// return ( - /// a - /// ? b - /// : c - /// ).member - /// ``` - fn should_extra_indent(&self, layout: &ConditionalLayout) -> bool { - enum Ancestor { - MemberChain(JsAnyExpression), - Root(JsSyntaxNode), - } +fn is_jsx_conditional_chain(outer_most: &JsConditionalExpression) -> bool { + fn recurse(expression: SyntaxResult) -> bool { + use JsAnyExpression::*; - let conditional = match self { - JsAnyConditional::JsConditionalExpression(conditional) => conditional, - JsAnyConditional::TsConditionalType(_) => { - return false; - } - }; + match expression { + Ok(JsConditionalExpression(conditional)) => is_jsx_conditional_chain(&conditional), + Ok(JsxTagExpression(_)) => true, + _ => false, + } + } - let ancestors = layout - .parent() - .into_iter() - .flat_map(|parent| parent.ancestors()); - let mut parent = None; - let mut expression = JsAnyExpression::from(conditional.clone()); + recurse(outer_most.test()) + || recurse(outer_most.consequent()) + || recurse(outer_most.alternate()) +} - // This tries to find the start of a member chain by iterating over all ancestors of the conditional - // expression, while skipping parenthesized expression. - // The iteration "breaks" as soon as a non-member-chain node is found. - for ancestor in ancestors { - let ancestor = match_ast! { - match &ancestor { - JsCallExpression(call_expression) => { - if call_expression - .callee() - .as_ref() - == Ok(&expression) - { - Ancestor::MemberChain(call_expression.into()) - } else { - Ancestor::Root(call_expression.into_syntax()) - } - }, +fn format_jsx_chain_consequent(expression: &JsAnyExpression) -> FormatJsxChainExpression { + FormatJsxChainExpression { + expression, + alternate: false, + } +} - JsStaticMemberExpression(member_expression) => { - if member_expression - .object() - .as_ref() - == Ok(&expression) - { - Ancestor::MemberChain(member_expression.into()) - } else { - Ancestor::Root(member_expression.into_syntax()) - } - }, - JsComputedMemberExpression(member_expression) => { - if member_expression - .object() - .as_ref() - == Ok(&expression) - { - Ancestor::MemberChain(member_expression.into()) - } else { - Ancestor::Root(member_expression.into_syntax()) - } - }, - TsNonNullAssertionExpression(non_null_assertion) => { - if non_null_assertion - .expression() - .as_ref() - == Ok(&expression) - { - Ancestor::MemberChain(non_null_assertion.into()) - } else { - Ancestor::Root(non_null_assertion.into_syntax()) - } - }, - JsNewExpression(new_expression) => { - // Skip over new expressions - if new_expression - .callee() - .as_ref() - == Ok(&expression) - { - parent = new_expression.syntax().parent(); - expression = new_expression.into(); - break; - } +fn format_jsx_chain_alternate(alternate: &JsAnyExpression) -> FormatJsxChainExpression { + FormatJsxChainExpression { + expression: alternate, + alternate: true, + } +} - Ancestor::Root(new_expression.into_syntax()) - }, - TsAsExpression(as_expression) => { - if as_expression - .expression() - .as_ref() - == Ok(&expression) - { - parent = as_expression.syntax().parent(); - expression = as_expression.into(); - break; - } +/// Wraps all expressions in parentheses if they break EXCEPT +/// * Nested conditionals in the alterante +/// * `null` +/// * `undefined` +struct FormatJsxChainExpression<'a> { + expression: &'a JsAnyExpression, + alternate: bool, +} - Ancestor::Root(as_expression.into_syntax()) - }, - _ => Ancestor::Root(ancestor), - } - }; +impl Format for FormatJsxChainExpression<'_> { + fn fmt(&self, f: &mut Formatter) -> FormatResult<()> { + use JsAnyExpression::*; + + let no_wrap = match self.expression { + JsIdentifierExpression(identifier) if identifier.name()?.is_undefined() => true, + JsAnyLiteralExpression( + rome_js_syntax::JsAnyLiteralExpression::JsNullLiteralExpression(_), + ) => true, + JsConditionalExpression(_) if self.alternate => true, + _ => false, + }; - match ancestor { - Ancestor::MemberChain(left) => { - // Store the node that is highest in the member chain - expression = left; - } - Ancestor::Root(root) => { - parent = Some(root); - break; - } + let format_expression = format_with(|f| match self.expression { + JsConditionalExpression(conditional) => { + write!( + f, + [conditional + .format() + .with_options(ConditionalJsxChain::Chain)] + ) } - } - - // Don't indent if this conditional isn't part of a member chain. - // e.g. don't indent for `return a ? b : c`, only for `return (a ? b : c).member` - if expression.syntax() == conditional.syntax() { - return false; - } - - match parent { - None => false, - Some(parent) => { - let argument = match parent.kind() { - JsSyntaxKind::JS_INITIALIZER_CLAUSE => { - let initializer = JsInitializerClause::unwrap_cast(parent); - initializer.expression().ok().map(JsAnyExpression::from) - } - JsSyntaxKind::JS_RETURN_STATEMENT => { - let return_statement = JsReturnStatement::unwrap_cast(parent); - return_statement.argument().map(JsAnyExpression::from) - } - JsSyntaxKind::JS_THROW_STATEMENT => { - let throw_statement = JsThrowStatement::unwrap_cast(parent); - throw_statement.argument().ok().map(JsAnyExpression::from) - } - JsSyntaxKind::JS_UNARY_EXPRESSION => { - let unary_expression = JsUnaryExpression::unwrap_cast(parent); - unary_expression.argument().ok().map(JsAnyExpression::from) - } - JsSyntaxKind::JS_YIELD_ARGUMENT => { - let yield_argument = JsYieldArgument::unwrap_cast(parent); - yield_argument.expression().ok().map(JsAnyExpression::from) - } - JsSyntaxKind::JS_ASSIGNMENT_EXPRESSION => { - let assignment_expression = JsAssignmentExpression::unwrap_cast(parent); - assignment_expression - .right() - .ok() - .map(JsAnyExpression::from) - } - _ => None, - }; - - argument.map_or(false, |argument| argument == expression) + expression => { + write!(f, [expression.format()]) } - } - } - - /// Returns `true` if this is the root conditional expression and the parent is a [JsStaticMemberExpression]. - fn is_parent_static_member_expression(&self, layout: &ConditionalLayout) -> bool { - if !self.is_conditional_expression() { - return false; - } + }); - match layout { - ConditionalLayout::Root { - parent: Some(parent), - } => JsStaticMemberExpression::can_cast(parent.kind()), - _ => false, + if no_wrap { + write!(f, [format_expression]) + } else { + write!( + f, + [ + if_group_breaks(&text("(")), + soft_block_indent(&format_expression), + if_group_breaks(&text(")")) + ] + ) } } } diff --git a/crates/rome_js_formatter/src/utils/mod.rs b/crates/rome_js_formatter/src/utils/mod.rs index 79907056855..626b76f3255 100644 --- a/crates/rome_js_formatter/src/utils/mod.rs +++ b/crates/rome_js_formatter/src/utils/mod.rs @@ -23,7 +23,7 @@ pub(crate) use assignment_like::{ pub(crate) use binary_like_expression::{ needs_binary_like_parentheses, JsAnyBinaryLikeExpression, JsAnyBinaryLikeLeftExpression, }; -pub(crate) use conditional::JsAnyConditional; +pub(crate) use conditional::{ConditionalJsxChain, JsAnyConditional}; pub(crate) use member_chain::get_member_chain; pub(crate) use object_like::JsObjectLike; pub(crate) use object_pattern_like::JsObjectPatternLike; diff --git a/crates/rome_js_formatter/tests/specs/jsx/conditional.jsx b/crates/rome_js_formatter/tests/specs/jsx/conditional.jsx new file mode 100644 index 00000000000..41d7601d637 --- /dev/null +++ b/crates/rome_js_formatter/tests/specs/jsx/conditional.jsx @@ -0,0 +1,49 @@ +(bifornCringerMoshedPerplexSawder ? ( + askTrovenaBeenaDependsRowans +) : ( + glimseGlyphsHazardNoopsTieTie +)) ? null : ( + + + + + +); + + +(bifornCringerMoshedPerplexSawder ? ( + askTrovenaBeenaDependsRowans +) : ( + glimseGlyphsHazardNoopsTieTie +)) ? undefined : ( + + + + + +); + +(bifornCringerMoshedPerplexSawder ? ( + askTrovenaBeenaDependsRowans +) : ( + glimseGlyphsHazardNoopsTieTie +)) ? x : ( + + + + + +); + + +(bifornCringerMoshedPerplexSawder ? ( + askTrovenaBeenaDependsRowans +) : ( + glimseGlyphsHazardNoopsTieTie +)) ? a ? b : ( + + + + + +) : null; diff --git a/crates/rome_js_formatter/tests/specs/jsx/conditional.jsx.snap b/crates/rome_js_formatter/tests/specs/jsx/conditional.jsx.snap new file mode 100644 index 00000000000..ea86bbd0743 --- /dev/null +++ b/crates/rome_js_formatter/tests/specs/jsx/conditional.jsx.snap @@ -0,0 +1,118 @@ +--- +source: crates/rome_js_formatter/tests/spec_test.rs +expression: conditional.jsx +--- +# Input +(bifornCringerMoshedPerplexSawder ? ( + askTrovenaBeenaDependsRowans +) : ( + glimseGlyphsHazardNoopsTieTie +)) ? null : ( + + + + + +); + + +(bifornCringerMoshedPerplexSawder ? ( + askTrovenaBeenaDependsRowans +) : ( + glimseGlyphsHazardNoopsTieTie +)) ? undefined : ( + + + + + +); + +(bifornCringerMoshedPerplexSawder ? ( + askTrovenaBeenaDependsRowans +) : ( + glimseGlyphsHazardNoopsTieTie +)) ? x : ( + + + + + +); + + +(bifornCringerMoshedPerplexSawder ? ( + askTrovenaBeenaDependsRowans +) : ( + glimseGlyphsHazardNoopsTieTie +)) ? a ? b : ( + + + + + +) : null; + +============================= +# Outputs +## Output 1 +----- +Indent style: Tab +Line width: 80 +Quote style: Double Quotes +Quote properties: As needed +----- +( + bifornCringerMoshedPerplexSawder + ? askTrovenaBeenaDependsRowans + : glimseGlyphsHazardNoopsTieTie +) ? null : ( + + + + + +); + +( + bifornCringerMoshedPerplexSawder + ? askTrovenaBeenaDependsRowans + : glimseGlyphsHazardNoopsTieTie +) ? undefined : ( + + + + + +); + +( + bifornCringerMoshedPerplexSawder + ? askTrovenaBeenaDependsRowans + : glimseGlyphsHazardNoopsTieTie +) ? ( + x +) : ( + + + + + +); + +( + bifornCringerMoshedPerplexSawder + ? askTrovenaBeenaDependsRowans + : glimseGlyphsHazardNoopsTieTie +) ? ( + a ? ( + b + ) : ( + + + + + + ) +) : null; + diff --git a/crates/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/break-parent.js.snap b/crates/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/break-parent.js.snap deleted file mode 100644 index 49f729e934a..00000000000 --- a/crates/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/break-parent.js.snap +++ /dev/null @@ -1,106 +0,0 @@ ---- -source: crates/rome_js_formatter/tests/prettier_tests.rs ---- - -# Input - -```js -({ - processors: [ - require("autoprefixer", { - browsers: ["> 1%", "last 2 versions", "ie >= 11", "Firefox ESR"] - }), - require("postcss-url")({ - url: url => - url.startsWith("/") || /^[a-z]+:/.test(url) ? url : `/static/${url}` - }) - ] -}); - -true - ? test({ - a: 1 - }) - :
; -``` - - -# Prettier differences - -```diff ---- Prettier -+++ Rome -@@ -10,18 +10,16 @@ - ], - }); - --true ? ( -- test({ -- a: 1, -- }) --) : ( --
--); -+true -+ ? test({ -+ a: 1, -+ }) -+ :
; -``` - -# Output - -```js -({ - processors: [ - require("autoprefixer", { - browsers: ["> 1%", "last 2 versions", "ie >= 11", "Firefox ESR"], - }), - require("postcss-url")({ - url: (url) => - url.startsWith("/") || /^[a-z]+:/.test(url) ? url : `/static/${url}`, - }), - ], -}); - -true - ? test({ - a: 1, - }) - :
; -``` - - - diff --git a/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/binary.js.snap b/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/binary.js.snap index 74481420525..8ab3d3a33c5 100644 --- a/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/binary.js.snap +++ b/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/binary.js.snap @@ -25,16 +25,9 @@ room = room.map((row, rowIndex) => ( ```diff --- Prettier +++ Rome -@@ -1,17 +1,19 @@ - const funnelSnapshotCard = - (report === MY_OVERVIEW && !ReportGK.xar_metrics_active_capitol_v2) || - (report === COMPANY_OVERVIEW && -- !ReportGK.xar_metrics_active_capitol_v2_company_metrics) ? ( -- -- ) : null; -+ !ReportGK.xar_metrics_active_capitol_v2_company_metrics) -+ ? -+ : null; +@@ -5,13 +5,15 @@ + + ) : null; -room = room.map((row, rowIndex) => - row.map((col, colIndex) => @@ -65,9 +58,9 @@ room = room.map((row, rowIndex) => ( const funnelSnapshotCard = (report === MY_OVERVIEW && !ReportGK.xar_metrics_active_capitol_v2) || (report === COMPANY_OVERVIEW && - !ReportGK.xar_metrics_active_capitol_v2_company_metrics) - ? - : null; + !ReportGK.xar_metrics_active_capitol_v2_company_metrics) ? ( + + ) : null; room = room.map( (row, rowIndex) => diff --git a/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/nested-in-condition.js.snap b/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/nested-in-condition.js.snap deleted file mode 100644 index bff219caf4c..00000000000 --- a/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/nested-in-condition.js.snap +++ /dev/null @@ -1,132 +0,0 @@ ---- -source: crates/rome_js_formatter/tests/prettier_tests.rs ---- - -# Input - -```js -$var = ($number % 10 >= 2 && ($number % 100 < 10 || $number % 100 >= 20) -? kochabCooieGameOnOboleUnweave -: annularCooeedSplicesWalksWayWay) - ? anodyneCondosMalateOverateRetinol - : averredBathersBoxroomBuggyNurl; - -const value = (bifornCringerMoshedPerplexSawder -? askTrovenaBeenaDependsRowans -: glimseGlyphsHazardNoopsTieTie) - ? true - ? true - : false - : true - ? true - : false; - -(bifornCringerMoshedPerplexSawder ? ( - askTrovenaBeenaDependsRowans -) : ( - glimseGlyphsHazardNoopsTieTie -)) ? ( - - - - - - - - -) : ( - - - - - -); -``` - - -# Prettier differences - -```diff ---- Prettier -+++ Rome -@@ -22,19 +22,17 @@ - bifornCringerMoshedPerplexSawder - ? askTrovenaBeenaDependsRowans - : glimseGlyphsHazardNoopsTieTie --) ? ( -- -- -- -- -- -- -- -- --) : ( -- -- -- -- -- --); -+) -+ ? -+ -+ -+ -+ -+ -+ -+ -+ : -+ -+ -+ -+ ; -``` - -# Output - -```js -$var = ( - $number % 10 >= 2 && ($number % 100 < 10 || $number % 100 >= 20) - ? kochabCooieGameOnOboleUnweave - : annularCooeedSplicesWalksWayWay -) - ? anodyneCondosMalateOverateRetinol - : averredBathersBoxroomBuggyNurl; - -const value = ( - bifornCringerMoshedPerplexSawder - ? askTrovenaBeenaDependsRowans - : glimseGlyphsHazardNoopsTieTie -) - ? true - ? true - : false - : true - ? true - : false; - -( - bifornCringerMoshedPerplexSawder - ? askTrovenaBeenaDependsRowans - : glimseGlyphsHazardNoopsTieTie -) - ? - - - - - - - - : - - - - ; -``` - - - diff --git a/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/nested.js.snap b/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/nested.js.snap index 5d861b3d721..5b3a410ba4b 100644 --- a/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/nested.js.snap +++ b/crates/rome_js_formatter/tests/specs/prettier/js/ternaries/nested.js.snap @@ -108,48 +108,7 @@ a ```diff --- Prettier +++ Rome -@@ -12,24 +12,22 @@ - : value4; - - const StorybookLoader = ({ match }) => -- match.params.storyId === "button" ? ( -- -- ) : match.params.storyId === "color" ? ( -- -- ) : match.params.storyId === "typography" ? ( -- -- ) : match.params.storyId === "loading" ? ( -- -- ) : match.params.storyId === "deal-list" ? ( -- -- ) : ( -- -- {"Missing story book"} -- -- -- -- -- ); -+ match.params.storyId === "button" -+ ? -+ : match.params.storyId === "color" -+ ? -+ : match.params.storyId === "typography" -+ ? -+ : match.params.storyId === "loading" -+ ? -+ : match.params.storyId === "deal-list" -+ ? -+ : -+ {"Missing story book"} -+ -+ -+ -+ ; - - const message = - i % 3 === 0 && i % 5 === 0 -@@ -60,7 +58,8 @@ +@@ -60,7 +60,8 @@ ? 3 //'There was an issue with your CVC number' : true //state == 'invalid_expiry' ? 4 //'Expiry must be sometime in the past.' @@ -178,22 +137,24 @@ const value = condition1 : value4; const StorybookLoader = ({ match }) => - match.params.storyId === "button" - ? - : match.params.storyId === "color" - ? - : match.params.storyId === "typography" - ? - : match.params.storyId === "loading" - ? - : match.params.storyId === "deal-list" - ? - : - {"Missing story book"} - - - - ; + match.params.storyId === "button" ? ( + + ) : match.params.storyId === "color" ? ( + + ) : match.params.storyId === "typography" ? ( + + ) : match.params.storyId === "loading" ? ( + + ) : match.params.storyId === "deal-list" ? ( + + ) : ( + + {"Missing story book"} + + + + + ); const message = i % 3 === 0 && i % 5 === 0 diff --git a/crates/rome_js_syntax/src/expr_ext.rs b/crates/rome_js_syntax/src/expr_ext.rs index ae813315a70..f8779037b5f 100644 --- a/crates/rome_js_syntax/src/expr_ext.rs +++ b/crates/rome_js_syntax/src/expr_ext.rs @@ -4,14 +4,32 @@ use crate::{ JsAnyExpression, JsAnyLiteralExpression, JsArrayExpression, JsArrayHole, JsAssignmentExpression, JsBinaryExpression, JsCallExpression, JsComputedMemberExpression, JsLiteralMemberName, JsLogicalExpression, JsNumberLiteralExpression, JsObjectExpression, - JsRegexLiteralExpression, JsStaticMemberExpression, JsStringLiteralExpression, JsSyntaxKind, - JsSyntaxToken, JsTemplate, JsUnaryExpression, OperatorPrecedence, T, + JsReferenceIdentifier, JsRegexLiteralExpression, JsStaticMemberExpression, + JsStringLiteralExpression, JsSyntaxKind, JsSyntaxToken, JsTemplate, JsUnaryExpression, + OperatorPrecedence, T, }; use crate::{JsPreUpdateExpression, JsSyntaxKind::*}; use rome_rowan::{ AstNode, AstSeparatedList, NodeOrToken, SyntaxNodeText, SyntaxResult, TextRange, TextSize, }; +impl JsReferenceIdentifier { + /// Returns `true` if this identifier refers to the `undefined` symbol. + /// + /// ## Examples + /// + /// ``` + /// use rome_js_factory::make::{js_reference_identifier, ident}; + /// + /// assert!(js_reference_identifier(ident("undefined")).is_undefined()); + /// assert!(!js_reference_identifier(ident("x")).is_undefined()); + /// ``` + pub fn is_undefined(&self) -> bool { + self.value_token() + .map_or(false, |name| name.text_trimmed() == "undefined") + } +} + impl JsLiteralMemberName { /// Returns the name of the member as a syntax text /// From 95a9d9a619d5e4a7b440b2b608bd7a7d742e0ce1 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 2 Sep 2022 15:07:39 +0200 Subject: [PATCH 2/3] Fix lint issues --- .../rome_js_formatter/src/utils/conditional.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/rome_js_formatter/src/utils/conditional.rs b/crates/rome_js_formatter/src/utils/conditional.rs index b793d239f4b..961badc0138 100644 --- a/crates/rome_js_formatter/src/utils/conditional.rs +++ b/crates/rome_js_formatter/src/utils/conditional.rs @@ -512,11 +512,11 @@ enum ConditionalLayout { /// /// ```javascript /// outerCondition - /// ? consequent - /// : nestedAlternate + - /// binary + // <- notice how the content is aligned to the `: ` - /// ? consequentOfnestedAlternate - /// : alternateOfNestedAlternate; + /// ? consequent + /// : nestedAlternate + + /// binary + // <- notice how the content is aligned to the `: ` + /// ? consequentOfnestedAlternate + /// : alternateOfNestedAlternate; /// ``` NestedAlternate { parent: JsAnyConditional }, @@ -539,10 +539,10 @@ enum ConditionalLayout { /// /// ```javascript /// condition1 - /// ? condition2 - /// ? consequent2 // <-- consequent and alternate gets indented - /// : alternate2 - /// : alternate1; + /// ? condition2 + /// ? consequent2 // <-- consequent and alternate gets indented + /// : alternate2 + /// : alternate1; /// ``` NestedConsequent { parent: JsAnyConditional }, From 33bab16a37ee732f08370791683df7f824e021cd Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 2 Sep 2022 15:24:59 +0200 Subject: [PATCH 3/3] Update crates/rome_js_formatter/src/utils/conditional.rs Co-authored-by: Emanuele Stoppa --- crates/rome_js_formatter/src/utils/conditional.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/rome_js_formatter/src/utils/conditional.rs b/crates/rome_js_formatter/src/utils/conditional.rs index 961badc0138..0bf70651994 100644 --- a/crates/rome_js_formatter/src/utils/conditional.rs +++ b/crates/rome_js_formatter/src/utils/conditional.rs @@ -141,7 +141,6 @@ impl FormatRule for FormatJsAnyConditionalRule { _ => { // Add an extra level of indent to nested consequences. if layout.is_nested_consequent() { - // if f.context().indent_style().is_tab() { // This may look silly but the `dedent` is to remove the outer `align` added by the parent's formatting of the consequent. // The `indent` is necessary to indent the content by one level with a tab. // Adding an `indent` without the `dedent` would result in the `outer` align being converted