Skip to content

Commit 0b63b1d

Browse files
ConaclosConaclos
authored
fix(js/useSortedKeys): don't compare named properties with nameless ones (#6551)
1 parent f62e748 commit 0b63b1d

7 files changed

Lines changed: 331 additions & 118 deletions

File tree

‎.changeset/khaki-pandas-sell.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@biomejs/biome": patch
3+
---
4+
5+
Fixed [#6536](https://github.com/biomejs/biome/issues/6536). `useSortedKeys` no longer panics in some edge cases where object spreads are involved.

‎crates/biome_js_analyze/src/assist/source/use_sorted_keys.rs‎

Lines changed: 37 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -88,54 +88,45 @@ impl Rule for UseSortedKeys {
8888
type Options = ();
8989

9090
fn run(ctx: &RuleContext<Self>) -> Self::Signals {
91-
let mut members = Vec::new();
92-
let mut groups = Vec::new();
91+
let member_list = ctx.query();
92+
let mut chunks = Vec::new();
93+
let mut current_chunk_members = Vec::with_capacity(member_list.len());
9394

9495
let get_name = |name: SyntaxResult<AnyJsObjectMemberName>| name.ok()?.name();
9596

96-
for element in ctx.query().elements() {
97-
if let Ok(element) = element.node() {
98-
match element {
99-
AnyJsObjectMember::JsSpread(_) | AnyJsObjectMember::JsBogusMember(_) => {
100-
// Keep the spread order because it's not safe to change order of it.
101-
// Logic here is similar to /crates/biome_js_analyze/src/assists/source/use_sorted_attributes.rs
102-
if !members.is_empty() && !members.is_sorted() {
103-
groups.push(members.clone());
104-
}
105-
// Reuse the same buffer
106-
members.clear();
107-
members.push(ObjectMember::new(element.clone(), None));
108-
}
109-
AnyJsObjectMember::JsPropertyObjectMember(member) => {
110-
members.push(ObjectMember::new(element.clone(), get_name(member.name())));
111-
}
112-
AnyJsObjectMember::JsGetterObjectMember(member) => {
113-
members.push(ObjectMember::new(element.clone(), get_name(member.name())));
114-
}
115-
AnyJsObjectMember::JsSetterObjectMember(member) => {
116-
members.push(ObjectMember::new(element.clone(), get_name(member.name())));
117-
}
118-
AnyJsObjectMember::JsMethodObjectMember(member) => {
119-
members.push(ObjectMember::new(element.clone(), get_name(member.name())));
120-
}
97+
for (index, element) in member_list.elements().enumerate() {
98+
if let Ok(element) = element.into_node() {
99+
let name = match &element {
100+
AnyJsObjectMember::JsSpread(_) | AnyJsObjectMember::JsBogusMember(_) => None,
101+
AnyJsObjectMember::JsPropertyObjectMember(member) => get_name(member.name()),
102+
AnyJsObjectMember::JsGetterObjectMember(member) => get_name(member.name()),
103+
AnyJsObjectMember::JsSetterObjectMember(member) => get_name(member.name()),
104+
AnyJsObjectMember::JsMethodObjectMember(member) => get_name(member.name()),
121105
AnyJsObjectMember::JsShorthandPropertyObjectMember(member) => {
122-
let name = member
123-
.name()
124-
.ok()
125-
.map(|name| name.name().ok())
126-
.unwrap_or_default();
127-
128-
members.push(ObjectMember::new(element.clone(), name));
106+
member.name().and_then(|name| name.name()).ok()
107+
}
108+
};
109+
if let Some(name) = name {
110+
current_chunk_members.push(ObjectMember::new(element, name));
111+
} else {
112+
// If a name cannot be extracted, then the current chunk of named properties stops here.
113+
if !current_chunk_members.is_empty() && !current_chunk_members.is_sorted() {
114+
chunks.push(current_chunk_members);
115+
// Create a new buffer with the number of remaining members to test
116+
current_chunk_members = Vec::with_capacity(member_list.len() - index - 1);
117+
} else {
118+
// Reuse the buffer
119+
current_chunk_members.clear();
129120
}
130121
}
131122
}
132123
}
133124

134-
if !members.is_empty() && !members.is_sorted() {
135-
groups.push(members);
125+
if !current_chunk_members.is_empty() && !current_chunk_members.is_sorted() {
126+
chunks.push(current_chunk_members);
136127
}
137128

138-
groups.into_boxed_slice()
129+
chunks.into_boxed_slice()
139130
}
140131

141132
fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> {
@@ -159,13 +150,9 @@ impl Rule for UseSortedKeys {
159150
fn action(ctx: &RuleContext<Self>, state: &Self::State) -> Option<JsRuleAction> {
160151
let mut sorted_state = state.clone();
161152

162-
sorted_state.sort_unstable();
163-
164-
// FIXME: why do we need to check it?
165-
// Checking if it is sorted in `run` should be enough.
166-
if &sorted_state == state {
167-
return None;
168-
}
153+
// We use a stable sort to ensure that properties with an identical name (a getter and a setter for a property)
154+
// keep their initial relative order.
155+
sorted_state.sort();
169156

170157
let mut mutation = ctx.root().begin();
171158

@@ -185,10 +172,10 @@ impl Rule for UseSortedKeys {
185172
#[derive(Debug, Clone)]
186173
pub struct ObjectMember {
187174
member: AnyJsObjectMember,
188-
name: Option<TokenText>,
175+
name: TokenText,
189176
}
190177
impl ObjectMember {
191-
fn new(member: AnyJsObjectMember, name: Option<TokenText>) -> Self {
178+
fn new(member: AnyJsObjectMember, name: TokenText) -> Self {
192179
Self { member, name }
193180
}
194181
}
@@ -200,16 +187,14 @@ impl PartialEq for ObjectMember {
200187
}
201188
impl Ord for ObjectMember {
202189
fn cmp(&self, other: &Self) -> Ordering {
203-
// If some doesn't have a name (e.g spread/calculated property) - keep the order.
204-
let (Some(self_name), Some(other_name)) = (&self.name, &other.name) else {
205-
return Ordering::Equal;
206-
};
207-
208-
self_name.text().ascii_nat_cmp(other_name.text())
190+
self.name.text().ascii_nat_cmp(other.name.text())
209191
}
210192
}
211193
impl PartialOrd for ObjectMember {
212194
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
213195
Some(self.cmp(other))
214196
}
215197
}
198+
199+
#[test]
200+
fn test() {}

‎crates/biome_js_analyze/tests/specs/source/useSortedKeys/issue6536.js‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
const newObj = {
2+
...obj,
3+
a: '',
4+
b: '',
5+
c: '',
6+
d: '',
7+
e: '',
8+
f: '',
9+
g: '',
10+
h: '',
11+
i: '',
12+
j: '',
13+
k: '',
14+
l: '',
15+
m: '',
16+
n: '',
17+
o: '',
18+
p: '',
19+
q: '',
20+
r: '',
21+
s: '',
22+
t: '',
23+
u: '',
24+
v: '',
25+
w: '',
26+
x: '',
27+
z: '',
28+
aa: '',
29+
ab: '',
30+
ac: '',
31+
ad: '',
32+
ae: '',
33+
af: '',
34+
ag: '',
35+
};

‎crates/biome_js_analyze/tests/specs/source/useSortedKeys/issue6536.js.snap‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
---
2+
source: crates/biome_js_analyze/tests/spec_tests.rs
3+
expression: issue6536.js
4+
---
5+
# Input
6+
```js
7+
const newObj = {
8+
...obj,
9+
a: '',
10+
b: '',
11+
c: '',
12+
d: '',
13+
e: '',
14+
f: '',
15+
g: '',
16+
h: '',
17+
i: '',
18+
j: '',
19+
k: '',
20+
l: '',
21+
m: '',
22+
n: '',
23+
o: '',
24+
p: '',
25+
q: '',
26+
r: '',
27+
s: '',
28+
t: '',
29+
u: '',
30+
v: '',
31+
w: '',
32+
x: '',
33+
z: '',
34+
aa: '',
35+
ab: '',
36+
ac: '',
37+
ad: '',
38+
ae: '',
39+
af: '',
40+
ag: '',
41+
};
42+
```
43+
44+
# Diagnostics
45+
```
46+
issue6536.js:2:5 assist/source/useSortedKeys FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
47+
48+
i The keys are not sorted.
49+
50+
1 │ const newObj = {
51+
> 2 │ ...obj,
52+
│ ^^^^^^^
53+
> 3 │ a: '',
54+
> 4 │ b: '',
55+
...
56+
> 32 │ ae: '',
57+
> 33 │ af: '',
58+
> 34 │ ag: '',
59+
│ ^^^^^^^
60+
35 │ };
61+
62+
i Safe fix: Sort the object properties.
63+
64+
2 2 │ ...obj,
65+
3 3 │ a: '',
66+
4 │ - ····b:·'',
67+
5 │ - ····c:·'',
68+
6 │ - ····d:·'',
69+
7 │ - ····e:·'',
70+
8 │ - ····f:·'',
71+
9 │ - ····g:·'',
72+
10 │ - ····h:·'',
73+
11 │ - ····i:·'',
74+
12 │ - ····j:·'',
75+
13 │ - ····k:·'',
76+
14 │ - ····l:·'',
77+
15 │ - ····m:·'',
78+
16 │ - ····n:·'',
79+
17 │ - ····o:·'',
80+
18 │ - ····p:·'',
81+
19 │ - ····q:·'',
82+
20 │ - ····r:·'',
83+
21 │ - ····s:·'',
84+
22 │ - ····t:·'',
85+
23 │ - ····u:·'',
86+
24 │ - ····v:·'',
87+
25 │ - ····w:·'',
88+
26 │ - ····x:·'',
89+
27 │ - ····z:·'',
90+
28 │ - ····aa:·'',
91+
29 │ - ····ab:·'',
92+
30 │ - ····ac:·'',
93+
31 │ - ····ad:·'',
94+
32 │ - ····ae:·'',
95+
33 │ - ····af:·'',
96+
34 │ - ····ag:·'',
97+
4 │ + ····aa:·'',
98+
5 │ + ····ab:·'',
99+
6 │ + ····ac:·'',
100+
7 │ + ····ad:·'',
101+
8 │ + ····ae:·'',
102+
9 │ + ····af:·'',
103+
10 │ + ····ag:·'',
104+
11 │ + ····b:·'',
105+
12 │ + ····c:·'',
106+
13 │ + ····d:·'',
107+
14 │ + ····e:·'',
108+
15 │ + ····f:·'',
109+
16 │ + ····g:·'',
110+
17 │ + ····h:·'',
111+
18 │ + ····i:·'',
112+
19 │ + ····j:·'',
113+
20 │ + ····k:·'',
114+
21 │ + ····l:·'',
115+
22 │ + ····m:·'',
116+
23 │ + ····n:·'',
117+
24 │ + ····o:·'',
118+
25 │ + ····p:·'',
119+
26 │ + ····q:·'',
120+
27 │ + ····r:·'',
121+
28 │ + ····s:·'',
122+
29 │ + ····t:·'',
123+
30 │ + ····u:·'',
124+
31 │ + ····v:·'',
125+
32 │ + ····w:·'',
126+
33 │ + ····x:·'',
127+
34 │ + ····z:·'',
128+
35 35 │ };
129+
130+
131+
```

‎crates/biome_js_analyze/tests/specs/source/useSortedKeys/unsorted.js.snap‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,32 @@ unsorted.js:3:2 assist/source/useSortedKeys FIXABLE ━━━━━━━━
9494
11 │ - → },
9595
11 │ + → ba:·2,
9696
12 12 │ [getProp()]: 2,
97+
13 13 │ aba: 2,
98+
99+
100+
```
101+
102+
```
103+
unsorted.js:3:2 assist/source/useSortedKeys FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
104+
105+
i The keys are not sorted.
106+
107+
1 │ const obj = {
108+
2 │ // Comment b
109+
> 3 │ b: 1,
110+
│ ^^^^^
111+
> 4 │ // Comment a
112+
...
113+
> 19 │ return "";
114+
> 20 │ },
115+
│ ^^
116+
21 │ };
117+
22 │
118+
119+
i Safe fix: Sort the object properties.
120+
121+
11 11 │ },
122+
12 12 │ [getProp()]: 2,
97123
13 │ - → aba:·2,
98124
14 │ - → abc:·3,
99125
15 │ - → abb:·3,

‎crates/biome_js_analyze/tests/suppression/source/useSortedKeys/unsorted.js.snap‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
---
22
source: crates/biome_js_analyze/tests/spec_tests.rs
33
expression: unsorted.js
4-
snapshot_kind: text
54
---
65
# Input
76
```jsx
@@ -91,3 +90,35 @@ unsorted.js:2:2 assist/source/useSortedKeys FIXABLE ━━━━━━━━
9190
9291
9392
```
93+
94+
```
95+
unsorted.js:2:2 assist/source/useSortedKeys FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
96+
97+
i The keys are not sorted.
98+
99+
1 │ const obj = {
100+
> 2 │ b: 1,
101+
│ ^^^^^
102+
> 3 │ a: 1,
103+
> 4 │ ...g,
104+
...
105+
> 17 │ return "";
106+
> 18 │ }
107+
│ ^
108+
19 │ }
109+
20 │
110+
111+
i Safe fix: Suppress action assist/source/useSortedKeys for this line.
112+
113+
1 │ + //·biome-ignore·assist/source/useSortedKeys:·<explanation>
114+
1 2 │ const obj = {
115+
2 3 │ b: 1,
116+
117+
i Safe fix: Suppress action assist/source/useSortedKeys for the whole file.
118+
119+
1 │ + /**·biome-ignore-all·assist/source/useSortedKeys:·<explanation>·*/
120+
1 2 │ const obj = {
121+
2 3 │ b: 1,
122+
123+
124+
```

0 commit comments

Comments
 (0)