Skip to content

Commit

Permalink
Add an optional warning to type inferred variables and params
Browse files Browse the repository at this point in the history
  • Loading branch information
jordi-star committed Mar 29, 2022
1 parent 94b7e38 commit 4f791d3
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 5 deletions.
18 changes: 17 additions & 1 deletion doc/classes/ProjectSettings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,12 @@
<member name="debug/file_logging/max_log_files" type="int" setter="" getter="" default="5">
Specifies the maximum amount of log files allowed (used for rotation).
</member>
<member name="debug/gdscript/type_inferencing/enforce_static_method_parameter_types" type="int" setter="" getter="" default="0">
If [code]enabled[/code], prints a warning or an error when a method parameter uses [url=https://en.wikipedia.org/wiki/Type_inference]type inferencing[/url].
</member>
<member name="debug/gdscript/type_inferencing/enforce_static_variable_types" type="int" setter="" getter="" default="0">
If [code]enabled[/code], prints a warning or an error when a variable uses [url=https://en.wikipedia.org/wiki/Type_inference]type inferencing[/url].
</member>
<member name="debug/gdscript/warnings/assert_always_false" type="int" setter="" getter="" default="1">
If [code]enabled[/code], prints a warning or an error when an [code]assert[/code] call always returns false.
</member>
Expand Down Expand Up @@ -374,7 +380,7 @@
If [code]enabled[/code], prints a warning or an error when using a property as if it was a function.
</member>
<member name="debug/gdscript/warnings/redundant_await" type="int" setter="" getter="" default="1">
If [code]enabled[/code], prints a warning or an error when a function that is not a coroutine is called with await.
If [code]enabled[/code], prints a warning or an error when a function that is not a coroutine is called with await.
</member>
<member name="debug/gdscript/warnings/return_value_discarded" type="int" setter="" getter="" default="1">
If [code]enabled[/code], prints a warning or an error when calling a function without using its return value (by assigning it to a variable or using it as a function argument). Such return values are sometimes used to denote possible errors using the [enum Error] enum.
Expand Down Expand Up @@ -1923,6 +1929,16 @@
</member>
<member name="rendering/vulkan/staging_buffer/texture_upload_region_size_px" type="int" setter="" getter="" default="64">
</member>
<member name="xr/openxr/default_action_map" type="String" setter="" getter="" default="&quot;res://default_action_map.tres&quot;">
</member>
<member name="xr/openxr/enabled" type="bool" setter="" getter="" default="false">
</member>
<member name="xr/openxr/form_factor" type="int" setter="" getter="" default="&quot;0&quot;">
</member>
<member name="xr/openxr/reference_space" type="int" setter="" getter="" default="&quot;1&quot;">
</member>
<member name="xr/openxr/view_configuration" type="int" setter="" getter="" default="&quot;1&quot;">
</member>
<member name="xr/shaders/enabled" type="bool" setter="" getter="" default="false">
If [code]true[/code], Godot will compile shaders required for XR.
</member>
Expand Down
46 changes: 46 additions & 0 deletions modules/gdscript/gdscript_analyzer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,14 @@ void GDScriptAnalyzer::resolve_class_interface(GDScriptParser::ClassNode *p_clas
}
}

#ifdef DEBUG_ENABLED
if (GLOBAL_GET("debug/gdscript/type_inferencing/enforce_static_variable_types")) {
if (!member.variable->datatype_specifier && !(datatype.kind == GDScriptParser::DataType::BUILTIN && datatype.builtin_type == Variant::NIL)) {
enforce_static_type((GDScriptParser::Node *)member.variable, member.variable->identifier->name);
}
}
#endif

// Check if initalizer is an unset identifier (ie: a variable within scope, but declared below)
if (member.variable->initializer && !member.variable->initializer->get_datatype().is_set()) {
if (member.variable->initializer->type == GDScriptParser::Node::IDENTIFIER) {
Expand Down Expand Up @@ -743,7 +751,15 @@ void GDScriptAnalyzer::resolve_class_interface(GDScriptParser::ClassNode *p_clas
#endif
}
}
#ifdef DEBUG_ENABLED
if (GLOBAL_GET("debug/gdscript/type_inferencing/enforce_static_variable_types")) {
if (!member.constant->datatype_specifier && !(datatype.kind == GDScriptParser::DataType::BUILTIN && datatype.builtin_type == Variant::NIL)) {
enforce_static_type((GDScriptParser::Node *)member.constant, member.constant->identifier->name);
}
}
#endif
}

datatype.is_constant = true;

member.constant->set_datatype(datatype);
Expand Down Expand Up @@ -1699,6 +1715,14 @@ void GDScriptAnalyzer::resolve_parameter(GDScriptParser::ParameterNode *p_parame
push_error(vformat(R"(Could not infer the type of the variable "%s" because the initial value is "null".)", p_parameter->identifier->name), p_parameter->default_value);
}

#ifdef DEBUG_ENABLED
if (GLOBAL_GET("debug/gdscript/type_inferencing/enforce_static_method_parameter_types")) {
if (!p_parameter->datatype_specifier && !(result.kind == GDScriptParser::DataType::BUILTIN && result.builtin_type == Variant::NIL)) {
enforce_static_type((GDScriptParser::Node *)p_parameter, p_parameter->identifier->name);
}
}
#endif

p_parameter->set_datatype(result);
}

Expand Down Expand Up @@ -4096,6 +4120,28 @@ void GDScriptAnalyzer::push_error(const String &p_message, const GDScriptParser:
parser->push_error(p_message, p_origin);
}

#ifdef DEBUG_ENABLED
void GDScriptAnalyzer::enforce_static_type(GDScriptParser::Node *p_node, String p_identifier) {
String identifier_type = "";
GDScriptWarning::Code warning_code = GDScriptWarning::Code::ENFORCE_STATIC_VARIABLE_TYPES;
switch (p_node->type) {
case GDScriptParser::Node::Type::CONSTANT:
identifier_type = "Constant";
break;
case GDScriptParser::Node::Type::VARIABLE:
identifier_type = "Variable";
break;
case GDScriptParser::Node::Type::PARAMETER:
identifier_type = "Parameter";
warning_code = GDScriptWarning::Code::ENFORCE_STATIC_METHOD_PARAMETER_TYPES;
break;
default: // Only enforce static types on Variables and parameters
return;
}
parser->push_warning(p_node, warning_code, identifier_type, p_identifier);
}
#endif

void GDScriptAnalyzer::mark_node_unsafe(const GDScriptParser::Node *p_node) {
#ifdef DEBUG_ENABLED
for (int i = p_node->start_line; i <= p_node->end_line; i++) {
Expand Down
1 change: 1 addition & 0 deletions modules/gdscript/gdscript_analyzer.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class GDScriptAnalyzer {
bool class_exists(const StringName &p_class) const;
Ref<GDScriptParserRef> get_parser_for(const String &p_path);
#ifdef DEBUG_ENABLED
void enforce_static_type(GDScriptParser::Node *p_node, String p_identifier);
bool is_shadowing(GDScriptParser::IdentifierNode *p_local, const String &p_context);
#endif

Expand Down
32 changes: 28 additions & 4 deletions modules/gdscript/gdscript_warning.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ String GDScriptWarning::get_message() const {
case INT_ASSIGNED_TO_ENUM: {
return "Integer used when an enum value is expected. If this is intended cast the integer to the enum type.";
}
case ENFORCE_STATIC_VARIABLE_TYPES: {
CHECK_SYMBOLS(2);
return vformat(R"(%s '%s' does not have a static type defined (Enforce Static Variable Types enabled in Project Settings).)", symbols[0], symbols[1]);
}
case ENFORCE_STATIC_METHOD_PARAMETER_TYPES: {
CHECK_SYMBOLS(2);
return vformat(R"(%s '%s' does not have a static type defined (Enforce Static Method Parameter Types enabled in Project Settings).)", symbols[0], symbols[1]);
}
case WARNING_MAX:
break; // Can't happen, but silences warning
}
Expand All @@ -164,10 +172,17 @@ String GDScriptWarning::get_message() const {
}

int GDScriptWarning::get_default_value(Code p_code) {
if (get_name_from_code(p_code).to_lower().begins_with("unsafe_")) {
return WarnLevel::IGNORE;
switch (p_code) {
case ENFORCE_STATIC_VARIABLE_TYPES:
case ENFORCE_STATIC_METHOD_PARAMETER_TYPES:
return WarnLevel::IGNORE;

default:
if (get_name_from_code(p_code).to_lower().begins_with("unsafe_")) {
return WarnLevel::IGNORE;
}
return WarnLevel::WARN;
}
return WarnLevel::WARN;
}

PropertyInfo GDScriptWarning::get_property_info(Code p_code) {
Expand Down Expand Up @@ -215,6 +230,8 @@ String GDScriptWarning::get_name_from_code(Code p_code) {
"EMPTY_FILE",
"SHADOWED_GLOBAL_IDENTIFIER",
"INT_ASSIGNED_TO_ENUM",
"ENFORCE_STATIC_VARIABLE_TYPES",
"ENFORCE_STATIC_METHOD_PARAMETER_TYPES"
};

static_assert((sizeof(names) / sizeof(*names)) == WARNING_MAX, "Amount of warning types don't match the amount of warning names.");
Expand All @@ -223,7 +240,14 @@ String GDScriptWarning::get_name_from_code(Code p_code) {
}

String GDScriptWarning::get_settings_path_from_code(Code p_code) {
return "debug/gdscript/warnings/" + get_name_from_code(p_code).to_lower();
switch (p_code) {
case ENFORCE_STATIC_VARIABLE_TYPES:
case ENFORCE_STATIC_METHOD_PARAMETER_TYPES:
return "debug/gdscript/type_inferencing/" + get_name_from_code(p_code).to_lower();

default:
return "debug/gdscript/warnings/" + get_name_from_code(p_code).to_lower();
}
}

GDScriptWarning::Code GDScriptWarning::get_code_from_name(const String &p_name) {
Expand Down
2 changes: 2 additions & 0 deletions modules/gdscript/gdscript_warning.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ class GDScriptWarning {
EMPTY_FILE, // A script file is empty.
SHADOWED_GLOBAL_IDENTIFIER, // A global class or function has the same name as variable.
INT_ASSIGNED_TO_ENUM, // An integer value was assigned to an enum-typed variable without casting.
ENFORCE_STATIC_VARIABLE_TYPES, // Disabled by default, some users would like to enforce static variable typing.
ENFORCE_STATIC_METHOD_PARAMETER_TYPES,
WARNING_MAX,
};

Expand Down
2 changes: 2 additions & 0 deletions modules/gdscript/tests/gdscript_test_runner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ GDScriptTestRunner::GDScriptTestRunner(const String &p_source_dir, bool p_init_l
// Enable all warnings for GDScript, so we can test them.
ProjectSettings::get_singleton()->set_setting("debug/gdscript/warnings/enable", true);
for (int i = 0; i < (int)GDScriptWarning::WARNING_MAX; i++) {
if (i == GDScriptWarning::ENFORCE_STATIC_VARIABLE_TYPES || i == GDScriptWarning::ENFORCE_STATIC_METHOD_PARAMETER_TYPES)
continue;
String warning = GDScriptWarning::get_name_from_code((GDScriptWarning::Code)i).to_lower();
ProjectSettings::get_singleton()->set_setting("debug/gdscript/warnings/" + warning, true);
}
Expand Down

0 comments on commit 4f791d3

Please sign in to comment.