-
-
Notifications
You must be signed in to change notification settings - Fork 20.3k
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
GDScript: Fix typed array hash #72514
base: master
Are you sure you want to change the base?
Conversation
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 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 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 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 |
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 Is there any other type combination for |
As I said before, it's okay if empty 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. |
To be clear, in the snippet 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. |
@vnen Is it ready to be merged in 4.1? |
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 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. |
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?