godotengine
GDScript: Fix typed arrays
This is complete rework how typed arrays do function. In terms of interactions now typed arrays act like packed arrays do.
No more implicit conversions between arrays on assigns.
var ints: Array[int] = [1]
var floats: Array[float] = ints # will not work
# same as you cannot do this:
var ints := PackedInt64Array()
var floats: PackedFloat64Array = ints
Array (without a parameter) for typed arrays is what Variant is for other types. It is possible to assign typed array to assignable (variable/parameter/constant) with type of basic array and it is possible to do the opposite, but later fails if types do not match.
var ints: Array[int] = [1]
var basic: Array = ints
ints = basic
var floats: Array[float] = [1.0]
basic = floats
ints = basic # runtime error because assigned is Array[float] and not Array[int]
basic = [2]
ints = basic # runtime error because assigned is Array and not Array[int]
Explicit inference no longer tries to guess what element type an array has, it will be typed as basic one.
var infer := [1, 2, 3] # type is Array, not Array[int]
Typed arrays in constants now work properly.
const ints: Array[int] = [1] # value now is really Array[int]
There is no implicit conversions between arrays, but there is for array literals. In all examples above literal on right side is a usual array literal, but it gets converted during analysis to be of proper type and fails if it is impossible to do. It applies to variable/parameters/constants initializers, assignments, returns, arguments and casts:
var ints: Array[int] = [1] # init
ints = [2] # assign
var floats := [3.0] as Array[float] # cast
var make_bools = func() -> Array[bool]: return [true] # return
func pass_strings(strings: Array[String]): pass # this is a member function, lambdas are untyped
pass_strings(['ok']) # arg
typed_assign now does only one thing - replaces contents of an array with contents from another with conversions if needed.
var ints: Array[int] = [1]
var floats: Array[float] = []
floats.typed_assign(ints)
Fixed resize not using resize_zeroed and not initializing new elements in typed arrays. This was leading to memory corruptions and crashes.
In VariantInterals::initialize add missing initailization for Projection and Color (alpha needs to be set at 1).
Added serialization/deserialization of typed arrays. Though it does not handle custom classes at the moment.
Fixed undo/redo for packed arrays, for some reason they were excluded from that before.
Changed editors for arrays/dictionaries a little - no need to do recursively duplicate them on each rerender, do it only when value is about to be modified.
Fixed default values for arrays/dictionaries not showing in editor.
Fixed GDScriptInstance::set not converting values when setters involved.
Related issues: Fixes #53847 (about parameters). Fixes #54643 (about parameters). Fixes #55916 (about constants). Fixes #56389 (about documentation). Fixes #58285 (about editor). Fixes #58636 (about parameters). Fixes #62753 (about editor). Fixes #63502 (about default values). Fixes #63570 (about resize). Fixes #67653 (about editor). Fixes #67889 (about literals). Fixes #68978 (about constants). Fixes #69198 (about constants). Fixes #69839 (about assigns). Fixes #70021 (about constants). Fixes #70235 (about resize). Fixes #71607 (about editor). Fixes #71653 (about parameters). Fixes #71756 (about enums). Fixes #71815 (about editor).
Rebased and fixed two more problems (one only partially) regarding constant arrays.
func _ready():
const x: Array[ int ] = [ 1 ]
var y := x[ 0 ] # before: error, after: inferring works
x.push_back( '1' )
print( x ) # before: [1, "1"], after: [1]
First, inference on constant arrays was not working because of misplaced line in subscript reduction that was setting result type to Variant even if type was known. In attribute subscript the line was fixed in #64046, but index was left as it was.
Second, partially fixes #68978, the problem was that reduced_value of constant arrays was an array on which set_typed was not called. Fixed only partially because while now it works for builtin types properly for custom scripts it seems not because script_type reference is not resolved yet...
Two more commits related to constants.
First makes all constants (local, member, outer, base) to use resolve_constant to resolve the type. Locals were already using it. Members were using copy-paste version of it. Outer/Base ones were only reducing an initializer, but it is insufficient since for constant parameterized arrays a declared type is important. (Also removed useless all_is_constant variable from const_fold_array/const_fold_dictionary.)
Second builds on it and clears up some current problems with deepness/flatness of constants. Now if you index constant by another constant a result is not a constant unless it is happening during constants resolution. Some examples of the problem is shown here - #62680. As I understand though there is a discussion about whenever or not make constants truly deep. Have no problem dropping that commit, but if constants will go as flat into version 4 then this at least makes it work properly.
Fixed another thing about arrays (not connected to literals or constants) - typed assigns. Right now if element types are somewhat compatible a destination array just references a source array losing its own type in a process.
class A: pass
class B extends A: pass
func _ready():
var b: Array[ B ] = [ B.new() ]
var a: Array[ A ] = b # before: a references b, becomes Array[ B ], after: a copies b contents
a.push_back( A.new() )
a.push_back( B.new() )
print( b.size() ) # before: 2, now: 1
print( a.size() ) # before: 2, now: 3
check( a ) # before: error, now: works
func check( a: Array[ A ] ) -> void: print( 'check' )
To make it correct array a should copy contents of array b and maintain its own type info.
By the way, initial commit fixes #69198 and #67889.
You need to squash the commits as required by our pipeline (https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#modifying-a-pull-request).
Oh, I was doing that initially, but then it grew and I assume became harder to review in a single commit. Also I assume that third commit might be dropped (about flatness/deepness of constants).
Fixed #63502, now default values should work properly with params of array type.
Also made array assign be used when typed array is assigned to non-typed one. In Array::_assign such case was handled by copying contents, but typed array assign was used only if a destination array had an element type and presence of element type in a source array was ignored (so before typed->untyped was just changing reference, now it copies contents).
Amended last commit, is it ok? Or should I still squash the commits into one?
Made changes to update_array_literal_element_type and is_type_compatible.
In update_array_literal_element_type made sure to guard against ending up with Variant in an element type. There was no such guards before but has_container_element_type implies that it is not Variant if present.
In is_type_compatible reflect that arrays with two different but compatible types are only compatible if implicit conversion is allowed. And same with an assignment of a typed to an untyped array.
Added a test.
After I made is_type_compatible to use exact equality for element types when no implicit conversion is allowed I've encountered bugs when a class type was not equal to the same class type. Turns out in is_type_compatible compassion was done by equating fqcns (fully-qualified class name) while == operator only checked equality of pointers to ClassNode, but it seems like the same class may have multiple ClassNode instances pointing to it (hence, I assume, why is_type_compatible used fqcn). So I added comparison of fqcn in == operator for classes types. Right now fqcn is a String, but is there anything preventing it from being NodePath?
Fixed another major thing about array assigns.
var first: Array[ A ] = []
var second: Array[ A ] = first
var value: Variant = A.new()
second = [ value ]
print( first.size() ) # before: 1, after: 0
print( second.size() ) # 1
Currently Array::_assign changes contents of an array it points to if there is a type mismatch instead of creating new array and properly copying contents. Here second starts referencing the same array as first as they have the same type, but during the assignment of the array literal which is Array[Variant] instead of trying to creating new Array[A] from what is given it simply replaces an underlying vector of the array it already has.
Rebased to resolve conflicts. Fixed one regression from #68747 and another preexisting problem that was revealed there.
The small regression is on this line: when changing the call from validate to validate_object Variant::NIL type was forgotten.
The thing revealed: there were additions of lines in GDScriptAnalyzer::is_type_compatible and Array::_assign to make exceptions for String/StringName pair. In fact those lines are not needed.
In _assign it showed that currently there is no check if arrays have types of convertible primitives. Meaning that before you could not assign Array[float] to Array[int] (where a conversion was allowed). There was no problem converting an array of floats to ints if an array of floats was typed as a simple array of variants, in that case each element would have been checked one by one if they are convertible to int, but no luck if you typed things properly. Now this case is handled.
In is_type_compatible nested call to resolve if element types of arrays are compatible passes false to p_allow_implicit_conversion when it should pass the current value down - if an implicit conversion allowed then arrays with convertible types should be ok, if not then not.
Squashed into one, removed commit about deepness/flatness of constants, added assertions about previous comment fixes.
Also made code structure of resolve_parameter match those of variables and constants (done to properly infer a param type when an array literal is a default value).
While doing this I noticed that resolve_variable, resolve_constant, resolve_parameter and code for resolving member variables and constants are the samish (it just deciding on a type based on an initializer and an annotated type). It looks like copy-paste with sometimes changed names and missing assertions here and there (I assume because with time people add changes here and there but don't know that the code is duplicated in other places). It looks doable to unify that under a single function (which I did here but only for member constants and local constants). But that just a note, will not do that in this PR.
Rebased to fix a conflict with #69992 which was fixing is_type_compatible like was written in the before last comment. Interestingly it also fixed an inconsistency between member and local constant resolutions but missed the fact that resolve_parameter has the same problem. This is an example for the last comment. Added missing true to is_type_compatible check in resolve_parameter.
Note on topic of arrays: currently plus operator returns an untyped array even if two arrays involved are typed and have the same type.
Fixed #63502
@vonagam Please add this and other similar notes to the PR's description, it will link such issue to be auto closed when this PR would be merged. See Linking a pull request to an issue for more details. And in general try to keep the PR description updated by editing it after doing some changes. Additional comments you're adding are of course fine but I'd say the summary in the PR description shouldn't be outdated.
@kleonc ok, updated the description. If there is anything that need to be added or clarified let me know.
This generally looks good! I like it, and it seems to fix a bunch of things!
I think the things I am concerned about, which @kleonc mentioned (namely that arrays should be referenced, not copied), might be concerns for another PR.
What I will say though, is that I would love to see a handful of error cases added to the unit tests, to show that the typechecking works properly and catches errors - and to make sure there are no more regressions in the future! :)
True about failure tests. I would have preferred if there was something of opposite of assert like fail in tests, which expects an error and if there is none - logs the message. Awkward to split positive and negative checks into different files, breaks the code flow and logic.
As for referenced/copied, I think it is an important matter, needs to be agreed on before proceeding. Have a concern about Array[String] being equal to Array[StringName] like recent PR wanted, otherwise should not be a problem to change the behavior.
Hi! Thanks for this PR, I will test locally.
I'd like to add a potential new case to fix (I don't know if this PR fixes this yet)
func test(items: Array[int]) -> void: pass
func _ready():
# Works
test([1])
# Doesn't work
test([1].map(func(x) -> int: x))
Edit: I've tested it with this branch, and it unfortunately doesn't fix it (but it gives a better error, from "should be Array but is Array" to "should be Array[int] but is Array".
This is the same as #58636, last example of this comment. map returns untyped array and there is no conversion at the moment when you pass arrays as arguments. So, unfortunately, it is not fixed by this PR.
Does this fix the following bug/issue?
extends Control
var _buttons: Array[Button]
func _ready() -> void:
# Error: Trying to assign a typed array with an array of different type.
_buttons = [$Button1, $Button2, $Button3]
While in some other cases this works:
extends Control
var _ints: Array[int]
func _ready() -> void:
_ints = [1, 2, 3]
This is probably because the type is guessed, although it is not explicitly specified.
print([1, 2, 3].get_typed_builtin()) # 0 (TYPE_NIL)
var a := [1, 2, 3]
print(a.get_typed_builtin()) # 2 (TYPE_INT) !!!
var b: Array = [1, 2, 3]
print(b.get_typed_builtin()) # 0 (TYPE_NIL)
# Should be the opposite, default is `Array` and `Array[int]` should be explicit.
I think we are missing typed array literals (like Array[int](1, 2, 3)) to explicitly specify the type without an extra variable. Implicit guessing is confusing and inconsistent.
Sorry for delay in reply, @dalexeev. Added a fix for the shown issue of yours.
Part of it was fixed before, but typed_assign in Array was still missing a case when you assign from an array of base classes (Node) to an array of extending classes (Button). (It is the same as the one where you assign variants to objects.)
Now it works. Also marked such assignments where a source array cannot be used without conversions/checks as type unsafe.
It seems to me this still needs discussion on what the proper behavior should be. Only then we can say whether the implementation is optimal.
There's also a change in Array that I'm not really convinced. Before it used COW (copy-on-write) which only made a copy if the array changed, but now it's always making a copy. This probably has a performance impact. It's also one thing to be discussed: when (or even if) an array should be copied instead of passed by reference.
I think that arrays should not be copied on usual assignment. As was said by @kleonc, arrays are reference types and when you receive an array then you can call push_back expecting original one to get element too. In my opinion the only question is convenient explicit api/syntax for converting arrays to other types (by copy).
var subclasses: Array[B] = []
func for_classes(classes: Array[A]): pass
What way for a user should there be to pass subclasses to for_classes (as copy) without too much of a hassle?
Another question is whenever or not inferring should change the type:
var untyped = [1]
var inferred := [2]
Should the inferred here be Array[int] or be just Array (of variants) like untyped? It can cause problems to have Array[int] if you plan to use it in places that expect just Array.
I think what @dalexeev said about syntax might solve both problems:
for_classes(Array[A](subclasses))
inferred_simple := [2]
inferred_int := Array[int]([2])
As for copy-on-write - that's sounds like a good idea. But I don't see any trace of it in master... Edit: Found it. It is two levels down in CowData. Quite easy to put its use back.
Another major problem with typed arrays is that they are not so much typed, meaning that all of their methods do return/accept same old variants. Array will not be the last parameterized thing in godot, so there is need to think about how to represent type parameters in info.
Small note (just a note): if braces {} were theoretically used for type parameters instead of brackets [] (or angle brackets <>) then it would have been much easier on parser, since there is no a{b} but there is a[b] (and a<b).
PR and its description are updated. Complete change of how things work.
It does not include Array[int](). Can be added later.
Added implicit conversion for array literals.
Copy-on-write is back and public method typed_assign has a reasonable purpose now.
Will add tests once confirmed that described behavior is ok with everybody.
Super exciting!!!! All of it checks out with what we discussed in the meeting, I think!
The only thing that I'd like to discuss a bit more is this one
var infer := [1, 2, 3] # type is Array, not Array[int]
Wouldn't it make sense to try to infer the type of the array elements here as well, and if they're not all of the exact same type, default to Variant array?
The only thing that I'd like to discuss a bit more is this one
var infer := [1, 2, 3] # type is Array, not Array[int]
My opinion is that it should just be Array (Array[Variant]) since GDScript is a dynamic language by default and Variant should be used everywhere unless explicitly stated otherwise. := asks us to guess the type, we guess it's an Array, but nothing more.
Otherwise, we will have problems such as:
print([1, 2, 3].get_typed_builtin()) # 0 (TYPE_NIL)
var a := [1, 2, 3]
print(a.get_typed_builtin()) # 2 (TYPE_INT) !!!
var b: Array = [1, 2, 3]
print(b.get_typed_builtin()) # 0 (TYPE_NIL)
# Should be the opposite, default is `Array` and `Array[int]` should be explicit.
and
var a = [1, 2, 3]
print(a.get_typed_builtin()) # 0 (TYPE_NIL)
var b := [1, 2, 3]
print(b.get_typed_builtin()) # 2 (TYPE_INT) !!!
Few things about it:
- By inferring typed array you remove ability to infer basic one (unless it is empty or complete mix).
-
Array[Class]andArray[SubClass]are not compatible like before, so it important to get type right. - Assigning a basic array to a variable with type of a typed array will give an error too.
- Classes may have multiple base classes, it is not exact which one to infer, inferring most specific is an arbitrary choice.
- It complicates codebase.
I do think that decision on whenever or not to make array typed and with which type should be made by a user since there will be consequences. Most people should be ok with a basic array. Have no problem with putting that logic back if that's the consensus though.
What I am more interested in and what we have not discussed is the implicit conversion of an array literal to typed one.
It can be perceived as somewhat confusing:
var good_int: Array[int] = [0] # this works
var basic = [0]
var bad_int: Array[int] = basic # this does not
The reasoning is that when a literal converted we are sure that there are no other references to that array (so nobody can write into it) so it is ok to convert it to typed one.
While some possible confusion is a minus, there is a big plus that you don't have to write Array[MyClassName]() anytime you want to create new array for assignment into a variable, passing as an argument or returning from a function. So it will make them much less frictionless to use.