Report type of expressions returning polyglot values #4111

hubertp · 2023-02-01T18:11:48Z

Pull Request Description

Expressions returning polyglot values were not reporting the type of the result because we have to do additional magic that infers the correct Enso type. Since this is exactly what TypeOfNode does, I re-used the logic.

Straightforward solution failed in tests because of assertions:

[enso] WARNING: Execution of function main failed (Invalid library usage. Cached library must be adopted by a RootNode before it is executed.).
java.lang.AssertionError: Invalid library usage. Cached library must be adopted by a RootNode before it is executed.

That is why this PR replaces ExecutionEventListener with ExecutionEventNodeFactory.

Important Notes

Usage of TypeOfNode for programs that do not import stdlib means that we report types that do not involve stdlib e.g.
Standard.Builtins.Main.Integer instead of Standard.Base.Data.Numbers.Integer. While surprising, this is correct and I would say desirable. While reviewing the code, notice the difference in expectations in our runtime tests.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

...rument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java

Expressions returning polyglot values were not reporting the type of the result because we have to do additional magic that infers the correct Enso type. Since this is exactly what `TypeOfNode` does, I re-used the logic. Except this doesn't work in tests because they have assertions on and we get ``` [enso] WARNING: Execution of function main failed (Invalid library usage. Cached library must be adopted by a RootNode before it is executed.). java.lang.AssertionError: Invalid library usage. Cached library must be adopted by a RootNode before it is executed. ```

Replacing ExecutionEventListener with ExecutionEventNodeFactory allows us to enhance EventNode with an additional child node, TypeOfNode. The latter gets adopted to the RootNode, meaning it won't blow up assertions anymore. Additionally reduced the scope of @TruffleBoundary so that the child can possibly get PE, as suggested by Jaroslav.

Rather than introducing a fallback case with `TypeOfNode` this change replaces the `Types.getName` completely. This has a number of consequences: - if one does not import stdlib, all those types have a builtin type - the type of atom constructor is no longer the fqn name of the atom but a Function - unresolved_symbol has to be treated specially All those changes, while surprising, are actually correct. I also believe they are desirable but I'm open for discussion.

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/TypeOfNode.java

hubertp · 2023-02-06T20:10:00Z

...-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/BuiltinTypesTest.scala

@@ -319,7 +319,7 @@ class BuiltinTypesTest
      3
    ) should contain theSameElementsAs Seq(
      Api.Response(requestId, Api.PushContextResponse(contextId)),
-      TestMessages.update(contextId, idMain, "Enso_Test.Test.Main.Foo.Bar"),
+      TestMessages.update(contextId, idMain, ConstantsGen.FUNCTION_BUILTIN),


This might seem like a weird change at first sight but consider that Bar is a constructor taking an argument. So the type of it is a Function. Types.getName had a special case for AtomConstructor and I think now we are more consistent.

JaroslavTulach

Usage of ExecutionEventNodeFactory seems correct. Using nodes from instruments is a good thing - and it is also fast. Sharing such "utility nodes" like TypeOfNode at various places is the way to go.

JaroslavTulach · 2023-02-07T13:37:21Z

engine/runtime/src/test/java/org/enso/interpreter/test/MetaObjectTest.java

@@ -77,8 +77,10 @@ public void checkAllConstantGenValuesArePresent() throws Exception {
    var g = ValuesGenerator.create(ctx);
    var expecting = new HashSet<String>();
    for (var f : ConstantsGen.class.getFields()) {
-      var s = (String) f.get(null);
-      expecting.add(s);
+      if (!f.getName().endsWith("_BUILTIN")) {


What is this?

This is so that we don't have to write (by hand) FQN of builtin types. We generate ConstantsGen for types from stdlib but if one does not import stdlib builtin types are essentially Standard.Builtin.Main.XYZ. So every type which is in stdlib, say Standard.Base.Foo.Bar.XYZ has an equivalent in Standard.Builtin.Main.XYZ. Rather than trying to keep those up-to-date, processor adds those to ConstantsGen.

You might ask why do we need this at all?
Well, in tests (but also in a real application), if you don't import stdlib you operate only on builtins and the reported type shouldn't really be some type from stdlib, should it? That is why the type reported by TypeOfNode is an actual type, not some hardcoded string (like Types.getName). Hence the changes in tests (the changed ones don't import stdlib).

Also, the test assumes that stdlib is imported. It does not operate on builtins.

…84336928

A combination of commands triggered separate compilation that only recompiled part of builtins. A mechanism that workaround annotation processor's problems with separate compilation was updated to include changes from #4111. This was pretty tough to find given the rather unusual circumstances in CI.

A combination of commands triggered separate compilation that only recompiled part of builtins. A mechanism that workaround annotation processor's problems with separate compilation was updated to include changes from #4111. This was pretty tough to find given the rather unusual circumstances in CI. # Important Notes This eliminates the _random_ failures in CI related to separate compilation. To reproduce a specific set of steps has to be executed: ``` sbt> all buildEngineDistribution engine-runner/assembly runtime/Benchmark/compile language-server/Benchmark/compile searcher/Benchmark/compile (exit sbt) sbt> test ```

JaroslavTulach approved these changes Feb 2, 2023

View reviewed changes

...rument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed Feb 2, 2023

View reviewed changes

...rument-id-execution/src/main/java/org/enso/interpreter/instrument/IdExecutionInstrument.java Outdated Show resolved Hide resolved

hubertp force-pushed the wip/hubert/infer-typeof-polyglot-values-184336928 branch from 7368a08 to 121e463 Compare February 3, 2023 17:07

hubertp added 3 commits February 6, 2023 09:48

hubertp force-pushed the wip/hubert/infer-typeof-polyglot-values-184336928 branch from 121e463 to c175be3 Compare February 6, 2023 14:14

Update CHANGELOG

66b7a70

hubertp marked this pull request as ready for review February 6, 2023 14:23

hubertp requested a review from 4e6 as a code owner February 6, 2023 14:23

hubertp commented Feb 6, 2023

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/TypeOfNode.java Show resolved Hide resolved

Remove remaining usage of Tests.getName

5018dfb

hubertp requested a review from JaroslavTulach February 6, 2023 20:07

hubertp commented Feb 6, 2023

View reviewed changes

jdunkerley linked an issue Feb 7, 2023 that may be closed by this pull request

Polyglot values are not recognized as their Enso equivalents by the IDE #5026

Closed

JaroslavTulach approved these changes Feb 7, 2023

View reviewed changes

Merge branch 'develop' into wip/hubert/infer-typeof-polyglot-values-1…

f6d64ad

…84336928

hubertp added CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge labels Feb 8, 2023

mergify bot added 4 commits February 8, 2023 10:48

Merge branch 'develop' into wip/hubert/infer-typeof-polyglot-values-1…

82da049

…84336928

Merge branch 'develop' into wip/hubert/infer-typeof-polyglot-values-1…

15b80c5

…84336928

Merge branch 'develop' into wip/hubert/infer-typeof-polyglot-values-1…

6caf4b7

…84336928

Merge branch 'develop' into wip/hubert/infer-typeof-polyglot-values-1…

bd613bb

…84336928

mergify bot merged commit 472580d into develop Feb 9, 2023

mergify bot deleted the wip/hubert/infer-typeof-polyglot-values-184336928 branch February 9, 2023 01:06

hubertp mentioned this pull request Feb 10, 2023

Fix separate compilation for ConstantsGen #5630

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report type of expressions returning polyglot values #4111

Report type of expressions returning polyglot values #4111

hubertp commented Feb 1, 2023 •

edited

Loading

hubertp Feb 6, 2023 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Feb 7, 2023

hubertp Feb 7, 2023 •

edited

Loading

Report type of expressions returning polyglot values #4111

Report type of expressions returning polyglot values #4111

Conversation

hubertp commented Feb 1, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

hubertp Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Feb 7, 2023

Choose a reason for hiding this comment

hubertp Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

hubertp commented Feb 1, 2023 •

edited

Loading

hubertp Feb 6, 2023 •

edited

Loading

hubertp Feb 7, 2023 •

edited

Loading