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

Jump to bottom

GDScript bugfixing #41674

Merged
merged 7 commits into from
Sep 1, 2020
Merged

GDScript bugfixing #41674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1ddb9b1
GDScript: Don't try to parse constant scripts that aren't valid
vnen Aug 31, 2020
34dc689
GDScript: Allow "self" to be used in class level
vnen Aug 31, 2020
edb4caf
GDScript: Allow "extends" to be used inside inner class
vnen Aug 31, 2020
f8fa5e4
GDScript: Fix analysis of singleton dependencies
vnen Aug 31, 2020
164cc87
GDScript: Check for missing exponent when parsing numbers
vnen Aug 31, 2020
c8e10aa
GDScript: Properly set class inheritance for global classes
vnen Aug 31, 2020
a889084
GDScript: Don't mark function parameters as constant
vnen Sep 1, 2020
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
115 changes: 74 additions & 41 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ Error GDScriptAnalyzer::resolve_inheritance(GDScriptParser::ClassNode *p_class,
push_error(vformat(R"(Could not resolve super class inheritance from "%s".)", name), p_class);
return err;
}
base = parser->get_parser()->head->get_datatype();
}
} else if (ProjectSettings::get_singleton()->has_autoload(name) && ProjectSettings::get_singleton()->get_autoload(name).is_singleton) {
const ProjectSettings::AutoloadInfo &info = ProjectSettings::get_singleton()->get_autoload(name);
Expand Down Expand Up @@ -1266,6 +1267,7 @@ void GDScriptAnalyzer::resolve_pararameter(GDScriptParser::ParameterNode *p_para
reduce_expression(p_parameter->default_value);
result = p_parameter->default_value->get_datatype();
result.type_source = GDScriptParser::DataType::INFERRED;
result.is_constant = false;
}

if (p_parameter->datatype_specifier != nullptr) {
Expand Down Expand Up @@ -1455,7 +1457,7 @@ void GDScriptAnalyzer::reduce_assignment(GDScriptParser::AssignmentNode *p_assig
bool compatible = true;
GDScriptParser::DataType op_type = p_assignment->assigned_value->get_datatype();
if (p_assignment->operation != GDScriptParser::AssignmentNode::OP_NONE) {
op_type = get_operation_type(p_assignment->variant_op, p_assignment->assignee->get_datatype(), p_assignment->assigned_value->get_datatype(), compatible);
op_type = get_operation_type(p_assignment->variant_op, p_assignment->assignee->get_datatype(), p_assignment->assigned_value->get_datatype(), compatible, p_assignment->assigned_value);
}

if (compatible) {
Expand Down Expand Up @@ -1618,7 +1620,7 @@ void GDScriptAnalyzer::reduce_binary_op(GDScriptParser::BinaryOpNode *p_binary_o
ERR_PRINT("Parser bug: unknown binary operation.");
}
}
p_binary_op->set_datatype(type_from_variant(p_binary_op->reduced_value));
p_binary_op->set_datatype(type_from_variant(p_binary_op->reduced_value, p_binary_op));

return;
}
Expand All @@ -1632,7 +1634,7 @@ void GDScriptAnalyzer::reduce_binary_op(GDScriptParser::BinaryOpNode *p_binary_o
} else {
if (p_binary_op->variant_op < Variant::OP_MAX) {
bool valid = false;
result = get_operation_type(p_binary_op->variant_op, p_binary_op->left_operand->get_datatype(), right_type, valid);
result = get_operation_type(p_binary_op->variant_op, p_binary_op->left_operand->get_datatype(), right_type, valid, p_binary_op);

if (!valid) {
push_error(vformat(R"(Invalid operands "%s" and "%s" for "%s" operator.)", p_binary_op->left_operand->get_datatype().to_string(), right_type.to_string(), Variant::get_operator_name(p_binary_op->variant_op)), p_binary_op);
Expand Down Expand Up @@ -2061,7 +2063,7 @@ void GDScriptAnalyzer::reduce_identifier_from_base(GDScriptParser::IdentifierNod
if (valid) {
p_identifier->is_constant = true;
p_identifier->reduced_value = result;
p_identifier->set_datatype(type_from_variant(result));
p_identifier->set_datatype(type_from_variant(result, p_identifier));
} else {
push_error(vformat(R"(Cannot find constant "%s" on type "%s".)", name, base.to_string()), p_identifier);
}
Expand Down Expand Up @@ -2192,7 +2194,7 @@ void GDScriptAnalyzer::reduce_identifier_from_base(GDScriptParser::IdentifierNod
if (valid) {
p_identifier->is_constant = true;
p_identifier->reduced_value = int_constant;
p_identifier->set_datatype(type_from_variant(int_constant));
p_identifier->set_datatype(type_from_variant(int_constant, p_identifier));
return;
}
}
Expand Down Expand Up @@ -2289,18 +2291,42 @@ void GDScriptAnalyzer::reduce_identifier(GDScriptParser::IdentifierNode *p_ident
return;
}

// Try singletons.
// Do this before globals because this might be a singleton loading another one before it's compiled.
if (ProjectSettings::get_singleton()->has_autoload(name)) {
const ProjectSettings::AutoloadInfo &autoload = ProjectSettings::get_singleton()->get_autoload(name);
if (autoload.is_singleton) {
// Singleton exists, so it's at least a Node.
GDScriptParser::DataType result;
result.kind = GDScriptParser::DataType::NATIVE;
result.type_source = GDScriptParser::DataType::ANNOTATED_EXPLICIT;
if (autoload.path.to_lower().ends_with(GDScriptLanguage::get_singleton()->get_extension())) {
Ref<GDScriptParserRef> parser = get_parser_for(autoload.path);
if (parser.is_valid()) {
Error err = parser->raise_status(GDScriptParserRef::INTERFACE_SOLVED);
if (err == OK) {
result = type_from_metatype(parser->get_parser()->head->get_datatype());
}
}
}
result.is_constant = true;
p_identifier->set_datatype(result);
return;
}
}

if (GDScriptLanguage::get_singleton()->get_global_map().has(name)) {
int idx = GDScriptLanguage::get_singleton()->get_global_map()[name];
Variant constant = GDScriptLanguage::get_singleton()->get_global_array()[idx];
p_identifier->set_datatype(type_from_variant(constant));
p_identifier->set_datatype(type_from_variant(constant, p_identifier));
p_identifier->is_constant = true;
p_identifier->reduced_value = constant;
return;
}

if (GDScriptLanguage::get_singleton()->get_named_globals_map().has(name)) {
Variant constant = GDScriptLanguage::get_singleton()->get_named_globals_map()[name];
p_identifier->set_datatype(type_from_variant(constant));
p_identifier->set_datatype(type_from_variant(constant, p_identifier));
p_identifier->is_constant = true;
p_identifier->reduced_value = constant;
return;
Expand All @@ -2322,7 +2348,7 @@ void GDScriptAnalyzer::reduce_literal(GDScriptParser::LiteralNode *p_literal) {
p_literal->reduced_value = p_literal->value;
p_literal->is_constant = true;

p_literal->set_datatype(type_from_variant(p_literal->reduced_value));
p_literal->set_datatype(type_from_variant(p_literal->reduced_value, p_literal));
}

void GDScriptAnalyzer::reduce_preload(GDScriptParser::PreloadNode *p_preload) {
Expand Down Expand Up @@ -2359,7 +2385,7 @@ void GDScriptAnalyzer::reduce_preload(GDScriptParser::PreloadNode *p_preload) {

p_preload->is_constant = true;
p_preload->reduced_value = p_preload->resource;
p_preload->set_datatype(type_from_variant(p_preload->reduced_value));
p_preload->set_datatype(type_from_variant(p_preload->reduced_value, p_preload));
}

void GDScriptAnalyzer::reduce_self(GDScriptParser::SelfNode *p_self) {
Expand Down Expand Up @@ -2408,7 +2434,7 @@ void GDScriptAnalyzer::reduce_subscript(GDScriptParser::SubscriptNode *p_subscri
} else {
p_subscript->is_constant = true;
p_subscript->reduced_value = value;
result_type = type_from_variant(value);
result_type = type_from_variant(value, p_subscript);
}
result_type.kind = GDScriptParser::DataType::VARIANT;
} else {
Expand Down Expand Up @@ -2448,7 +2474,7 @@ void GDScriptAnalyzer::reduce_subscript(GDScriptParser::SubscriptNode *p_subscri
} else {
p_subscript->is_constant = true;
p_subscript->reduced_value = value;
result_type = type_from_variant(value);
result_type = type_from_variant(value, p_subscript);
}
result_type.kind = GDScriptParser::DataType::VARIANT;
} else {
Expand Down Expand Up @@ -2672,13 +2698,13 @@ void GDScriptAnalyzer::reduce_unary_op(GDScriptParser::UnaryOpNode *p_unary_op)
if (p_unary_op->operand->is_constant) {
p_unary_op->is_constant = true;
p_unary_op->reduced_value = Variant::evaluate(p_unary_op->variant_op, p_unary_op->operand->reduced_value, Variant());
result = type_from_variant(p_unary_op->reduced_value);
result = type_from_variant(p_unary_op->reduced_value, p_unary_op);
} else if (p_unary_op->operand->get_datatype().is_variant()) {
result.kind = GDScriptParser::DataType::VARIANT;
mark_node_unsafe(p_unary_op);
} else {
bool valid = false;
result = get_operation_type(p_unary_op->variant_op, p_unary_op->operand->get_datatype(), p_unary_op->operand->get_datatype(), valid);
result = get_operation_type(p_unary_op->variant_op, p_unary_op->operand->get_datatype(), p_unary_op->operand->get_datatype(), valid, p_unary_op);

if (!valid) {
push_error(vformat(R"(Invalid operand of type "%s" for unary operator "%s".)", p_unary_op->operand->get_datatype().to_string(), Variant::get_operator_name(p_unary_op->variant_op)), p_unary_op->operand);
Expand All @@ -2688,7 +2714,7 @@ void GDScriptAnalyzer::reduce_unary_op(GDScriptParser::UnaryOpNode *p_unary_op)
p_unary_op->set_datatype(result);
}

GDScriptParser::DataType GDScriptAnalyzer::type_from_variant(const Variant &p_value) {
GDScriptParser::DataType GDScriptAnalyzer::type_from_variant(const Variant &p_value, const GDScriptParser::Node *p_source) {
GDScriptParser::DataType result;
result.is_constant = true;
result.kind = GDScriptParser::DataType::BUILTIN;
Expand All @@ -2710,37 +2736,44 @@ GDScriptParser::DataType GDScriptAnalyzer::type_from_variant(const Variant &p_va
scr = obj->get_script();
}
if (scr.is_valid()) {
result.script_type = scr;
result.script_path = scr->get_path();
Ref<GDScript> gds = scr;
if (gds.is_valid()) {
result.kind = GDScriptParser::DataType::CLASS;
// This might be an inner class, so we want to get the parser for the root.
// But still get the inner class from that tree.
GDScript *current = gds.ptr();
List<StringName> class_chain;
while (current->_owner) {
// Push to front so it's in reverse.
class_chain.push_front(current->name);
current = current->_owner;
}
if (scr->is_valid()) {
result.script_type = scr;
result.script_path = scr->get_path();
Ref<GDScript> gds = scr;
if (gds.is_valid()) {
result.kind = GDScriptParser::DataType::CLASS;
// This might be an inner class, so we want to get the parser for the root.
// But still get the inner class from that tree.
GDScript *current = gds.ptr();
List<StringName> class_chain;
while (current->_owner) {
// Push to front so it's in reverse.
class_chain.push_front(current->name);
current = current->_owner;
}

Ref<GDScriptParserRef> ref = get_parser_for(current->path);
ref->raise_status(GDScriptParserRef::INTERFACE_SOLVED);
Ref<GDScriptParserRef> ref = get_parser_for(current->path);
ref->raise_status(GDScriptParserRef::INTERFACE_SOLVED);

GDScriptParser::ClassNode *found = ref->get_parser()->head;
GDScriptParser::ClassNode *found = ref->get_parser()->head;

// It should be okay to assume this exists, since we have a complete script already.
for (const List<StringName>::Element *E = class_chain.front(); E; E = E->next()) {
found = found->get_member(E->get()).m_class;
}
// It should be okay to assume this exists, since we have a complete script already.
for (const List<StringName>::Element *E = class_chain.front(); E; E = E->next()) {
found = found->get_member(E->get()).m_class;
}

result.class_type = found;
result.script_path = ref->get_parser()->script_path;
result.class_type = found;
result.script_path = ref->get_parser()->script_path;
} else {
result.kind = GDScriptParser::DataType::SCRIPT;
}
result.native_type = scr->get_instance_base_type();
} else {
result.kind = GDScriptParser::DataType::SCRIPT;
push_error(vformat(R"(Constant value uses script from "%s" which is loaded but not compiled.)", scr->get_path()), p_source);
result.kind = GDScriptParser::DataType::VARIANT;
result.type_source = GDScriptParser::DataType::UNDETECTED;
result.is_meta_type = false;
}
result.native_type = scr->get_instance_base_type();
} else {
result.kind = GDScriptParser::DataType::NATIVE;
if (result.native_type == GDScriptNativeClass::get_class_static()) {
Expand Down Expand Up @@ -2996,7 +3029,7 @@ bool GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_local, con
}
#endif

GDScriptParser::DataType GDScriptAnalyzer::get_operation_type(Variant::Operator p_operation, const GDScriptParser::DataType &p_a, const GDScriptParser::DataType &p_b, bool &r_valid) {
GDScriptParser::DataType GDScriptAnalyzer::get_operation_type(Variant::Operator p_operation, const GDScriptParser::DataType &p_a, const GDScriptParser::DataType &p_b, bool &r_valid, const GDScriptParser::Node *p_source) {
// This function creates dummy variant values and apply the operation to those. Less error-prone than keeping a table of valid operations.

GDScriptParser::DataType result;
Expand Down Expand Up @@ -3069,7 +3102,7 @@ GDScriptParser::DataType GDScriptAnalyzer::get_operation_type(Variant::Operator
Variant::evaluate(p_operation, a, b, ret, r_valid);

if (r_valid) {
return type_from_variant(ret);
return type_from_variant(ret, p_source);
}

return result;
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 @@ -90,15 +90,15 @@ class GDScriptAnalyzer {
void reduce_unary_op(GDScriptParser::UnaryOpNode *p_unary_op);

// Helpers.
GDScriptParser::DataType type_from_variant(const Variant &p_value);
GDScriptParser::DataType type_from_variant(const Variant &p_value, const GDScriptParser::Node *p_source);
GDScriptParser::DataType type_from_metatype(const GDScriptParser::DataType &p_meta_type) const;
GDScriptParser::DataType type_from_property(const PropertyInfo &p_property) const;
GDScriptParser::DataType make_global_class_meta_type(const StringName &p_class_name);
bool get_function_signature(GDScriptParser::Node *p_source, GDScriptParser::DataType base_type, const StringName &p_function, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, bool &r_static, bool &r_vararg);
bool function_signature_from_info(const MethodInfo &p_info, GDScriptParser::DataType &r_return_type, List<GDScriptParser::DataType> &r_par_types, int &r_default_arg_count, bool &r_static, bool &r_vararg);
bool validate_call_arg(const List<GDScriptParser::DataType> &p_par_types, int p_default_args_count, bool p_is_vararg, const GDScriptParser::CallNode *p_call);
bool validate_call_arg(const MethodInfo &p_method, const GDScriptParser::CallNode *p_call);
GDScriptParser::DataType get_operation_type(Variant::Operator p_operation, const GDScriptParser::DataType &p_a, const GDScriptParser::DataType &p_b, bool &r_valid);
GDScriptParser::DataType get_operation_type(Variant::Operator p_operation, const GDScriptParser::DataType &p_a, const GDScriptParser::DataType &p_b, bool &r_valid, const GDScriptParser::Node *p_source);
bool is_type_compatible(const GDScriptParser::DataType &p_target, const GDScriptParser::DataType &p_source, bool p_allow_implicit_conversion = false) const;
void push_error(const String &p_message, const GDScriptParser::Node *p_origin);
void mark_node_unsafe(const GDScriptParser::Node *p_node);
Expand Down
12 changes: 10 additions & 2 deletions modules/gdscript/gdscript_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,14 @@ GDScriptParser::ClassNode *GDScriptParser::parse_class() {
return n_class;
}

if (match(GDScriptTokenizer::Token::EXTENDS)) {
if (n_class->extends_used) {
push_error(R"(Cannot use "extends" more than once in the same class.)");
}
parse_extends();
end_statement("superclass");
}

parse_class_body();

consume(GDScriptTokenizer::Token::DEDENT, R"(Missing unindent at the end of the class body.)");
Expand Down Expand Up @@ -1941,8 +1949,8 @@ GDScriptParser::ExpressionNode *GDScriptParser::parse_literal(ExpressionNode *p_
}

GDScriptParser::ExpressionNode *GDScriptParser::parse_self(ExpressionNode *p_previous_operand, bool p_can_assign) {
if (!current_function || current_function->is_static) {
push_error(R"(Cannot use "self" outside a non-static function.)");
if (current_function && current_function->is_static) {
push_error(R"(Cannot use "self" inside a static function.)");
}
SelfNode *self = alloc_node<SelfNode>();
self->current_class = current_class;
Expand Down
32 changes: 32 additions & 0 deletions modules/gdscript/gdscript_tokenizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,19 @@ GDScriptTokenizer::Token GDScriptTokenizer::number() {
}

// Allow '_' to be used in a number, for readability.
bool previous_was_underscore = false;
while (digit_check_func(_peek()) || _peek() == '_') {
if (_peek() == '_') {
if (previous_was_underscore) {
Token error = make_error(R"(Only one underscore can be used as a numeric separator.)");
error.start_column = column;
error.leftmost_column = column;
error.end_column = column + 1;
error.rightmost_column = column + 1;
push_error(error);
}
previous_was_underscore = true;
}
_advance();
}

Expand Down Expand Up @@ -672,7 +684,27 @@ GDScriptTokenizer::Token GDScriptTokenizer::number() {
_advance();
}
// Consume exponent digits.
if (!_is_digit(_peek())) {
Token error = make_error(R"(Expected exponent value after "e".)");
error.start_column = column;
error.leftmost_column = column;
error.end_column = column + 1;
error.rightmost_column = column + 1;
push_error(error);
}
previous_was_underscore = false;
while (_is_digit(_peek()) || _peek() == '_') {
if (_peek() == '_') {
if (previous_was_underscore) {
Token error = make_error(R"(Only one underscore can be used as a numeric separator.)");
error.start_column = column;
error.leftmost_column = column;
error.end_column = column + 1;
error.rightmost_column = column + 1;
push_error(error);
}
previous_was_underscore = true;
}
_advance();
}
}
Expand Down