Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Support explicit interface and hidden member of super class #1613

Merged
merged 1 commit into from
Aug 6, 2023

Conversation

viruscamp
Copy link
Contributor

Close #1611 Support explicit interface and hidden member of super class
Add field _clrType to ObjectWrapper
Add unwrapClr helper method

…of super class

Add field `_clrType` to `ObjectWrapper`
Add `unwrapClr` helper method
Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work and kudos for the great test coverage!

@lahma lahma enabled auto-merge (squash) August 6, 2023 07:27
@lahma lahma merged commit 0ebf7cd into sebastienros:main Aug 6, 2023
3 checks passed
@CreepGin
Copy link
Contributor

CreepGin commented Oct 2, 2023

I think this PR is making all public extern members null. In my case, it's making a lot of Unity APIs inaccessible now.

Edit: I'll try to make a minimal test case in a few (public extern may not be the root problem).

@CreepGin
Copy link
Contributor

CreepGin commented Oct 2, 2023

The following Assertion will fail for this PR.

public class Container
{
    public Child Child => _child;

    Child _child = new Child();

    public BaseClass Get()
    {
        return _child;
    }
}

public class BaseClass
{
    public int index = 123;
}

public class Child : BaseClass { }

[Fact]
public void CustomTest()
{
    var engine = new Engine().SetValue("container", new Container());
    var res = engine.Evaluate(@"container.Child === container.Get()"); // These two should be the same object. But this PR makes `container.Get()` return a different object

    Assert.True(res.AsBoolean());
}

Also in the context of Unity, if Child and BaseClass contain public extern members, these will now be null if the object is retrieved via container.Get().

@viruscamp
Copy link
Contributor Author

viruscamp commented Oct 3, 2023

@CreepGin , I think this behavior is as design.
Maybe we can improve it a little:

Assert.False(engine.Evaluate(@"container.Child === container.Get()").AsBoolean()); // as design
Assert.True(engine.Evaluate(@"container.Child == container.Get()").AsBoolean()); // TODO

BTW, it provides unwrap method:

Assert.True(engine.Evaluate(@"container.Child === clrHelper.unwrap(container.Get())").AsBoolean());

@CreepGin
Copy link
Contributor

CreepGin commented Oct 3, 2023

So this is quite a big breaking change, at least for me. Now the "wrapped" version of the object is making all the public extern members null. I'm assuming that's not by design right?

I just tried ClrHelper.Unwrap() and it works; I can access those public extern members again on the unwrapped object. But I'd much prefer to avoid the extra unwrap.

@viruscamp
Copy link
Contributor Author

@CreepGin

public interface I1
{
  string MethodFromInterface();
}
public class C1 : I1
{
  public extern string MethodFromInterface();
  public extern string MethodInClass();

  public static I1 InterfaceInstance => new C1();
}

Prepare js tests: engine.SetValue("C1", TypeReference.CreateTypeReference(engine, typeof(C1)));

var i1 = C1.InterfaceInstance; // typed ObjectWrapper with type I1
i1.MethodFromInterface(); // should success, if you cannot call it, fire an issue
i1.MethodInClass(); // failed as design, you wanna this works?
clrHelper.unwrap(i1).MethodInClass(); // should success, if you cannot call it, fire an issue
clrHelper.wrap(i1, C1).MethodInClass(); // it's better than just unwrap, like explicit conversion in C# ((C1)i1).MethodInClass();

@CreepGin
Copy link
Contributor

CreepGin commented Oct 8, 2023

i1.MethodInClass(); // failed as design, you wanna this works?

Yes. But maybe make explicit interface optional (via Jint.Options)? Or provide a way to disable it? (If this is already the case, please let me know; I must've missed it)

I do think the old behaviour should also be useful for a dynamic language like JS. (A good chunk of my codebase depends on it too)

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

Successfully merging this pull request may close these issues.

Support explicit interface
3 participants