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

Fix typed arrays not working with concatenation operator + #73540

Merged
merged 1 commit into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions core/variant/variant_op.h
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,11 @@ class OperatorEvaluatorAddArray {
_FORCE_INLINE_ static void _add_arrays(Array &sum, const Array &array_a, const Array &array_b) {
int asize = array_a.size();
int bsize = array_b.size();

if (array_a.is_typed() && array_a.is_same_typed(array_b)) {

Choose a reason for hiding this comment

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

Do we need to check array_b.is_typed()?
Apologies for the drive by comment... :^) Testing a github action PR review bot and was using your PR as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically I don't even think array_a.is_typed() needs to be checked. is_same_typed might just be enough because if they are both untyped then set_typed pretty much acts like a no-op. I guess it can be a bit expensive to call set_typed just to set no type though so it helps optimize that use-case.

To answer your question, checking if array_b.is_typed() would be redundant. If array_a.is_typed() and array_a.is_same_typed(array_b) then it can be safe to assume array_b.is_typed().

sum.set_typed(array_a.get_typed_builtin(), array_a.get_typed_class_name(), array_a.get_typed_script());
}

sum.resize(asize + bsize);
for (int i = 0; i < asize; i++) {
sum[i] = array_a[i];
Expand Down
25 changes: 22 additions & 3 deletions modules/gdscript/gdscript_analyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4306,7 +4306,16 @@ GDScriptParser::DataType GDScriptAnalyzer::type_from_variant(const Variant &p_va
result.builtin_type = p_value.get_type();
result.type_source = GDScriptParser::DataType::ANNOTATED_EXPLICIT; // Constant has explicit type.

if (p_value.get_type() == Variant::OBJECT) {
if (p_value.get_type() == Variant::ARRAY) {
const Array &array = p_value;
if (array.get_typed_script()) {
result.set_container_element_type(type_from_metatype(make_script_meta_type(array.get_typed_script())));
} else if (array.get_typed_class_name()) {
result.set_container_element_type(type_from_metatype(make_native_meta_type(array.get_typed_class_name())));
} else if (array.get_typed_builtin() != Variant::NIL) {
result.set_container_element_type(type_from_metatype(make_builtin_meta_type((Variant::Type)array.get_typed_builtin())));
}
} else if (p_value.get_type() == Variant::OBJECT) {
// Object is treated as a native type, not a builtin type.
result.kind = GDScriptParser::DataType::NATIVE;

Expand Down Expand Up @@ -4741,11 +4750,21 @@ GDScriptParser::DataType GDScriptAnalyzer::get_operation_type(Variant::Operator
}
}

Variant::ValidatedOperatorEvaluator op_eval = Variant::get_validated_operator_evaluator(p_operation, a_type, b_type);
GDScriptParser::DataType result;
bool hard_operation = p_a.is_hard_type() && p_b.is_hard_type();

if (p_operation == Variant::OP_ADD && a_type == Variant::ARRAY && b_type == Variant::ARRAY) {
if (p_a.get_container_element_type() == p_a.get_container_element_type()) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like these need to be guarded with has_container_element_type or it causes errors with untyped arrays: https://github.com/godotengine/godot/actions/runs/5323011730/jobs/9640246948#step:11:558

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it compares the value with itself...

Copy link
Contributor

Choose a reason for hiding this comment

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

See #78478

r_valid = true;
result = p_a;
result.type_source = hard_operation ? GDScriptParser::DataType::ANNOTATED_INFERRED : GDScriptParser::DataType::INFERRED;
return result;
}
}

Variant::ValidatedOperatorEvaluator op_eval = Variant::get_validated_operator_evaluator(p_operation, a_type, b_type);
bool validated = op_eval != nullptr;

GDScriptParser::DataType result;
if (validated) {
r_valid = true;
result.type_source = hard_operation ? GDScriptParser::DataType::ANNOTATED_INFERRED : GDScriptParser::DataType::INFERRED;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# https://github.com/godotengine/godot/issues/72948

class Example:
extends RefCounted

const const_ints : Array[int] = [1, 2, 3]

func test():
var ints: Array[int] = [1, 2, 3]
var strings: Array[String] = ["4", "5", "6"]

var ints_concatenated: Array[int] = ints + ints
var strings_concatenated: Array[String] = strings + strings
var untyped_concatenated: Array = ints + strings
var const_ints_concatenated: Array[int] = const_ints + const_ints

print(ints_concatenated.get_typed_builtin())
print(strings_concatenated.get_typed_builtin())
print(untyped_concatenated.get_typed_builtin())
print(const_ints_concatenated.get_typed_builtin())

var objects: Array[Object] = []
var objects_concatenated: Array[Object] = objects + objects
print(objects_concatenated.get_typed_class_name())

var examples: Array[Example] = []
var examples_concatenated: Array[Example] = examples + examples
print(examples_concatenated.get_typed_script() == Example)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
GDTEST_OK
2
4
0
2
Object
true