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

Allow setters and getters to take additional arguments #78991

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+415 −70
Open

Allow setters and getters to take additional arguments #78991

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
141 changes: 95 additions & 46 deletions modules/gdscript/gdscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,6 +864,31 @@ void GDScript::unload_static() const {
GDScriptCache::remove_script(fully_qualified_name);
}

Callable::CallError GDScript::call_getter(const MemberInfo &p_member, const StringName &p_name, Variant &r_ret) const {
Callable::CallError ce;
if (p_member.getter_with_name) {
Variant property_name = p_name;
const Variant *args = &property_name;
r_ret = const_cast<GDScript *>(this)->callp(p_member.getter, &args, 1, ce);
} else {
r_ret = const_cast<GDScript *>(this)->callp(p_member.getter, nullptr, 0, ce);
}
return ce;
}

Callable::CallError GDScript::call_setter(const MemberInfo &p_member, const StringName &p_name, const Variant &p_value) {
Callable::CallError ce;
if (p_member.setter_with_name) {
Variant property_name = p_name;
const Variant *args[2] = { &property_name, &p_value };
callp(p_member.setter, args, 2, ce);
} else {
const Variant *args = &p_value;
callp(p_member.setter, &args, 1, ce);
}
return ce;
}

Variant GDScript::callp(const StringName &p_method, const Variant **p_args, int p_argcount, Callable::CallError &r_error) {
GDScript *top = this;
while (top) {
Expand Down Expand Up @@ -896,13 +921,12 @@ bool GDScript::_get(const StringName &p_name, Variant &r_ret) const {
return true;
}
}

{
HashMap<StringName, MemberInfo>::ConstIterator E = top->static_variables_indices.find(p_name);
if (E) {
if (E->value.getter) {
Callable::CallError ce;
r_ret = const_cast<GDScript *>(this)->callp(E->value.getter, nullptr, 0, ce);
const GDScript::MemberInfo *static_member = &E->value;
call_getter(*static_member, p_name, r_ret); // TODO: returned call error not used.
return true;
}
r_ret = top->static_variables[E->value.index];
Expand Down Expand Up @@ -942,32 +966,30 @@ bool GDScript::_set(const StringName &p_name, const Variant &p_value) {
reload();
return true;
}

GDScript *top = this;
while (top) {
HashMap<StringName, MemberInfo>::ConstIterator E = top->static_variables_indices.find(p_name);
if (E) {
const MemberInfo *member = &E->value;
const MemberInfo *static_member = &E->value;
Variant value = p_value;
if (member->data_type.has_type && !member->data_type.is_type(value)) {
if (static_member->data_type.has_type && !static_member->data_type.is_type(value)) {
const Variant *args = &p_value;
Callable::CallError err;
Variant::construct(member->data_type.builtin_type, value, &args, 1, err);
if (err.error != Callable::CallError::CALL_OK || !member->data_type.is_type(value)) {
Variant::construct(static_member->data_type.builtin_type, value, &args, 1, err);
if (err.error != Callable::CallError::CALL_OK || !static_member->data_type.is_type(value)) {
return false;
}
}
if (member->setter) {
const Variant *args = &value;
Callable::CallError err;
callp(member->setter, &args, 1, err);
return err.error == Callable::CallError::CALL_OK;
if (static_member->setter) {
Callable::CallError ce = call_setter(*static_member, p_name, value);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
}
Comment on lines +984 to +987
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
Callable::CallError ce = call_setter(*static_member, p_name, value);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
}
call_setter(*static_member, p_name, value); // TODO: returned call error not used.
return true;

} else {
top->static_variables.write[member->index] = value;
top->static_variables.write[static_member->index] = value;
return true;
}
}

top = top->_base;
}

Expand Down Expand Up @@ -1569,10 +1591,10 @@ bool GDScriptInstance::set(const StringName &p_name, const Variant &p_value) {
}
}
if (member->setter) {
const Variant *args = &value;
Callable::CallError err;
callp(member->setter, &args, 1, err);
return err.error == Callable::CallError::CALL_OK;
Callable::CallError ce = call_setter(*member, p_name, value);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
}
Comment on lines +1594 to +1597
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
Callable::CallError ce = call_setter(*member, p_name, value);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
}
call_setter(*member, p_name, value); // TODO: returned call error not used.
return true;

} else {
members.write[member->index] = value;
return true;
Expand All @@ -1585,23 +1607,23 @@ bool GDScriptInstance::set(const StringName &p_name, const Variant &p_value) {
{
HashMap<StringName, GDScript::MemberInfo>::ConstIterator E = sptr->static_variables_indices.find(p_name);
if (E) {
const GDScript::MemberInfo *member = &E->value;
const GDScript::MemberInfo *static_member = &E->value;
Variant value = p_value;
if (member->data_type.has_type && !member->data_type.is_type(value)) {
if (static_member->data_type.has_type && !static_member->data_type.is_type(value)) {
const Variant *args = &p_value;
Callable::CallError err;
Variant::construct(member->data_type.builtin_type, value, &args, 1, err);
if (err.error != Callable::CallError::CALL_OK || !member->data_type.is_type(value)) {
Variant::construct(static_member->data_type.builtin_type, value, &args, 1, err);
if (err.error != Callable::CallError::CALL_OK || !static_member->data_type.is_type(value)) {
return false;
}
}
if (member->setter) {
const Variant *args = &value;
Callable::CallError err;
callp(member->setter, &args, 1, err);
return err.error == Callable::CallError::CALL_OK;
if (static_member->setter) {
Callable::CallError ce = call_setter(*static_member, p_name, value);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
Comment on lines +1621 to +1623
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
Callable::CallError ce = call_setter(*static_member, p_name, value);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
call_setter(*static_member, p_name, value); // TODO: returned call error not used.
return true;

}
} else {
sptr->static_variables.write[member->index] = value;
sptr->static_variables.write[static_member->index] = value;
return true;
}
}
Expand Down Expand Up @@ -1631,14 +1653,14 @@ bool GDScriptInstance::get(const StringName &p_name, Variant &r_ret) const {
{
HashMap<StringName, GDScript::MemberInfo>::ConstIterator E = script->member_indices.find(p_name);
if (E) {
if (E->value.getter) {
Callable::CallError err;
r_ret = const_cast<GDScriptInstance *>(this)->callp(E->value.getter, nullptr, 0, err);
if (err.error == Callable::CallError::CALL_OK) {
return true;
const GDScript::MemberInfo *member = &E->value;
if (member->getter) {
Callable::CallError ce = call_getter(*member, p_name, r_ret);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
}
Comment on lines +1658 to 1661
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
Callable::CallError ce = call_getter(*member, p_name, r_ret);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
}
call_getter(*member, p_name, r_ret); // TODO: returned call error not used.
return true;

}
r_ret = members[E->value.index];
r_ret = members[member->index];
return true;
}
}
Expand All @@ -1656,12 +1678,14 @@ bool GDScriptInstance::get(const StringName &p_name, Variant &r_ret) const {
{
HashMap<StringName, GDScript::MemberInfo>::ConstIterator E = sptr->static_variables_indices.find(p_name);
if (E) {
if (E->value.getter) {
Callable::CallError ce;
r_ret = const_cast<GDScript *>(sptr)->callp(E->value.getter, nullptr, 0, ce);
return true;
const GDScript::MemberInfo *static_member = &E->value;
if (static_member->getter) {
Callable::CallError ce = sptr->call_getter(*static_member, p_name, r_ret);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
}
Comment on lines +1683 to +1686
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
Callable::CallError ce = sptr->call_getter(*static_member, p_name, r_ret);
if (ce.error == Callable::CallError::CALL_OK) {
return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
}
sptr->call_getter(*static_member, p_name, r_ret); // TODO: returned call error not used.
return true;

}
r_ret = sptr->static_variables[E->value.index];
r_ret = sptr->static_variables[static_member->index];
return true;
}
}
Expand Down Expand Up @@ -1697,8 +1721,8 @@ bool GDScriptInstance::get(const StringName &p_name, Variant &r_ret) const {
{
HashMap<StringName, GDScriptFunction *>::ConstIterator E = sptr->member_functions.find(GDScriptLanguage::get_singleton()->strings._get);
if (E) {
Variant name = p_name;
const Variant *args[1] = { &name };
Variant property_name = p_name;
const Variant *args[1] = { &property_name };

Callable::CallError err;
Variant ret = const_cast<GDScriptFunction *>(E->value)->call(const_cast<GDScriptInstance *>(this), (const Variant **)args, 1, err);
Expand Down Expand Up @@ -1828,8 +1852,8 @@ void GDScriptInstance::get_property_list(List<PropertyInfo> *p_properties) const
}

bool GDScriptInstance::property_can_revert(const StringName &p_name) const {
Variant name = p_name;
const Variant *args[1] = { &name };
Variant property_name = p_name;
const Variant *args[1] = { &property_name };

const GDScript *sptr = script.ptr();
while (sptr) {
Expand All @@ -1848,8 +1872,8 @@ bool GDScriptInstance::property_can_revert(const StringName &p_name) const {
}

bool GDScriptInstance::property_get_revert(const StringName &p_name, Variant &r_ret) const {
Variant name = p_name;
const Variant *args[1] = { &name };
Variant property_name = p_name;
const Variant *args[1] = { &property_name };

const GDScript *sptr = script.ptr();
while (sptr) {
Expand Down Expand Up @@ -1896,6 +1920,31 @@ bool GDScriptInstance::has_method(const StringName &p_method) const {
return false;
}

Callable::CallError GDScriptInstance::call_getter(const GDScript::MemberInfo &p_member, const StringName &p_name, Variant &r_ret) const {
Callable::CallError ce;
if (p_member.getter_with_name) {
Variant property_name = p_name;
const Variant *args = &property_name;
r_ret = const_cast<GDScriptInstance *>(this)->callp(p_member.getter, &args, 1, ce);
} else {
r_ret = const_cast<GDScriptInstance *>(this)->callp(p_member.getter, nullptr, 0, ce);
}
return ce;
}

Callable::CallError GDScriptInstance::call_setter(const GDScript::MemberInfo &p_member, const StringName &p_name, const Variant &p_value) {
Callable::CallError ce;
if (p_member.setter_with_name) {
Variant property_name = p_name;
const Variant *args[2] = { &property_name, &p_value };
callp(p_member.setter, args, 2, ce);
} else {
const Variant *args = &p_value;
callp(p_member.setter, &args, 1, ce);
}
return ce;
}

Variant GDScriptInstance::callp(const StringName &p_method, const Variant **p_args, int p_argcount, Callable::CallError &r_error) {
GDScript *sptr = script.ptr();
if (unlikely(p_method == SNAME("_ready"))) {
Expand Down
8 changes: 8 additions & 0 deletions modules/gdscript/gdscript.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ class GDScript : public Script {
struct MemberInfo {
int index = 0;
StringName setter;
bool setter_with_name = false;
StringName getter;
bool getter_with_name = false;
GDScriptDataType data_type;
};

Expand Down Expand Up @@ -180,6 +182,9 @@ class GDScript : public Script {
GDScript *_get_gdscript_from_variant(const Variant &p_variant);
void _get_dependencies(RBSet<GDScript *> &p_dependencies, const GDScript *p_except);

Callable::CallError call_getter(const MemberInfo &p_member_info, const StringName &p_name, Variant &r_ret) const;
Callable::CallError call_setter(const MemberInfo &p_member_info, const StringName &p_name, const Variant &p_value);

protected:
bool _get(const StringName &p_name, Variant &r_ret) const;
bool _set(const StringName &p_name, const Variant &p_value);
Expand Down Expand Up @@ -314,6 +319,9 @@ class GDScriptInstance : public ScriptInstance {

SelfList<GDScriptFunctionState>::List pending_func_states;

Callable::CallError call_getter(const GDScript::MemberInfo &p_member_info, const StringName &p_name, Variant &r_ret) const;
Callable::CallError call_setter(const GDScript::MemberInfo &p_member_info, const StringName &p_name, const Variant &p_value);

public:
virtual Object *get_owner() { return owner; }

Expand Down
57 changes: 38 additions & 19 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1335,15 +1335,23 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co
return_datatype.is_meta_type = false;
}

if (getter_function->parameters.size() != 0 || return_datatype.has_no_type()) {
push_error(vformat(R"(Function "%s" cannot be used as getter because of its signature.)", getter_function->identifier->name), member.variable);
if (getter_function->mandatory_parameter_count > 1) {
push_error(R"(A getter cannot have more than 1 mandatory argument.)", member.variable);
} else if (!is_type_compatible(member.variable->datatype, return_datatype, true)) {
push_error(vformat(R"(Function with return type "%s" cannot be used as getter for a property of type "%s".)", return_datatype.to_string(), member.variable->datatype.to_string()), member.variable);

push_error(vformat(R"(Getter return type "%s" is incompatible with variable type "%s".)", return_datatype.to_string(), member.variable->datatype.to_string()), member.variable);
} else {
has_valid_getter = true;
if (getter_function->mandatory_parameter_count == 1) {
GDScriptParser::DataType name_arg_type = getter_function->parameters[0]->get_datatype();
if (!name_arg_type.is_variant() && name_arg_type.is_hard_type()) {
if (name_arg_type.kind != GDScriptParser::DataType::BUILTIN || (name_arg_type.builtin_type != Variant::STRING && name_arg_type.builtin_type != Variant::STRING_NAME)) {
push_error(R"(First mandatory argument of a getter must be a property name of type String or StringName.)", member.variable);
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
push_error(R"(First mandatory argument of a getter must be a property name of type String or StringName.)", member.variable);
push_error(R"(First mandatory argument of a getter is a property name and must be of type String or StringName.)", member.variable);

has_valid_getter = false;
}
}
}
#ifdef DEBUG_ENABLED
if (member.variable->datatype.builtin_type == Variant::INT && return_datatype.builtin_type == Variant::FLOAT) {
if (has_valid_getter && member.variable->datatype.builtin_type == Variant::INT && return_datatype.builtin_type == Variant::FLOAT) {
parser->push_warning(member.variable, GDScriptWarning::NARROWING_CONVERSION);
}
#endif
Expand All @@ -1358,27 +1366,38 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co

if (setter_function == nullptr) {
push_error(vformat(R"(Setter "%s" not found.)", member.variable->setter_pointer->name), member.variable);

} else if (setter_function->parameters.size() != 1) {
push_error(vformat(R"(Function "%s" cannot be used as setter because of its signature.)", setter_function->identifier->name), member.variable);

} else if (!is_type_compatible(member.variable->datatype, setter_function->parameters[0]->datatype, true)) {
push_error(vformat(R"(Function with argument type "%s" cannot be used as setter for a property of type "%s".)", setter_function->parameters[0]->datatype.to_string(), member.variable->datatype.to_string()), member.variable);

} else {
has_valid_setter = true;

if (setter_function->mandatory_parameter_count == 0 || setter_function->mandatory_parameter_count > 2) {
push_error(vformat(R"(Setters must have 1 or 2 mandatory arguments.)"), member.variable);
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
push_error(vformat(R"(Setters must have 1 or 2 mandatory arguments.)"), member.variable);
push_error(vformat(R"(A setter must have 1 or 2 mandatory arguments.)"), member.variable);

For consistency with the getter error message.

} else {
GDScriptParser::ParameterNode *value_arg = setter_function->parameters[setter_function->mandatory_parameter_count - 1];
if (!is_type_compatible(member.variable->datatype, value_arg->datatype, true)) {
push_error(vformat(R"(Setter argument "%s" of type "%s" is incompatible with variable type "%s".)", value_arg->identifier->name, value_arg->get_datatype().to_string(), member.variable->datatype.to_string()), member.variable);
} else {
has_valid_setter = true;
if (setter_function->mandatory_parameter_count == 2) {
GDScriptParser::DataType name_arg_type = setter_function->parameters[0]->datatype;
if (!name_arg_type.is_variant() && name_arg_type.is_hard_type()) {
if (name_arg_type.kind != GDScriptParser::DataType::BUILTIN || (name_arg_type.builtin_type != Variant::STRING && name_arg_type.builtin_type != Variant::STRING_NAME)) {
push_error(R"(First mandatory argument of a setter with 2 mandatory arguments must be a property name of type String or StringName.)", member.variable);
has_valid_setter = false;
}
}
}
#ifdef DEBUG_ENABLED
if (member.variable->datatype.builtin_type == Variant::FLOAT && setter_function->parameters[0]->datatype.builtin_type == Variant::INT) {
parser->push_warning(member.variable, GDScriptWarning::NARROWING_CONVERSION);
}
if (has_valid_setter && member.variable->datatype.builtin_type == Variant::FLOAT && value_arg->datatype.builtin_type == Variant::INT) {
parser->push_warning(member.variable, GDScriptWarning::NARROWING_CONVERSION);
}
#endif
}
}
}
}

if (member.variable->datatype.is_variant() && has_valid_getter && has_valid_setter) {
if (!is_type_compatible(getter_function->datatype, setter_function->parameters[0]->datatype, true)) {
push_error(vformat(R"(Getter with type "%s" cannot be used along with setter of type "%s".)", getter_function->datatype.to_string(), setter_function->parameters[0]->datatype.to_string()), member.variable);
GDScriptParser::DataType value_arg_type = setter_function->parameters[setter_function->mandatory_parameter_count - 1]->datatype;
if (!is_type_compatible(getter_function->datatype, value_arg_type, true)) {
push_error(vformat(R"(Getter with type "%s" cannot be used along with setter of type "%s".)", getter_function->datatype.to_string(), value_arg_type.to_string()), member.variable);
}
}
}
Expand Down
Loading