Arrays/Dictionaries allow modifying contents during iteration

[KoBeWi]

Godot version: 2e065d8

Issue description: While debugging my game, I bumped into this piece of code:

for id in shared_objects:
    if not is_instance_valid(shared_objects[id]):
        shared_objects.erase(id)

Godot doesn't see any problem here, even though doing this will totally break iteration and skip some elements. IMO modifying collection during iteration should raise an error, because it's never an intended behavior and yields unexpected results.

[raymoo]

I think this would be a GDScript-specific feature, for these reasons:

• It concerns a partivu==cular language feature not present in all languages
• This feature would require a check on every mutating operation so might add significant overhead in faster languages.

[KoBeWi]

C# already have this. At least for standard (non-Godot) collections.

[isaacremnant]

I do this but by looping backward so erasing won't mess up the indexing. What's the proper way to rewrite that piece of code if editing arrays in loops is forbidden?

Just raising a warning should be sufficient, no?

[KoBeWi]

What's the proper way to rewrite that piece of code if editing arrays in loops is forbidden?

I use this code:

var invalid_objects := []

for id in shared_objects:
    if not is_instance_valid(shared_objects[id]):
        invalid_objects.append(id)

for id in invalid_objects:
    shared_objects.erase(id)

[Zylann]

With arrays, it's easy to remove elements while iterating. It's just not as efficient to write in GDScript, because it requires a small tweak to the iterator:

    var i = 0
    while i < len(array):
        if not is_instance_valid(array[i]):
            array.remove(i)
        else:
            i += 1

Or, if order doesn't matter, this variant spares shifting every element (may matter for very large arrays, or if indexes matter cuz it's less housekeeping):

    var i = 0
    while i < len(array):
        if not is_instance_valid(array[i]):
            array[i] = array[-1]
            array.pop_back()
        else:
            i += 1

But when iterating dictionaries there is pretty much no workaround at that language level... unless you iterate a copy of the keys, or use a second array. It might be possible with maps based on a sorted tree but these don't exist in GDScript.

[KoBeWi]

Still valid in db5ea78b7

[Error7Studios]

It's also possible to create an infinite loop by adding elements to the array while iterating over it.

func method_A() -> void: # OK
    var my_array := [7]
    for i in my_array.size():
        var element = my_array[i]
        my_array.append(element + i + 1)
    print(my_array) # [7, 8]

func method_B() -> void: # Bug. Infinite loop
    var my_array := [7]
    var counter := 0
    for element in my_array: # only 1 element here; should only run once
        counter += 1
        my_array.append(element + 1)
        if counter == 3:
            break # infinite loop without this break
    print(my_array) # [7, 8, 9, 10]

func _ready() -> void:
    method_A() # [7, 8]
    method_B() # [7, 8, 9, 10]

[tobydjones]

So, it's 2024 and I just fell foul of this issue because this fails silently:

for id in players:
    if players[id]["gamecode"]==deletedgamecode:
        players.erase(id)

If it doesn't work as "expected" (that it hits all elements in the dictionary) then it should throw a run time error. Or even be caught by the editor.

[officialduke99]

Just ran into the same thing with this code

for callable in callables:
    if callable.is_valid():
        callable.call(event)
    else:
        callables.erase(callable)

I would like to mention, that a solution I haven't seen anyone mention would be to allow erasing and instead, leave the internal iteration counter unchanged when an element is erased. This would mean there wouldn't be sudden silent skips, which is unexpected behavior, and it would fit with GDScript's idea of an accessible dynamic language. However, if the performance cost of such checks is too large, I do agree that at least a warning should be raised.