feat(rome_js_formatter): Format Member Chain #3273
Conversation
✅ Deploy Preview for rometools canceled.
|
|
!bench_formatter |
Formatter Benchmark Results |
MichaReiser
force-pushed
the
feat/member-chain
branch
from
90f7538
to
3733287
Compare
MichaReiser
force-pushed
the
feat/member-chain
branch
from
3733287
to
3dc58c3
Compare
MichaReiser
changed the base branch from
main
to
refactor/best-fitting-fits
MichaReiser
force-pushed
the
feat/member-chain
branch
from
3dc58c3
to
d654c32
Compare
MichaReiser
force-pushed
the
feat/member-chain
branch
from
d654c32
to
f6d611b
Compare
|
!bench_formatter |
Formatter Benchmark Results |
| for (index, argument) in separated.iter_mut().enumerate() { | ||
| let breaks = argument.inspect(f)?.will_break(); | ||
|
|
||
| any_argument_breaks = any_argument_breaks || breaks; |
There was a problem hiding this comment.
| any_argument_breaks = any_argument_breaks || breaks; | |
| any_argument_breaks |= breaks; |
There was a problem hiding this comment.
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.
| if (should_group_first_argument && index > 0) | ||
| || (should_group_last_argument && index < args.len() - 1) | ||
| { | ||
| first_last_breaks = first_last_breaks || breaks; |
There was a problem hiding this comment.
| first_last_breaks = first_last_breaks || breaks; | |
| first_last_breaks |= breaks; |
There was a problem hiding this comment.
Same as above. |= performs a bitwise or where || performance a logical or.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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], alYou 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_10It 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.
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).
crates/rome_js_formatter/src/utils/member_chain/chain_member.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Summary
This PR implements member chain formatting the same way as prettier does.
There are still some differences in combination with call arguments because our call arguments formatting doesn't match Prettier's yet. I plan to tackle this as a separate PR.
Test Plan
Average compatibility: 88.42 -> 89.48%
Compatible lines: 89.3 -> 90.47%