godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.07k stars 20.19k forks source link

Array `map`/`filter` fail with typed-array variables and functions #72566

Open rblopes opened 1 year ago

rblopes commented 1 year ago

Godot version

v4.0.beta17.official [c40020513]

System information

KDE neon 22.04 (Ubuntu-derived)

Issue description

After beta.17, trying to assign the result of Array#map or Array#filter to typed-array variables, or returning their values as the result of typed-array functions fail.

The following sample code demonstrates the issue:

@tool
extends EditorScript

func test1() -> Array[int]:
    return range(5).map(func(n): return n) # → [0, 1, 2, 3, 4]

func test2() -> Array[int]:
    var result: Array[int] = range(10).filter(func(n): return n % 2 == 0) # → [0, 2, 4, 6, 8]
    return result

func _run() -> void:
    print(test1())
    print(test2())

Screenshot_20230202_013030

Casting the returned values doesn't work either.

Steps to reproduce

Assign the result of Array#map or Array#filter to a variable whose type is a qualified typed-array.

Minimal reproduction project

N/A

dalexeev commented 1 year ago
func test2() -> Array[int]:
  var result: Array[int] = range(10).filter(func(n): return n % 2 == 0) # → [0, 2, 4, 6, 8]
  return result

The problem is that Array methods return a generic Array (Array[Variant]). For example Array[T].filter() returns an Array, not an Array[T].

For now, you can use Array.assign() method:

func test() -> Array[int]:
    var result: Array[int]
    result.assign(range(10).filter(func(n): return n % 2 == 0))
    return result

In the future, if #71336 is accepted, you will be able to do:

func test() -> Array[int]:
    return Array[int](range(10).filter(func(n): return n % 2 == 0))

And, just in case, you can range(0, 10, 2) instead of range(10).filter(func(n): return n % 2 == 0). But I think you know this, and this is just an example. :)

rblopes commented 1 year ago

Ok, looking at it again, I've found this code snippet works as intended, no warnings or errors issued.

@tool
extends EditorScript

var some_var: Array[int] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

func test() -> Array[int]:
    return some_var.filter(func(n): return n >= 5)

func _run() -> void:
    print(test()) # → [5, 6, 7, 8, 9]

My guess is that the GDScript parser infers the filtered array type from some_var.

However, When I updated test to something like this:

func test() -> Array[int]:
    return some_var.filter(func(n): return n >= 5).map(func(n): return n * 2)

it no longer works.

(By the way, replacing the some_var init expression with range() crashes the editor when I run that script.)

kleonc commented 1 year ago

Array.filter preserves the typing. Even though it returns the generic Array, the internal type-data is copied to it from the original typed array. So the resulting generic array is still assignable to that specific typed array. Hence:

var some_var: Array[int] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
var result: Array[int] = some_var.filter(func(n): return n >= 5)

works fine. The problem with:

var result: Array[int] = range(10).filter(func(n): return n % 2 == 0)

is that range currently returns an untyped Array and hence the types mismatch. #67025 should fix this.


Regarding Array.map: it returns an untyped Array, it doesn't preserve the typing of the original array. That's intended, as elements can be mapped to any type.

To potentially make map return a typed array based on the return type of the passed Callable, the Callable would need to provide a way to obtain its return type in the first place (currently it's not supported).


(By the way, replacing the some_var init expression with range() crashes the editor when I run that script.)

Example with no range involved:

@tool
extends EditorScript

var a: Array = []
var b: Array[int] = a # (1) res://path.gd:5 - Trying to assign an array of type "Array" to a variable of type "Array[int]".

func _run() -> void: # (0) Execute in editor.
    print(b) # (2) <null>
    print(b.size()) # (3) Crash.

@vnen @vonagam Is this expected? :thinking:

vonagam commented 1 year ago
var a: Array = []
var b: Array[int] = a # error

Yes, a points to an untyped array. Cannot assign an untyped array to a typed array variable.

kleonc commented 1 year ago

@vonagam Thanks. And sorry, I should have been more precise as I wasn't asking specifically about the reason of the error, I understand that part. I was rather asking about the nature of the error / post-error behavior. Like e.g. if it should be a fatal error disallowing further execution (and thus potential crash because of the <null> value), or maybe if it should just error and assign an empty array instead?

For comparison when conversion of the elements fails it also errors but results in an empty array instead:

@tool
extends EditorScript

var integer = "0"
var array_of_integers = [integer]

var a: Array[int] = [integer]
var b: Array[int] = array_of_integers

func _run():
    print(a)
    print(b)

Output (v4.0.beta17.official [c40020513]):

  Unable to convert array index 0 from 'String' to 'int'.
  res://ES.gd:8 - Trying to assign an array of type "Array" to a variable of type "Array[int]".
[]
<null>

Again, I kinda understand why it currently gives such output. Just not sure whether it's truly intended. Shouldn't both cases result in unitialized value / empty array? :thinking:

dalexeev commented 1 year ago

Array.filter preserves the typing, it returns an array typed the same as the original array.

Yes, you're right, that's a good clarification. But the static type is not inferred, it's a generic Array, despite the fact that the array (object in memory) is typed at runtime.

Details And this is another reason why the exception was made for `Array` (`Array[Variant]`). ![](https://user-images.githubusercontent.com/47700418/216631793-946beb27-559f-43a6-8773-2296d826e99c.png) We could avoid this if `Array[T]` were a true parametric type (if return values and method parameters were of the static types `T` and `Array[T]` rather than `Variant` and `Array` wherever needed). And if we didn't worry about dynamic typing in GDScript.
vonagam commented 1 year ago

Shouldn't both cases result in unitialized value / empty array?

In my opinion both cases should result in a fatal error. It is the same as assigning String value to int variable, everything that will happen after such wrong deed is undefined. For example, if it was a function that deletes files and an array of strings was supposed to be whitelist, to prevent files from being deleted, one wouldn't want it to proceed with an empty array.

MikeSchulze commented 1 year ago

Hi I ran into the same problem today, is there an update here? It's really inconvenient to use an extra assignment instead of directly returning the result of a map() function. The array map function is not usable for typed arrays. This is a bug and should definitely be fixed. The map function should return the type of the mapping function used. Yes, the workaround works, but makes the code more complex than necessary, this negates the benefits of the map function again.

var values :Array[String] = ["_1", "_2", "_3"]

# fails at runtime
func convert_to_id1() -> Array[int]:
    return values.map(_to_id)

# works with the assign workaround
func convert_to_id2() -> Array[int]:
    var result :Array[int] = []
    result.assign(values.map(_to_id))
    return result

func _to_id(value :String) -> int:
    return value.replace("_", "").to_int()
portwatcher commented 8 months ago

The only workaround for me is to create a new typed array manually:

    var recipes = craft_recipes.filter(
        func(recipe: RecipeItem):
                        # do something
            return true
    ).map(
        func(recipe: RecipeItem) -> RecipeItem:
            # do something
            return recipe
    )

    var ret_recipes: Array[RecipeItem]

    # a godot bug:
    # https://github.com/godotengine/godot/issues/72566
    # https://github.com/godotengine/godot/pull/71336
    for recipe in recipes:
        ret_recipes.push_back(recipe)

    return ret_recipes
limbonaut commented 8 months ago

@portwatcher You can use Array.assign(), which should be faster.

portwatcher commented 8 months ago

@portwatcher You can use Array.assign(), which should be faster.

Yeah but I only have like 20 recipes and Array.assign is not visibly straightforward to me

AThousandShips commented 8 months ago

How do you mean? It's just:

    var recipes = craft_recipes.filter(
        func(recipe: RecipeItem):
                        # do something
            return true
    ).map(
        func(recipe: RecipeItem) -> RecipeItem:
            # do something
            return recipe
    )

    var ret_recipes: Array[RecipeItem]

    ret_recipes.assign(recipes)

    return ret_recipes
portwatcher commented 8 months ago

How do you mean? It's just:

  var recipes = craft_recipes.filter(
      func(recipe: RecipeItem):
                        # do something
          return true
  ).map(
      func(recipe: RecipeItem) -> RecipeItem:
          # do something
          return recipe
  )

  var ret_recipes: Array[RecipeItem]

  ret_recipes.assign(recipes)

  return ret_recipes

Yes you are right. I thought Array.assign() was something taking Callable as parameter. I've changed my code in my game with your advice.

Oblepikha commented 5 months ago

If you desperately need a one-liner there is a workaround

func do(array: Array[ClassA]) -> Array[ClassB]:
    #var result: Array[ClassB] = []
    #result.assign(array.map(func(element: ClassA) -> ClassB: return ClassB.new(element)))
    #return result
    return Array(array.map(func(element: ClassA) -> ClassB: return ClassB.new(element)), TYPE_OBJECT, "RefCounted", ClassB)
badunius commented 3 months ago

``

How do you mean? It's just:

  var ret_recipes: Array[RecipeItem]

  ret_recipes.assign(recipes)

  return ret_recipes

why this even here? when map is called with a typed Callable it supposed to return an Array[typeof ReturnType<Callable>] actually it should work like this all the time, since ReturnType of untyped Callable is Variant then we're going to have an experience consistent with what we have now and actually respect typing

for now it's easier to just drop typing in such cases

codevogel commented 6 days ago

At the very least, it would make sense that if you add a type-hint to the func used in the map function, it would be able to infer the type of the array.

Say we have this:

func to_path() -> Array[Vector2]:
    return _tiles.map(func(tile) -> Vector2: return tile.world_position)

Then the only plausible type for the resulting array is Vector2, no?

Yet when you do this it throws

Trying to return an array of type "Array" where expected return type is "Array[Vector2]".