From b9b1980a79a3a136997e2c232af1aef85f7c3995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Fri, 24 Oct 2025 10:24:25 +0100 Subject: [PATCH 1/9] Don't iterate iterable arguments twice in PbList --- protobuf/lib/src/protobuf/pb_list.dart | 60 +++++-- .../test/list_iterator_args_test.dart | 170 ++++++++++++++++++ 2 files changed, 217 insertions(+), 13 deletions(-) create mode 100644 protoc_plugin/test/list_iterator_args_test.dart diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index 1c2c8ce4..8addf69f 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,21 @@ 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, + // Note: In case `take()` returns less than `end - start` elements, the + // `_wrappedList` will fail with a `StateError`. + iterable.skip(skipCount).take(end - start).map((E e) { + _check(e); + return e; + }), + 0, + ); + } else { + _wrappedList.setRange(start, end, iterable, skipCount); } - _wrappedList.setRange(start, end, iterable, skipCount); } @override @@ -181,11 +208,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..6ada50ad --- /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(1 << 40); + }, 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"]); + }); +} From 65da48c177d0f278fcc5ba268a5cc2f0f5dbd060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Fri, 24 Oct 2025 10:43:53 +0100 Subject: [PATCH 2/9] Update a check --- protoc_plugin/test/list_iterator_args_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protoc_plugin/test/list_iterator_args_test.dart b/protoc_plugin/test/list_iterator_args_test.dart index 6ada50ad..da54ab32 100644 --- a/protoc_plugin/test/list_iterator_args_test.dart +++ b/protoc_plugin/test/list_iterator_args_test.dart @@ -11,7 +11,7 @@ void main() { // `PbList`, and half of the tests below can be removed. test("PbList checks value validity", () { expect(() { - TestAllTypes().repeatedInt32.add(1 << 40); + TestAllTypes().repeatedInt32.add(2147483647 + 1); }, throwsArgumentError); }); From 6f025bcc3ebcf9d6aebbff418d5d6bf6049d77d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Fri, 24 Oct 2025 10:48:04 +0100 Subject: [PATCH 3/9] Update CHANGELOG --- protobuf/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) 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 From 971d59f629924fb8e71e973d675a6091133382ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Fri, 24 Oct 2025 10:53:19 +0100 Subject: [PATCH 4/9] Simplify --- protobuf/lib/src/protobuf/pb_list.dart | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index 8addf69f..eed80e2e 100644 --- a/protobuf/lib/src/protobuf/pb_list.dart +++ b/protobuf/lib/src/protobuf/pb_list.dart @@ -177,9 +177,7 @@ class PbList extends ListBase { _wrappedList.setRange( start, end, - // Note: In case `take()` returns less than `end - start` elements, the - // `_wrappedList` will fail with a `StateError`. - iterable.skip(skipCount).take(end - start).map((E e) { + iterable.skip(skipCount).map((E e) { _check(e); return e; }), From 3e188eb01941f466a4ab5aca75a75393ffda2b4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Thu, 30 Oct 2025 10:01:47 +0000 Subject: [PATCH 5/9] Update insertAll --- protobuf/lib/src/protobuf/pb_list.dart | 29 ++++++++++++++++++-------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index eed80e2e..0211d65c 100644 --- a/protobuf/lib/src/protobuf/pb_list.dart +++ b/protobuf/lib/src/protobuf/pb_list.dart @@ -111,17 +111,28 @@ class PbList extends ListBase { @override void insertAll(int index, Iterable iterable) { _checkModifiable('insertAll'); - if (_check != null) { - _wrappedList.insertAll( - index, - iterable.map((E e) { - _check(e); - return e; - }), - ); + + // The standard library will convert the iterable to list to be able to find + // the number of elements added and shift the elements the right amount, so + // it's not extra work to convert it here. + final List iterableList; + if (iterable is List) { + if (iterable is PbList) { + iterableList = iterable._wrappedList; + } else { + iterableList = iterable; + } } else { - _wrappedList.insertAll(index, iterable); + iterableList = List.of(iterable); + } + + if (_check != null) { + for (E e in iterableList) { + _check(e); + } } + + _wrappedList.insertAll(index, iterableList); } @override From 9cf9c31ff4fb385c5627e1eaec8fe8c8a62c6d77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Thu, 30 Oct 2025 10:19:22 +0000 Subject: [PATCH 6/9] Update setALl --- protobuf/lib/src/protobuf/pb_list.dart | 54 +++++++++++++++++++++----- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index 0211d65c..fb0ae946 100644 --- a/protobuf/lib/src/protobuf/pb_list.dart +++ b/protobuf/lib/src/protobuf/pb_list.dart @@ -138,17 +138,53 @@ class PbList extends ListBase { @override void setAll(int index, Iterable iterable) { _checkModifiable('setAll'); - if (_check != null) { - _wrappedList.setAll( - index, - iterable.map((E e) { - _check(e); - return e; - }), - ); + + // Unlike `insertAll`, the standard library won't be converting the iterable + // to a list as `setAll` doesn't shift elements at inserted locations. + // + // However, when the iterable is already a list we want to avoid converting + // it to a non-list as the standard library can do a `memmove` when the + // iterable is a list. + // + // So when the iterable is a list we check the elements in a separate pass + // and then pass the list to the standard library `setAll`. + // + // To have the same exception behavior when checking the elements and the + // iterable is not a list, we also need to check elements of the iterable in + // a separate pass (without modifying the wrapped list). So we convert the + // non-list iterables to list first, check the elements, then pass to the + // standard library. + final List iterableList; + if (iterable is List) { + if (iterable is PbList) { + if (_check != null) { + for (E e in iterable._wrappedList) { + _check(e); + } + } + iterableList = iterable._wrappedList; + } else { + if (_check != null) { + for (E e in iterable) { + _check(e); + } + } + iterableList = iterable; + } } else { - _wrappedList.setAll(index, iterable); + if (_check != null) { + // Iterate and check one element at a time, to be consistent with the + // previous version of this function. + iterableList = []; + for (E e in iterable) { + _check(e); + iterableList.add(e); + } + } else { + iterableList = List.of(iterable); + } } + _wrappedList.setAll(index, iterableList); } @override From 296f52c96e86616a46b90cc2f84f559144e2cd3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Thu, 30 Oct 2025 10:32:16 +0000 Subject: [PATCH 7/9] Update addAll --- protobuf/lib/src/protobuf/pb_list.dart | 35 ++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index fb0ae946..79e57280 100644 --- a/protobuf/lib/src/protobuf/pb_list.dart +++ b/protobuf/lib/src/protobuf/pb_list.dart @@ -67,13 +67,38 @@ class PbList extends ListBase { @pragma('dart2js:never-inline') void addAll(Iterable iterable) { _checkModifiable('addAll'); - if (_check != null) { - for (final e in iterable) { - _check(e); - _addUnchecked(e); + // Defer the adding to the standard library `addAll` when possible as + // standard library can do it faster with low level operations. + if (iterable is List) { + if (iterable is PbList) { + if (_check != null) { + for (final e in iterable._wrappedList) { + _check(e); + } + } + _wrappedList.addAll(iterable._wrappedList); + } else { + if (_check != null) { + for (final e in iterable) { + _check(e); + } + } + _wrappedList.addAll(iterable); } } else { - _wrappedList.addAll(iterable); + if (_check != null) { + // To have the consistent exception behavior when the iterable is a list + // and not a list whiel also calling the standard library `addAll`, we + // have to collect the elements into a list here. + final iterableList = []; + for (E e in iterable) { + _check(e); + iterableList.add(e); + } + _wrappedList.addAll(iterableList); + } else { + _wrappedList.addAll(iterable); + } } } From 74a928f288b9cb6923b65ca6380b708e25686bf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Thu, 30 Oct 2025 10:38:13 +0000 Subject: [PATCH 8/9] Update replaceRange --- protobuf/lib/src/protobuf/pb_list.dart | 29 +++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index 79e57280..189639a9 100644 --- a/protobuf/lib/src/protobuf/pb_list.dart +++ b/protobuf/lib/src/protobuf/pb_list.dart @@ -278,18 +278,27 @@ class PbList extends ListBase { @override void replaceRange(int start, int end, Iterable newContents) { _checkModifiable('replaceRange'); - if (_check != null) { - _wrappedList.replaceRange( - start, - end, - newContents.map((E e) { - _check(e); - return e; - }), - ); + + // Similar to `insertAll`, the standard library will convert the iterable to + // a list anyway. Do it here to be able to check efficiently. + final List newContentsList; + if (newContents is List) { + if (newContents is PbList) { + newContentsList = newContents._wrappedList; + } else { + newContentsList = newContents; + } } else { - _wrappedList.replaceRange(start, end, newContents); + newContentsList = List.of(newContents); + } + + if (_check != null) { + for (E e in newContentsList) { + _check(e); + } } + + _wrappedList.replaceRange(start, end, newContentsList); } @override From 28da76305c39f4a6ab0c4e8c72849d4540a6fecd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96mer=20A=C4=9Facan?= Date: Fri, 31 Oct 2025 08:47:47 +0000 Subject: [PATCH 9/9] Reset to 971d59f --- protobuf/lib/src/protobuf/pb_list.dart | 147 ++++++------------------- 1 file changed, 33 insertions(+), 114 deletions(-) diff --git a/protobuf/lib/src/protobuf/pb_list.dart b/protobuf/lib/src/protobuf/pb_list.dart index 189639a9..eed80e2e 100644 --- a/protobuf/lib/src/protobuf/pb_list.dart +++ b/protobuf/lib/src/protobuf/pb_list.dart @@ -67,38 +67,13 @@ class PbList extends ListBase { @pragma('dart2js:never-inline') void addAll(Iterable iterable) { _checkModifiable('addAll'); - // Defer the adding to the standard library `addAll` when possible as - // standard library can do it faster with low level operations. - if (iterable is List) { - if (iterable is PbList) { - if (_check != null) { - for (final e in iterable._wrappedList) { - _check(e); - } - } - _wrappedList.addAll(iterable._wrappedList); - } else { - if (_check != null) { - for (final e in iterable) { - _check(e); - } - } - _wrappedList.addAll(iterable); + if (_check != null) { + for (final e in iterable) { + _check(e); + _addUnchecked(e); } } else { - if (_check != null) { - // To have the consistent exception behavior when the iterable is a list - // and not a list whiel also calling the standard library `addAll`, we - // have to collect the elements into a list here. - final iterableList = []; - for (E e in iterable) { - _check(e); - iterableList.add(e); - } - _wrappedList.addAll(iterableList); - } else { - _wrappedList.addAll(iterable); - } + _wrappedList.addAll(iterable); } } @@ -136,80 +111,33 @@ class PbList extends ListBase { @override void insertAll(int index, Iterable iterable) { _checkModifiable('insertAll'); - - // The standard library will convert the iterable to list to be able to find - // the number of elements added and shift the elements the right amount, so - // it's not extra work to convert it here. - final List iterableList; - if (iterable is List) { - if (iterable is PbList) { - iterableList = iterable._wrappedList; - } else { - iterableList = iterable; - } - } else { - iterableList = List.of(iterable); - } - if (_check != null) { - for (E e in iterableList) { - _check(e); - } + _wrappedList.insertAll( + index, + iterable.map((E e) { + _check(e); + return e; + }), + ); + } else { + _wrappedList.insertAll(index, iterable); } - - _wrappedList.insertAll(index, iterableList); } @override void setAll(int index, Iterable iterable) { _checkModifiable('setAll'); - - // Unlike `insertAll`, the standard library won't be converting the iterable - // to a list as `setAll` doesn't shift elements at inserted locations. - // - // However, when the iterable is already a list we want to avoid converting - // it to a non-list as the standard library can do a `memmove` when the - // iterable is a list. - // - // So when the iterable is a list we check the elements in a separate pass - // and then pass the list to the standard library `setAll`. - // - // To have the same exception behavior when checking the elements and the - // iterable is not a list, we also need to check elements of the iterable in - // a separate pass (without modifying the wrapped list). So we convert the - // non-list iterables to list first, check the elements, then pass to the - // standard library. - final List iterableList; - if (iterable is List) { - if (iterable is PbList) { - if (_check != null) { - for (E e in iterable._wrappedList) { - _check(e); - } - } - iterableList = iterable._wrappedList; - } else { - if (_check != null) { - for (E e in iterable) { - _check(e); - } - } - iterableList = iterable; - } - } else { - if (_check != null) { - // Iterate and check one element at a time, to be consistent with the - // previous version of this function. - iterableList = []; - for (E e in iterable) { + if (_check != null) { + _wrappedList.setAll( + index, + iterable.map((E e) { _check(e); - iterableList.add(e); - } - } else { - iterableList = List.of(iterable); - } + return e; + }), + ); + } else { + _wrappedList.setAll(index, iterable); } - _wrappedList.setAll(index, iterableList); } @override @@ -278,27 +206,18 @@ class PbList extends ListBase { @override void replaceRange(int start, int end, Iterable newContents) { _checkModifiable('replaceRange'); - - // Similar to `insertAll`, the standard library will convert the iterable to - // a list anyway. Do it here to be able to check efficiently. - final List newContentsList; - if (newContents is List) { - if (newContents is PbList) { - newContentsList = newContents._wrappedList; - } else { - newContentsList = newContents; - } - } else { - newContentsList = List.of(newContents); - } - if (_check != null) { - for (E e in newContentsList) { - _check(e); - } + _wrappedList.replaceRange( + start, + end, + newContents.map((E e) { + _check(e); + return e; + }), + ); + } else { + _wrappedList.replaceRange(start, end, newContents); } - - _wrappedList.replaceRange(start, end, newContentsList); } @override