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

Add an optional warning to type inferred variables and parameters #59428

Closed
wants to merge 1 commit into from
Closed

Add an optional warning to type inferred variables and parameters #59428

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions doc/classes/ProjectSettings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,20 +334,26 @@
<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_parameter_types" type="int" setter="" getter="" default="0">
If [code]enabled[/code] prints a warning or an error when a function parameter is not statically typed.
</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 or constant is not statically typed.
</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>
<member name="debug/gdscript/warnings/assert_always_true" type="int" setter="" getter="" default="1">
If [code]enabled[/code], prints a warning or an error when an [code]assert[/code] call always returns true.
</member>
<member name="debug/gdscript/warnings/constant_used_as_function" type="int" setter="" getter="" default="1">
If [code]enabled[/code], prints a warning or an error when a constant is used as a function.
If not set to [code]Ignore[/code], enables warnings when a constant is used as a function.
</member>
<member name="debug/gdscript/warnings/deprecated_keyword" type="int" setter="" getter="" default="1">
If [code]enabled[/code], prints a warning or an error when deprecated keywords are used.
If [code]true[/code], enables warnings when deprecated keywords are used.
</member>
<member name="debug/gdscript/warnings/empty_file" type="int" setter="" getter="" default="1">
If [code]enabled[/code], prints a warning or an error when an empty file is parsed.
If [code]true[/code], enables warnings when an empty file is parsed.
</member>
<member name="debug/gdscript/warnings/enable" type="bool" setter="" getter="" default="true">
If [code]true[/code], enables specific GDScript warnings (see [code]debug/gdscript/warnings/*[/code] settings). If [code]false[/code], disables all GDScript warnings.
Expand Down
45 changes: 45 additions & 0 deletions modules/gdscript/gdscript_analyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,13 @@ void GDScriptAnalyzer::resolve_class_interface(GDScriptParser::ClassNode *p_clas
datatype.type_source = GDScriptParser::DataType::ANNOTATED_INFERRED;
}
}
#ifdef DEBUG_ENABLED
if (GLOBAL_GET("debug/gdscript/type_inferencing/enforce_static_variable_types") && datatype.type_source != GDScriptParser::DataType::ANNOTATED_INFERRED) {
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

datatype.is_constant = false;
member.variable->set_datatype(datatype);
Expand Down Expand Up @@ -745,6 +752,7 @@ void GDScriptAnalyzer::resolve_class_interface(GDScriptParser::ClassNode *p_clas
}
}
}

datatype.is_constant = true;

member.constant->set_datatype(datatype);
Expand Down Expand Up @@ -1571,6 +1579,13 @@ void GDScriptAnalyzer::resolve_constant(GDScriptParser::ConstantNode *p_constant
type.type_source = GDScriptParser::DataType::ANNOTATED_INFERRED;
}

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

Expand Down Expand Up @@ -1726,6 +1741,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_parameter_types") && result.type_source != GDScriptParser::DataType::ANNOTATED_INFERRED) {
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 @@ -4186,6 +4209,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_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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,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
42 changes: 36 additions & 6 deletions modules/gdscript/gdscript_warning.cpp
View file
Open in desktop
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.)", symbols[0], symbols[1]);
jordi-star marked this conversation as resolved.
Show resolved Hide resolved
}
case ENFORCE_STATIC_PARAMETER_TYPES: {
CHECK_SYMBOLS(2);
return vformat(R"(%s '%s' does not have a static type defined.)", symbols[0], symbols[1]);
jordi-star marked this conversation as resolved.
Show resolved Hide resolved
}
case WARNING_MAX:
break; // Can't happen, but silences warning
}
Expand All @@ -164,15 +172,28 @@ 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_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) {
// Making this a separate function in case a warning needs different PropertyInfo in the future.
return PropertyInfo(Variant::INT, get_settings_path_from_code(p_code), PROPERTY_HINT_ENUM, "Ignore,Warn,Error");
switch (p_code) {
case ENFORCE_STATIC_VARIABLE_TYPES:
case ENFORCE_STATIC_PARAMETER_TYPES:
return PropertyInfo(Variant::INT, get_settings_path_from_code(p_code), PROPERTY_HINT_ENUM, "Disabled,Warn,Error");

default:
return PropertyInfo(Variant::INT, get_settings_path_from_code(p_code), PROPERTY_HINT_ENUM, "Ignore,Warn,Error");
}
}

String GDScriptWarning::get_name() const {
Expand Down Expand Up @@ -215,6 +236,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_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 +246,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_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
View file
Open in desktop
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_PARAMETER_TYPES,
WARNING_MAX,
};

Expand Down
3 changes: 3 additions & 0 deletions modules/gdscript/tests/gdscript_test_runner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ 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_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