Update Map documentation to note that modifying a map while iterating is illegal

[ronawho]

Removing from or adding to a Map (and set and probably other types) while iterating over it is illegal. The following example is invalid, but will generally work since we won't end up resizing the underlying data structure for this particular example:

use Map;

var m: map(string, int);
m.add("one", 1); m.add("two", 2);

for k in m {
  if k == "one" { m.remove(k); }
  if k == "two" { m.add("three", 3); }
}

writeln(m);

I think we need to make the map (and friends) documentation more clear that modifications while iterating are invalid. I also wonder if we can add a check for this if bounds checking is enabled. Something like the following (incomplete) checking seems to work:

diff --git a/modules/standard/Map.chpl b/modules/standard/Map.chpl
index 536fda7e08..ce197df2c5 100644
--- a/modules/standard/Map.chpl
+++ b/modules/standard/Map.chpl
@@ -76,6 +76,8 @@ module Map {
     pragma "no doc"
     var _lock$ = if parSafe then new _LockWrapper() else none;

+    var iterating: if boundsChecking then bool else none;
+
     pragma "no doc"
     inline proc _enter() {
       if parSafe then
@@ -448,9 +450,11 @@ module Map {
       :yields: A reference to one of the keys contained in this map.
     */
     iter these() const ref {
+      if boundsChecking then iterating = true;
       for key in this.keys() {
         yield key;
       }
+      if boundsChecking then iterating = false;
     }

     /*
@@ -558,6 +562,7 @@ module Map {
     */
     proc add(in k: keyType, in v: valType): bool lifetime this < v {
       _enter(); defer _leave();
+      if boundsChecking && iterating then halt("Can't modify map while iterating");
       var (found, slot) = table.findAvailableSlot(k);
       if found {
         return false;
@@ -615,6 +620,7 @@ module Map {
     */
     proc remove(k: keyType): bool {
       _enter(); defer _leave();
+      if boundsChecking && iterating then halt("Can't modify map while iterating");
       var (found, slot) = table.findFullSlot(k);
       if !found {
         return false;

[ronawho]

See Bears-R-Us/arkouda#518 (comment) for additional context/motivation

[ronawho]

FYI @daviditen and @dlongnecke-cray

[dlongnecke-cray]

I'm not all present yet. I thought his problem was with the new parSafe=true indexing semantics...

I think that the reason why we don't bother documenting this on collection iterators is because the whole "don't modify collections while iterating over them" is supposed to be a language level rule AFAIK?

I think just updating the documentation should be a good start for now.

[ronawho]

And just FYI we have run into this in our own module code a fair number of times:

• Don't remove from map while iterating over it #15957
• Fix set &= operator #15962
• Fix associative domain ^= to avoid removing while iterating #15966