Merge pull request #72608 from vnen/gdscript-warning-default-error

GDScript: Add warnings that are set to error by default (take 2)

doc/classes/ProjectSettings.xml

@@ -417,9 +417,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>
@@ -432,6 +438,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>
@@ -489,7 +501,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">

modules/gdscript/gdscript_analyzer.h

@@ -117,7 +117,7 @@ class GDScriptAnalyzer {
 	static GDScriptParser::DataType type_from_metatype(const GDScriptParser::DataType &p_meta_type);
 	GDScriptParser::DataType type_from_property(const PropertyInfo &p_property, bool p_is_arg = false, bool p_is_readonly = false) const;
 	GDScriptParser::DataType make_global_class_meta_type(const StringName &p_class_name, const GDScriptParser::Node *p_source);
-	bool get_function_signature(GDScriptParser::Node *p_source, bool p_is_constructor, 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 get_function_signature(GDScriptParser::Node *p_source, bool p_is_constructor, 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, StringName *r_native_class = nullptr);
 	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);
 	void validate_call_arg(const List<GDScriptParser::DataType> &p_par_types, int p_default_args_count, bool p_is_vararg, const GDScriptParser::CallNode *p_call);
 	void validate_call_arg(const MethodInfo &p_method, const GDScriptParser::CallNode *p_call);

modules/gdscript/gdscript_parser.cpp

@@ -158,14 +158,10 @@ void GDScriptParser::push_warning(const Node *p_source, GDScriptWarning::Code p_
 		return;
 	}
 
-	if (ignored_warning_codes.has(p_code)) {
+	if (ignored_warnings.has(p_code)) {
 		return;
 	}
 
-	String warn_name = GDScriptWarning::get_name_from_code((GDScriptWarning::Code)p_code).to_lower();
-	if (ignored_warnings.has(warn_name)) {
-		return;
-	}
 	int warn_level = (int)GLOBAL_GET(GDScriptWarning::get_settings_path_from_code(p_code));
 	if (!warn_level) {
 		return;
@@ -180,7 +176,7 @@ void GDScriptParser::push_warning(const Node *p_source, GDScriptWarning::Code p_
 	warning.rightmost_column = p_source->rightmost_column;
 
 	if (warn_level == GDScriptWarning::WarnLevel::ERROR || bool(GLOBAL_GET("debug/gdscript/warnings/treat_warnings_as_errors"))) {
-		push_error(warning.get_message(), p_source);
+		push_error(warning.get_message() + String(" (Warning treated as error.)"), p_source);
 		return;
 	}
 
@@ -3548,7 +3544,11 @@ const GDScriptParser::SuiteNode::Local &GDScriptParser::SuiteNode::get_local(con
 	return empty;
 }
 
-bool GDScriptParser::AnnotationNode::apply(GDScriptParser *p_this, Node *p_target) const {
+bool GDScriptParser::AnnotationNode::apply(GDScriptParser *p_this, Node *p_target) {
+	if (is_applied) {
+		return true;
+	}
+	is_applied = true;
 	return (p_this->*(p_this->valid_annotations[name].apply))(this, p_target);
 }
 

modules/gdscript/gdscript_parser.h

@@ -299,7 +299,9 @@ class GDScriptParser {
 		int leftmost_column = 0, rightmost_column = 0;
 		Node *next = nullptr;
 		List<AnnotationNode *> annotations;
-		Vector<uint32_t> ignored_warnings;
+#ifdef DEBUG_ENABLED
+		Vector<GDScriptWarning::Code> ignored_warnings;
+#endif
 
 		DataType datatype;
 
@@ -331,8 +333,10 @@ class GDScriptParser {
 
 		AnnotationInfo *info = nullptr;
 		PropertyInfo export_info;
+		bool is_resolved = false;
+		bool is_applied = false;
 
-		bool apply(GDScriptParser *p_this, Node *p_target) const;
+		bool apply(GDScriptParser *p_this, Node *p_target);
 		bool applies_to(uint32_t p_target_kinds) const;
 
 		AnnotationNode() {
@@ -1265,8 +1269,7 @@ class GDScriptParser {
 #ifdef DEBUG_ENABLED
 	bool is_ignoring_warnings = false;
 	List<GDScriptWarning> warnings;
-	HashSet<String> ignored_warnings;
-	HashSet<uint32_t> ignored_warning_codes;
+	HashSet<GDScriptWarning::Code> ignored_warnings;
 	HashSet<int> unsafe_lines;
 #endif
 

modules/gdscript/gdscript_warning.cpp

@@ -170,6 +170,21 @@ String GDScriptWarning::get_message() const {
 		case RENAMED_IN_GD4_HINT: {
 			break; // Renamed identifier hint is taken care of by the GDScriptAnalyzer. No message needed here.
 		}
+		case INFERENCE_ON_VARIANT: {
+			CHECK_SYMBOLS(1);
+			return vformat("The %s type is being inferred from a Variant value, so it will be typed as Variant.", symbols[0]);
+		}
+		case NATIVE_METHOD_OVERRIDE: {
+			CHECK_SYMBOLS(2);
+			return vformat(R"(The method "%s" overrides a method from native class "%s". This won't be called by the engine and may not work as expected.)", symbols[0], symbols[1]);
+		}
+		case GET_NODE_DEFAULT_WITHOUT_ONREADY: {
+			CHECK_SYMBOLS(1);
+			return vformat(R"*(The default value is using "%s" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.)*", symbols[0]);
+		}
+		case ONREADY_WITH_EXPORT: {
+			return R"(The "@onready" annotation will make the default value to be set after the "@export" takes effect and will override it.)";
+		}
 		case WARNING_MAX:
 			break; // Can't happen, but silences warning
 	}
@@ -179,14 +194,8 @@ 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;
-	}
-	// Too spammy by default on common cases (connect, Tween, etc.).
-	if (p_code == RETURN_VALUE_DISCARDED) {
-		return WarnLevel::IGNORE;
-	}
-	return WarnLevel::WARN;
+	ERR_FAIL_INDEX_V_MSG(p_code, WARNING_MAX, WarnLevel::IGNORE, "Getting default value of invalid warning code.");
+	return default_warning_levels[p_code];
 }
 
 PropertyInfo GDScriptWarning::get_property_info(Code p_code) {
@@ -240,7 +249,11 @@ String GDScriptWarning::get_name_from_code(Code p_code) {
 		"INT_AS_ENUM_WITHOUT_MATCH",
 		"STATIC_CALLED_ON_INSTANCE",
 		"CONFUSABLE_IDENTIFIER",
-		"RENAMED_IN_GODOT_4_HINT"
+		"RENAMED_IN_GODOT_4_HINT",
+		"INFERENCE_ON_VARIANT",
+		"NATIVE_METHOD_OVERRIDE",
+		"GET_NODE_DEFAULT_WITHOUT_ONREADY",
+		"ONREADY_WITH_EXPORT",
 	};
 
 	static_assert((sizeof(names) / sizeof(*names)) == WARNING_MAX, "Amount of warning types don't match the amount of warning names.");

modules/gdscript/gdscript_warning.h

@@ -82,9 +82,58 @@ class GDScriptWarning {
 		STATIC_CALLED_ON_INSTANCE, // A static method was called on an instance of a class instead of on the class itself.
 		CONFUSABLE_IDENTIFIER, // The identifier contains misleading characters that can be confused. E.g. "usеr" (has Cyrillic "е" instead of Latin "e").
 		RENAMED_IN_GD4_HINT, // A variable or function that could not be found has been renamed in Godot 4
+		INFERENCE_ON_VARIANT, // The declaration uses type inference but the value is typed as Variant.
+		NATIVE_METHOD_OVERRIDE, // The script method overrides a native one, this may not work as intended.
+		GET_NODE_DEFAULT_WITHOUT_ONREADY, // A class variable uses `get_node()` (or the `$` notation) as its default value, but does not use the @onready annotation.
+		ONREADY_WITH_EXPORT, // The `@onready` annotation will set the value after `@export` which is likely not intended.
 		WARNING_MAX,
 	};
 
+	constexpr static WarnLevel default_warning_levels[] = {
+		WARN, // UNASSIGNED_VARIABLE
+		WARN, // UNASSIGNED_VARIABLE_OP_ASSIGN
+		WARN, // UNUSED_VARIABLE
+		WARN, // UNUSED_LOCAL_CONSTANT
+		WARN, // SHADOWED_VARIABLE
+		WARN, // SHADOWED_VARIABLE_BASE_CLASS
+		WARN, // UNUSED_PRIVATE_CLASS_VARIABLE
+		WARN, // UNUSED_PARAMETER
+		WARN, // UNREACHABLE_CODE
+		WARN, // UNREACHABLE_PATTERN
+		WARN, // STANDALONE_EXPRESSION
+		WARN, // NARROWING_CONVERSION
+		WARN, // INCOMPATIBLE_TERNARY
+		WARN, // UNUSED_SIGNAL
+		IGNORE, // RETURN_VALUE_DISCARDED // Too spammy by default on common cases (connect, Tween, etc.).
+		WARN, // PROPERTY_USED_AS_FUNCTION
+		WARN, // CONSTANT_USED_AS_FUNCTION
+		WARN, // FUNCTION_USED_AS_PROPERTY
+		WARN, // INTEGER_DIVISION
+		IGNORE, // UNSAFE_PROPERTY_ACCESS // Too common in untyped scenarios.
+		IGNORE, // UNSAFE_METHOD_ACCESS // Too common in untyped scenarios.
+		IGNORE, // UNSAFE_CAST // Too common in untyped scenarios.
+		IGNORE, // UNSAFE_CALL_ARGUMENT // Too common in untyped scenarios.
+		WARN, // UNSAFE_VOID_RETURN
+		WARN, // DEPRECATED_KEYWORD
+		WARN, // STANDALONE_TERNARY
+		WARN, // ASSERT_ALWAYS_TRUE
+		WARN, // ASSERT_ALWAYS_FALSE
+		WARN, // REDUNDANT_AWAIT
+		WARN, // EMPTY_FILE
+		WARN, // SHADOWED_GLOBAL_IDENTIFIER
+		WARN, // INT_AS_ENUM_WITHOUT_CAST
+		WARN, // INT_AS_ENUM_WITHOUT_MATCH
+		WARN, // STATIC_CALLED_ON_INSTANCE
+		WARN, // CONFUSABLE_IDENTIFIER
+		WARN, // RENAMED_IN_GD4_HINT
+		ERROR, // INFERENCE_ON_VARIANT // Most likely done by accident, usually inference is trying for a particular type.
+		ERROR, // NATIVE_METHOD_OVERRIDE // May not work as expected.
+		ERROR, // GET_NODE_DEFAULT_WITHOUT_ONREADY // May not work as expected.
+		ERROR, // ONREADY_WITH_EXPORT // May not work as expected.
+	};
+
+	static_assert((sizeof(default_warning_levels) / sizeof(default_warning_levels[0])) == WARNING_MAX, "Amount of default levels does not match the amount of warnings.");
+
 	Code code = WARNING_MAX;
 	int start_line = -1, end_line = -1;
 	int leftmost_column = -1, rightmost_column = -1;

modules/gdscript/tests/gdscript_test_runner.cpp

@@ -146,11 +146,11 @@ GDScriptTestRunner::GDScriptTestRunner(const String &p_source_dir, bool p_init_l
 		init_language(p_source_dir);
 	}
 #ifdef DEBUG_ENABLED
-	// Enable all warnings for GDScript, so we can test them.
+	// Set all warning levels to "Warn" in order to test them properly, even the ones that default to error.
 	ProjectSettings::get_singleton()->set_setting("debug/gdscript/warnings/enable", true);
 	for (int i = 0; i < (int)GDScriptWarning::WARNING_MAX; i++) {
-		String warning = GDScriptWarning::get_name_from_code((GDScriptWarning::Code)i).to_lower();
-		ProjectSettings::get_singleton()->set_setting("debug/gdscript/warnings/" + warning, true);
+		String warning_setting = GDScriptWarning::get_settings_path_from_code((GDScriptWarning::Code)i);
+		ProjectSettings::get_singleton()->set_setting(warning_setting, (int)GDScriptWarning::WARN);
 	}
 #endif
 

modules/gdscript/tests/scripts/analyzer/errors/function_dont_match_parent_signature_parameter_default_values.out

@@ -1,2 +1,2 @@
 GDTEST_ANALYZER_ERROR
-The function signature doesn't match the parent. Parent signature is "my_function(int = default) -> int".
+The function signature doesn't match the parent. Parent signature is "my_function(int = <default>) -> int".

modules/gdscript/tests/scripts/analyzer/features/allow_get_node_with_onready.gd

@@ -0,0 +1,18 @@
+extends Node
+
+@onready var shorthand = $Node
+@onready var call = get_node(^"Node")
+@onready var shorthand_with_cast = $Node as Node
+@onready var call_with_cast = get_node(^"Node") as Node
+
+func _init():
+	var node := Node.new()
+	node.name = "Node"
+	add_child(node)
+
+func test():
+	# Those are expected to be `null` since `_ready()` is never called on tests.
+	prints("shorthand", shorthand)
+	prints("call", call)
+	prints("shorthand_with_cast", shorthand_with_cast)
+	prints("call_with_cast", call_with_cast)

modules/gdscript/tests/scripts/analyzer/features/allow_get_node_with_onready.out

@@ -0,0 +1,5 @@
+GDTEST_OK
+shorthand <null>
+call <null>
+shorthand_with_cast <null>
+call_with_cast <null>

modules/gdscript/tests/scripts/analyzer/features/assign_to_native_enum_property.gd

@@ -0,0 +1,13 @@
+# https://github.com/godotengine/godot/issues/72501
+extends Node
+
+func test():
+	prints("before", process_mode)
+	process_mode = PROCESS_MODE_PAUSABLE
+	prints("after", process_mode)
+
+	var node := Node.new()
+	add_child(node)
+	prints("before", node.process_mode)
+	node.process_mode = PROCESS_MODE_PAUSABLE
+	prints("after", node.process_mode)

modules/gdscript/tests/scripts/analyzer/features/assign_to_native_enum_property.out

@@ -0,0 +1,5 @@
+GDTEST_OK
+before 0
+after 1
+before 0
+after 1

modules/gdscript/tests/scripts/analyzer/features/hard_variants.gd

@@ -2,16 +2,18 @@ func variant() -> Variant: return null
 
 var member_weak = variant()
 var member_typed: Variant = variant()
+@warning_ignore("inference_on_variant")
 var member_inferred := variant()
 
 func param_weak(param = variant()) -> void: print(param)
 func param_typed(param: Variant = variant()) -> void: print(param)
+@warning_ignore("inference_on_variant")
 func param_inferred(param := variant()) -> void: print(param)
 
 func return_untyped(): return variant()
 func return_typed() -> Variant: return variant()
 
-@warning_ignore("unused_variable")
+@warning_ignore("unused_variable", "inference_on_variant")
 func test() -> void:
 	var weak = variant()
 	var typed: Variant = variant()

modules/gdscript/tests/scripts/analyzer/warnings/get_node_without_onready.gd

@@ -0,0 +1,17 @@
+extends Node
+
+var add_node = do_add_node() # Hack to have one node on init and not fail at runtime.
+
+var shorthand = $Node
+var with_self = self.get_node(^"Node")
+var without_self = get_node(^"Node")
+var with_cast = get_node(^"Node") as Node
+var shorthand_with_cast = $Node as Node
+
+func test():
+	print("warn")
+
+func do_add_node():
+	var node = Node.new()
+	node.name = "Node"
+	add_child(node)

modules/gdscript/tests/scripts/analyzer/warnings/get_node_without_onready.out

@@ -0,0 +1,22 @@
+GDTEST_OK
+>> WARNING
+>> Line: 5
+>> GET_NODE_DEFAULT_WITHOUT_ONREADY
+>> The default value is using "$" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.
+>> WARNING
+>> Line: 6
+>> GET_NODE_DEFAULT_WITHOUT_ONREADY
+>> The default value is using "get_node()" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.
+>> WARNING
+>> Line: 7
+>> GET_NODE_DEFAULT_WITHOUT_ONREADY
+>> The default value is using "get_node()" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.
+>> WARNING
+>> Line: 8
+>> GET_NODE_DEFAULT_WITHOUT_ONREADY
+>> The default value is using "get_node()" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.
+>> WARNING
+>> Line: 9
+>> GET_NODE_DEFAULT_WITHOUT_ONREADY
+>> The default value is using "$" which won't return nodes in the scene tree before "_ready()" is called. Use the "@onready" annotation to solve this.
+warn

modules/gdscript/tests/scripts/analyzer/warnings/inference_with_variant.gd

@@ -0,0 +1,6 @@
+func test():
+	var inferred_with_variant := return_variant()
+	print(inferred_with_variant)
+
+func return_variant() -> Variant:
+	return "warn"

modules/gdscript/tests/scripts/analyzer/warnings/inference_with_variant.out

@@ -0,0 +1,6 @@
+GDTEST_OK
+>> WARNING
+>> Line: 2
+>> INFERENCE_ON_VARIANT
+>> The variable type is being inferred from a Variant value, so it will be typed as Variant.
+warn

modules/gdscript/tests/scripts/analyzer/warnings/onready_with_export.gd

@@ -0,0 +1,6 @@
+extends Node
+
+@onready @export var conflict = ""
+
+func test():
+	print("warn")

modules/gdscript/tests/scripts/analyzer/warnings/onready_with_export.out

@@ -0,0 +1,6 @@
+GDTEST_OK
+>> WARNING
+>> Line: 3
+>> ONREADY_WITH_EXPORT
+>> The "@onready" annotation will make the default value to be set after the "@export" takes effect and will override it.
+warn

modules/gdscript/tests/scripts/analyzer/warnings/overriding_native_method.gd

@@ -0,0 +1,5 @@
+func test():
+	print("warn")
+
+func get(_property: StringName) -> Variant:
+	return null

modules/gdscript/tests/scripts/analyzer/warnings/overriding_native_method.out

@@ -0,0 +1,6 @@
+GDTEST_OK
+>> WARNING
+>> Line: 4
+>> NATIVE_METHOD_OVERRIDE
+>> The method "get" overrides a method from native class "Object". This won't be called by the engine and may not work as expected.
+warn