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

Fix displaying of host values in chrome devtools #11468

Merged
merged 28 commits into from
Nov 27, 2024

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Nov 1, 2024

Fixes #7890

Pull Request Description

Chrome inspector now displays polyglot objects that have a corresponding Enso builtin type as enso objects:

image

Important Notes

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 there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@Akirathan Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 1, 2024
@Akirathan Akirathan self-assigned this Nov 1, 2024
HostMethodCallNode.getPolyglotCallType(value, symbol, interopUncached);
try {
switch (polyglotCallType) {
case CONVERT_TO_DATE -> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the main idea. Chrome inspector instrument builds DebugLocalScope which holds a materialized frame with all the bindings. If a binding is a host object that can/should be converted to an appropriate Enso builtin type before sending back to the inspector, it is converted here. This is pretty much the same kind of conversion that is done inside InvokeMethodNode on the self argument before any kind of method dispatch is done.

@Akirathan Akirathan marked this pull request as ready for review November 8, 2024 13:23
@Akirathan Akirathan requested review from 4e6 and hubertp as code owners November 8, 2024 13:23
@Akirathan Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Nov 8, 2024
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

  • I'd expand this to other languages than Java - e.g. Python & co.
  • I don't understand the different between PolyglotCall & the other enum
  • I still believe the code should be in EnsoLanguage.getLanguageView

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Having all of this in getLanguageView should show beneficial in the future. Hopefully.

@Akirathan
Copy link
Member Author

Blocked by #11538

@JaroslavTulach
Copy link
Member

Blocked by #11538

How exactly this is blocked by

?

@Akirathan
Copy link
Member Author

How exactly this is blocked by

* [Implement hasLanguage interop message for all enso objects #11538](https://github.com/enso-org/enso/pull/11538)

?

The test in

assertEquals("Types without and with warnings are the same", type, warningType);
started to fail on this PR for a Value of primitive 42L. On this PR, v.getMetaObject() on that value returns non-null, but on develop it returns null. And for some reason, warning2.getMetaObject() returns null, thus the test fails. I figured out it would be just easier to first ensure that we stick to correct Truffle contracts and do that in a separate PR.

# Conflicts:
#	engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/WarningsTest.java
Primitive values is expected not to have meta object.
@JaroslavTulach
Copy link
Member

JaroslavTulach commented Nov 27, 2024

Right now I see following four failures:

  • DebuggingEnsoTest fails on NullPointerException: Cannot invoke "java.util.Collection.size()" because the return value of "com.oracle.truffle.api.debug.DebugValue.getProperties()" is null
  • testAtomFieldsAreReadable fails on NullPointerException: Cannot invoke "com.oracle.truffle.api.debug.DebugValue.isReadable()" because "field1Prop" is null
  • InsightForEnsoTest.computeFactorial fails with assertion error at org.enso.interpreter.test.InsightForEnsoTest.computeFactorial(InsightForEnsoTest.java:90)
  • WarningsTest fails on Types without and with warnings are the same expected: but was:

This is a fix that helps at least with the WarningsTest:

diff --git engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/ValuesGenerator.java engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/ValuesGenerator.java
index 4b23c33002..5b2288e9c2 100644
--- engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/ValuesGenerator.java
+++ engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/ValuesGenerator.java
@@ -445,7 +445,9 @@ public final class ValuesGenerator {
     }
 
     for (var v : collect) {

    assertTrue("It's a number" + v, v.isNumber());

+      if (!v.isNumber()) {
+        assertTrue("It's a number" + v, v.isNumber());
+      }
     }
     return collect;
   }
@@ -504,7 +506,9 @@ public final class ValuesGenerator {
     }
 
     for (var v : collect) {

    assertTrue("It is an boolean" + v, v.isBoolean());

+      if (!v.isBoolean()) {
+        assertTrue("It is an boolean" + v, v.isBoolean());
+      }
     }
     return collect;
   }
diff --git engine/runtime/src/main/java/org/enso/interpreter/EnsoLanguage.java engine/runtime/src/main/java/org/enso/interpreter/EnsoLanguage.java
index 3167895cb7..9ec91872a3 100644
--- engine/runtime/src/main/java/org/enso/interpreter/EnsoLanguage.java
+++ engine/runtime/src/main/java/org/enso/interpreter/EnsoLanguage.java
@@ -397,6 +397,9 @@ public final class EnsoLanguage extends TruffleLanguage<EnsoContext> {
       var cons = b ? bool.getTrue() : bool.getFalse();
       return AtomNewInstanceNode.getUncached().newInstance(cons);
     }
+    if (value instanceof Double d) {
+        return  new LanguageViewWrapper(d);
+    }
     var interop = InteropLibrary.getUncached();
     // We want to know if the value can be converted to some Enso builtin type.
     // In order to do that, we are trying to infer PolyglotCallType of value.to method.

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Nov 27, 2024
@mergify mergify bot merged commit 5d7e89b into develop Nov 27, 2024
42 of 45 checks passed
@mergify mergify bot deleted the wip/akirathan/7890-java-values-devtools branch November 27, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Java Values are displayed differently in the Chrome Devtools debugger
2 participants