Don't iterate iterable arguments twice in PbList

[osa1]

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.

---

cl/823443311

[osa1]

@rakudrama I can't add you as a reviewer, could you also have a look?

[github-actions]

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:protobuf	5.1.0-wip	WIP (no publish necessary)
package:protoc_plugin	24.0.0	ready to publish	protoc_plugin-v24.0.0

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

[osa1]

With all the branching testing this becomes a quarter long engineering effort.. We need to test all combinations of:

(1) when the iterable is a: PbList, List, and other
(2) when _check is available and not
(3) for all possible error conditions (depends on the method)

Note that error conditions are already not tested today, I wouldn't be surprised if error checking is different based on the check function and argument type today.

I'm inclined to do the simplest possible thing here at the cost of performance. Use a List if you want fast setRange etc. and add your stuff to the message afterwards.

[osa1]

@sigurdm all internal downstream breakage has been fixed and this is ready for merging now. I think @rakudrama is away this week so he won't be reviewing it.

I didn't address the @rakudrama's comments (I did, but then reverted the changes). The reason is because there are just too many special cases if we want to do the most performant way possible in all possible ways of calling these methods, and all those special cases will need to be tested too. It's also very tricky to not have different exception behavior when e.g. the argument is a List vs. a non-list iterable. By calling the standard library methods as directly as possible (with the map) functions I avoid the special cases and make this easier to test, at the cost of performance.

We can improve performance of these methods as needed. Note that the performance does not change when the check function is not available, and I think even with the check function the impact may be negligible. For example, compare before and after:

// Before
void insertAll(int index, Iterable<E> iterable) {
  _checkModifiable('insertAll');
  if (_check != null) {
    iterable.forEach(_check);
  }
  _wrappedList.insertAll(index, iterable);
}

// After
void insertAll(int index, Iterable<E> iterable) {
_checkModifiable('insertAll');
if (_check != null) {
  _wrappedList.insertAll(
    index,
    iterable.map((E e) {
      _check(e);
      return e;
    })
  );
} else {
  _wrappedList.insertAll(index, iterable);
}

When the check function is not available they're the same. When it's available, the old version already iterates once with forEach, which calls the check function indirectly (as a closure). The new version does the same with map instead of forEach, but it doesn't do an insertAll separately after the iteration.

So I suspect the new code performance should not be too bad (maybe even better) than the old one because the new code iterates once, and the old code already iterated using forEach and called the check functions indirectly.