Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Invoke() symbol resolution should check base class #138

Closed
harryr0se opened this issue Apr 18, 2017 · 5 comments
Closed

Invoke() symbol resolution should check base class #138

harryr0se opened this issue Apr 18, 2017 · 5 comments
Assignees
Labels

Comments

@harryr0se
Copy link

harryr0se commented Apr 18, 2017

At the moment using the Invoke function with a method from a base class will "Fail to resolve symbol"

@citizenmatt citizenmatt self-assigned this Apr 18, 2017
@citizenmatt citizenmatt added this to the v1.8 Stuff milestone Apr 18, 2017
@citizenmatt
Copy link
Member

Can the invoked function be a private method in the base class? Or does it have to be public/protected?

@CAD97
Copy link

CAD97 commented Apr 18, 2017

Assuming the most misbehaved Invoke possible, I think the following cases enumerate the different ways this could present itself for method resolution.

I don't have access to a compiler right now, so some of these may just not be legal code. In any case, in all but the virtual override case (assuming the override is the one called), don't do this: it's not clear which of these should (or will!) be called.

public class Base : MonoBehaviour;
public class Derived : Base;

private void Base.Message();
private void Derived.Message();

private void Base.Message();
public void Derived.Message();

public void Base.Message();
private void new Derived.Message();

public void Base.Message();
public void new Derived.Mesage();

public void virtual Base.Message();
public void override Derived.Message();

public void virtual Base.Message();
public void sealed Derived.Message();

public void virtual Base.Message();
private void new Derived.Message();

I don't think there could be a difference between ((Base) member).Invoke("Message") and ((Derived) member).Invoke("Message"), because Invoke is always run with member statically as a Unity type and not a user type. But honestly, I don't know.

Unity's docs specifically say they don't do overload resolution for arguments, so that doesn't need to be tested (though it could be checked (if it isn't already)).

And let me just say again. Please don't do this. Especially when using dynamic-like behavior in Invoke, SendMessage and friends, don't make it more complicated than it needs to be to determine which implementation is called.

@harryr0se
Copy link
Author

@citizenmatt Running the following test it seems public, protected and private are supported.

public class BaseClass : MonoBehaviour
{
    public void PublicTest() { Debug.Log("Invoked public"); }
    protected void ProtectedTest() { Debug.Log("Invoked protected"); }
    private void PrivateTest() { Debug.Log("Invoked private"); }
}

public class DerivedClass : BaseClass
{
    public void Start()
    {
        Invoke("PublicTest", 1.0f);
        Invoke("ProtectedTest", 1.0f);
        Invoke("PrivateTest", 1.0f);
    }
}

However as pointed out by @CAD97 There's a few things to keep in mind if users try to invoke functions that share names in the base and the derived type. I just ran the following tests.

Virtual

public class BaseClass : MonoBehaviour
{
    public virtual void PublicTest() { Debug.Log("Invoked public base"); }
    protected virtual void ProtectedTest() { Debug.Log("Invoked protected base"); }
}

public class DerivedClass : BaseClass
{
    public void Start()
    {
        Invoke("PublicTest", 1.0f);
        Invoke("ProtectedTest", 1.0f);
    }

    public override void PublicTest() { Debug.Log("Invoked public derived"); }
    protected override void ProtectedTest() { Debug.Log("Invoked protected derived"); }
}

Non-Virtual

public class BaseClass : MonoBehaviour
{
    public void PublicTest() { Debug.Log("Invoked public base"); }
    protected void ProtectedTest() { Debug.Log("Invoked protected base"); }
    private void PrivateTest() { Debug.Log("Invoked private base"); }
}

public class DerivedClass : BaseClass
{
    public void Start()
    {
        Invoke("PublicTest", 1.0f);
        Invoke("ProtectedTest", 1.0f);
        Invoke("PrivateTest", 1.0f);
    }

    private new void PublicTest() { Debug.Log("Invoked public derived"); }
    protected new void ProtectedTest() { Debug.Log("Invoked protected derived"); }
    private new void PrivateTest() { Debug.Log("Invoked private derived"); }
}

In both cases Unity (5.6.0f3) seemed to consistently invoke the derived class function.
Perhaps rider could provide a hint/warning if the user is trying to call a function name that exists in the both the base and the derived type as it might cause bugs if they're not aware of how Invoke handles such cases.

I still think that resolving symbols that are Unique to the base class should be supported (which was my original use case) :)

@citizenmatt
Copy link
Member

Fantastic, thanks both! I've reworked this feature, and it will now include base class methods, including private methods on base classes. It automatically warns if a method name is ambiguous, because I'm not replicating Unity's logic to pick the most derived instance. I could add that, but I agree that I think it's useful to get the warning. We can revisit if anyone ever complains about it 😄

screen shot 2017-04-19 at 9 53 55 am

The other thing I've done is that now it can include base class methods, I filter out any methods that are defined in a base class that lives in the UnityEngine namespace, or it would show all of the methods and, more importantly, private methods of e.g. MonoBehaviour, Component and Object.

@harryr0se
Copy link
Author

That sounds great, thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants