12

I don't understand why the List<T>.ForEach() extension method implements a for loop under the hood. This opens up the possibility of the collection being modified. A normal foreach will throw an exception in this case so surely ForEach() should react the same way?

If you MUST mutate a collection for whatever reason, then surely you should be manually iterating through the collection in a for loop?

There seems to be a bit of a semantic contradiction here between foreach and List<T>.ForEach().

Am I missing something?

Dave New
  • 38,496
  • 59
  • 215
  • 394
  • 5
    "This opens up the possibility of the collection being modified." [Exactly why it is now gone from .NET for Metro-style apps.](http://stackoverflow.com/questions/10299458/is-the-listt-foreach-extension-method-gone/10299492#10299492) Ho hum. – BoltClock Jul 15 '12 at 15:56
  • 3
    Eric Lippert's (disapproving) [comments on `.ForEach`](http://blogs.msdn.com/b/ericlippert/archive/2009/05/18/foreach-vs-foreach.aspx) are always worth a read in this context. – Kirk Woll Jul 15 '12 at 16:10
  • by the way: http://stackoverflow.com/questions/10299458/is-the-listt-foreach-extension-method-gone – user287107 Jul 15 '12 at 16:23
  • @user287107: That's what I linked to not very long ago. – BoltClock Jul 15 '12 at 16:34
  • 1
    ... well thats what you get if you mutate data - sorry LINQ has deep FP roots and there is a reason why immutable data and referential transparency are *encouraged* in FP ... if you don't like the implementation then make your own - or better embrace immutable data! – Random Dev Nov 16 '12 at 06:42

3 Answers3

5

Because List.ForEach following the definition from MSDN:

Performs the specified action on each element of the List.

That means that Action executed over the element, can potentially change element, or collection itself. In this case, there is no other way (if not creating costy clone collection, if it's possible) to afford this, then using a simple for.

If you change the collection during the iteration in foreach, it, naturally, raises an exception.

Dave New
  • 38,496
  • 59
  • 215
  • 394
Tigran
  • 61,654
  • 8
  • 86
  • 123
  • 1
    Ah, by definition it is correct then. I still feel that it is slightly misleading. I guess that's the reason for all the controversy (and the reason why they are dropping it from .NET metro, as BoltClock pointed out)? – Dave New Jul 15 '12 at 16:06
  • 2
    @davenewza No, the linked documentation says _Modificare la raccolta sottostante nel corpo di `Action` il delegato non è supportato e non[?] causa un comportamento indefinito._ Or, if one prefers English for some reason: _Modifying the underlying collection in the body of the `Action` delegate is not supported and causes undefined behavior._ So they say you shouldn't modify the `List<>` in the `action` delegate. – Jeppe Stig Nielsen Nov 03 '12 at 19:16
5

foreach is a C# language element. It plays by the rules of C#.

List<T>.ForEach is a .NET Framework method. It plays by the rules of .NET, where foreach doesn't exist.

This is an example of "language vs framework" confusion. Framework methods must work in many languages, and the languages (usually) have contradictory semantics.

Another example of this "language vs framework" confusion is the breaking change to Enumerable.Cast between .net 3 and .NET 3.5. In .NET 3, Cast used C# semantics. In .net 3.5, it was changed to use .net semantics.

Dave New
  • 38,496
  • 59
  • 215
  • 394
Amy B
  • 108,202
  • 21
  • 135
  • 185
5

Only a member of the BCL team can tell us for sure, but it was probably just an oversight that List<T>.ForEach lets you modify the list.

First, David B's answer doesn't make sense to me. It's List<T>, not C#, that checks if you modify the list within a foreach loop and throws an InvalidOperationException if you do. It has nothing to do with the language you're using.

Second, there's this warning in the documentation:

Modifying the underlying collection in the body of the Action<T> delegate is not supported and causes undefined behavior.

I find it unlikely that the BCL team wanted such a simple method like ForEach to have undefined behavior.

Third, as of .NET 4.5, List<T>.ForEach will throw an InvalidOperationException if the delegate modifies the list. If a program depends on the old behavior, it will stop working when it's recompiled to target .NET 4.5. The fact that Microsoft is willing to accept this breaking change strongly suggests that the original behavior was unintended and should not be relied upon.

For reference, here's how List<T>.ForEach is implemented in .NET 4.0, straight from the reference source:

public void ForEach(Action<T> action) {
    if( action == null) {
        ThrowHelper.ThrowArgumentNullException(ExceptionArgument.match);
    }
    Contract.EndContractBlock();

    for(int i = 0 ; i < _size; i++) {
        action(_items[i]);
    }
}

And here's how it's been changed in .NET 4.5:

public void ForEach(Action<T> action) {
    if( action == null) {
        ThrowHelper.ThrowArgumentNullException(ExceptionArgument.match);
    }
    Contract.EndContractBlock();

    int version = _version;

    for(int i = 0 ; i < _size; i++) {
        if (version != _version && BinaryCompatibility.TargetsAtLeast_Desktop_V4_5) {
            break;
        }
        action(_items[i]);
    }

    if (version != _version && BinaryCompatibility.TargetsAtLeast_Desktop_V4_5)
        ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_EnumFailedVersion);
}
Michael Liu
  • 52,147
  • 13
  • 117
  • 150
  • @nawfal: Probably not. The cost of checking `if (version != _version)` is unlikely to be measurable. – Michael Liu May 30 '13 at 17:51
  • Actually, it's documented to throw the exception not only if the list is modified but even if "an element in the calling collection is modified". That's crazy. Why deliberately break a benign behavior? – Suncat2000 Jan 30 '20 at 16:30