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

Java Values are displayed differently in the Chrome Devtools debugger #7890

Closed
radeusgd opened this issue Sep 25, 2023 · 20 comments · Fixed by #11468
Closed

Java Values are displayed differently in the Chrome Devtools debugger #7890

radeusgd opened this issue Sep 25, 2023 · 20 comments · Fixed by #11468
Assignees
Labels
-compiler p-low Low priority

Comments

@radeusgd
Copy link
Member

radeusgd commented Sep 25, 2023

Consider the following program:

from Standard.Base import all

main =
    d_enso = Date.new 2011 12 13
    d_java = Date.parse "2014-12-15"
    
    IO.println d_enso
    IO.println d_java

run it with --inspect and set the breakpoint at IO.println d_java.

We will see the following Scope:
image

We can see that the value coming from Java is displayed differently than the one from Enso, even though both are a Date. It is probably because Enso did not manage yet to perform its polyglot conversions. Shall they be performed before displaying the object?

Another issue can be seen when we interact with the REPL:
image

We can see that values coming from polyglot calls (e.g. next) behave Java-like and the ones that are 'correctly' displayed come from builtin methods (e.g. Date_Time.date) which ensure the Enso polyglot conversion happens.


I'm not sure if this is a bug - although ideally the behaviour of the Debugger should be consistent regardless of where the value is coming from.

What may be especially misleading is that the Java values display all kinds of Java methods they have:
image

But these methods won't work, because of how the Enso interop works (once we do a call, we do the polyglot conversion and it's no longer a Java LocalDate but an EnsoDate):
image
image

That is quite misleading, so I think ideally we should fix it and show the value for what it is to the user - an Enso date. I'm not sure how big of a priority this is though.

@jdunkerley jdunkerley added p-low Low priority -compiler and removed triage labels Sep 29, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Oct 10, 2023
@JaroslavTulach
Copy link
Member

Enso did not manage yet to perform its polyglot conversions.

Yes, Enso doesn't convert non-primitive values until they need to be converted.

Shall they be performed before displaying the object?

No. The general rule is to convert values as late as possible.

show the value for what it is to the user - an Enso date

There is a concept of a view EnsoLanguage could probably convert those values (for example by passing them to identity function).

@enso-bot
Copy link

enso-bot bot commented Nov 11, 2024

Pavel Marek reports a new STANDUP for today (2024-11-11):

Progress: - Assisting Hubert with JPMS-related settings.

  • Implement EnsoLanguage.getLanguageView.
    • Need to tweak some existing types, like EnsoBigInteger because of various (at first sight unrelated) test failures.
    • Double primitive wrapped in WithWarnings answers to hasMetaObject with false?? It should be finished by 2024-11-14.

@JaroslavTulach
Copy link
Member

  • Double primitive wrapped in WithWarnings answers to hasMetaObject with false??
  • I believe this is a result of the Executing (parts of) Truffle TCK with Enso values #8685
  • the TCK (probably) asserts primitive values to not have a meta object
  • at least I think I had to do a delicate dance around the meta objects for Enso objects
  • for example I am sure that Nothing - e.g. isNull cannot have a meta object according to the TCK

@JaroslavTulach
Copy link
Member

  • Need to tweak some existing types, like EnsoBigInteger because of various (at first sight unrelated) test failures.

One of my first tasks in Enso was to handle dates, times and datetimes interop properly. That was done by converting the values to Enso before method invocation:

@Akirathan
Copy link
Member

the TCK (probably) asserts primitive values to not have a meta object

@JaroslavTulach That does not seem right. For a primitive int i = 5432 created in host, this is not true:

  @Test // Put this test inside MetaObjectTest.java
  public void myTest() {
    var primNum = ctx.asValue((int) 5432);
    assertThat(primNum.fitsInInt(), is(true));
    assertThat(primNum.getMetaObject(), is(notNullValue()));
  }

This test passes. So host language clearly assigns meta objects to primitive values. Do you think guest languages should have a different contract?

@Akirathan
Copy link
Member

Moreover, the current state of Enso primitive values is as follows (this test passes on develop):

  @Test
  public void myTest() {
    var g = ValuesGenerator.create(ctx, ValuesGenerator.Language.ENSO);
    g.numbers()
        .stream()
        .filter(Value::fitsInLong)
        .forEach(num -> {
          assertThat(num + " Enso primitive long numbers don't have meta object",
              num.getMetaObject(),
              is(nullValue()));
        });
    g.numbers()
        .stream()
        .filter(num -> !num.fitsInLong() && !num.fitsInDouble() && num.fitsInBigInteger())
        .forEach(num -> {
          assertThat(num + " Enso big integer numbers have meta object",
              num.getMetaObject(),
              is(notNullValue()));
        });
    g.booleans()
        .forEach(bool -> {
          assertThat(bool + " Enso primitive booleans have meta object",
              bool.getMetaObject(),
              is(notNullValue()));
        });
  }

In other words. Primitive numbers (those that fit inside long) don't have a meta object, but big integers have. Moreover, primitive booleans have meta objects. This is confusing and not consistent. I assume that we should ensure that all the primitive Enso values have meta objects. Right?

@Akirathan
Copy link
Member

Host primitive boolean also has meta object:

  @Test
  public void javaBoolHasMeta() {
    var b = ctx.asValue(true);
    assertThat(b.isBoolean(), is(true));
    assertThat(b.getMetaObject(), is(notNullValue()));
  }

@Akirathan
Copy link
Member

The same is true for js and python - their primitive values have meta objects: (the following test passes on develop as well)

  @Test
  public void guestBoolHasMeta() {
    var jsBool = ctx.eval("js", "true");
    var pyBool = ctx.eval("python", "True");
    assertThat(jsBool.isBoolean(), is(true));
    assertThat(pyBool.isBoolean(), is(true));
    assertThat(jsBool.getMetaObject(), is(notNullValue()));
    assertThat(pyBool.getMetaObject(), is(notNullValue()));
  }

  @Test
  public void guestIntHasMeta() {
    var jsInt = ctx.eval("js", "42");
    var pyInt = ctx.eval("python", "42");
    assertThat(jsInt.isNumber(), is(true));
    assertThat(pyInt.isNumber(), is(true));
    assertThat(jsInt.getMetaObject(), is(notNullValue()));
    assertThat(pyInt.getMetaObject(), is(notNullValue()));
  }

@enso-bot
Copy link

enso-bot bot commented Nov 12, 2024

Pavel Marek reports a new STANDUP for today (2024-11-12):

Progress: - Found out violations of Truffle contracts (on develop):

@enso-bot
Copy link

enso-bot bot commented Nov 13, 2024

Pavel Marek reports a new STANDUP for today (2024-11-13):

Progress: - Fixing CodeLocationsTest - need to add at least import from Numbers again in some tests.

@JaroslavTulach
Copy link
Member

So host language clearly assigns meta objects to primitive values. Do you think guest languages should have a different contract?

No, if ...

the TCK (probably) asserts primitive values to not have a meta object

...the TCK is OK with that, then having ...

...long(s)... don't have a meta object, but big integers have

... consistency is important.

@JaroslavTulach
Copy link
Member

* Implement this contract in a separate PR - [Implement hasLanguage interop message for all enso objects #11538](https://github.com/enso-org/enso/pull/11538)
  * Inspired by GraalJS and GraalPython 

I am not sure what you mean by inspired, but PythonAbstractObject has to be abstract because it extends DynamicObject. Enso objects don't use DynamicObject.

@Akirathan
Copy link
Member

I am not sure what you mean by inspired, but PythonAbstractObject has to be abstract because it extends DynamicObject. Enso objects don't use DynamicObject.

Yes, but how else would you ensure that hasLanguage and getLanguage messages are implemented for every object? @ExportMessage annotation does not work on default methods on interfaces, it has to be on a method in abstract class.

@JaroslavTulach
Copy link
Member

how else would you ensure that hasLanguage and getLanguage messages are implemented for every object?

I would write a test that uses ValuesGenerator and checks that all Enso values support this message.

@ExportMessage annotation does not work on default methods on interfaces, it has to be on a method in abstract class.

I see. I am checking with Christian Humer, but I guess you are right.

@enso-bot
Copy link

enso-bot bot commented Nov 14, 2024

Pavel Marek reports a new STANDUP for today (2024-11-14):

Progress: - Trying to fix last bits on #11538 and #11500 so they both can be merged ASAP. It should be finished by 2024-11-14.

@enso-bot
Copy link

enso-bot bot commented Nov 15, 2024

Pavel Marek reports a new STANDUP for today (2024-11-15):

Progress: - Still refactoring EnsoObject to an abstract base class.

@enso-bot
Copy link

enso-bot bot commented Nov 18, 2024

Pavel Marek reports a new STANDUP for today (2024-11-18):

Progress: - Hunting a nasty bug related to warnings - adb2d05

  • All gates are green except for some tiny issues with native-image. It should be finished by 2024-11-20.

@enso-bot
Copy link

enso-bot bot commented Nov 19, 2024

Pavel Marek reports a new STANDUP for today (2024-11-19):

Progress: - Rerun benchmarks - stdlib benchmarks failed to compiled

  • Comparing benchmarks - ready to merge It should be finished by 2024-11-20.

@enso-bot
Copy link

enso-bot bot commented Nov 26, 2024

Pavel Marek reports a new STANDUP for today (2024-11-26):

Progress: - Testing of meta objects on numbers revealed a weird incosistency - https://github.com/enso-org/enso/pull/11468/files#r1859020769

GitHub
Fixes #7890 Pull Request Description Chrome inspector now displays polyglot objects that have a corresponding Enso builtin type as enso objects:

Important Notes

Checklist
Please ensure that the f...

@enso-bot
Copy link

enso-bot bot commented Nov 27, 2024

Pavel Marek reports a new STANDUP for today (2024-11-27):

Progress: - Turns out I misunderstood definitions of double and long.

  • Fixed rest of the tests, marking as ready to merge. It should be finished by 2024-11-29.

@mergify mergify bot closed this as completed in #11468 Nov 27, 2024
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler p-low Low priority
Projects
Status: 🟢 Accepted
Development

Successfully merging a pull request may close this issue.

4 participants