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

Static typing for for loop variable errors on invalid references #101423

Open
Kylemcarthur opened this issue Jan 11, 2025 · 2 comments · May be fixed by #87296
Open

Static typing for for loop variable errors on invalid references #101423

Kylemcarthur opened this issue Jan 11, 2025 · 2 comments · May be fixed by #87296

Comments

@Kylemcarthur
Copy link

Kylemcarthur commented Jan 11, 2025

Tested versions

v4.3.stable.official [77dcf97]

System information

Godot v4.3.stable - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2060 (NVIDIA; 32.0.15.6636) - AMD Ryzen 5 7600X 6-Core Processor (12 Threads)

Issue description

v4.3.stable.official [77dcf97]

The recently added static typing for for loop variables here #80247 has a weird quirk that I feel like I may be considered a bug, or at least strange behavior.

If you are looping over an array that contains node references, and one of those nodes gets freed, <Freed Object> remains in its index. While normally you can use is_instance_valid() for validating your node references, trying to do this in a typed for loop will fail with the error "Trying to assign invalid previously freed instance" at the "for" line, before it even gets to where you can validate it. However, it's possible to get around this by removing the type from the loop variable and checking it after the instance is validated, but that then makes typing the loop variable questionable in the first place. Since I think that may be unclear, here's some example code of what I mean:

# Tested with an Array with some generic Node2Ds
var objects_to_be_freed: Array = get_tree().get_nodes_in_group("object_freeing")

# After any of those nodes is freed...

# Runs without error
for object in objects_to_be_freed:
	if is_instance_valid(object):
		print("Valid!")
	else:
		print("Invalid!")

# Will cause error: "Trying to assign invalid previously freed instance"
for object: Node2D in objects_to_be_freed: # Errors at this line
	if is_instance_valid(object): # I would expect this line to be how to catch the invalid reference before error
		print("Valid!")
	else:
		print("Invalid!")

# Runs without error and effectively keeps static typing as far as I can tell
for object in objects_to_be_freed:
	if is_instance_valid(object) and object is Node2D:
		print("Valid!")
	else:
		print("Invalid!")

This makes me wonder if this behavior is intended, and if it is intended, why? The 3rd block (the workaround) seems like it would be the best way to handle this in all cases, and if that's the case, then why isn't the first block just interpreted the same way by the engine?

I would expect the first block to work without error as written, simply printing "Invalid!" if the object was freed.

Steps to reproduce

Make an array, add node references to it, free any of the nodes without clearing the reference, create a for loop to iterate over that array, and make the for loop variable typed to those nodes. Tested this with the code snippet in the issue description.

Minimal reproduction project (MRP)

N/A

@dalexeev
Copy link
Member

@Kylemcarthur
Copy link
Author

Kylemcarthur commented Jan 11, 2025

I realized that I made a mistake in my third code block, as that actually does not provide autocomplete quite the same as I believed when both of these checks are on the same line:

if is_instance_valid(object) and object is Node2D:

So it would need to look something even more convoluted like this to more achieve the same desired purpose:

# Runs without error and effectively keeps static typing as far as I can tell
for object in objects_to_be_freed:
	if not is_instance_valid(object):
		print("Invalid!")
		continue
	if object is Node2D: # now you get static typing benefits like autocomplete
		print("Valid!")		

Which is getting really silly.

That said, I think the PR dalexeev mentioned seems like it would solve this issue regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: For team assessment
Development

Successfully merging a pull request may close this issue.

2 participants