Skip to content
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 - 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 this issue Jan 5, 2024 · 14 comments

Comments

@LeandroHPerez
Copy link

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:

  • Both of these should result in a parse error for attempting to append an element that breaks the array type.
    And if not a parse error for sure a runtime error, but it should occur in .append() line and not in line like this:
    print(str(array1[0])) or print(str(array2[0]))

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

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
Copy link
Member

AThousandShips commented Jan 5, 2024

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
Copy link
Author

LeandroHPerez commented Jan 5, 2024

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
Copy link
Member

Look in the debugger, the error is there

@Bromeon
Copy link
Contributor

Bromeon commented Jan 6, 2024

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
Copy link
Member

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

@LeandroHPerez
Copy link
Author

LeandroHPerez commented Jan 6, 2024

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
Copy link

Lippanon commented Jan 6, 2024

@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
Copy link
Member

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
Copy link
Author

@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
Copy link
Author

LeandroHPerez commented Jan 6, 2024

@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
Copy link
Member

dalexeev commented Jan 6, 2024

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
Copy link

LBdN commented Aug 16, 2024

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
Copy link
Member

Calinou commented Aug 16, 2024

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

There's a related proposal here: godotengine/godot-proposals#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
Copy link

LBdN commented Aug 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants