Skip to content

Commit 4dfedaf

Browse files
osa1osa1
authored
Don't iterate iterable arguments twice in PbList (#1070)
Update `PbList` methods that update the list with an `Iterable` argument to avoid iterating the iterable twice: once for checking the element validity and once again for actually adding the values. Methods updated: - addAll - insertAll - replaceRange - setAll - setRange Exception handling behavior before this PR was undefined (same as the standard library `List`), and it's slightly changed with this PR: - addAll: previously if the iterator throws the list was left unchanged, now the elements until the exception will be added. - Other methods: exception behaviors are now the same as the standard library `List` methods. It's hard to tell whether the previous behavior was the same or different with the standard library `List` methods, as the exception behavior of those are undefined. To avoid allocating new iterators when a check function is not available, we have conditionals in each of these methods and call the standard library `List` methods directly when there isn't a check function. To avoid increasing number of cases needed to be tested, we don't special case iterable types like `PbList` and `List` in these methods. When the check function is available we simply `map` them with the check function. Otherwise call the same method on `wrappedList` directly. Fixes #730.
1 parent bed508e commit 4dfedaf

File tree

3 files changed

+220
-13
lines changed

3 files changed

+220
-13
lines changed

‎protobuf/CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,13 @@
1313
message extension field set is initialized and frozen and initialized but not
1414
frozen. ([#1062])
1515

16+
* Fix `PbList` methods `addAll`, `insertAll`, `replaceRange`, `setAll`,
17+
`setRange` iterating the `Iterable` argument twice. ([#730], [#1070])
18+
1619
[#1060]: https://github.com/google/protobuf.dart/pull/1060
1720
[#1062]: https://github.com/google/protobuf.dart/pull/1062
21+
[#730]: https://github.com/google/protobuf.dart/issues/730
22+
[#1070]: https://github.com/google/protobuf.dart/pull/1070
1823

1924
## 5.0.0
2025

‎protobuf/lib/src/protobuf/pb_list.dart‎

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,13 @@ class PbList<E> extends ListBase<E> {
6868
void addAll(Iterable<E> iterable) {
6969
_checkModifiable('addAll');
7070
if (_check != null) {
71-
iterable.forEach(_check);
71+
for (final e in iterable) {
72+
_check(e);
73+
_addUnchecked(e);
74+
}
75+
} else {
76+
_wrappedList.addAll(iterable);
7277
}
73-
_wrappedList.addAll(iterable);
7478
}
7579

7680
@override
@@ -108,18 +112,32 @@ class PbList<E> extends ListBase<E> {
108112
void insertAll(int index, Iterable<E> iterable) {
109113
_checkModifiable('insertAll');
110114
if (_check != null) {
111-
iterable.forEach(_check);
115+
_wrappedList.insertAll(
116+
index,
117+
iterable.map((E e) {
118+
_check(e);
119+
return e;
120+
}),
121+
);
122+
} else {
123+
_wrappedList.insertAll(index, iterable);
112124
}
113-
_wrappedList.insertAll(index, iterable);
114125
}
115126

116127
@override
117128
void setAll(int index, Iterable<E> iterable) {
118129
_checkModifiable('setAll');
119130
if (_check != null) {
120-
iterable.forEach(_check);
131+
_wrappedList.setAll(
132+
index,
133+
iterable.map((E e) {
134+
_check(e);
135+
return e;
136+
}),
137+
);
138+
} else {
139+
_wrappedList.setAll(index, iterable);
121140
}
122-
_wrappedList.setAll(index, iterable);
123141
}
124142

125143
@override
@@ -155,12 +173,19 @@ class PbList<E> extends ListBase<E> {
155173
@override
156174
void setRange(int start, int end, Iterable<E> iterable, [int skipCount = 0]) {
157175
_checkModifiable('setRange');
158-
// NOTE: In case `take()` returns less than `end - start` elements, the
159-
// _wrappedList will fail with a `StateError`.
160176
if (_check != null) {
161-
iterable.skip(skipCount).take(end - start).forEach(_check);
177+
_wrappedList.setRange(
178+
start,
179+
end,
180+
iterable.skip(skipCount).map((E e) {
181+
_check(e);
182+
return e;
183+
}),
184+
0,
185+
);
186+
} else {
187+
_wrappedList.setRange(start, end, iterable, skipCount);
162188
}
163-
_wrappedList.setRange(start, end, iterable, skipCount);
164189
}
165190

166191
@override
@@ -181,11 +206,18 @@ class PbList<E> extends ListBase<E> {
181206
@override
182207
void replaceRange(int start, int end, Iterable<E> newContents) {
183208
_checkModifiable('replaceRange');
184-
final values = newContents.toList();
185209
if (_check != null) {
186-
newContents.forEach(_check);
210+
_wrappedList.replaceRange(
211+
start,
212+
end,
213+
newContents.map((E e) {
214+
_check(e);
215+
return e;
216+
}),
217+
);
218+
} else {
219+
_wrappedList.replaceRange(start, end, newContents);
187220
}
188-
_wrappedList.replaceRange(start, end, values);
189221
}
190222

191223
@override

‎protoc_plugin/test/list_iterator_args_test.dart‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
import 'package:test/test.dart';
6+
7+
import 'gen/google/protobuf/unittest.pb.dart';
8+
9+
void main() {
10+
// If this test fails, it means we no longer have the "check" functions in
11+
// `PbList`, and half of the tests below can be removed.
12+
test("PbList checks value validity", () {
13+
expect(() {
14+
TestAllTypes().repeatedInt32.add(2147483647 + 1);
15+
}, throwsArgumentError);
16+
});
17+
18+
// For every `PbList` method that updates the list with an `Iterable`
19+
// argument, check that the `Iterable` argument is iterated only once.
20+
test("addAll iterates iterator argument once (with check function)", () {
21+
var mapCount = 0;
22+
final list = TestAllTypes().repeatedInt32;
23+
list.addAll(
24+
[1, 2, 3].map((i) {
25+
mapCount += 1;
26+
return i;
27+
}),
28+
);
29+
expect(mapCount, 3);
30+
expect(list, [1, 2, 3]);
31+
});
32+
33+
test("addAll iterates iterator argument once (without check function)", () {
34+
var mapCount = 0;
35+
final list = TestAllTypes().repeatedString;
36+
list.addAll(
37+
["a", "b", "c"].map((i) {
38+
mapCount += 1;
39+
return i;
40+
}),
41+
);
42+
expect(mapCount, 3);
43+
expect(list, ["a", "b", "c"]);
44+
});
45+
46+
test("insertAll iterates iterator argument once (with check function)", () {
47+
var mapCount = 0;
48+
final list = TestAllTypes().repeatedInt32..addAll([1, 2]);
49+
list.insertAll(
50+
1,
51+
[4, 5, 6].map((i) {
52+
mapCount += 1;
53+
return i;
54+
}),
55+
);
56+
expect(mapCount, 3);
57+
expect(list, [1, 4, 5, 6, 2]);
58+
});
59+
60+
test(
61+
"insertAll iterates iterator argument once (without check function)",
62+
() {
63+
var mapCount = 0;
64+
final list = TestAllTypes().repeatedString..addAll(["a", "b"]);
65+
list.insertAll(
66+
1,
67+
["c", "d", "e"].map((i) {
68+
mapCount += 1;
69+
return i;
70+
}),
71+
);
72+
expect(mapCount, 3);
73+
expect(list, ["a", "c", "d", "e", "b"]);
74+
},
75+
);
76+
77+
test(
78+
"replaceRange iterates iterator argument once (with check function)",
79+
() {
80+
var mapCount = 0;
81+
final list = TestAllTypes().repeatedInt32..addAll([1, 2, 3]);
82+
list.replaceRange(
83+
1,
84+
2,
85+
[4, 5, 6].map((i) {
86+
mapCount += 1;
87+
return i;
88+
}),
89+
);
90+
expect(mapCount, 3);
91+
expect(list, [1, 4, 5, 6, 3]);
92+
},
93+
);
94+
95+
test(
96+
"replaceRange iterates iterator argument once (without check function)",
97+
() {
98+
var mapCount = 0;
99+
final list = TestAllTypes().repeatedString..addAll(["a", "b", "c"]);
100+
list.replaceRange(
101+
1,
102+
2,
103+
["d", "e", "f"].map((i) {
104+
mapCount += 1;
105+
return i;
106+
}),
107+
);
108+
expect(mapCount, 3);
109+
expect(list, ["a", "d", "e", "f", "c"]);
110+
},
111+
);
112+
113+
test("setAll iterates iterator argument once (with check function)", () {
114+
var mapCount = 0;
115+
final list = TestAllTypes().repeatedInt32..addAll([1, 2, 3, 4]);
116+
list.setAll(
117+
1,
118+
[5, 6, 7].map((i) {
119+
mapCount += 1;
120+
return i;
121+
}),
122+
);
123+
expect(mapCount, 3);
124+
expect(list, [1, 5, 6, 7]);
125+
});
126+
127+
test("setAll iterates iterator argument once (without check function)", () {
128+
var mapCount = 0;
129+
final list = TestAllTypes().repeatedString..addAll(["a", "b", "c", "d"]);
130+
list.setAll(
131+
1,
132+
["e", "f", "g"].map((i) {
133+
mapCount += 1;
134+
return i;
135+
}),
136+
);
137+
expect(mapCount, 3);
138+
expect(list, ["a", "e", "f", "g"]);
139+
});
140+
141+
test("setRange iterates iterator argument once (with check function)", () {
142+
var mapCount = 0;
143+
final list = TestAllTypes().repeatedInt32..addAll([1, 2, 3]);
144+
list.setRange(
145+
0,
146+
3,
147+
[4, 5, 6].map((i) {
148+
mapCount += 1;
149+
return i;
150+
}),
151+
);
152+
expect(mapCount, 3);
153+
expect(list, [4, 5, 6]);
154+
});
155+
156+
test("setRange iterates iterator argument once (without check function)", () {
157+
var mapCount = 0;
158+
final list = TestAllTypes().repeatedString..addAll(["a", "b", "c"]);
159+
list.setRange(
160+
0,
161+
3,
162+
["d", "e", "f"].map((i) {
163+
mapCount += 1;
164+
return i;
165+
}),
166+
);
167+
expect(mapCount, 3);
168+
expect(list, ["d", "e", "f"]);
169+
});
170+
}

0 commit comments

Comments
 (0)