GDScript: Add CONFUSABLE_TEMPORARY_MODIFICATION warning

doc/classes/ProjectSettings.xml

@@ -556,6 +556,9 @@
 		<member name="debug/gdscript/warnings/confusable_local_usage" type="int" setter="" getter="" default="1">
 			When set to [b]Warn[/b] or [b]Error[/b], produces a warning or an error respectively when an identifier that will be shadowed below in the block is used.
 		</member>
+		<member name="debug/gdscript/warnings/confusable_temporary_modification" type="int" setter="" getter="" default="1">
+			When set to [b]Warn[/b] or [b]Error[/b], produces a warning or an error respectively when a built-in property of type [code]Packed*Array[/code] is modified using a complex assignment chain or a non-[code]const[/code] method call. In this case, you are only modifying a temporary value, and the property's value remains unchanged.
+		</member>
 		<member name="debug/gdscript/warnings/deprecated_keyword" type="int" setter="" getter="" default="1">
 			When set to [b]Warn[/b] or [b]Error[/b], produces a warning or an error respectively when deprecated keywords are used.
 			[b]Note:[/b] There are currently no deprecated keywords, so this warning is never produced.

modules/gdscript/gdscript_analyzer.cpp

@@ -2905,6 +2905,8 @@ void GDScriptAnalyzer::reduce_assignment(GDScriptParser::AssignmentNode *p_assig
 			}
 		}
 	}
+
+	warn_confusable_temporary_modification(p_assignment);
 #endif // DEBUG_ENABLED
 
 	if (p_assignment->assigned_value == nullptr || p_assignment->assignee == nullptr) {
@@ -3586,6 +3588,10 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 			base_type = subscript->base->get_datatype();
 			is_self = subscript->base->type == GDScriptParser::Node::SELF;
 		}
+
+#ifdef DEBUG_ENABLED
+		warn_confusable_temporary_modification(subscript);
+#endif // DEBUG_ENABLED
 	} else {
 		// Invalid call. Error already sent in parser.
 		// TODO: Could check if Callable here too.
@@ -5127,6 +5133,10 @@ void GDScriptAnalyzer::reduce_subscript(GDScriptParser::SubscriptNode *p_subscri
 	}
 
 	p_subscript->set_datatype(result_type);
+
+#ifdef DEBUG_ENABLED
+	warn_confusable_temporary_modification(p_subscript);
+#endif // DEBUG_ENABLED
 }
 
 void GDScriptAnalyzer::reduce_ternary_op(GDScriptParser::TernaryOpNode *p_ternary_op, bool p_is_root) {
@@ -6137,6 +6147,7 @@ void GDScriptAnalyzer::validate_call_arg(const List<GDScriptParser::DataType> &p
 }
 
 #ifdef DEBUG_ENABLED
+
 void GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_identifier, const String &p_context, const bool p_in_local_scope) {
 	const StringName &name = p_identifier->name;
 
@@ -6215,6 +6226,62 @@ void GDScriptAnalyzer::is_shadowing(GDScriptParser::IdentifierNode *p_identifier
 		native_base_class = ClassDB::get_parent_class(native_base_class);
 	}
 }
+
+void GDScriptAnalyzer::warn_confusable_temporary_modification(GDScriptParser::ExpressionNode *p_expression) {
+	ERR_FAIL_NULL(p_expression);
+
+	GDScriptParser::ExpressionNode *current = nullptr;
+	if (p_expression->type == GDScriptParser::Node::ASSIGNMENT) {
+		current = static_cast<GDScriptParser::AssignmentNode *>(p_expression)->assignee;
+	} else if (p_expression->type == GDScriptParser::Node::SUBSCRIPT) {
+		current = static_cast<GDScriptParser::SubscriptNode *>(p_expression);
+	} else {
+		ERR_FAIL_MSG("GDScript bug: Invalid expression type.");
+	}
+
+	while (current && current->type == GDScriptParser::Node::SUBSCRIPT) {
+		GDScriptParser::SubscriptNode *subscript = static_cast<GDScriptParser::SubscriptNode *>(current);
+		GDScriptParser::ExpressionNode *base = subscript->base;
+		if (base && base->datatype.is_typed_container_type() && !base->datatype.is_meta_type) {
+			GDScriptParser::IdentifierNode *base_id = nullptr;
+			GDScriptParser::DataType scope_type;
+			if (base->type == GDScriptParser::Node::IDENTIFIER) {
+				base_id = static_cast<GDScriptParser::IdentifierNode *>(base);
+				scope_type = type_from_metatype(parser->current_class->datatype);
+			} else if (base->type == GDScriptParser::Node::SUBSCRIPT) {
+				GDScriptParser::SubscriptNode *base_subscript = static_cast<GDScriptParser::SubscriptNode *>(base);
+				if (base_subscript->is_attribute) {
+					base_id = base_subscript->attribute;
+					if (base_subscript->base) {
+						scope_type = base_subscript->base->datatype;
+					}
+				}
+			}
+
+			if (base_id && base_id->source == GDScriptParser::IdentifierNode::INHERITED_VARIABLE) {
+				const StringName &scope_native_type = scope_type.native_type;
+				if (class_exists(scope_native_type) && ClassDB::has_property(scope_native_type, base_id->name)) {
+					if (p_expression->type == GDScriptParser::Node::ASSIGNMENT) {
+						parser->push_warning(p_expression, GDScriptWarning::CONFUSABLE_TEMPORARY_MODIFICATION, scope_native_type, base_id->name);
+					} else if (p_expression->type == GDScriptParser::Node::SUBSCRIPT) {
+						StringName member;
+						if (subscript->is_attribute && subscript->attribute) {
+							member = subscript->attribute->name;
+						}
+
+						const Variant::Type builtin_type = base->datatype.builtin_type;
+						if (member && Variant::has_builtin_method(builtin_type, member) && !Variant::is_builtin_method_const(builtin_type, member)) {
+							parser->push_warning(subscript, GDScriptWarning::CONFUSABLE_TEMPORARY_MODIFICATION, scope_native_type, base_id->name, member);
+						}
+					}
+				}
+			}
+		}
+
+		current = base;
+	}
+}
+
 #endif // DEBUG_ENABLED
 
 GDScriptParser::DataType GDScriptAnalyzer::get_operation_type(Variant::Operator p_operation, const GDScriptParser::DataType &p_a, bool &r_valid, const GDScriptParser::Node *p_source) {

modules/gdscript/gdscript_analyzer.h

@@ -157,9 +157,11 @@ class GDScriptAnalyzer {
 	Ref<GDScriptParserRef> find_cached_external_parser_for_class(const GDScriptParser::ClassNode *p_class, const Ref<GDScriptParserRef> &p_dependant_parser);
 	Ref<GDScriptParserRef> find_cached_external_parser_for_class(const GDScriptParser::ClassNode *p_class, GDScriptParser *p_dependant_parser);
 	Ref<GDScript> get_depended_shallow_script(const String &p_path, Error &r_error);
+
 #ifdef DEBUG_ENABLED
 	void is_shadowing(GDScriptParser::IdentifierNode *p_identifier, const String &p_context, const bool p_in_local_scope);
-#endif
+	void warn_confusable_temporary_modification(GDScriptParser::ExpressionNode *p_expression);
+#endif // DEBUG_ENABLED
 
 public:
 	Error resolve_inheritance();

modules/gdscript/gdscript_warning.cpp

@@ -154,6 +154,12 @@ String GDScriptWarning::get_message() const {
 		case CONFUSABLE_CAPTURE_REASSIGNMENT:
 			CHECK_SYMBOLS(1);
 			return vformat(R"(Reassigning lambda capture does not modify the outer local variable "%s".)", symbols[0]);
+		case CONFUSABLE_TEMPORARY_MODIFICATION:
+			CHECK_SYMBOLS(2);
+			if (symbols.size() > 2) {
+				return vformat(R"*(The built-in property "%s" will not be modified as a result of calling the method "%s()". Consider assigning the desired value to the property instead, or check the "%s" class for a specialized API.)*", symbols[1], symbols[2], symbols[0]);
+			}
+			return vformat(R"(The built-in property "%s" will not be modified in the assignment chain. Consider using a temporary variable and assigning it back instead, or check the "%s" class for a specialized API.)", symbols[1], symbols[0]);
 		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]);
@@ -238,6 +244,7 @@ String GDScriptWarning::get_name_from_code(Code p_code) {
 		PNAME("CONFUSABLE_LOCAL_DECLARATION"),
 		PNAME("CONFUSABLE_LOCAL_USAGE"),
 		PNAME("CONFUSABLE_CAPTURE_REASSIGNMENT"),
+		PNAME("CONFUSABLE_TEMPORARY_MODIFICATION"),
 		PNAME("INFERENCE_ON_VARIANT"),
 		PNAME("NATIVE_METHOD_OVERRIDE"),
 		PNAME("GET_NODE_DEFAULT_WITHOUT_ONREADY"),

modules/gdscript/gdscript_warning.h

@@ -87,6 +87,7 @@ public:
 		CONFUSABLE_LOCAL_DECLARATION, // The parent block declares an identifier with the same name below.
 		CONFUSABLE_LOCAL_USAGE, // The identifier will be shadowed below in the block.
 		CONFUSABLE_CAPTURE_REASSIGNMENT, // Reassigning lambda capture does not modify the outer local variable.
+		CONFUSABLE_TEMPORARY_MODIFICATION, // Modifying a temporary value in a complex assignment chain or calling a non-`const` method.
 		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.
@@ -145,6 +146,7 @@ public:
 		WARN, // CONFUSABLE_LOCAL_DECLARATION
 		WARN, // CONFUSABLE_LOCAL_USAGE
 		WARN, // CONFUSABLE_CAPTURE_REASSIGNMENT
+		WARN, // CONFUSABLE_TEMPORARY_MODIFICATION
 		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.

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

@@ -0,0 +1,49 @@
+class TestNativeProperty extends Line2D:
+	func _ready() -> void:
+		points = PackedVector2Array([Vector2()])
+		points[0] = Vector2.ONE # Warn.
+		points[0].x = 2.0 # Warn.
+
+		var _size := points.size()
+		points.clear() # Warn.
+		var _size_callable := points.size
+		var _clear_callable := points.clear # Warn.
+
+		var base: TestNativeProperty = self
+
+		base.points = PackedVector2Array([Vector2()])
+		base.points[0] = Vector2.ONE # Warn.
+		base.points[0].x = 2.0 # Warn.
+
+		var _base_size := base.points.size()
+		base.points.clear() # Warn.
+		var _base_size_callable := base.points.size
+		var _base_clear_callable := base.points.clear # Warn.
+
+# No warnings for a custom property.
+class TestCustomProperty extends Node:
+	var points: PackedVector2Array
+
+	func _ready() -> void:
+		points = PackedVector2Array([Vector2()])
+		points[0] = Vector2.ONE
+		points[0].x = 2.0
+
+		var _size := points.size()
+		points.clear()
+		var _size_callable := points.size
+		var _clear_callable := points.clear
+
+		var base: TestCustomProperty = self
+
+		base.points = PackedVector2Array([Vector2()])
+		base.points[0] = Vector2.ONE
+		base.points[0].x = 2.0
+
+		var _base_size := base.points.size()
+		base.points.clear()
+		var _base_size_callable := base.points.size
+		var _base_clear_callable := base.points.clear
+
+func test():
+	pass

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

@@ -0,0 +1,9 @@
+GDTEST_OK
+~~ WARNING at line 4: (CONFUSABLE_TEMPORARY_MODIFICATION) The built-in property "points" will not be modified in the assignment chain. Consider using a temporary variable and assigning it back instead, or check the "Line2D" class for a specialized API.
+~~ WARNING at line 5: (CONFUSABLE_TEMPORARY_MODIFICATION) The built-in property "points" will not be modified in the assignment chain. Consider using a temporary variable and assigning it back instead, or check the "Line2D" class for a specialized API.
+~~ WARNING at line 8: (CONFUSABLE_TEMPORARY_MODIFICATION) The built-in property "points" will not be modified as a result of calling the method "clear()". Consider assigning the desired value to the property instead, or check the "Line2D" class for a specialized API.
+~~ WARNING at line 10: (CONFUSABLE_TEMPORARY_MODIFICATION) The built-in property "points" will not be modified as a result of calling the method "clear()". Consider assigning the desired value to the property instead, or check the "Line2D" class for a specialized API.
+~~ WARNING at line 15: (CONFUSABLE_TEMPORARY_MODIFICATION) The built-in property "points" will not be modified in the assignment chain. Consider using a temporary variable and assigning it back instead, or check the "Line2D" class for a specialized API.
+~~ WARNING at line 16: (CONFUSABLE_TEMPORARY_MODIFICATION) The built-in property "points" will not be modified in the assignment chain. Consider using a temporary variable and assigning it back instead, or check the "Line2D" class for a specialized API.
+~~ WARNING at line 19: (CONFUSABLE_TEMPORARY_MODIFICATION) The built-in property "points" will not be modified as a result of calling the method "clear()". Consider assigning the desired value to the property instead, or check the "Line2D" class for a specialized API.
+~~ WARNING at line 21: (CONFUSABLE_TEMPORARY_MODIFICATION) The built-in property "points" will not be modified as a result of calling the method "clear()". Consider assigning the desired value to the property instead, or check the "Line2D" class for a specialized API.