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.

Sign up for GitHub

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: Begin making constants deep, not shallow or flat #71051

Merged
merged 1 commit into from
Jan 9, 2023
Merged

GDScript: Begin making constants deep, not shallow or flat #71051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 0 additions & 10 deletions core/variant/array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,6 @@ void Array::_ref(const Array &p_from) const {

ERR_FAIL_COND(!_fp); // should NOT happen.

if (unlikely(_fp->read_only != nullptr)) {
// If p_from is a read-only array, just copy the contents to avoid further modification.
_unref();
_p = memnew(ArrayPrivate);
_p->refcount.init();
_p->array = _fp->array;
_p->typed = _fp->typed;
return;
}

Comment on lines -57 to -66
Copy link
Member

Choose a reason for hiding this comment

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

I believe this was done to allow mutability on assignments, e.g.:

const arr = [1, 2, 3]
var copy = arr # Create copy so it's a regular variable.
copy.push_back(4) # Only changes `copy`.
print(copy) # [1, 2, 3, 4]
print(arr) # [1, 2, 3]

Which wouldn't work with this change (push_back() would fail). But I don't think there's a valid use case for this and the user can call duplicate() if this is needed.

if (_fp == _p) {
return; // whatever it is, nothing to do here move along
}
Expand Down
10 changes: 0 additions & 10 deletions core/variant/dictionary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,16 +211,6 @@ bool Dictionary::recursive_equal(const Dictionary &p_dictionary, int recursion_c
}

void Dictionary::_ref(const Dictionary &p_from) const {
if (unlikely(p_from._p->read_only != nullptr)) {
// If p_from is a read-only dictionary, just copy the contents to avoid further modification.
if (_p) {
_unref();
}
_p = memnew(DictionaryPrivate);
_p->refcount.init();
_p->variant_map = p_from._p->variant_map;
return;
}
//make a copy first (thread safe)
if (!p_from._p->refcount.ref()) {
return; // couldn't copy
Expand Down
34 changes: 20 additions & 14 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1507,9 +1507,9 @@ void GDScriptAnalyzer::resolve_assignable(GDScriptParser::AssignableNode *p_assi

if (is_constant) {
if (p_assignable->initializer->type == GDScriptParser::Node::ARRAY) {
const_fold_array(static_cast<GDScriptParser::ArrayNode *>(p_assignable->initializer));
const_fold_array(static_cast<GDScriptParser::ArrayNode *>(p_assignable->initializer), true);
} else if (p_assignable->initializer->type == GDScriptParser::Node::DICTIONARY) {
const_fold_dictionary(static_cast<GDScriptParser::DictionaryNode *>(p_assignable->initializer));
const_fold_dictionary(static_cast<GDScriptParser::DictionaryNode *>(p_assignable->initializer), true);
}
if (!p_assignable->initializer->is_constant) {
push_error(vformat(R"(Assigned value for %s "%s" isn't a constant expression.)", p_kind, p_assignable->identifier->name), p_assignable->initializer);
Expand Down Expand Up @@ -2063,17 +2063,17 @@ void GDScriptAnalyzer::reduce_assignment(GDScriptParser::AssignmentNode *p_assig

GDScriptParser::DataType assignee_type = p_assignment->assignee->get_datatype();

if (assignee_type.is_constant || (p_assignment->assignee->type == GDScriptParser::Node::SUBSCRIPT && static_cast<GDScriptParser::SubscriptNode *>(p_assignment->assignee)->base->is_constant)) {
push_error("Cannot assign a new value to a constant.", p_assignment->assignee);
}

// Check if assigned value is an array literal, so we can make it a typed array too if appropriate.
if (assignee_type.has_container_element_type() && p_assignment->assigned_value->type == GDScriptParser::Node::ARRAY) {
update_array_literal_element_type(assignee_type, static_cast<GDScriptParser::ArrayNode *>(p_assignment->assigned_value));
}

GDScriptParser::DataType assigned_value_type = p_assignment->assigned_value->get_datatype();

if (assignee_type.is_constant) {
push_error("Cannot assign a new value to a constant.", p_assignment->assignee);
}

bool compatible = true;
GDScriptParser::DataType op_type = assigned_value_type;
if (p_assignment->operation != GDScriptParser::AssignmentNode::OP_NONE) {
Expand Down Expand Up @@ -3411,9 +3411,9 @@ void GDScriptAnalyzer::reduce_subscript(GDScriptParser::SubscriptNode *p_subscri
reduce_expression(p_subscript->base);

if (p_subscript->base->type == GDScriptParser::Node::ARRAY) {
const_fold_array(static_cast<GDScriptParser::ArrayNode *>(p_subscript->base));
const_fold_array(static_cast<GDScriptParser::ArrayNode *>(p_subscript->base), false);
} else if (p_subscript->base->type == GDScriptParser::Node::DICTIONARY) {
const_fold_dictionary(static_cast<GDScriptParser::DictionaryNode *>(p_subscript->base));
const_fold_dictionary(static_cast<GDScriptParser::DictionaryNode *>(p_subscript->base), false);
}
}

Expand Down Expand Up @@ -3738,16 +3738,16 @@ void GDScriptAnalyzer::reduce_unary_op(GDScriptParser::UnaryOpNode *p_unary_op)
p_unary_op->set_datatype(result);
}

void GDScriptAnalyzer::const_fold_array(GDScriptParser::ArrayNode *p_array) {
void GDScriptAnalyzer::const_fold_array(GDScriptParser::ArrayNode *p_array, bool p_is_const) {
bool all_is_constant = true;

for (int i = 0; i < p_array->elements.size(); i++) {
GDScriptParser::ExpressionNode *element = p_array->elements[i];

if (element->type == GDScriptParser::Node::ARRAY) {
const_fold_array(static_cast<GDScriptParser::ArrayNode *>(element));
const_fold_array(static_cast<GDScriptParser::ArrayNode *>(element), p_is_const);
} else if (element->type == GDScriptParser::Node::DICTIONARY) {
const_fold_dictionary(static_cast<GDScriptParser::DictionaryNode *>(element));
const_fold_dictionary(static_cast<GDScriptParser::DictionaryNode *>(element), p_is_const);
}

all_is_constant = all_is_constant && element->is_constant;
Expand All @@ -3761,20 +3761,23 @@ void GDScriptAnalyzer::const_fold_array(GDScriptParser::ArrayNode *p_array) {
for (int i = 0; i < p_array->elements.size(); i++) {
array[i] = p_array->elements[i]->reduced_value;
}
if (p_is_const) {
array.set_read_only(true);
}
p_array->is_constant = true;
p_array->reduced_value = array;
}

void GDScriptAnalyzer::const_fold_dictionary(GDScriptParser::DictionaryNode *p_dictionary) {
void GDScriptAnalyzer::const_fold_dictionary(GDScriptParser::DictionaryNode *p_dictionary, bool p_is_const) {
bool all_is_constant = true;

for (int i = 0; i < p_dictionary->elements.size(); i++) {
const GDScriptParser::DictionaryNode::Pair &element = p_dictionary->elements[i];

if (element.value->type == GDScriptParser::Node::ARRAY) {
const_fold_array(static_cast<GDScriptParser::ArrayNode *>(element.value));
const_fold_array(static_cast<GDScriptParser::ArrayNode *>(element.value), p_is_const);
} else if (element.value->type == GDScriptParser::Node::DICTIONARY) {
const_fold_dictionary(static_cast<GDScriptParser::DictionaryNode *>(element.value));
const_fold_dictionary(static_cast<GDScriptParser::DictionaryNode *>(element.value), p_is_const);
}

all_is_constant = all_is_constant && element.key->is_constant && element.value->is_constant;
Expand All @@ -3788,6 +3791,9 @@ void GDScriptAnalyzer::const_fold_dictionary(GDScriptParser::DictionaryNode *p_d
const GDScriptParser::DictionaryNode::Pair &element = p_dictionary->elements[i];
dict[element.key->reduced_value] = element.value->reduced_value;
}
if (p_is_const) {
dict.set_read_only(true);
}
p_dictionary->is_constant = true;
p_dictionary->reduced_value = dict;
}
Expand Down
4 changes: 2 additions & 2 deletions modules/gdscript/gdscript_analyzer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ class GDScriptAnalyzer {
void reduce_ternary_op(GDScriptParser::TernaryOpNode *p_ternary_op);
void reduce_unary_op(GDScriptParser::UnaryOpNode *p_unary_op);

void const_fold_array(GDScriptParser::ArrayNode *p_array);
void const_fold_dictionary(GDScriptParser::DictionaryNode *p_dictionary);
void const_fold_array(GDScriptParser::ArrayNode *p_array, bool p_is_const);
void const_fold_dictionary(GDScriptParser::DictionaryNode *p_dictionary, bool p_is_const);

// Helpers.
GDScriptParser::DataType type_from_variant(const Variant &p_value, const GDScriptParser::Node *p_source);
Expand Down
5 changes: 5 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/constant_array_index_assign.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const array: Array = [0]

func test():
var key: int = 0
array[key] = 0
2 changes: 2 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/constant_array_index_assign.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_ANALYZER_ERROR
Cannot assign a new value to a constant.
5 changes: 5 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/constant_dictionary_index_assign.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const dictionary := {}

func test():
var key: int = 0
dictionary[key] = 0
2 changes: 2 additions & 0 deletions modules/gdscript/tests/scripts/analyzer/errors/constant_dictionary_index_assign.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
GDTEST_ANALYZER_ERROR
Cannot assign a new value to a constant.
6 changes: 6 additions & 0 deletions modules/gdscript/tests/scripts/runtime/errors/constant_array_is_deep.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
const array: Array = [{}]

func test():
var dictionary := array[0]
var key: int = 0
dictionary[key] = 0
6 changes: 6 additions & 0 deletions modules/gdscript/tests/scripts/runtime/errors/constant_array_is_deep.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
GDTEST_RUNTIME_ERROR
>> SCRIPT ERROR
>> on function: test()
>> runtime/errors/constant_array_is_deep.gd
>> 6
>> Invalid set index '0' (on base: 'Dictionary') with value of type 'int'
Copy link
Member

Choose a reason for hiding this comment

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

Note for future work: we should make this error message more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, hopefully as an analyzer error rather than as a runtime error?

4 changes: 4 additions & 0 deletions modules/gdscript/tests/scripts/runtime/errors/constant_array_push_back.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const array: Array = [0]

func test():
array.push_back(0)
7 changes: 7 additions & 0 deletions modules/gdscript/tests/scripts/runtime/errors/constant_array_push_back.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
GDTEST_RUNTIME_ERROR
>> ERROR
>> on function: push_back()
>> core/variant/array.cpp
>> 253
>> Condition "_p->read_only" is true.
>> Array is in read-only state.
4 changes: 4 additions & 0 deletions modules/gdscript/tests/scripts/runtime/errors/constant_dictionary_erase.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const dictionary := {}

func test():
dictionary.erase(0)
7 changes: 7 additions & 0 deletions modules/gdscript/tests/scripts/runtime/errors/constant_dictionary_erase.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
GDTEST_RUNTIME_ERROR
>> ERROR
>> on function: erase()
>> core/variant/dictionary.cpp
>> 177
>> Condition "_p->read_only" is true. Returning: false
>> Dictionary is in read-only state.
6 changes: 6 additions & 0 deletions modules/gdscript/tests/scripts/runtime/errors/constant_dictionary_is_deep.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
const dictionary := {0: [0]}

func test():
var array := dictionary[0]
var key: int = 0
array[key] = 0
6 changes: 6 additions & 0 deletions modules/gdscript/tests/scripts/runtime/errors/constant_dictionary_is_deep.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
GDTEST_RUNTIME_ERROR
>> SCRIPT ERROR
>> on function: test()
>> runtime/errors/constant_dictionary_is_deep.gd
>> 6
>> Invalid set index '0' (on base: 'Array') with value of type 'int'