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

Calling toString on an Enso object implementing a Java interface causes Panic #7437

Closed
radeusgd opened this issue Jul 31, 2023 · 11 comments · Fixed by #7487
Closed

Calling toString on an Enso object implementing a Java interface causes Panic #7437

radeusgd opened this issue Jul 31, 2023 · 11 comments · Fixed by #7487
Assignees
Labels

Comments

@radeusgd
Copy link
Member

Sometimes when passing Enso objects to Java, we can make them implement some interface by ensuring that the Enso object has all methods of that interface. This makes it easier to interact with Enso objects on the Java side (once can just use the interface instead of Polyglot methods).

It seems that toString which is expected from all Java objects is not picked up properly.

I'm not exactly sure if this is a bug in Enso or Graal itself.

Repro

Let's create a file Foo.java:

package foo;

public class Foo {
    public static interface Fooable {
        public long foo();
    }

    public static long callFoo(Fooable f) {
        long x = f.foo();
        System.out.println("Foo.foo() returned " + x);
        return x;
    }

    public static void showObject(Object o) {
        System.out.println("Object = " + o.toString());
    }

    public static long callFooAndShow(Fooable f) {
        long x = f.foo();
        System.out.println("{" + f.toString() + "}.foo() returned " + x);
        return x;
    }
}

and then put it in a polyglot project, with src/Main.enso being:

from Standard.Base import all

polyglot java import foo.Foo

type My_Fooable_Implementation
    Instance x

    foo : Integer
    foo self = 100+self.x

main =
   fooable = My_Fooable_Implementation.Instance 23
   IO.println fooable.foo
   IO.println fooable.to_text
   IO.println (Foo.callFoo fooable)
   IO.println (Foo.showObject fooable)
   IO.println (Foo.callFooAndShow fooable)

Actual behaviour

The project as described above, when run yields:

123
(Instance 23)
Foo.foo() returned 123
123
Object = (Instance 23)
Nothing
Execution finished with an error: Method `toString` of type My_Fooable_Implementation could not be found.
        at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotObjectProxyHandler.invoke(PolyglotObjectProxyHandler.java:105)
        at <java> jdk.proxy5/jdk.proxy5.$Proxy44.toString(Unknown Source)
        at <java> foo.Foo.callFooAndShow(Foo.java:20)
        at <enso> Main.main<arg-1>(src\Main.enso:17:16-41)
        at <enso> Main.main(src\Main.enso:17:4-42)

Expected behaviour

The program should execute without errors and yield:

123
(Instance 23)
Foo.foo() returned 123
123
Object = (Instance 23)
Nothing
{(Instance 23)}.foo() returned 123
123

The toString call on the Proxy object should delegate to the to_text method of the Enso object. As we can see, if we pass it as an Object that correctly happens. But if we pass it as this 'interface proxy' - that fails.

@JaroslavTulach
Copy link
Member

Exposing java.lang.Object methods to guest languages is considered as security problem. I fixed that by designing HostAccess. Enso is using HostAccess.PUBLIC, but even that doesn't allow access to methods like getClass(), hashCode() and toString().

I understand that logging is problematic when toString() doesn't return anything at all. But I am not sure whether there is something to do about it.

@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Aug 1, 2023
@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to ⚙️ Design in Issues Board Aug 2, 2023
@JaroslavTulach
Copy link
Member

Here is a small program that demonstrates the behavior on Python object:

import org.graalvm.polyglot.Context;
import org.graalvm.polyglot.HostAccess;

public class ToString {
    public interface WithX {
        int x();
    }

    public static void main(String[] args) {
        try (
            var ctx = Context.newBuilder()
                .allowHostAccess(HostAccess.ALL)
                .build()
        ) {
            var obj = ctx.eval("python", """
            class WithX:
                def __init__(self, v):
                    self.x = v

            WithX(42)
            """).as(WithX.class);

            System.out.println("obj.x: " + obj.x());
            var txt = obj.toString();
            System.out.println("obj  : " + txt);
        }
    }
}

when executed as graalvm-17/bin/java ToString.java it works and it prints:

obj.x: 42
obj  : <__main__.WithX object at 0x254cc548>

Something else must be wrong in the Enso case...

@JaroslavTulach
Copy link
Member

Probably related to PolyglotObjectProxyHandler that caches UnsupportedOperationException and then handles Object.toString in a special way. But we throw different exception:

    @Override
    public Object invoke(Object proxy, Method method, Object[] arguments) throws Throwable {
        CompilerAsserts.neverPartOfCompilation();
        Object[] resolvedArguments = arguments == null ? EMPTY : arguments;
        try {
            return invoke.call(languageContext, obj, method, resolvedArguments);
        } catch (UnsupportedOperationException e) {
            try {
                return PolyglotFunctionProxyHandler.invokeDefault(this, proxy, method, resolvedArguments);
            } catch (Exception innerE) {
                e.addSuppressed(innerE);
                throw e;
            }
        }
    }

@radeusgd
Copy link
Member Author

radeusgd commented Aug 2, 2023

when executed as graalvm-17/bin/java ToString.java it works and it prints:

so that sounds like we should be able to fix this issue in Enso, because it's not a Graal issue, right? :) That sounds nice

@enso-bot
Copy link

enso-bot bot commented Aug 3, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-02):

Progress: - Investigating toString(): #7437 (comment)

Next Day: Benchmarking BigInteger and working on toString()

@JaroslavTulach JaroslavTulach linked a pull request Aug 3, 2023 that will close this issue
2 tasks
@JaroslavTulach JaroslavTulach moved this from ⚙️ Design to 👁️ Code review in Issues Board Aug 3, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 4, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-03):

Progress: - toString PR: #7487

Next Day: Finish toString() & other bugfixing

GitHub
Closes #7323 Pull Request Description Motivation There are a lot of Enso-only benchmarks in test/Benchmarks project. These benchmarks are not run on the CI (they are, but only in dry-run). We want ...
GitHub
Checklist Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if...

@enso-bot
Copy link

enso-bot bot commented Aug 5, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-04):

Progress: - Investigating toString exception: 1c3f396

Next Day: Finish toString()

@enso-bot
Copy link

enso-bot bot commented Aug 7, 2023

Jaroslav Tulach reports a new 🔴 DELAY for yesterday (2023-08-06):

Summary: There is 7 days delay in implementation of the Calling toString on an Enso object implementing a Java interface causes Panic (#7437) task.
It will cause 7 days delay for the delivery of this weekly plan.

Got additional requests to fix

Delay Cause: Additional fixes were needed. More than originally estimated.

Possible solutions: Working over weekend fixed all the issues (review pending).

1 similar comment
@enso-bot
Copy link

enso-bot bot commented Aug 7, 2023

Jaroslav Tulach reports a new 🔴 DELAY for yesterday (2023-08-06):

Summary: There is 7 days delay in implementation of the Calling toString on an Enso object implementing a Java interface causes Panic (#7437) task.
It will cause 7 days delay for the delivery of this weekly plan.

Got additional requests to fix

Delay Cause: Additional fixes were needed. More than originally estimated.

Possible solutions: Working over weekend fixed all the issues (review pending).

@enso-bot
Copy link

enso-bot bot commented Aug 7, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-06):

Progress: - toString() is (almost) green: #7487 (comment)

Next Day: EnsoContext & other bugfixes

@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Aug 7, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 8, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-07):

Progress: - demo & other meetings It should be finished by 2023-08-18.

Next Day: EnsoContext & other bugfixes

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

Successfully merging a pull request may close this issue.

2 participants