Don't iterate iterable arguments twice in PbList

protobuf/CHANGELOG.md

@@ -13,8 +13,13 @@
   message extension field set is initialized and frozen and initialized but not
   frozen. ([#1062])
 
+* Fix `PbList` methods `addAll`, `insertAll`, `replaceRange`, `setAll`,
+  `setRange` iterating the `Iterable` argument twice. ([#730], [#1070])
+
 [#1060]: https://github.com/google/protobuf.dart/pull/1060
 [#1062]: https://github.com/google/protobuf.dart/pull/1062
+[#730]: https://github.com/google/protobuf.dart/issues/730
+[#1070]: https://github.com/google/protobuf.dart/pull/1070
 
 ## 5.0.0
 

protobuf/lib/src/protobuf/pb_list.dart

@@ -68,9 +68,13 @@ class PbList<E> extends ListBase<E> {
   void addAll(Iterable<E> iterable) {
     _checkModifiable('addAll');
     if (_check != null) {
-      iterable.forEach(_check);
+      for (final e in iterable) {
+        _check(e);
+        _addUnchecked(e);
+      }
+    } else {
+      _wrappedList.addAll(iterable);
     }
-    _wrappedList.addAll(iterable);
   }
 
   @override
@@ -108,18 +112,32 @@ class PbList<E> extends ListBase<E> {
   void insertAll(int index, Iterable<E> iterable) {
     _checkModifiable('insertAll');
     if (_check != null) {
-      iterable.forEach(_check);
+      _wrappedList.insertAll(
+        index,
+        iterable.map((E e) {
+          _check(e);
+          return e;
+        }),
+      );
+    } else {
+      _wrappedList.insertAll(index, iterable);
     }
-    _wrappedList.insertAll(index, iterable);
   }
 
   @override
   void setAll(int index, Iterable<E> iterable) {
     _checkModifiable('setAll');
     if (_check != null) {
-      iterable.forEach(_check);
+      _wrappedList.setAll(
+        index,
+        iterable.map((E e) {
+          _check(e);
+          return e;
+        }),
+      );
+    } else {
+      _wrappedList.setAll(index, iterable);
     }
-    _wrappedList.setAll(index, iterable);
   }
 
   @override
@@ -155,12 +173,19 @@ class PbList<E> extends ListBase<E> {
   @override
   void setRange(int start, int end, Iterable<E> iterable, [int skipCount = 0]) {
     _checkModifiable('setRange');
-    // NOTE: In case `take()` returns less than `end - start` elements, the
-    // _wrappedList will fail with a `StateError`.
     if (_check != null) {
-      iterable.skip(skipCount).take(end - start).forEach(_check);
+      _wrappedList.setRange(
+        start,
+        end,
+        iterable.skip(skipCount).map((E e) {
+          _check(e);
+          return e;
+        }),
+        0,
+      );
+    } else {
+      _wrappedList.setRange(start, end, iterable, skipCount);
     }
-    _wrappedList.setRange(start, end, iterable, skipCount);
   }
 
   @override
@@ -181,11 +206,18 @@ class PbList<E> extends ListBase<E> {
   @override
   void replaceRange(int start, int end, Iterable<E> newContents) {
     _checkModifiable('replaceRange');
-    final values = newContents.toList();
     if (_check != null) {
-      newContents.forEach(_check);
+      _wrappedList.replaceRange(
+        start,
+        end,
+        newContents.map((E e) {
+          _check(e);
+          return e;
+        }),
+      );
+    } else {
+      _wrappedList.replaceRange(start, end, newContents);
     }
-    _wrappedList.replaceRange(start, end, values);
   }
 
   @override

protoc_plugin/test/list_iterator_args_test.dart

@@ -0,0 +1,170 @@
+// Copyright (c) 2025, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:test/test.dart';
+
+import 'gen/google/protobuf/unittest.pb.dart';
+
+void main() {
+  // If this test fails, it means we no longer have the "check" functions in
+  // `PbList`, and half of the tests below can be removed.
+  test("PbList checks value validity", () {
+    expect(() {
+      TestAllTypes().repeatedInt32.add(2147483647 + 1);
+    }, throwsArgumentError);
+  });
+
+  // For every `PbList` method that updates the list with an `Iterable`
+  // argument, check that the `Iterable` argument is iterated only once.
+  test("addAll iterates iterator argument once (with check function)", () {
+    var mapCount = 0;
+    final list = TestAllTypes().repeatedInt32;
+    list.addAll(
+      [1, 2, 3].map((i) {
+        mapCount += 1;
+        return i;
+      }),
+    );
+    expect(mapCount, 3);
+    expect(list, [1, 2, 3]);
+  });
+
+  test("addAll iterates iterator argument once (without check function)", () {
+    var mapCount = 0;
+    final list = TestAllTypes().repeatedString;
+    list.addAll(
+      ["a", "b", "c"].map((i) {
+        mapCount += 1;
+        return i;
+      }),
+    );
+    expect(mapCount, 3);
+    expect(list, ["a", "b", "c"]);
+  });
+
+  test("insertAll iterates iterator argument once (with check function)", () {
+    var mapCount = 0;
+    final list = TestAllTypes().repeatedInt32..addAll([1, 2]);
+    list.insertAll(
+      1,
+      [4, 5, 6].map((i) {
+        mapCount += 1;
+        return i;
+      }),
+    );
+    expect(mapCount, 3);
+    expect(list, [1, 4, 5, 6, 2]);
+  });
+
+  test(
+    "insertAll iterates iterator argument once (without check function)",
+    () {
+      var mapCount = 0;
+      final list = TestAllTypes().repeatedString..addAll(["a", "b"]);
+      list.insertAll(
+        1,
+        ["c", "d", "e"].map((i) {
+          mapCount += 1;
+          return i;
+        }),
+      );
+      expect(mapCount, 3);
+      expect(list, ["a", "c", "d", "e", "b"]);
+    },
+  );
+
+  test(
+    "replaceRange iterates iterator argument once (with check function)",
+    () {
+      var mapCount = 0;
+      final list = TestAllTypes().repeatedInt32..addAll([1, 2, 3]);
+      list.replaceRange(
+        1,
+        2,
+        [4, 5, 6].map((i) {
+          mapCount += 1;
+          return i;
+        }),
+      );
+      expect(mapCount, 3);
+      expect(list, [1, 4, 5, 6, 3]);
+    },
+  );
+
+  test(
+    "replaceRange iterates iterator argument once (without check function)",
+    () {
+      var mapCount = 0;
+      final list = TestAllTypes().repeatedString..addAll(["a", "b", "c"]);
+      list.replaceRange(
+        1,
+        2,
+        ["d", "e", "f"].map((i) {
+          mapCount += 1;
+          return i;
+        }),
+      );
+      expect(mapCount, 3);
+      expect(list, ["a", "d", "e", "f", "c"]);
+    },
+  );
+
+  test("setAll iterates iterator argument once (with check function)", () {
+    var mapCount = 0;
+    final list = TestAllTypes().repeatedInt32..addAll([1, 2, 3, 4]);
+    list.setAll(
+      1,
+      [5, 6, 7].map((i) {
+        mapCount += 1;
+        return i;
+      }),
+    );
+    expect(mapCount, 3);
+    expect(list, [1, 5, 6, 7]);
+  });
+
+  test("setAll iterates iterator argument once (without check function)", () {
+    var mapCount = 0;
+    final list = TestAllTypes().repeatedString..addAll(["a", "b", "c", "d"]);
+    list.setAll(
+      1,
+      ["e", "f", "g"].map((i) {
+        mapCount += 1;
+        return i;
+      }),
+    );
+    expect(mapCount, 3);
+    expect(list, ["a", "e", "f", "g"]);
+  });
+
+  test("setRange iterates iterator argument once (with check function)", () {
+    var mapCount = 0;
+    final list = TestAllTypes().repeatedInt32..addAll([1, 2, 3]);
+    list.setRange(
+      0,
+      3,
+      [4, 5, 6].map((i) {
+        mapCount += 1;
+        return i;
+      }),
+    );
+    expect(mapCount, 3);
+    expect(list, [4, 5, 6]);
+  });
+
+  test("setRange iterates iterator argument once (without check function)", () {
+    var mapCount = 0;
+    final list = TestAllTypes().repeatedString..addAll(["a", "b", "c"]);
+    list.setRange(
+      0,
+      3,
+      ["d", "e", "f"].map((i) {
+        mapCount += 1;
+        return i;
+      }),
+    );
+    expect(mapCount, 3);
+    expect(list, ["d", "e", "f"]);
+  });
+}