Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arrays/Dictionaries allow modifying contents during iteration #32261

Open
KoBeWi opened this issue Sep 22, 2019 · 7 comments
Open

Arrays/Dictionaries allow modifying contents during iteration #32261

KoBeWi opened this issue Sep 22, 2019 · 7 comments

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Sep 22, 2019

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
Copy link
Contributor

raymoo commented Sep 23, 2019

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
Copy link
Member Author

KoBeWi commented Sep 23, 2019

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

@isaacremnant
Copy link
Contributor

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
Copy link
Member Author

KoBeWi commented Sep 23, 2019

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
Copy link
Contributor

Zylann commented Sep 23, 2019

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
Copy link
Member Author

KoBeWi commented Aug 30, 2020

Still valid in db5ea78

@Error7Studios
Copy link

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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants