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: Fix type compatibility check between Array[T] and Packed*Arrays #99950

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JackErb
Copy link
Contributor

@JackErb JackErb commented Dec 3, 2024

Closes #94454

Verifies that, when assigning an Array to a PackedArray, that the array element is compatible with the packed array's element.

image

This is a potentially breaking change. Before this change, the invalid type would be converted to a default value. In this example, the packed array would silently become [(0,0), (0,0)]. This could be mitigated with a warning but I believe the proper behavior is to spout an error.

@JackErb JackErb requested review from a team as code owners December 3, 2024 04:19
@@ -6054,6 +6054,17 @@ bool GDScriptAnalyzer::check_type_compatibility(const GDScriptParser::DataType &
valid = p_target.get_container_element_type(0) == p_source.get_container_element_type(0);
}
}
if (valid && p_target.builtin_type != Variant::ARRAY && p_source.builtin_type == Variant::ARRAY) {
Copy link
Member

@akien-mga akien-mga Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only checks for p_target.builtin_type != Variant::ARRAY so anything that isn't an Array. That would include all other Variant types including Dictionary, float, etc., no?

Also note, once the correct approach is found here, a similar check should likely be done for typed Dictionaries just below.

Copy link
Contributor Author

@JackErb JackErb Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a lil confusing, but the implementation of the Variant::can_convert_strict call above here (line 6045) guarantees that if valid == true, and p_source is an ARRAY, then p_target is either an ARRAY or a PACKED_ARRAY. I will try to make this more explicit, since this could be a bug farm if the implementation of Variant::can_convert_strict changes.

container_type.type_source = p_target.type_source;
valid = p_source.get_container_element_type(0) == container_type;
} else {
// don't support converting a non-static array into a packed array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// don't support converting a non-static array into a packed array
// Don't support converting a non-static array into a packed array.

var array: Array[int]
var packed_array: PackedVector2Array

# these types should not be compatible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# these types should not be compatible
# These types should not be compatible.

@dalexeev dalexeev changed the title Fix type compatibility check between Array<T> and PackedArrays GDScript: Fix type compatibility check between Array[T] and Packed*Arrays Dec 3, 2024
@dalexeev dalexeev added this to the 4.4 milestone Dec 3, 2024
@dalexeev
Copy link
Member

dalexeev commented Dec 3, 2024

Note that this is "obvious" only for array literals with invalid elements. Instead of:

var array: PackedVector2Array
array = ["abc", "def"]

it could be:

var array: PackedVector2Array
array = [Vector2(), Vector2()]
var array: PackedVector2Array
array = [vector2_expr, vector2_expr]
var array: PackedVector2Array
array = [variant_expr, variant_expr]
var array: PackedVector2Array
array = array_expr

If the analyzer cannot guarantee that the assigned Array expression contains elements of incompatible types, then there should be no compile-time error. Instead, the conversion should be checked at runtime.

@JackErb
Copy link
Contributor Author

JackErb commented Dec 21, 2024

If the analyzer cannot guarantee that the assigned Array expression contains elements of incompatible types, then there should be no compile-time error. Instead, the conversion should be checked at runtime.

@dalexeev That is not currently how it works when converting from Array -> Array[T]. Right now it will always fail the conversion at runtime, without checking if the element types are compatible.

image

Based on what you said, I think the above screenshot should succeed since the elements of vectors_untyped are all Vector2. But it fails with a runtime error.

I see two paths here. Regardless, I want to keep the behavior of Array -> Packed*Array the same as Array -> Array[T].

  1. Make both of these use a runtime element conversion check.
  2. Disallow Array -> Packed*Array just like Array -> Array[T] currently is, and move the error to analyze-time since it makes more sense there.

How should I proceed?

@dalexeev
Copy link
Member

I agree that this behavior can be confusing, but there is a difference between implicit conversions Array[T] <-> Packed*Array and checking for type compatibility between Array[T1] <-> Array[T2] (including untyped arrays).

Arrays are passed by reference and untyped arrays can reference typed ones. This exception was made for compatibility with 3.x and for the convenience untyped GDScript users.

Array types check for reference compatibility. When passing Arrays you will never get an implicit copy of the array contents (instead you should explicitly use assign() if you want to copy the array contents to an array of incompatible type).

Unlike the Array[T] family, Packed*Array types are treated as fundamentally different. A Packed*Array cannot hold a reference to an Array or Packed*Array of a different type. When you assign an Array to a Packed*Array or vice versa, an implicit copy occurs, you get an independent array copy, not a reference copy.

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

Successfully merging this pull request may close these issues.

Type checker can't distinguish between PackedVector2Array and Array[PackedVector2Array]
3 participants