Skip to content

Commit 564dabd

Browse files
azjezzazjezz
authored
fix(formatter): idempotence issues (#320)
Signed-off-by: azjezz <[email protected]>
1 parent d59024d commit 564dabd

File tree

9 files changed

+264
-16
lines changed

9 files changed

+264
-16
lines changed

‎crates/formatter/src/internal/format/call_arguments.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ pub(super) fn print_argument_list<'arena>(
173173

174174
parts.push(print_right_parenthesis(f, dangling_comments.as_ref(), &right_parenthesis, Some(true)));
175175

176-
Document::Group(Group::new(parts))
176+
Document::Group(Group::new(parts).with_break(true))
177177
};
178178

179179
if should_break_all {

‎crates/formatter/src/internal/format/member_access.rs‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -158,10 +158,6 @@ impl<'arena> MemberAccessChain<'arena> {
158158

159159
#[inline]
160160
pub fn is_eligible_for_chaining(&self, f: &FormatterState<'_, 'arena>) -> bool {
161-
if f.settings.preserve_breaking_member_access_chain && self.is_already_broken(f) {
162-
return true;
163-
}
164-
165161
if self.has_comments_in_chain(f) {
166162
return true;
167163
}
@@ -223,7 +219,6 @@ impl<'arena> MemberAccessChain<'arena> {
223219

224220
#[inline]
225221
fn is_already_broken(&self, f: &FormatterState) -> bool {
226-
// Check if there are comments after the base expression
227222
if let Some(first_access) = self.accesses.first() {
228223
let base_end = self.base.span().end;
229224
let first_op_start = first_access.get_operator_span().start;
@@ -445,7 +440,7 @@ pub(super) fn print_member_access_chain<'arena>(
445440
f: &mut FormatterState<'_, 'arena>,
446441
) -> Document<'arena> {
447442
let base_document = member_access_chain.base.format(f);
448-
let mut parts = if base_needs_parerns(f, member_access_chain.base) {
443+
let mut parts = if base_needs_parens(f, member_access_chain.base) {
449444
vec![in f.arena; Document::String("("), base_document, Document::String(")")]
450445
} else {
451446
vec![in f.arena; base_document]
@@ -541,9 +536,9 @@ pub(super) fn print_member_access_chain<'arena>(
541536
Document::Group(Group::new(parts).with_id(group_id))
542537
}
543538

544-
fn base_needs_parerns<'arena>(f: &FormatterState<'_, 'arena>, base: &'arena Expression<'arena>) -> bool {
539+
fn base_needs_parens<'arena>(f: &FormatterState<'_, 'arena>, base: &'arena Expression<'arena>) -> bool {
545540
if let Expression::Parenthesized(parenthesized) = base {
546-
return base_needs_parerns(f, parenthesized.expression);
541+
return base_needs_parens(f, parenthesized.expression);
547542
}
548543

549544
match base {

‎crates/formatter/src/internal/macros.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,10 @@ macro_rules! wrap {
99
$f.is_wrapped_in_parens |= needed_to_wrap_in_parens;
1010
let leading = $f.print_leading_comments(node.span());
1111
let doc = $block;
12-
let doc = if needed_to_wrap_in_parens { $f.add_parens(doc, node) } else { doc };
1312
let trailing = $f.print_trailing_comments_for_node(node);
13+
let has_leading_comments = leading.is_some();
1414
let doc = $f.print_comments(leading, doc, trailing);
15+
let doc = if needed_to_wrap_in_parens { $f.add_parens(doc, node, has_leading_comments) } else { doc };
1516
$f.leave_node();
1617
$f.is_wrapped_in_parens = was_wrapped_in_parens;
1718
doc

‎crates/formatter/src/internal/parens.rs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,13 @@ use crate::document::Line;
1313
use crate::internal::FormatterState;
1414

1515
impl<'ctx, 'arena> FormatterState<'ctx, 'arena> {
16-
pub(crate) fn add_parens(&mut self, document: Document<'arena>, node: Node<'arena, 'arena>) -> Document<'arena> {
17-
if self.should_indent(node) {
16+
pub(crate) fn add_parens(
17+
&mut self,
18+
document: Document<'arena>,
19+
node: Node<'arena, 'arena>,
20+
has_leading_comments: bool,
21+
) -> Document<'arena> {
22+
if has_leading_comments || self.should_indent(node) {
1823
Document::Group(Group::new(vec![
1924
in self.arena;
2025
Document::String("("),

‎crates/formatter/tests/cases/hug_last_new_expression_with_named_args/after.php‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
<?php
22

3-
$foo = new \Lcobucci\JWT\Validation\Validator()->assert($token, new SignedWith(
4-
signer: new Sha256(),
5-
key: InMemory::plainText($this->jwtKey),
6-
));
3+
$foo = new \Lcobucci\JWT\Validation\Validator()->assert(
4+
$token,
5+
new SignedWith(
6+
signer: new Sha256(),
7+
key: InMemory::plainText($this->jwtKey),
8+
),
9+
);

‎crates/formatter/tests/cases/issue_204/after.php‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
<?php
2+
3+
function a()
4+
{
5+
$promises = (
6+
/**
7+
* @return \Generator
8+
* @return \Generator
9+
*/
10+
static function () use ($paginator): \Generator {
11+
yield $paginator->getCurrentPageResultsAsync();
12+
13+
while ($paginator->hasNextPage()) {
14+
$paginator->nextPage();
15+
16+
yield $paginator->getCurrentPageResultsAsync();
17+
}
18+
}
19+
)();
20+
}
21+
22+
function b()
23+
{
24+
Psl\invariant(
25+
null
26+
=== Iter\search(
27+
Vec\concat(...Vec\map($this->section->getRows(), static fn(ProductRow $row): array => $row->getFields())),
28+
static fn(ProductField $existing): bool => $existing->getName() === $field->getName(),
29+
),
30+
'Field names need to be unique per section.',
31+
);
32+
}
33+
34+
function c()
35+
{
36+
[
37+
'claim_coverage_descriptions' =>
38+
$claimCoverageDescriptions = Vec\map(
39+
$filter->claimCoverageDescriptions,
40+
static fn(ClaimCoverageDescription $claimCoverageDescription): string => $claimCoverageDescription->value,
41+
),
42+
];
43+
}
44+
45+
function d()
46+
{
47+
return (
48+
Iter\search(
49+
$infos,
50+
static fn(ProductCommercialPropertyInfo $existing): bool => (
51+
$existing->getSubLocation()->getLocation()->getUuid()->equals($location->getUuid())
52+
&& $existing->getSubLocation()->getNumber() === $subLocationNumber
53+
),
54+
)
55+
?? Iter\search(
56+
$infos,
57+
static fn(ProductCommercialPropertyInfo $existing): bool => (
58+
$existing->getSubLocation()->getLocation()->getNumber() === $location->getNumber()
59+
&& $existing->getSubLocation()->getNumber() === $subLocationNumber
60+
),
61+
) ?? null
62+
);
63+
}
64+
65+
function e()
66+
{
67+
foreach ($target->getQuoteTemplateDefaults()->getFormSpecifications() as $formSpecification) {
68+
if (
69+
null
70+
=== Iter\search(
71+
$selectedFormSpecifications,
72+
static fn(FormSpecificationDTO $possible): bool => $formSpecification->getDocumentTemplateConfiguration() === $possible->documentTemplateConfiguration,
73+
)
74+
) {
75+
$target->getQuoteTemplateDefaults()->removeFormSpecification($formSpecification);
76+
}
77+
}
78+
}
79+
80+
function getProductCount($division): int
81+
{
82+
return 1;
83+
}
84+
85+
function getFormsStep(Division $division): int
86+
{
87+
return (
88+
3 + // foo
89+
(+\count([1, 2, 3]))
90+
+ 1 // bar
91+
+ 1 // baz
92+
+ getProductCount($division)
93+
+ 1 // bay
94+
);
95+
}
96+
97+
function f()
98+
{
99+
if (true) {
100+
$a = !$subject->dto->inBusinessGreaterThanThreeYears
101+
? Eligibility::create(
102+
EligibilityStatus::REFER,
103+
[
104+
'Builders in the construction business for less than 3 years require an underwriter to review. A resume showing related construction experience will be required.',
105+
],
106+
) : $currentEligibility;
107+
}
108+
}
109+
110+
function g()
111+
{
112+
$coverageCodes = isset($options['coverage'])
113+
? [$options['coverage']]
114+
/** @phpstan-ignore offsetAccess.notFound */
115+
: Type\vec(Type\instance_of(CoverageCode::class))->assert($options['coverage_codes']);
116+
}
117+
118+
function h()
119+
{
120+
if (true) {
121+
Vec\sort_by(
122+
Vec\concat(
123+
($this->dataPointContextResolver)(
124+
[$context],
125+
$dataPointReplacements,
126+
static fn(TemplateReplacementInterface $templateReplacement) => Type\instance_of(DataPointDataSource::class)
127+
->assert($templateReplacement->getDataSource())->value,
128+
),
129+
$this->getProductReplacementsFor($context),
130+
),
131+
static fn(TemplateReplacementInterface $replacement): string => $replacement->getType()->getReadable(),
132+
);
133+
}
134+
}

‎crates/formatter/tests/cases/issue_204/before.php‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
<?php
2+
3+
function a() {
4+
$promises = (/**
5+
* @return \Generator
6+
* @return \Generator
7+
*/
8+
static function () use ($paginator): \Generator {
9+
yield $paginator->getCurrentPageResultsAsync();
10+
11+
while ($paginator->hasNextPage()) {
12+
$paginator->nextPage();
13+
14+
yield $paginator->getCurrentPageResultsAsync();
15+
}
16+
}
17+
)();
18+
}
19+
20+
function b()
21+
{
22+
Psl\invariant(null === Iter\search(Vec\concat(...Vec\map($this->section->getRows(), static fn(ProductRow $row): array => $row->getFields())), static fn(ProductField $existing): bool => $existing->getName() === $field->getName()), 'Field names need to be unique per section.');
23+
}
24+
25+
function c()
26+
{
27+
[
28+
'claim_coverage_descriptions' => $claimCoverageDescriptions = Vec\map($filter->claimCoverageDescriptions, static fn(ClaimCoverageDescription $claimCoverageDescription): string => $claimCoverageDescription->value),
29+
];
30+
}
31+
32+
function d()
33+
{
34+
return Iter\search($infos, static fn(ProductCommercialPropertyInfo $existing): bool => $existing->getSubLocation()->getLocation()->getUuid()->equals($location->getUuid()) && $existing->getSubLocation()->getNumber() === $subLocationNumber)
35+
?? Iter\search($infos, static fn(ProductCommercialPropertyInfo $existing): bool => $existing->getSubLocation()->getLocation()->getNumber() === $location->getNumber() && $existing->getSubLocation()->getNumber() === $subLocationNumber)
36+
?? null;
37+
}
38+
39+
function e()
40+
{
41+
foreach ($target->getQuoteTemplateDefaults()->getFormSpecifications() as $formSpecification) {
42+
if (null === Iter\search($selectedFormSpecifications, static fn(FormSpecificationDTO $possible): bool => $formSpecification->getDocumentTemplateConfiguration() === $possible->documentTemplateConfiguration)) {
43+
$target->getQuoteTemplateDefaults()->removeFormSpecification($formSpecification);
44+
}
45+
}
46+
}
47+
48+
function getProductCount($division): int{
49+
return 1;
50+
}
51+
52+
function getFormsStep(Division $division): int
53+
{
54+
return 3 + // foo
55+
+\count([1,2,3])
56+
+ 1 // bar
57+
+ 1 // baz
58+
+ getProductCount($division)
59+
+ 1; // bay
60+
}
61+
62+
function f()
63+
{
64+
if(true) {
65+
$a = !$subject->dto->inBusinessGreaterThanThreeYears ? Eligibility::create(
66+
EligibilityStatus::REFER,
67+
['Builders in the construction business for less than 3 years require an underwriter to review. A resume showing related construction experience will be required.'],
68+
) : $currentEligibility;
69+
}
70+
}
71+
72+
function g()
73+
{
74+
$coverageCodes = isset($options['coverage'])
75+
? [$options['coverage']]
76+
/** @phpstan-ignore offsetAccess.notFound */
77+
: Type\vec(Type\instance_of(CoverageCode::class))->assert($options['coverage_codes']);
78+
}
79+
80+
function h()
81+
{
82+
if(true){
83+
Vec\sort_by(
84+
Vec\concat(
85+
($this->dataPointContextResolver)(
86+
[$context],
87+
$dataPointReplacements,
88+
static fn(TemplateReplacementInterface $templateReplacement) => Type\instance_of(DataPointDataSource::class)->assert($templateReplacement->getDataSource())->value,
89+
),
90+
$this->getProductReplacementsFor($context),
91+
),
92+
static fn(TemplateReplacementInterface $replacement): string => $replacement->getType()->getReadable(),
93+
);
94+
}
95+
96+
}

‎crates/formatter/tests/cases/issue_204/settings.inc‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
FormatSettings {
2+
print_width: 180,
3+
tab_width: 4,
4+
use_tabs: false,
5+
always_break_named_arguments_list: false,
6+
preserve_breaking_argument_list: true,
7+
preserve_breaking_array_like: true,
8+
preserve_breaking_attribute_list: true,
9+
preserve_breaking_conditional_expression: true,
10+
preserve_breaking_member_access_chain: true,
11+
preserve_breaking_parameter_list: true,
12+
..Default::default()
13+
}

‎crates/formatter/tests/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,3 +205,4 @@ test_case!(issue_300);
205205
test_case!(issue_301);
206206
test_case!(issue_317);
207207
test_case!(issue_238);
208+
test_case!(issue_204);

0 commit comments

Comments
 (0)