Reimplent soft access restriction for members prefixed with _

Author: Lazy-Rabbit-2001

doc/classes/EditorInterface.xml

@@ -292,7 +292,7 @@
 			<param index="4" name="type_blocklist" type="StringName[]" default="[]" />
 			<description>
 				Pops up an editor dialog for creating an object.
-				The [param callback] must take a single argument of type [StringName] which will contain the type name of the selected object or be empty if no item is selected.
+				The [param callback] must take a single argument of type [String], which will contain the type name of the selected object (or the script path of the type, if the type is created from a script), or be an empty string if no item is selected.
 				The [param base_type] specifies the base type of objects to display. For example, if you set this to "Resource", all types derived from [Resource] will display in the create dialog.
 				The [param current_type] will be passed in the search box of the create dialog, and the specified type can be immediately selected when the dialog pops up. If the [param current_type] is not derived from [param base_type], there will be no result of the type in the dialog.
 				The [param dialog_title] allows you to define a custom title for the dialog. This is useful if you want to accurately hint the usage of the dialog. If the [param dialog_title] is an empty string, the dialog will use "Create New 'Base Type'" as the default title.

doc/classes/ProjectSettings.xml

@@ -509,12 +509,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 [code]warn[/code] or [code]error[/code], 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 [code]warn[/code] or [code]error[/code], 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 [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>

editor/gui/create_dialog.cpp

@@ -370,7 +370,9 @@ void CreateDialog::_configure_search_option_item(TreeItem *r_item, const StringN
 		if (is_tool) {
 			tooltip = TTR("The script will run in the editor.") + "\n" + tooltip;
 		}
-		r_item->add_button(0, get_editor_theme_icon(SNAME("Script")), 1, false, vformat(tooltip, ScriptServer::get_global_class_path(p_type)));
+		const String script_path = ScriptServer::get_global_class_path(p_type);
+		r_item->add_button(0, get_editor_theme_icon(SNAME("Script")), 1, false, vformat(tooltip, script_path));
+		r_item->set_meta(SNAME("_script_path"), script_path);
 		if (is_tool) {
 			int button_index = r_item->get_button_count(0) - 1;
 			r_item->set_button_color(0, button_index, get_theme_color(SNAME("accent_color"), EditorStringName(Editor)));
@@ -608,11 +610,13 @@ void CreateDialog::select_base() {
 
 String CreateDialog::get_selected_type() {
 	TreeItem *selected = search_options->get_selected();
+
 	if (!selected) {
 		return String();
 	}
 
-	return selected->get_text(0);
+	String type = selected->get_text(0);
+	return ClassDB::class_exists(type) ? type : String(selected->get_meta("_script_path", ""));
 }
 
 void CreateDialog::set_base_type(const String &p_base) {

modules/gdscript/gdscript_analyzer.cpp

@@ -2049,6 +2049,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];
@@ -3552,10 +3579,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) {
@@ -3589,6 +3625,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.
@@ -4520,6 +4559,10 @@ void GDScriptAnalyzer::reduce_identifier(GDScriptParser::IdentifierNode *p_ident
 			}
 		}
 
+#ifdef DEBUG_ENABLED
+		check_access_private_member(p_identifier, p_identifier->get_datatype());
+#endif
+
 		return;
 	}
 
@@ -4881,6 +4924,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

@@ -162,6 +162,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.
@@ -238,6 +244,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

@@ -89,6 +89,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.
@@ -146,6 +148,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/features/warning_ignore_warnings.out

@@ -1 +1,2 @@
-GDTEST_OK
+GDTEST_PARSER_ERROR
+Used space character for indentation instead of tab as used before in the file.