Skip to content

Commit

Permalink
GDScript: Add warnings that are set to error by default
Browse files Browse the repository at this point in the history 
- Adds a list of default levels for all warning so they can be set
  individually.
- Add warnings set by default to error for:
  - Using `get_node()` without `@onready`.
  - Using `@onready` together with `@export`.
  - Inferring a static type with a Variant value.
  - Overriding a native engine method.
- Adjust how annotations to ignore warnings are treated so they also
  apply to method parameters.
- Clean up a bit how ignored warnings are set. There were two sets but
  only one was actually being used.
- Set all warnings to the `WARN` level for tests, so they they can be
  properly tested.
- Fix enum types in native methods signatures being set to `int`.
- Fix native enums being treated as Dictionary by mistake.
- Make name of native enum types use the class they are defined in, not
  the direct super class of the script. This ensures they are always
  equal even when coming from different sources.
- Fix error for signature mismatch that was only showing the first
  default argument as having a default. Now it shows for all.
  • Loading branch information
@vnen
vnen committed Feb 1, 2023
1 parent 0810eca commit a166833
Show file tree
Hide file tree
Showing 18 changed files with 313 additions and 57 deletions.
14 changes: 13 additions & 1 deletion doc/classes/ProjectSettings.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,9 +405,15 @@
<member name="debug/gdscript/warnings/function_used_as_property" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when using a function as if it is a property.
</member>
<member name="debug/gdscript/warnings/get_node_default_without_onready" type="int" setter="" getter="" default="2">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when [method Node.get_node] (or the shorthand [code]$[/code]) is used as default value of a class variable without the [code]@onready[/code] annotation.
</member>
<member name="debug/gdscript/warnings/incompatible_ternary" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a ternary operator may emit values with incompatible types.
</member>
<member name="debug/gdscript/warnings/inference_on_variant" type="int" setter="" getter="" default="2">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a static inferred type uses a [Variant] as initial value, which makes the static type to also be Variant.
</member>
<member name="debug/gdscript/warnings/int_as_enum_without_cast" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when trying to use an integer as an enum without an explicit cast.
</member>
Expand All @@ -420,6 +426,12 @@
<member name="debug/gdscript/warnings/narrowing_conversion" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when passing a floating-point value to a function that expects an integer (it will be converted and lose precision).
</member>
<member name="debug/gdscript/warnings/native_method_override" type="int" setter="" getter="" default="2">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when a method in the script overrides a native method, because it may not behave as expected.
</member>
<member name="debug/gdscript/warnings/onready_with_export" type="int" setter="" getter="" default="2">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when the [code]@onready[/code] annotation is used together with the [code]@export[/code] annotation, since it may not behave as expected.
</member>
<member name="debug/gdscript/warnings/property_used_as_function" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when using a property as if it is a function.
</member>
Expand Down Expand Up @@ -477,7 +489,7 @@
<member name="debug/gdscript/warnings/unsafe_property_access" type="int" setter="" getter="" default="0">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when accessing a property whose presence is not guaranteed at compile-time in the class.
</member>
<member name="debug/gdscript/warnings/unsafe_void_return" type="int" setter="" getter="" default="0">
<member name="debug/gdscript/warnings/unsafe_void_return" type="int" setter="" getter="" default="1">
When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when returning a call from a [code]void[/code] function when such call cannot be guaranteed to be also [code]void[/code].
</member>
<member name="debug/gdscript/warnings/unused_local_constant" type="int" setter="" getter="" default="1">
Expand Down
173 changes: 143 additions & 30 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,25 @@ static GDScriptParser::DataType make_enum_type(const StringName &p_enum_name, co
}

static GDScriptParser::DataType make_native_enum_type(const StringName &p_enum_name, const StringName &p_native_class, const bool p_meta = true) {
GDScriptParser::DataType type = make_enum_type(p_enum_name, p_native_class, p_meta);
// Find out which base class declared the enum, so the name is always the same even when coming from other contexts.
StringName native_base = p_native_class;
while (true && native_base != StringName()) {
if (ClassDB::has_enum(native_base, p_enum_name, true)) {
break;
}
native_base = ClassDB::get_parent_class_nocheck(native_base);
}

GDScriptParser::DataType type = make_enum_type(p_enum_name, native_base, p_meta);
if (p_meta) {
type.builtin_type = Variant::NIL; // Native enum types are not Dictionaries
}

List<StringName> enum_values;
ClassDB::get_enum_constants(p_native_class, p_enum_name, &enum_values);
ClassDB::get_enum_constants(native_base, p_enum_name, &enum_values, true);

for (const StringName &E : enum_values) {
type.enum_values[E] = ClassDB::get_integer_constant(p_native_class, E);
type.enum_values[E] = ClassDB::get_integer_constant(native_base, E);
}

return type;
Expand Down Expand Up @@ -782,6 +794,22 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
resolving_datatype.kind = GDScriptParser::DataType::RESOLVING;

{
#ifdef DEBUG_ENABLED
HashSet<GDScriptWarning::Code> previously_ignored_warnings = parser->ignored_warnings;
GDScriptParser::Node *member_node = member.get_source_node();
if (member_node && member_node->type != GDScriptParser::Node::ANNOTATION) {
// Apply @warning_ignore annotations before resolving member.
for (GDScriptParser::AnnotationNode *&E : member_node->annotations) {
if (E->name == SNAME("@warning_ignore")) {
resolve_annotation(E);
E->apply(parser, member.variable);
}
}
for (GDScriptWarning::Code ignored_warning : member_node->ignored_warnings) {
parser->ignored_warnings.insert(ignored_warning);
}
}
#endif
switch (member.type) {
case GDScriptParser::ClassNode::Member::VARIABLE: {
check_class_member_name_conflict(p_class, member.variable->identifier->name, member.variable);
Expand All @@ -790,9 +818,16 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,

// Apply annotations.
for (GDScriptParser::AnnotationNode *&E : member.variable->annotations) {
resolve_annotation(E);
E->apply(parser, member.variable);
if (E->name != SNAME("@warning_ignore")) {
resolve_annotation(E);
E->apply(parser, member.variable);
}
}
#ifdef DEBUG_ENABLED
if (member.variable->exported && member.variable->onready) {
parser->push_warning(member.variable, GDScriptWarning::ONREADY_WITH_EXPORT);
}
#endif
} break;
case GDScriptParser::ClassNode::Member::CONSTANT: {
check_class_member_name_conflict(p_class, member.constant->identifier->name, member.constant);
Expand Down Expand Up @@ -878,6 +913,10 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
}
} break;
case GDScriptParser::ClassNode::Member::FUNCTION:
for (GDScriptParser::AnnotationNode *&E : member.function->annotations) {
resolve_annotation(E);
E->apply(parser, member.function);
}
resolve_function_signature(member.function, p_source);
break;
case GDScriptParser::ClassNode::Member::ENUM_VALUE: {
Expand Down Expand Up @@ -931,6 +970,9 @@ void GDScriptAnalyzer::resolve_class_member(GDScriptParser::ClassNode *p_class,
ERR_PRINT("Trying to resolve undefined member.");
break;
}
#ifdef DEBUG_ENABLED
parser->ignored_warnings = previously_ignored_warnings;
#endif
}

parser->current_class = previous_class;
Expand Down Expand Up @@ -1059,19 +1101,7 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co
resolve_annotation(E);
E->apply(parser, member.function);
}

#ifdef DEBUG_ENABLED
HashSet<uint32_t> previously_ignored = parser->ignored_warning_codes;
for (uint32_t ignored_warning : member.function->ignored_warnings) {
parser->ignored_warning_codes.insert(ignored_warning);
}
#endif // DEBUG_ENABLED

resolve_function_body(member.function);

#ifdef DEBUG_ENABLED
parser->ignored_warning_codes = previously_ignored;
#endif // DEBUG_ENABLED
} else if (member.type == GDScriptParser::ClassNode::Member::VARIABLE && member.variable->property != GDScriptParser::VariableNode::PROP_NONE) {
if (member.variable->property == GDScriptParser::VariableNode::PROP_INLINE) {
if (member.variable->getter != nullptr) {
Expand Down Expand Up @@ -1102,9 +1132,9 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co
GDScriptParser::ClassNode::Member member = p_class->members[i];
if (member.type == GDScriptParser::ClassNode::Member::VARIABLE) {
#ifdef DEBUG_ENABLED
HashSet<uint32_t> previously_ignored = parser->ignored_warning_codes;
for (uint32_t ignored_warning : member.function->ignored_warnings) {
parser->ignored_warning_codes.insert(ignored_warning);
HashSet<GDScriptWarning::Code> previously_ignored_warnings = parser->ignored_warnings;
for (GDScriptWarning::Code ignored_warning : member.variable->ignored_warnings) {
parser->ignored_warnings.insert(ignored_warning);
}
if (member.variable->usages == 0 && String(member.variable->identifier->name).begins_with("_")) {
parser->push_warning(member.variable->identifier, GDScriptWarning::UNUSED_PRIVATE_CLASS_VARIABLE, member.variable->identifier->name);
Expand Down Expand Up @@ -1179,7 +1209,7 @@ void GDScriptAnalyzer::resolve_class_body(GDScriptParser::ClassNode *p_class, co
}
}
#ifdef DEBUG_ENABLED
parser->ignored_warning_codes = previously_ignored;
parser->ignored_warnings = previously_ignored_warnings;
#endif // DEBUG_ENABLED
}
}
Expand Down Expand Up @@ -1289,6 +1319,11 @@ void GDScriptAnalyzer::resolve_node(GDScriptParser::Node *p_node, bool p_is_root
void GDScriptAnalyzer::resolve_annotation(GDScriptParser::AnnotationNode *p_annotation) {
ERR_FAIL_COND_MSG(!parser->valid_annotations.has(p_annotation->name), vformat(R"(Annotation "%s" not found to validate.)", p_annotation->name));

if (p_annotation->is_resolved) {
return;
}
p_annotation->is_resolved = true;

const MethodInfo &annotation_info = parser->valid_annotations[p_annotation->name].info;

const List<PropertyInfo>::Element *E = annotation_info.arguments.front();
Expand Down Expand Up @@ -1355,6 +1390,13 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
}
p_function->resolved_signature = true;

#ifdef DEBUG_ENABLED
HashSet<GDScriptWarning::Code> previously_ignored_warnings = parser->ignored_warnings;
for (GDScriptWarning::Code ignored_warning : p_function->ignored_warnings) {
parser->ignored_warnings.insert(ignored_warning);
}
#endif

GDScriptParser::FunctionNode *previous_function = parser->current_function;
parser->current_function = p_function;

Expand Down Expand Up @@ -1421,7 +1463,8 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
int default_par_count = 0;
bool is_static = false;
bool is_vararg = false;
if (!p_is_lambda && get_function_signature(p_function, false, base_type, function_name, parent_return_type, parameters_types, default_par_count, is_static, is_vararg)) {
StringName native_base;
if (!p_is_lambda && get_function_signature(p_function, false, base_type, function_name, parent_return_type, parameters_types, default_par_count, is_static, is_vararg, &native_base)) {
bool valid = p_function->is_static == is_static;
valid = valid && parent_return_type == p_function->get_datatype();

Expand All @@ -1447,8 +1490,8 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
parameter = "Variant";
}
parent_signature += parameter;
if (j == parameters_types.size() - default_par_count) {
parent_signature += " = default";
if (j >= parameters_types.size() - default_par_count) {
parent_signature += " = <default>";
}

j++;
Expand All @@ -1464,6 +1507,11 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *

push_error(vformat(R"(The function signature doesn't match the parent. Parent signature is "%s".)", parent_signature), p_function);
}
#ifdef DEBUG_ENABLED
if (native_base != StringName()) {
parser->push_warning(p_function, GDScriptWarning::NATIVE_METHOD_OVERRIDE, function_name, native_base);
}
#endif
}
#endif // TOOLS_ENABLED
}
Expand All @@ -1472,6 +1520,9 @@ void GDScriptAnalyzer::resolve_function_signature(GDScriptParser::FunctionNode *
p_function->set_datatype(prev_datatype);
}

#ifdef DEBUG_ENABLED
parser->ignored_warnings = previously_ignored_warnings;
#endif
parser->current_function = previous_function;
}

Expand All @@ -1481,6 +1532,13 @@ void GDScriptAnalyzer::resolve_function_body(GDScriptParser::FunctionNode *p_fun
}
p_function->resolved_body = true;

#ifdef DEBUG_ENABLED
HashSet<GDScriptWarning::Code> previously_ignored_warnings = parser->ignored_warnings;
for (GDScriptWarning::Code ignored_warning : p_function->ignored_warnings) {
parser->ignored_warnings.insert(ignored_warning);
}
#endif

GDScriptParser::FunctionNode *previous_function = parser->current_function;
parser->current_function = p_function;

Expand All @@ -1498,6 +1556,9 @@ void GDScriptAnalyzer::resolve_function_body(GDScriptParser::FunctionNode *p_fun
}
}

#ifdef DEBUG_ENABLED
parser->ignored_warnings = previously_ignored_warnings;
#endif
parser->current_function = previous_function;
}

Expand Down Expand Up @@ -1538,16 +1599,16 @@ void GDScriptAnalyzer::resolve_suite(GDScriptParser::SuiteNode *p_suite) {
}

#ifdef DEBUG_ENABLED
HashSet<uint32_t> previously_ignored = parser->ignored_warning_codes;
for (uint32_t ignored_warning : stmt->ignored_warnings) {
parser->ignored_warning_codes.insert(ignored_warning);
HashSet<GDScriptWarning::Code> previously_ignored_warnings = parser->ignored_warnings;
for (GDScriptWarning::Code ignored_warning : stmt->ignored_warnings) {
parser->ignored_warnings.insert(ignored_warning);
}
#endif // DEBUG_ENABLED

resolve_node(stmt);

#ifdef DEBUG_ENABLED
parser->ignored_warning_codes = previously_ignored;
parser->ignored_warnings = previously_ignored_warnings;
#endif // DEBUG_ENABLED

decide_suite_type(p_suite, stmt);
Expand Down Expand Up @@ -1599,6 +1660,11 @@ void GDScriptAnalyzer::resolve_assignable(GDScriptParser::AssignableNode *p_assi
} else if (initializer_type.kind == GDScriptParser::DataType::BUILTIN && initializer_type.builtin_type == Variant::NIL && !is_constant) {
push_error(vformat(R"(Cannot infer the type of "%s" %s because the value is "null".)", p_assignable->identifier->name, p_kind), p_assignable->initializer);
}
#ifdef DEBUG_ENABLED
if (initializer_type.is_hard_type() && initializer_type.is_variant()) {
parser->push_warning(p_assignable, GDScriptWarning::INFERENCE_ON_VARIANT, p_kind);
}
#endif
} else {
if (!initializer_type.is_set()) {
push_error(vformat(R"(Could not resolve type for %s "%s".)", p_kind, p_assignable->identifier->name), p_assignable->initializer);
Expand Down Expand Up @@ -1658,6 +1724,32 @@ void GDScriptAnalyzer::resolve_variable(GDScriptParser::VariableNode *p_variable
}

is_shadowing(p_variable->identifier, kind);
} else {
// Check if it is call to get_node() on self (using shorthand $ or not), so we can check if @onready is needed.
if (p_variable->initializer && (p_variable->initializer->type == GDScriptParser::Node::GET_NODE || p_variable->initializer->type == GDScriptParser::Node::CALL)) {
bool is_get_node = p_variable->initializer->type == GDScriptParser::Node::GET_NODE;
bool is_using_shorthand = is_get_node;
if (!is_get_node) {
is_using_shorthand = false;
GDScriptParser::CallNode *call = static_cast<GDScriptParser::CallNode *>(p_variable->initializer);
if (call->function_name == SNAME("get_node")) {
switch (call->get_callee_type()) {
case GDScriptParser::Node::IDENTIFIER: {
is_get_node = true;
} break;
case GDScriptParser::Node::SUBSCRIPT: {
GDScriptParser::SubscriptNode *subscript = static_cast<GDScriptParser::SubscriptNode *>(call->callee);
is_get_node = subscript->is_attribute && subscript->base->type == GDScriptParser::Node::SELF;
} break;
default:
break;
}
}
}
if (is_get_node) {
parser->push_warning(p_variable, GDScriptWarning::GET_NODE_DEFAULT_WITHOUT_ONREADY, is_using_shorthand ? "$" : "get_node()");
}
}
}
#endif
}
Expand Down Expand Up @@ -2931,7 +3023,11 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a

// Enums do not have functions other than the built-in dictionary ones.
if (base_type.kind == GDScriptParser::DataType::ENUM && base_type.is_meta_type) {
push_error(vformat(R"*(Enums only have Dictionary built-in methods. Function "%s()" does not exist for enum "%s".)*", p_call->function_name, base_type.enum_type), p_call->callee);
if (base_type.builtin_type == Variant::DICTIONARY) {
push_error(vformat(R"*(Enums only have Dictionary built-in methods. Function "%s()" does not exist for enum "%s".)*", p_call->function_name, base_type.enum_type), p_call->callee);
} else {
push_error(vformat(R"*(The native enum "%s" does not behave like Dictionary and does not have methods of its own.)*", base_type.enum_type), p_call->callee);
}
} else if (!p_call->is_super && callee_type != GDScriptParser::Node::NONE) { // Check if the name exists as something else.
GDScriptParser::IdentifierNode *callee_id;
if (callee_type == GDScriptParser::Node::IDENTIFIER) {
Expand Down Expand Up @@ -4302,15 +4398,26 @@ GDScriptParser::DataType GDScriptAnalyzer::type_from_property(const PropertyInfo
}
elem_type.is_constant = false;
result.set_container_element_type(elem_type);
} else if (p_property.type == Variant::INT) {
// Check if it's enum.
if (p_property.class_name != StringName()) {
Vector<String> names = String(p_property.class_name).split(".");
if (names.size() == 2) {
result = make_native_enum_type(names[1], names[0]);
}
}
}
}
return result;
}

bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bool p_is_constructor, GDScriptParser::DataType p_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 GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bool p_is_constructor, GDScriptParser::DataType p_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, StringName *r_native_class) {
r_static = false;
r_vararg = false;
r_default_arg_count = 0;
if (r_native_class) {
*r_native_class = StringName();
}
StringName function_name = p_function;

bool was_enum = false;
Expand Down Expand Up @@ -4445,6 +4552,12 @@ bool GDScriptAnalyzer::get_function_signature(GDScriptParser::Node *p_source, bo
if (valid && Engine::get_singleton()->has_singleton(base_native)) {
r_static = true;
}
#ifdef DEBUG_ENABLED
MethodBind *native_method = ClassDB::get_method(base_native, function_name);
if (native_method && r_native_class) {
*r_native_class = native_method->get_instance_class();
}
#endif
return valid;
}

Expand Down
Loading

0 comments on commit a166833

Please sign in to comment.