Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_formatter): Format Member Chain #3273

Merged
merged 4 commits into from
Sep 28, 2022
Merged

feat(rome_js_formatter): Format Member Chain #3273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e9d996
feat(rome_js_formatter): Member chain formatting
MichaReiser Sep 27, 2022
f6d611b
Undo literalline
MichaReiser Sep 27, 2022
2ab6086
Update crates/rome_js_formatter/src/utils/member_chain/mod.rs
MichaReiser Sep 27, 2022
6ccd897
address code review feedback
MichaReiser Sep 27, 2022
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
2 changes: 1 addition & 1 deletion crates/rome_js_formatter/src/js/expressions/arrow_function_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ fn template_literal_contains_new_line(template: &JsTemplate) -> bool {
/// ```
///
/// Returns `false` because the template isn't on the same line as the '+' token.
fn is_multiline_template_starting_on_same_line(template: &JsTemplate) -> bool {
pub(crate) fn is_multiline_template_starting_on_same_line(template: &JsTemplate) -> bool {
let contains_new_line = template_literal_contains_new_line(template);

let starts_on_same_line = template.syntax().first_token().map_or(false, |token| {
Expand Down
193 changes: 141 additions & 52 deletions crates/rome_js_formatter/src/js/expressions/call_arguments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use crate::js::expressions::arrow_function_expression::is_multiline_template_starting_on_same_line;
use crate::prelude::*;
use crate::utils::{is_call_like_expression, write_arguments_multi_line};
use rome_formatter::{format_args, write, CstFormatContext};
use rome_js_syntax::{
JsAnyCallArgument, JsAnyExpression, JsAnyFunctionBody, JsAnyLiteralExpression, JsAnyName,
JsAnyStatement, JsArrayExpression, JsArrowFunctionExpression, JsCallArgumentList,
JsCallArguments, JsCallArgumentsFields, JsCallExpression, TsReferenceType,
JsCallArguments, JsCallArgumentsFields, JsCallExpression, JsExpressionStatement,
TsReferenceType,
};
use rome_rowan::{AstSeparatedList, SyntaxResult, SyntaxTokenText};

Expand Down Expand Up @@ -33,6 +35,28 @@ impl FormatNodeRule<JsCallArguments> for FormatJsCallArguments {
);
}

let call_expression = node.parent::<JsCallExpression>();

if is_commonjs_or_amd_call(node, call_expression.as_ref())?
|| is_multiline_template_only_args(node)
{
return write!(
f,
[
l_paren_token.format(),
format_with(|f| {
f.join_with(space())
.entries(
args.format_separated(",")
.with_trailing_separator(TrailingSeparator::Omit),
)
.finish()
}),
r_paren_token.format()
]
);
}

let mut iter = args.iter();
let first_argument = iter.next();
let second_argument = iter.next();
Expand Down Expand Up @@ -97,19 +121,29 @@ impl FormatNodeRule<JsCallArguments> for FormatJsCallArguments {
.map(|e| e.memoized())
.collect();

let an_argument_breaks =
separated
.iter_mut()
.enumerate()
.any(|(index, element)| match element.inspect(f) {
Ok(element) => {
let in_relevant_range = should_group_first_argument && index > 0
|| (should_group_last_argument && index < args.len() - 1);
let mut any_argument_breaks = false;
let mut first_last_breaks = false;

in_relevant_range && element.will_break()
}
Err(_) => false,
});
for (index, argument) in separated.iter_mut().enumerate() {
let breaks = argument.inspect(f)?.will_break();

any_argument_breaks = any_argument_breaks || breaks;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
any_argument_breaks = any_argument_breaks || breaks;
any_argument_breaks |= breaks;

Copy link
Contributor Author

@MichaReiser MichaReiser Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the same to my understanding because || is the logical or whereas |= does a bitwise or. The logical or allows to short circuit the right hand side if the left side is already true which isn't the case for bitwise or.

rust operators


if (should_group_first_argument && index > 0)
|| (should_group_last_argument && index < args.len() - 1)
{
first_last_breaks = first_last_breaks || breaks;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
first_last_breaks = first_last_breaks || breaks;
first_last_breaks |= breaks;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. |= performs a bitwise or where || performance a logical or.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do the same thing in another part of the code, why here is different? https://github.com/rome/tools/blob/main/crates/rome_cli/src/traversal.rs#L356

Copy link
Contributor Author

@MichaReiser MichaReiser Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's important to be different here because it allows the compiler to short circuit to not compute breaks.

I don't know if this is something we want (or don't want) in the link you shared but I would be careful about using bitwise operations (it's most often not what you want).

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this example:


use ::std::*;

fn some_function(x: usize) -> bool {
    x % 2 == 0
}


pub fn main ()
{


    let mut ord = false;
    
    let mut bitwise_or = false;
    
    for x in 0..10 {
        bitwise_or |= some_function(x);
        ord = ord || some_function(x);
    }
    
    dbg!(ord);
    dbg!(bitwise_or);
}

Rust generates very different code for |= and ||

for |=

        call    example::some_function
        mov     byte ptr [rsp + 159], al
        mov     cl, byte ptr [rsp + 159]
        mov     al, byte ptr [rsp + 199]
        or      al, cl
        and     al, 1
        mov     byte ptr [rsp + 199], al

You can see how it unconditionally calls some_function and then ors the result

for ||

        test    byte ptr [rsp + 198], 1
        jne     .LBB20_9
        mov     rdi, qword ptr [rsp + 144]
        call    example::some_function
        mov     byte ptr [rsp + 143], al
        jmp     .LBB20_11
.LBB20_9:
        mov     byte ptr [rsp + 254], 1
.LBB20_10:
        mov     al, byte ptr [rsp + 254]
        and     al, 1
        mov     byte ptr [rsp + 198], al
        jmp     .LBB20_2
.LBB20_11:
        mov     al, byte ptr [rsp + 143]
        and     al, 1
        mov     byte ptr [rsp + 254], al
        jmp     .LBB20_10

It first tests if ord is true. If so, it jumps to the block LBB20_9 (not calling some_function). It only calls into some_function if ord is false.

LLVM may still be able to optimise to the same output BUT using | has stricter calling conventions than || where it's OK to not compute the right value if the left value is true.

compiler explorer

Whether the use of |= is correct in the other places is probably best answered by @leops who wrote the code (in some places it certainly is because it uses bitflags. In some cases it isn't clear to me because left and right are booleans and maybe using || would have been more appropriate).

if breaks {
break;
}
}
}

let format_flat_arguments = format_with(|f| {
f.join_with(soft_line_break_or_space())
.entries(separated.iter())
.finish()
});

// We now cache them the delimiters tokens. This is needed because `[rome_formatter::best_fitting]` will try to
// print each version first
Expand All @@ -125,27 +159,18 @@ impl FormatNodeRule<JsCallArguments> for FormatJsCallArguments {
// function, but here we use a different way to print the trailing separator
write!(
f,
[
&l_paren,
&group(&format_with(|f| {
write!(
f,
[
&soft_block_indent(&format_args![
format_with(|f| {
write_arguments_multi_line(separated.iter(), f)
}),
soft_line_break()
]),
&r_paren
]
)
[group(&format_args![
l_paren,
soft_block_indent(&format_with(|f| {
write_arguments_multi_line(separated.iter(), f)
})),
]
r_paren
])
.should_expand(true)]
)
});

if an_argument_breaks {
if first_last_breaks {
return write!(f, [all_arguments_expanded]);
}

Expand All @@ -160,39 +185,26 @@ impl FormatNodeRule<JsCallArguments> for FormatJsCallArguments {
let mut iter = separated.iter();
// SAFETY: check on the existence of at least one argument are done before
let first = iter.next().unwrap();
f.join_with(&space())
.entry(&format_with(|f| {
write!(f, [&format_args![first, expand_parent()]])
}))
.entries(iter)
.finish()?;
f.join_with(&space()).entry(&first).entries(iter).finish()?;
} else {
// special formatting of the last element
let mut iter = separated.iter();
// SAFETY: check on the existence of at least one argument are done before
let last = iter.next_back().unwrap();

f.join_with(&space())
.entries(iter)
.entry(&format_with(|f| {
write!(f, [&format_args![last, expand_parent()]])
}))
.finish()?;
f.join_with(&space()).entries(iter).entry(&last).finish()?;
}
write!(f, [r_paren])
});

if any_argument_breaks {
write!(f, [expand_parent()])?;
}

write!(
f,
[best_fitting![
format_args![
l_paren,
group(&format_with(|f| {
write_arguments_multi_line(separated.iter(), f)
})),
r_paren,
],
edge_arguments_do_not_break,
format_args![l_paren, format_flat_arguments, r_paren],
group(&edge_arguments_do_not_break).should_expand(true),
all_arguments_expanded
]]
)
Expand All @@ -209,7 +221,7 @@ impl FormatNodeRule<JsCallArguments> for FormatJsCallArguments {
write_arguments_multi_line(separated, f)
})),
r_paren,
]),]
])]
)
}
}
Expand Down Expand Up @@ -412,6 +424,83 @@ fn could_group_expression_argument(
Ok(result)
}

/// Tests if this is a call to commonjs [`require`](https://nodejs.org/api/modules.html#requireid)
/// or amd's [`define`](https://github.com/amdjs/amdjs-api/wiki/AMD#define-function-) function.
fn is_commonjs_or_amd_call(
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
arguments: &JsCallArguments,
call: Option<&JsCallExpression>,
) -> SyntaxResult<bool> {
let call = match call {
Some(call) => call,
None => return Ok(false),
ematipico marked this conversation as resolved.
Show resolved Hide resolved
};

let callee = call.callee()?;

Ok(match callee {
JsAnyExpression::JsIdentifierExpression(identifier) => {
let reference = identifier.name()?;

if reference.has_name("require") {
true
} else if reference.has_name("define") {
let in_statement = call.parent::<JsExpressionStatement>().is_some();

if in_statement {
let args = arguments.args();
match args.len() {
1 => true,
2 => matches!(
args.first(),
Some(Ok(JsAnyCallArgument::JsAnyExpression(
JsAnyExpression::JsArrayExpression(_)
)))
),
3 => {
let mut iter = args.iter();
let first = iter.next();
let second = iter.next();
matches!(
(first, second),
(
Some(Ok(JsAnyCallArgument::JsAnyExpression(
JsAnyExpression::JsAnyLiteralExpression(
JsAnyLiteralExpression::JsStringLiteralExpression(_)
)
))),
Some(Ok(JsAnyCallArgument::JsAnyExpression(
JsAnyExpression::JsArrayExpression(_)
)))
)
)
}
_ => false,
}
} else {
false
}
} else {
false
}
}
_ => false,
})
}

/// Returns `true` if `arguments` contains a single [multiline template literal argument that starts on its own ](is_multiline_template_starting_on_same_line).
fn is_multiline_template_only_args(arguments: &JsCallArguments) -> bool {
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
let args = arguments.args();

match args.first() {
Some(Ok(JsAnyCallArgument::JsAnyExpression(JsAnyExpression::JsTemplate(template))))
if args.len() == 1 =>
{
is_multiline_template_starting_on_same_line(&template)
}
_ => false,
}
}

/// This function is used to check if the code is a hook-like code:
///
/// ```js
Expand Down
49 changes: 45 additions & 4 deletions crates/rome_js_formatter/src/js/expressions/call_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,58 @@
use crate::prelude::*;
use rome_formatter::write;

use crate::parentheses::NeedsParentheses;
use crate::utils::get_member_chain;
use rome_js_syntax::{JsCallExpression, JsSyntaxKind, JsSyntaxNode};
use crate::utils::member_chain::MemberChain;
use rome_js_syntax::{
JsAnyExpression, JsCallExpression, JsCallExpressionFields, JsSyntaxKind, JsSyntaxNode,
};

#[derive(Debug, Clone, Default)]
pub struct FormatJsCallExpression;

impl FormatNodeRule<JsCallExpression> for FormatJsCallExpression {
fn fmt_fields(&self, node: &JsCallExpression, f: &mut JsFormatter) -> FormatResult<()> {
let member_chain = get_member_chain(node, f)?;
let JsCallExpressionFields {
callee,
optional_chain_token,
type_arguments,
arguments,
} = node.as_fields();

member_chain.fmt(f)
let callee = callee?;

if matches!(
callee,
JsAnyExpression::JsStaticMemberExpression(_)
| JsAnyExpression::JsComputedMemberExpression(_)
) && !callee.needs_parentheses()
{
let member_chain = MemberChain::from_call_expression(
node.clone(),
f.comments(),
f.options().tab_width(),
)?;

member_chain.fmt(f)
} else {
let format_inner = format_with(|f| {
write!(
f,
[
callee.format(),
optional_chain_token.format(),
type_arguments.format(),
arguments.format()
]
)
});

if matches!(callee, JsAnyExpression::JsCallExpression(_)) {
write!(f, [group(&format_inner)])
} else {
write!(f, [format_inner])
}
}
}

fn needs_parentheses(&self, item: &JsCallExpression) -> bool {
Expand Down
32 changes: 23 additions & 9 deletions crates/rome_js_formatter/src/js/expressions/computed_member_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,30 +34,44 @@ impl Format<JsFormatContext> for JsAnyComputedMemberLike {
fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
write!(f, [self.object().format()])?;

match self.member()? {
FormatComputedMemberLookup(self).fmt(f)
}
}

/// Formats the lookup portion (everything except the object) of a computed member like.
pub(crate) struct FormatComputedMemberLookup<'a>(&'a JsAnyComputedMemberLike);

impl<'a> FormatComputedMemberLookup<'a> {
pub(crate) fn new(member_like: &'a JsAnyComputedMemberLike) -> Self {
Self(member_like)
}
}

impl Format<JsFormatContext> for FormatComputedMemberLookup<'_> {
fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
match self.0.member()? {
JsAnyExpression::JsAnyLiteralExpression(
JsAnyLiteralExpression::JsNumberLiteralExpression(literal),
) => {
write!(
f,
[
self.optional_chain_token().format(),
self.l_brack_token().format(),
self.0.optional_chain_token().format(),
self.0.l_brack_token().format(),
literal.format(),
self.r_brack_token().format()
self.0.r_brack_token().format()
]
)
}
member => {
write![
f,
[group(&format_args![
self.optional_chain_token().format(),
self.l_brack_token().format(),
soft_line_break(),
self.0.optional_chain_token().format(),
self.0.l_brack_token().format(),
soft_block_indent(&member.format()),
self.r_brack_token().format()
]),]
self.0.r_brack_token().format()
])]
]
}
}
Expand Down
Loading