Merge pull request #93691 from dalexeev/gds-confusable-capture-reassignment

GDScript: Add `CONFUSABLE_CAPTURE_REASSIGNMENT` warning

doc/classes/ProjectSettings.xml

@@ -477,6 +477,9 @@
 		<member name="debug/gdscript/warnings/assert_always_true" type="int" setter="" getter="" default="1">
 			When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when an [code]assert[/code] call always evaluates to true.
 		</member>
+		<member name="debug/gdscript/warnings/confusable_capture_reassignment" 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 local variable captured by a lambda is reassigned, since this does not modify the outer local variable.
+		</member>
 		<member name="debug/gdscript/warnings/confusable_identifier" type="int" setter="" getter="" default="1">
 			When set to [code]warn[/code] or [code]error[/code], produces a warning or an error respectively when an identifier contains characters that can be confused with something else, like when mixing different alphabets.
 		</member>

modules/gdscript/gdscript_analyzer.cpp

@@ -2663,6 +2663,21 @@ void GDScriptAnalyzer::reduce_assignment(GDScriptParser::AssignmentNode *p_assig
 
 	reduce_expression(p_assignment->assignee);
 
+#ifdef DEBUG_ENABLED
+	{
+		GDScriptParser::ExpressionNode *base = p_assignment->assignee;
+		while (base && base->type == GDScriptParser::Node::SUBSCRIPT) {
+			base = static_cast<GDScriptParser::SubscriptNode *>(base)->base;
+		}
+		if (base && base->type == GDScriptParser::Node::IDENTIFIER) {
+			GDScriptParser::IdentifierNode *id = static_cast<GDScriptParser::IdentifierNode *>(base);
+			if (current_lambda && current_lambda->captures_indices.has(id->name)) {
+				parser->push_warning(p_assignment, GDScriptWarning::CONFUSABLE_CAPTURE_REASSIGNMENT, id->name);
+			}
+		}
+	}
+#endif
+
 	if (p_assignment->assigned_value == nullptr || p_assignment->assignee == nullptr) {
 		return;
 	}

modules/gdscript/gdscript_warning.cpp

@@ -145,6 +145,9 @@ String GDScriptWarning::get_message() const {
 		case CONFUSABLE_LOCAL_USAGE:
 			CHECK_SYMBOLS(1);
 			return vformat(R"(The identifier "%s" will be shadowed below in the block.)", symbols[0]);
+		case CONFUSABLE_CAPTURE_REASSIGNMENT:
+			CHECK_SYMBOLS(1);
+			return vformat(R"(Reassigning lambda capture does not modify the outer local variable "%s".)", 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]);
@@ -231,6 +234,7 @@ String GDScriptWarning::get_name_from_code(Code p_code) {
 		"CONFUSABLE_IDENTIFIER",
 		"CONFUSABLE_LOCAL_DECLARATION",
 		"CONFUSABLE_LOCAL_USAGE",
+		"CONFUSABLE_CAPTURE_REASSIGNMENT",
 		"INFERENCE_ON_VARIANT",
 		"NATIVE_METHOD_OVERRIDE",
 		"GET_NODE_DEFAULT_WITHOUT_ONREADY",

modules/gdscript/gdscript_warning.h

@@ -85,6 +85,7 @@ public:
 		CONFUSABLE_IDENTIFIER, // The identifier contains misleading characters that can be confused. E.g. "usеr" (has Cyrillic "е" instead of Latin "e").
 		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.
 		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.
@@ -137,6 +138,7 @@ public:
 		WARN, // CONFUSABLE_IDENTIFIER
 		WARN, // CONFUSABLE_LOCAL_DECLARATION
 		WARN, // CONFUSABLE_LOCAL_USAGE
+		WARN, // CONFUSABLE_CAPTURE_REASSIGNMENT
 		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_capture_reassignment.gd

@@ -0,0 +1,27 @@
+var member := 1
+
+func test():
+	var number := 1
+	var string := "1"
+	var vector := Vector2i(1, 0)
+	var array_assign := [1]
+	var array_append := [1]
+	var f := func ():
+		member = 2
+		number = 2
+		string += "2"
+		vector.x = 2
+		array_assign = [2]
+		array_append.append(2)
+		var g := func ():
+			member = 3
+			number = 3
+			string += "3"
+			vector.x = 3
+			array_assign = [3]
+			array_append.append(3)
+			prints("g", member, number, string, vector, array_assign, array_append)
+		g.call()
+		prints("f", member, number, string, vector, array_assign, array_append)
+	f.call()
+	prints("test", member, number, string, vector, array_assign, array_append)

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

@@ -0,0 +1,36 @@
+GDTEST_OK
+>> WARNING
+>> Line: 11
+>> CONFUSABLE_CAPTURE_REASSIGNMENT
+>> Reassigning lambda capture does not modify the outer local variable "number".
+>> WARNING
+>> Line: 12
+>> CONFUSABLE_CAPTURE_REASSIGNMENT
+>> Reassigning lambda capture does not modify the outer local variable "string".
+>> WARNING
+>> Line: 13
+>> CONFUSABLE_CAPTURE_REASSIGNMENT
+>> Reassigning lambda capture does not modify the outer local variable "vector".
+>> WARNING
+>> Line: 14
+>> CONFUSABLE_CAPTURE_REASSIGNMENT
+>> Reassigning lambda capture does not modify the outer local variable "array_assign".
+>> WARNING
+>> Line: 18
+>> CONFUSABLE_CAPTURE_REASSIGNMENT
+>> Reassigning lambda capture does not modify the outer local variable "number".
+>> WARNING
+>> Line: 19
+>> CONFUSABLE_CAPTURE_REASSIGNMENT
+>> Reassigning lambda capture does not modify the outer local variable "string".
+>> WARNING
+>> Line: 20
+>> CONFUSABLE_CAPTURE_REASSIGNMENT
+>> Reassigning lambda capture does not modify the outer local variable "vector".
+>> WARNING
+>> Line: 21
+>> CONFUSABLE_CAPTURE_REASSIGNMENT
+>> Reassigning lambda capture does not modify the outer local variable "array_assign".
+g 3 3 123 (3, 0) [3] [1, 2, 3]
+f 3 2 12 (2, 0) [2] [1, 2, 3]
+test 3 1 1 (1, 0) [1] [1, 2, 3]

modules/gdscript/tests/scripts/parser/features/lambda_ends_with_new_line.gd

@@ -9,6 +9,7 @@ func four_parameters(_a, callable : Callable, b=func(): print(10)):
 
 func test():
     var v
+    @warning_ignore("confusable_capture_reassignment")
     v=func():v=1
     if true: v=1
     print(v)