godotengine / godot

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

Gdscript - should generate an error when adding (append) an element of a different type that does not match a typed array - No type validation errors will be generated, and the array will not have the element added #86851

Open LeandroHPerez opened 11 months ago

LeandroHPerez commented 11 months ago

Tested versions

Godot 4.2.1 without .net

System information

Macbook Apple M2 Pro - MacOs Sonoma 14.2.1

Issue description

Create a typed array and add an element of different type to it. No type validation errors will be generated, and the array will not have the element added

Steps to reproduce

Resume: call this simple Gdscript function attached and see what happens when trying append an value that doesn't match with array type

Steps: 1- call attached script function test_typed_array_data01() inside Godot 4.2.1 without .net on any node, inside func ready(), just call it. 2- See lines where .append()_ is called passing an arg of type String and not one that combine/match with type of array 3- See stacktrace errors and when it happens 4- Comment lines like print 02 in the editor, save 5- reexecute steps 1,2,3

Expected result:

When i call an line like print(str(array1[0])) this array has zero size where i expect that had an value...

Evidences: 1) image

2) image

Minimal reproduction project (MRP)

Just call this GDscript function:

func test_typed_array_data01():

var array1: Array[PackedVector2Array]
var array2: Array[PackedInt32Array]

print("Yes, array1 is appending an String!")
array1.append("sad string")

print(array1)

print("Theres nothing to print")
print(str(array1[0]))

print("Yes, array2 is appending an String!")

array2.append("evil string")

print(array2)
print("Theres nothing to print")
print(str(array2[0]))
AThousandShips commented 11 months ago

No type validation errors will be generated

You get an error though:

E 0:00:00:0832   test.gd:15 @ _ready(): Attempted to push_back a variable of type 'String' into a TypedArray of type 'PackedInt32Array'.
  <C++ Error>    Method/function failed. Returning: false
  <C++ Source>   ./core/variant/container_type_validate.h:97 @ validate()
  <Stack Trace>  test.gd:15 @ _ready()

Not a parse error here, this is a runtime check

LeandroHPerez commented 11 months ago

Hi @AThousandShips, The error does not happen to me at runtime at the correct time, but rather later on. For me, the error at run time should already occur at append() and it would not even be possible to reach and execute the lines:

print(array1) print("There is nothing to print")

In fact, append() "works", but it DOES NOT add the element and does not generate an error. Here the problem at runtime occurs because I tried to obtain an element that should exist, but was not inserted, that is, it happened very late, it should happen in append() and not when retrieving the element from the Array.

For me the append should have already given the error.

An improvement would be if the editor itself at design time already issued an alert such as those lines that have a red background, even so, the error at runtime for me is right at append() with an "instant crash". I'm adding the Minimum Project and I'm sorry for not doing it sooner.

I believe that if it continues as it is, it will be a point of friction for hobbyist programming users.

Therefore, I thank you for your attention and if you still consider a point that does not need improvement, just close the bug. Thanks :)

Prints of Minimum Project image image

image Minimum project here: array_error.zip

AThousandShips commented 11 months ago

Look in the debugger, the error is there

Bromeon commented 11 months ago

Is there a reason why this does not show in the Godot output console like all the other user errors? This is indeed unintuitive, and I was also under the impression that it failed silently.

AThousandShips commented 11 months ago

It's a runtime error, they generally are in the debugger, by design I believe, unsure

LeandroHPerez commented 11 months ago

For me it's a big problem because it can hide bugs in the gdscript code. For example: I add (append) an invalid element/incompatible type (it will not be added), after a moment, I add a VALID element, finally I "get" / access the array[zero_position]. What will happen? I thought there were 2 elements and I took the first one, in practice, there was only 1. Imagine the number of bugs in the logic that could result from this problem.

We must remember that not every GDscript user is a professional programmer/software engineer who understands the smallest details. For me GDscript is supposed to be an easy and friendly high-level language, very high-level, I would say.

Lippanon commented 11 months ago

@LeandroHPerez You didn't actually add a MRP, just some text that says "array_error.zip".

The error does not happen to me at runtime at the correct time, but rather later on. For me, the error at run time should already occur at append()

It does give you an error though, you're not meant to ignore the red Debugger text that indicates you have errors, they are not hidden, so I can't agree that it hides bugs in code.

I will say that you are using a typed Array of PackedVector2Array in your screenshots, this is unusual if you're a begginer. Are you sure you intend to use that instead of simply PackedVector2Array?

If you use PackedVector2Array you do get a parser error: image

Typed arrays seem to give only a runtime error, they are special types and I'm not sure how feasible it is to change this: image

But an error is an error, you're not meant to ignore these. After running the game, I think the user is expected to notice the red "Debugger" text and have a look.

AThousandShips commented 11 months ago

This wouldn't really be easy to convert into a different type of error

This is an error that happens in Array and that method doesn't return Error, changing that would be breaking compatibility, and that would be the only way for this to trigger a breakpoint in the editor, the debugger only prints errors, it can't react to them in that way, at least not the printed ones like these

I'd say that it's not feasible to change this, and it's instead something you need to pay attention to, it isn't hidden, it shows up as a red marker in the bottom bar, as your screenshot clearly displays 🙂

LeandroHPerez commented 11 months ago

@Lippanon Ops, relayy sorry for this: "You didn't actually add a MRP, just some text that says "array_error.zip". I'm used to copying and pasting files into fields... usually the upload works fine. array_error.zip

LeandroHPerez commented 11 months ago

@Lippanon "will say that you are using a typed Array of PackedVector2Array in your screenshots, this is unusual if you're a begginer. Are you sure you intend to use that instead of simply PackedVector2Array?""

Yes, I intend to. I need to create navigation mesh / NavigationPolygon dynamically / procedurally and for this I need a list containing polygons / vertices and also a list of which indices need to be connected.

I will fill it with a huge list of points and indices representing many polygons

Like this example in documentation, but with thousands of points and indices: image

Therefore, I was forced to use these "exotic data types".

In fact, now I created a custom class and I will have a typed array of it "var array_with_tile_representation: Array[TileRepresentation] where is my custom class

It's hard to know what a user will do...

In the sense of beginner users, it's more because I care a lot about them, some points that may seem obvious to very experienced devs, who deal with ultra complex projects like Godot, are not obvious to most users of the tool, believe me. Anything that can be made simpler is always welcome.

In the case of game development, I'm a complete beginner. But I'm reading the documentation and learning :)

dalexeev commented 11 months ago

There are no static checks because generic types are not fully implemented. Type inference is only implemented for the index operation (array[i]) and iteration using for loop. "Generic" method parameters and return values use Variant, not the element type. See #59721 for details.

As for runtime checks, they are do not cause the debugger to stop (like many other code executed by the core, not GDScript), see the Errors tab instead of Stack Trace. I'm not sure about this, but I think the reason for not using the debugger is performance.

LBdN commented 3 months ago

This bit me today. I wrote the wrong type for the parameters of a function, and the array wasn't filled because of that as the code in that function was appending the wrong type to that array.

in my mind, a type error should stop the game from running. That's the whole point of giving type information about data : documenting expectations about types and trust that the types are enforced. It does in other cases, so that missing part in append easily catch you off guard. ( I didn't look in the errors panel because most errors there are actually harmless warnings and but also because giving a type to an array is a bit of work you do to avoid to have to look. You document the correct state of the system, it should not run at all if it is isn't in that state ).

I don't case as much about the runtime vs compile time distinction, but having confidence in the typing system is important. In this precise case, I expect at runtime a behaviour similar to an assertion because anything can happen as soon as types are wrong. Trying to append to a type array, should be equivalent to write assert( t is TypeInTheArray ), then append.

Calinou commented 3 months ago

in my mind, a type error should stop the game from running.

There's a related proposal here: https://github.com/godotengine/godot-proposals/issues/1729

Having a generic "pause on error" project setting might be useful, but it can hinder productivity as many errors can be difficult to resolve during development or are unintended due to bugs.

LBdN commented 3 months ago

My personal preference would be to distinguish coding errors from content errors even if they both eventually bubble up to the editor. Coding has it own workflow, and having a specific channel for code errors would be useful to have better integration into IDE/Editors. Also content is more flexible, prone to many movement and errors that are transient. It could also helps to split the work. But, better is better than nothing and the implementors are the ones to decide.