diff --git a/protobuf/CHANGELOG.md b/protobuf/CHANGELOG.md index a376c83b..3a41a329 100644 --- a/protobuf/CHANGELOG.md +++ b/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 diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index 1c2c8ce4..eed80e2e 100644 --- a/protobuf/lib/src/protobuf/pb_list.dart +++ b/protobuf/lib/src/protobuf/pb_list.dart @@ -68,9 +68,13 @@ class PbList extends ListBase { void addAll(Iterable 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 extends ListBase { void insertAll(int index, Iterable 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 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 extends ListBase { @override void setRange(int start, int end, Iterable 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 extends ListBase { @override void replaceRange(int start, int end, Iterable 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 diff --git a/protoc_plugin/test/list_iterator_args_test.dart b/protoc_plugin/test/list_iterator_args_test.dart new file mode 100644 index 00000000..da54ab32 --- /dev/null +++ b/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"]); + }); +}