Reimplement soft access restriction for members prefixed with _

Author: Lazy-Rabbit-2001

doc/classes/ProjectSettings.xml

@@ -530,12 +530,18 @@
 			Specifies the maximum number of log files allowed (used for rotation). Set to [code]1[/code] to disable log file rotation.
 			If the [code]--log-file <file>[/code] [url=$DOCS_URL/tutorials/editor/command_line_tutorial.html]command line argument[/url] is used, log rotation is always disabled.
 		</member>
+		<member name="debug/gdscript/warnings/access_private_member" 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 script's private member (property, method, or signal) is accessed from an external script that does not inherit the original script. A member is considered private when its name begins with an underscore ([code]_[/code]).
+		</member>
 		<member name="debug/gdscript/warnings/assert_always_false" 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 [code]assert[/code] call always evaluates to [code]false[/code].
 		</member>
 		<member name="debug/gdscript/warnings/assert_always_true" 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 [code]assert[/code] call always evaluates to [code]true[/code].
 		</member>
+		<member name="debug/gdscript/warnings/call_private_method" 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 script's private method is called from an external script that does not inherit the original script. A method is considered private when its name begins with an underscore ([code]_[/code]).
+		</member>
 		<member name="debug/gdscript/warnings/confusable_capture_reassignment" 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 local variable captured by a lambda is reassigned, since this does not modify the outer local variable.
 		</member>

modules/gdscript/gdscript_analyzer.cpp

@@ -2051,6 +2051,33 @@ void GDScriptAnalyzer::decide_suite_type(GDScriptParser::Node *p_suite, GDScript
 	}
 }
 
+#ifdef DEBUG_ENABLED
+void GDScriptAnalyzer::check_access_private_member(GDScriptParser::IdentifierNode *p_identifier, const GDScriptParser::DataType &p_datatype, const bool p_is_call) {
+	if (p_identifier == nullptr) {
+		return;
+	}
+	if (!String(p_identifier->name).begins_with("_")) {
+		return;
+	}
+	if (p_datatype.kind != GDScriptParser::DataType::Kind::CLASS && p_datatype.kind != GDScriptParser::DataType::Kind::SCRIPT) {
+		return; // Accessing from self
+	}
+	if (parser->current_function && parser->current_function->body && parser->current_function->body->has_local(p_identifier->name)) {
+		return; // Accessing local variable
+	}
+
+	GDScriptParser::DataType target_resolved = type_from_metatype(p_datatype);
+	if (target_resolved.class_type != nullptr && !target_resolved.class_type->has_member(p_identifier->name)) {
+		return; // Accessing an inexisting method
+	}
+	if (is_type_compatible(target_resolved, type_from_metatype(parser->current_class->get_datatype()))) {
+		return; // Not accessing from the external places
+	}
+
+	parser->push_warning(p_identifier, p_is_call ? GDScriptWarning::CALL_PRIVATE_METHOD : GDScriptWarning::ACCESS_PRIVATE_MEMBER, p_identifier->name);
+}
+#endif // DEBUG_ENABLED
+
 void GDScriptAnalyzer::resolve_suite(GDScriptParser::SuiteNode *p_suite) {
 	for (int i = 0; i < p_suite->statements.size(); i++) {
 		GDScriptParser::Node *stmt = p_suite->statements[i];
@@ -3560,10 +3587,19 @@ void GDScriptAnalyzer::reduce_call(GDScriptParser::CallNode *p_call, bool p_is_a
 		if (p_call->callee == nullptr && current_lambda != nullptr) {
 			push_error("Cannot use `super()` inside a lambda.", p_call);
 		}
+
+#ifdef DEBUG_ENABLED
+		GDScriptParser::IdentifierNode *identifier = static_cast<GDScriptParser::IdentifierNode *>(p_call->callee);
+		check_access_private_member(identifier, p_call->get_datatype(), true);
+#endif
 	} else if (callee_type == GDScriptParser::Node::IDENTIFIER) {
 		base_type = parser->current_class->get_datatype();
 		base_type.is_meta_type = false;
 		is_self = true;
+#ifdef DEBUG_ENABLED
+		GDScriptParser::IdentifierNode *identifier = static_cast<GDScriptParser::IdentifierNode *>(p_call->callee);
+		check_access_private_member(identifier, p_call->get_datatype(), true);
+#endif
 	} else if (callee_type == GDScriptParser::Node::SUBSCRIPT) {
 		GDScriptParser::SubscriptNode *subscript = static_cast<GDScriptParser::SubscriptNode *>(p_call->callee);
 		if (subscript->base == nullptr) {
@@ -3597,6 +3633,9 @@ 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
+		check_access_private_member(subscript->attribute, subscript->base->get_datatype(), true);
+#endif
 	} else {
 		// Invalid call. Error already sent in parser.
 		// TODO: Could check if Callable here too.
@@ -4536,6 +4575,10 @@ void GDScriptAnalyzer::reduce_identifier(GDScriptParser::IdentifierNode *p_ident
 			}
 		}
 
+#ifdef DEBUG_ENABLED
+		check_access_private_member(p_identifier, p_identifier->get_datatype());
+#endif
+
 		return;
 	}
 
@@ -4898,6 +4941,10 @@ void GDScriptAnalyzer::reduce_subscript(GDScriptParser::SubscriptNode *p_subscri
 			}
 			result_type.kind = GDScriptParser::DataType::VARIANT;
 		}
+
+#ifdef DEBUG_ENABLED
+		check_access_private_member(p_subscript->attribute, p_subscript->base->get_datatype());
+#endif
 	} else {
 		if (p_subscript->index == nullptr) {
 			return;

modules/gdscript/gdscript_analyzer.h

@@ -71,6 +71,10 @@ class GDScriptAnalyzer {
 
 	void decide_suite_type(GDScriptParser::Node *p_suite, GDScriptParser::Node *p_statement);
 
+#ifdef DEBUG_ENABLED
+	void check_access_private_member(GDScriptParser::IdentifierNode *p_identifier, const GDScriptParser::DataType &p_datatype, const bool p_is_call = false);
+#endif
+
 	void resolve_annotation(GDScriptParser::AnnotationNode *p_annotation);
 	void resolve_class_member(GDScriptParser::ClassNode *p_class, const StringName &p_name, const GDScriptParser::Node *p_source = nullptr);
 	void resolve_class_member(GDScriptParser::ClassNode *p_class, int p_index, const GDScriptParser::Node *p_source = nullptr);

modules/gdscript/gdscript_warning.cpp

@@ -164,6 +164,12 @@ String GDScriptWarning::get_message() const {
 			return vformat(R"*(The default value uses "%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"("@onready" will set the default value after "@export" takes effect and will override it.)";
+		case ACCESS_PRIVATE_MEMBER:
+			CHECK_SYMBOLS(1);
+			return vformat(R"(The member "%s" is private. It should not be accessed from an external script.)", symbols[0]);
+		case CALL_PRIVATE_METHOD:
+			CHECK_SYMBOLS(1);
+			return vformat(R"*(The method "%s()" is private. It should not be called from an external script.)*", symbols[0]);
 #ifndef DISABLE_DEPRECATED
 		// Never produced. These warnings migrated from 3.x by mistake.
 		case PROPERTY_USED_AS_FUNCTION: // There is already an error.
@@ -241,6 +247,8 @@ String GDScriptWarning::get_name_from_code(Code p_code) {
 		PNAME("NATIVE_METHOD_OVERRIDE"),
 		PNAME("GET_NODE_DEFAULT_WITHOUT_ONREADY"),
 		PNAME("ONREADY_WITH_EXPORT"),
+		PNAME("ACCESS_PRIVATE_MEMBER"),
+		PNAME("CALL_PRIVATE_METHOD"),
 #ifndef DISABLE_DEPRECATED
 		"PROPERTY_USED_AS_FUNCTION",
 		"CONSTANT_USED_AS_FUNCTION",

modules/gdscript/gdscript_warning.h

@@ -90,6 +90,8 @@ class GDScriptWarning {
 		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.
+		ACCESS_PRIVATE_MEMBER, // Accessing a private member from external places. E.g. accessing an `_`-prefixed member from other classes that are not derived from the class where the member is defined.
+		CALL_PRIVATE_METHOD, // Calling a private method from external places. E.g. calling an `_`-prefixed method from other classes that are not derived from the class where the method is defined.
 #ifndef DISABLE_DEPRECATED
 		PROPERTY_USED_AS_FUNCTION, // Function not found, but there's a property with the same name.
 		CONSTANT_USED_AS_FUNCTION, // Function not found, but there's a constant with the same name.
@@ -148,6 +150,8 @@ class GDScriptWarning {
 		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.
+		WARN, // ACCESS_PRIVATE_MEMBER
+		WARN, // CALL_PRIVATE_METHOD
 #ifndef DISABLE_DEPRECATED
 		WARN, // PROPERTY_USED_AS_FUNCTION
 		WARN, // CONSTANT_USED_AS_FUNCTION

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

@@ -15,6 +15,7 @@ class SuperMethodsRecognized extends BaseClass:
 		return result
 
 func test():
+	@warning_ignore_start("call_private_method")
 	var test1 = SuperClassMethodsRecognized.new()
 	print(test1._get_property_list()) # Calls base class's method.
 	var test2 = SuperMethodsRecognized.new()

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

@@ -151,6 +151,30 @@ func test_unsafe_void_return() -> void:
 func get_class():
 	pass
 
+class PrivA:
+	var _t = 1
+
+	func _priv_method():
+		_t = 5
+
+class PrivB:
+	var a = PrivA.new()
+
+	func _foo():
+		@warning_ignore("access_private_member")
+		a._t = 2
+		@warning_ignore("call_private_method")
+		a._priv_method()
+
+class PrivC extends PrivA:
+	var a = PrivA.new()
+
+	func _foo():
+		@warning_ignore("access_private_member")
+		a._t = 2
+		@warning_ignore("call_private_method")
+		a._priv_method()
+
 # We don't want to execute it because of errors, just analyze.
 func test():
 	pass

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

@@ -0,0 +1,22 @@
+class A:
+	var _t = 1
+
+	func _priv_method():
+		_t = 5
+
+class B:
+	var a = A.new()
+
+	func _foo():
+		a._t = 2
+		a._priv_method()
+
+class C extends A:
+	var a = A.new()
+
+	func _foo():
+		a._t = 2
+		a._priv_method()
+
+func test():
+	pass

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

@@ -0,0 +1,3 @@
+GDTEST_OK
+~~ WARNING at line 11: (ACCESS_PRIVATE_MEMBER) The member "_t" is private. It should not be accessed from an external script.
+~~ WARNING at line 12: (CALL_PRIVATE_METHOD) The method "_priv_method()" is private. It should not be called from an external script.

modules/gdscript/tests/scripts/runtime/features/onready_base_before_subclass.gd

@@ -14,5 +14,6 @@ class B extends A:
 
 func test():
 	var node := B.new()
+	@warning_ignore("call_private_method")
 	node._ready()
 	node.free()