godotengine
GDScript: Fix typed array hash
Continuation of #69248.
Constants are compared by hash, so to store arrays of different types but with same contents they need to include type info into hash and equality considerations.
This was a part of the linked PR, but was removed after discussion because I forgot why I've added it...
@reduz, @vnen, are there any alternatives?
I've been thinking about this and I'm honestly not really sure. One one hand I understand why you have to do this, as it will otherwise share arrays that should not be shared, on the other I don't really like that an untyped array is different from a typed one (I don't mean two different types, those are fine to be different even if empty, I mean only untyped == typed). That is mainly because GDScript should still be usable untyped.
Imagine a scenario like this:
extends CodeEdit
func _ready():
# code_completion_prefixes is of type Array[String]
var arr = ["."]
code_completion_prefixes = arr # Works fine, which is expected.
print(arr == ["."]) # true
print(code_completion_prefixes == ["."]) # false
print(code_completion_prefixes == arr) # false
Essentially it assigns ["."] to a property but the property is not equal to ["."] after assigning. This might get some users not expecting it to require a type match when they're not using types.
Another example:
func find_path():
var astar_grid = AStarGrid2D.new()
astar_grid.size = Vector2i(4, 4)
astar_grid.update()
for i in 4: # Create impassable wall.
astar_grid.set_point_solid(Vector2i(2, i), true)
var path = astar_grid.get_id_path(Vector2i(0, 0), Vector2i(3, 2)) # Impossible path.
print(path) # Prints "[]" since no path is found.
if path == []:
print("Path not found")
else:
prints("Path found:", path)
In this case the path == [] will be false which is quite unexpected if the user isn't aware of the peculiarity of types. Even if they are, they might slip and commit a mistake like this and spend some time trying to figure out what went wrong.
So I agree the bug must be fixed, but I don't think this is the best way.
If we look at packed arrays, which also accept (and silently convert) arrays.
var strings := PackedStringArray()
var untyped = ["."]
strings = untyped
print(untyped == ["."])
print(strings == ["."]) # analyzer error
print(strings == untyped) # runtime error (because untyped is a weak variable, otherwise analyzer error)
And we can customize == behavior for arrays. Meaning that 5 == 5.0 is true, even though their hashes are different.
I just remembered this PR: #56338 which essentially avoids the cases of sharing by making the constant map in the compiler use a different way of comparing values. It is in my backlog but never got to fully assess it. Maybe it is a good solution that can fix GDScript without changing how equality comparison works.
So you want this fix not to apply to usual dictionary keys?
I don't think it is a problem for regular dictionary keys, it's probably even expected those are shared since the user may want to use a literal value as the key and those should match. I don't think they need to be differentiated by type.
Another thing to consider:
print(a == b) # true
x.prop = a # ok
x.prop = b # error
With a being empty Array[int] and b being empty Array[String] the code above will be possible if type info is not included in equality.
Is there any other type combination for a and b that will lead to same inconsistency?
As I said before, it's okay if empty Array[int] is different than empty Array[String]. The problem is when comparing typed with untyped:
var a: Array[int] = [1, 2, 3]
print(a == [1, 2, 3]) # Should be true
If including type info in the equality can still account for this case, then it think it is fine.
As I said before, it's okay if empty Array[int] is different than empty Array[String]. The problem is when comparing typed with untyped.
To be clear, in the snippet a can be a typed array and b can be untyped one.
And I do understand that this PR will be closed and a custom hasher/comparator specifically for constants will be made.
By the way, maybe the problem comes from the fact that currently an array literal implies different incompatible things in different contexts and this adds a requirement for equality between typed and untyped array.
Right now array literal has to behave like that otherwise working with typed arrays will be unbearable. But once typed array constructor syntax is implemented it is possible to remove fluidity of array literal to match strictness of the system and remove confusion and inconsistency.
If one could not assign an array literal to a typed array variable then, I assume, your requirement for equality between incompatible array types will go away as well.
So that's an option to consider.
I don't think we have a consensus about which behavior people do expect. There was an idea to have a public discussion/poll about this, but so fat it have not materialized.
@vonagam Moved the milestone to 4.2, then. I added the PR to the list of PRs to review as a team in the next weeks. I'm taking a note to do the survey.
Constants are compared by hash, so to store arrays of different types but with same contents they need to include type info into hash and equality considerations.
I confirm the issue:
const A: Array[int] = []
const B: Array = []
const C: Array[int] = [1]
const D: Array = [2]
func _run() -> void:
print(var_to_str(A)) # Array[int]([])
print(var_to_str(B)) # Array[int]([])
print(var_to_str(C)) # Array[int]([1])
print(var_to_str(D)) # [2]
I encountered this while working on #78837, but I thought it was a compiler/codegen bug.
