core icon indicating copy to clipboard operation
core copied to clipboard

Behavior of data structures (like Map/Set/etc) when remove() is called during iteration

Open jayphelps opened this issue 8 months ago • 6 comments

Map, Set, etc. If you add or remove an item while you're actively iterating on it, what is the expected behavior?

For example, this code will print 1 then end, which seems wrong to me?

fn main {
  let values = Set::from_array([1, 2, 3])
  for value in values {
    values.remove(value)
    println("\{value}")
  }
}

My use case is some relatively complex code that indeed in fact needs to use a Set that has both insertions and deletions mid-iteration, potentially even the same item being added back, something most Set's I've used allowed, even during iteration; though I know doing that is hard for folks to debug/grok.

jayphelps avatar May 11 '25 21:05 jayphelps

You can use immutable collection(s), for mutable collection(s), we have no guarantee on modify self while iterating.

fn main {
  let mut values = @immut/sorted_set.from_array([1, 2, 3])
  for value in values {
    values = values.remove(value)
    println("\{value}")
  }
}

hackwaly avatar May 12 '25 02:05 hackwaly

While I disagree with that choice (and of course that's OK) if it's intentional that you don't support that it should probably error when you do that, otherwise things silently behave strangely and people might not notice their code is buggy.

jayphelps avatar May 12 '25 12:05 jayphelps

/cc @peter-jerry-ye

hackwaly avatar May 13 '25 15:05 hackwaly

This would mean that all the APIs that make modifications would have to throw some kind of ConcurrentModificationException, which would pollute the interface under the current error system as we don't have the RuntimeException concept.

I think the answer would be : it's undefined behavior and use with caution.

Reference : https://go.dev/ref/spec#For_range

The iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next. If a map entry that has not yet been reached is removed during iteration, the corresponding iteration value will not be produced. If a map entry is created during iteration, that entry may be produced during the iteration or may be skipped. The choice may vary for each entry created and from one iteration to the next. If the map is nil, the number of iterations is 0.

cc @bobzhang

peter-jerry-ye avatar May 13 '25 15:05 peter-jerry-ye

This would mean that all the APIs that make modifications would have to throw some kind of ConcurrentModificationException, which would pollute the interface under the current error system as we don't have the RuntimeException concept.

Ah, a Java guy I see! Yeah, I meant more like using an abort(), like is used in some other misuse cases in core. To me, I'd MUCH rather debug a random trap than my code randomly and silently skipping items in the set, as it took me a while to figure out that was happening and even then, I suspected that was the case, something inexperienced folks might not, especially if they didn't realize their code was mutating the collection mid-iteration.

Ultimately it's of course your team's call, just sharing my perspective.

jayphelps avatar May 13 '25 16:05 jayphelps

Add runtime check will result performance penalty. Maybe we should provide an eslint rule like thing.

For rust: it's automatically solved by borrow check.

hackwaly avatar May 14 '25 09:05 hackwaly