CSharpScript should not own method infos of the base class

[huisedenanhai]

CSharpScript::methods should only hold methods defined by current class.

Previous implementation stores methods defined by current class and all base classes, thus
result returned by CSharpInstance::get_method_list (which gather all method infos from current to base classes) contains duplicated method infos of base class.

The duplication can be visualized in method picking dialogue of signal connection dialogue panel.

For example, there are two C# scripts

// Test.cs
using Godot;

public partial class Test : TestBase
{
    public void Bar()
    {
        GD.Print("C# Bar");
    }
}

// TestBase.cs
using Godot;

public partial class TestBase : Node3D
{
    public void Foo()
    {
        GD.Print("C# Base");
    }
}

Attach the Test class to a node, and try to connect a signal to it. In method picking dialogue, the Foo method defined by TestBase is duplicated twice.

This pr fix the duplication issue. The behaviour of methods like CSharpScript::has_method, CSharpInstance::get_method_list are consistent with the corresponding methods of GDScript.

[raulsntos]

It feels weird to change this loop for methods and not for the others (rpc functions and event signals).

I think the proper fix would be to remove the loop from CSharpInstance::get_method_list. Other similar methods (like CSharpInstance::has_method and CSharpInstance::get_property_list) also don't have a loop because they assume the script already contains inherited methods and properties.

[huisedenanhai]

I suggest there to be some clarification for the meaning of Script::has_method and ScriptInstance::has_method.

In current implementation, GDScript::has_method and CSharpScript::has_method all do a simple check without looking at the base class.

godot/modules/gdscript/gdscript.cpp

Lines 357 to 359 in 809a982

	bool GDScript::has_method(const StringName &p_method) const {
	return member_functions.has(p_method);
	}

godot/modules/mono/csharp_script.cpp

Lines 2473 to 2485 in 809a982

	bool CSharpScript::has_method(const StringName &p_method) const {
	if (!valid) {
	return false;
	}
	for (const CSharpMethodInfo &E : methods) {
	if (E.name == p_method) {
	return true;
	}
	}
	return false;
	}

GDScriptInstance::has_method and CSharpInstance::has_method all check the method in the current class AND the base class. CSharpInstance::has_method check the existance of a method by do recursion in HasGodotClassMethod generated by ScriptMethodsGenerator.

godot/modules/gdscript/gdscript.cpp

Lines 1757 to 1768 in 809a982

	bool GDScriptInstance::has_method(const StringName &p_method) const {
	const GDScript *sptr = script.ptr();
	while (sptr) {
	HashMap<StringName, GDScriptFunction *>::ConstIterator E = sptr->member_functions.find(p_method);
	if (E) {
	return true;
	}
	sptr = sptr->_base;
	}
	return false;
	}

In current implementation, CSharpScript::members contains methods from the current class and its base, whileGDScript:: member_functions ONLY contains method from the current class.

This PR tries to correct the contents in CSharpScript::members, which is modified by ScriptManagerBridge.UpdateScriptClassInfo, to fix the behaviour for both CSharpScript::has_method and CSharpInstance::get_method_list.

By removing the loop, CSharpScript::members only contains methods from the current class. Then the behaviour for theses method reflection functions are all consistent.

Only remove the loop from CSharpInstance::get_method_list won't fix the problem with CSharpScript::has_method.

I haven't check properties, rpc functions and signal. They might have similar issues, but I need more time to figure out.

[huisedenanhai]

Emm.. I just found in editor mode, there might be no actual script instance, instead a PlaceHolderScriptInstance will be created.

PlaceHolderScriptInstance::get_method_list will in turn calls Script::get_script_method_list. CSharpScript::get_script_method_list also iterates on the inheritance chain. Only remove the loop in CSharpInstance::get_method_list may not fix the issue.

Sorry for my inaccurate explain in the first comment. The duplication in method picking dialogue actually comes from CSharpScript::get_script_method_list rather than CSharpInstance::get_method_list, though CSharpInstance::get_method_list also returns duplicated method info. Actually these two methods contains duplicated code :(

[raulsntos]

I think it makes sense for CSharpScript to only contain the members declared in the C# type that it represents, then the inherited members can be retrieved by iterating the hierarchy chain.

The same change should likely be made to the other members (properties and signals), and we must make sure the behavior matches GDScript (i.e. if GDScript::get_method_list iterates the hierarchy chain and returns inherited methods, then CSharpScript::get_method_list should return inherited methods too).

[raulsntos]

I'm not sure this change is worth doing, and there might've been reasons why this code was duplicated before.

---

You should probably check that the script is valid, as it was done before.

Suggested change

	script->get_script_method_list(p_list);
	if (!script->is_valid() || !script->valid) {
	return;
	}
	script->get_script_method_list(p_list);

[raulsntos]

If we are removing the loop, we can replace top here with scriptType which should never be null so no need to check.

Suggested change

	if (top != null && top != native)
	{
	var methodList = GetMethodListForType(top);
	if (scriptType != native)
	{
	var methodList = GetMethodListForType(scriptType);

[YuriSizov]

@huisedenanhai Will you be available to finish this PR based on the feedback above? It's not in scope for 4.2 at this point, but we could merge it as soon as it's fixed up and the consensus is reached.

[raulsntos]

• Superseded by CSharpScript should not own method infos of the base class #91564. Thanks for the contribution nonetheless.