Deliver full intersection type of a value to the IDE #11583

4e6 · 2024-11-18T17:06:32Z

Pull Request Description

close #11481

Changelog:

update: handle MultiTypeValue results in the execution instrument
update: language server protocol supports multi-type values
update: GUI uses only the first type of a multi-type value when

Important Notes

GUI uses only the first type of the intersection. See the difference between Integer&Text and Text&Integer:

enso-11481-intersection-types.mp4

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.

JaroslavTulach

The change itself is fine, but it reveals long term unresolved issues:

are incompatible changes in the protocol OK?
versioning on the protocol is missing
duplication of functionality instead of encapsulating it in TypeOfNode

JaroslavTulach · 2024-11-19T15:12:10Z

app/ydoc-shared/src/languageServerTypes.ts

@@ -195,7 +195,7 @@ export interface ExpressionUpdate {
  /** The id of updated expression. */
  expressionId: ExpressionId
  /** The updated type of the expression. */
-  type?: string
+  type: string[]


This is an incompatible message change. Single values used to be represented as "string" and now they will be [ "string" ]. Are we sure we don't need compatibility?

JaroslavTulach · 2024-11-19T15:14:38Z

docs/language-server/protocol-language-server.md

@@ -367,7 +367,7 @@ interface ExpressionUpdate {
  /** The id of updated expression. */
  expressionId: ExpressionId;
  /** The updated type of the expression. */
-  type?: string;
+  type: string[];


I feel incompatible changes should be versioned and documented. We should:

increase the version of the protocol

describe what change has happened and why it is incompatible

Sure, I know we have no version of the protocol. However I asked for it a year ago. Moreover recently there were complains about incompatibilities between cloud/IDE and the conclusion was: we should at least detect the incompatibilities by versioning our protocols.

engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala

...runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionCallbacks.java

JaroslavTulach · 2024-11-19T15:19:09Z

...ment-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala

@@ -198,14 +198,14 @@ object ProgramExecutionSupport {
        val notExecuted =
          methodCallsCache.getNotExecuted(executionFrame.cache.getCalls)
        notExecuted.forEach { expressionId =>
-          val expressionType = executionFrame.cache.getType(expressionId)
-          val expressionCall = executionFrame.cache.getCall(expressionId)
+          val expressionTypes = executionFrame.cache.getType(expressionId)


Shouldn't the getType method be renamed to getTypes?

...tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeIntersectionTypesTest.scala

kazcw · 2024-11-19T15:48:30Z

docs/language-server/protocol-language-server.md

@@ -367,7 +367,7 @@ interface ExpressionUpdate {
  /** The id of updated expression. */
  expressionId: ExpressionId;
  /** The updated type of the expression. */


The current docs are not clear about some details of the protocol. An empty array seems to be treated specially; judging from the implementation, it looks like:

When the status is Pending, an empty array can indicate that the type has not been determined yet.

When the status is not Pending, an empty array indicates that no specific type is known for the value--maybe this can happen either if the type is known to be Any. Can this also happen if the type could not be determined?

Are these updates incremental? It's not clear whether in the pending state [] might be sent to indicate "no change", or every update will contain all currently-known type information even if it hasn't changed.

I updated the docs. The logic did not change. The array is just a way to encode intersection types

[] indicates no type information (previously null)

[T] represent a type of the expression (previously T)

[T,U] represents an intersection type T & U (previously not supported, probably delivered just as a T type)

hubertp

In this particular case I would agree with Jaroslav that the binary incompatible change this PR introduces might be problematic.
We could have both, type and types, fields be present for the time being at the cost of some duplication. Then GUI could make a switch to support intersection types more gently and we could deprecate (and eventually remove)type.

hubertp · 2024-11-20T10:28:18Z

engine/language-server/src/main/scala/org/enso/languageserver/search/SuggestionsHandler.scala

-    case msg: Api.ExpressionUpdates if state.isSuggestionUpdatesRunning =>
-      state.suggestionUpdatesQueue.enqueue(msg)
-
-    case Api.ExpressionUpdates(_, updates) =>


Why this change? Seems rather unrelated. Or is that feature no longer used?

Yes, after the component browser is finished, and GUI can handle suggestions itself, we don't need to do this logic anymore

engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala

JaroslavTulach

The change in TypeOfNode isn't correct and fails on Truffle asserts. I'll prepare an alternative approach.

JaroslavTulach · 2024-11-21T09:53:13Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/library/dispatch/TypeOfNode.java

@@ -75,6 +75,11 @@ Object doWarning(WithWarnings value, @Cached TypeOfNode withoutWarning) {
    return withoutWarning.execute(value.getValue());
  }

+  @Specialization
+  Object doEnsoMultiValue(EnsoMultiValue value) {
+    return value.allTypes();


Hello Dmitry,
thanks for trying to implement my request to concentrate the logic of obtaining a type array into TypeOfNode in 86bfb98, but this cannot work - the Object here means: any valid interop value and Java arrays aren't such values.

Such a code change isn't likely to pass the Truffle assert checks as for example this failure indicates

The change has to be done differently. I'll prepare a PR for review:

Giving TypeOfNode richer query API #11618

With 87e738b there is findAllTypes method that provides all the Type[] associated with a value.

I can wrap the result in EnsoObject

I can wrap the result in EnsoObject

Interesting idea, yes that would fix the Truffle assertion errors:

use ArrayLikeHelpers.wrapEnsoObjects

let's see what other failures we get

I guess, I should wait for #11618

yes, use Giving TypeOfNode richer query API #11618 API now when it has been integrated

engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala

somebody1234 · 2024-11-21T12:11:09Z

mockSuggestions.json probably needs to be regenerated here (@farmaazon?)

4e6 · 2024-11-21T16:21:26Z

@somebody1234 it does not change the suggestion interface. This change is only about the expression update.

github-actions · 2024-11-22T14:07:22Z

🧪 Storybook is successfully deployed!

📊 Dashboard:

👀 Review changes: https://www.chromatic.com/build?appId=673738989fc3e8430364197e&number=252
👨‍🎨 Preview storybook: https://673738989fc3e8430364197e-xpfjdgarac.chromatic.com/

@storybook/core - Storybook

JaroslavTulach · 2024-11-22T16:38:44Z

...runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionCallbacks.java

+    }
+
+    var typeOfNode = TypeOfNode.getUncached();
+    Type[] allTypes = value == null ? null : typeOfNode.findAllTypesOrNull(value);


- Update mock data for multi-type expression updates (#11583)

Fix failing integration tests: - Fix a Vue Teleporter crash that became reachable when the dropdown arrow is displayed more often (#11620). - Fix a new drag-and-drop test that didn't work in CI. - Update mock data for multi-type expression updates (#11583). # Important Notes - The new `ConditionalTeleport` component should be used for any `Teleport` that uses the `disabled` prop and has a `to` that isn't always a valid teleportation target.

4e6 added 3 commits November 15, 2024 18:08

feat: multi type expression update

de884a4

update: language server api

58fbc9c

update: gui

6cdeeb2

4e6 added CI: No changelog needed Do not require a changelog entry for this PR. -language-server -gui labels Nov 18, 2024

4e6 self-assigned this Nov 18, 2024

4e6 requested review from JaroslavTulach, hubertp, Akirathan, Frizi, farmaazon, vitvakatu, kazcw and AdRiley as code owners November 18, 2024 17:06

4e6 added 2 commits November 18, 2024 22:19

fix: searcher bench

cfd2dff

fix: instrument tests

386f402

JaroslavTulach approved these changes Nov 19, 2024

View reviewed changes

kazcw approved these changes Nov 19, 2024

View reviewed changes

enso-bot bot mentioned this pull request Nov 20, 2024

Allow (multi_value : Table) : Column to succeed #11482

Closed

hubertp reviewed Nov 20, 2024

View reviewed changes

4e6 added 3 commits November 20, 2024 16:57

update: TypeOfNode

86bfb98

doc: clarify type parameter

fead15c

Merge branch 'develop' into wip/db/11481-intersection-type-update

d4a56cb

JaroslavTulach requested changes Nov 21, 2024

View reviewed changes

Merge branch 'develop' into wip/db/11481-intersection-type-update

99cca04

4e6 added 2 commits November 22, 2024 13:33

Merge branch 'develop' into wip/db/11481-intersection-type-update

8b9e1eb

update: execution callbacks

8f4c728

refactor: remove intersection types test suite

f63b1b8

4e6 requested a review from JaroslavTulach November 22, 2024 16:21

JaroslavTulach approved these changes Nov 22, 2024

View reviewed changes

4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Nov 22, 2024

mergify bot merged commit 2b9ed57 into develop Nov 22, 2024
55 of 58 checks passed

mergify bot deleted the wip/db/11481-intersection-type-update branch November 22, 2024 16:41

kazcw added a commit that referenced this pull request Nov 25, 2024

Fix tableVisualisation/tests

b152acd

- Update mock data for multi-type expression updates (#11583)

kazcw added a commit that referenced this pull request Nov 25, 2024

Fix tableVisualisation/typesOnNodeHover tests

afa5460

- Update mock data for multi-type expression updates (#11583)

This was referenced Nov 25, 2024

Fix tableVisualisation/typesOnNodeHover tests #11659

Closed

Fix teleporter crash and integration tests #11656

Merged

kazcw added a commit that referenced this pull request Nov 26, 2024

Fix tableVisualisation/typesOnNodeHover tests

7ce67dd

- Update mock data for multi-type expression updates (#11583)

JaroslavTulach mentioned this pull request Nov 27, 2024

Intersection types & type checks #11600

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deliver full intersection type of a value to the IDE #11583

Deliver full intersection type of a value to the IDE #11583

4e6 commented Nov 18, 2024

JaroslavTulach left a comment

JaroslavTulach Nov 19, 2024

JaroslavTulach Nov 19, 2024

JaroslavTulach Nov 19, 2024

kazcw Nov 19, 2024

4e6 Nov 20, 2024

hubertp left a comment

hubertp Nov 20, 2024

4e6 Nov 20, 2024

JaroslavTulach left a comment

JaroslavTulach Nov 21, 2024 •

edited

Loading

4e6 Nov 21, 2024

JaroslavTulach Nov 21, 2024

4e6 Nov 21, 2024

JaroslavTulach Nov 22, 2024

somebody1234 commented Nov 21, 2024

4e6 commented Nov 21, 2024

github-actions bot commented Nov 22, 2024 •

edited by unfurl-links bot

Loading

JaroslavTulach Nov 22, 2024

Deliver full intersection type of a value to the IDE #11583

Deliver full intersection type of a value to the IDE #11583

Conversation

4e6 commented Nov 18, 2024

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somebody1234 commented Nov 21, 2024

4e6 commented Nov 21, 2024

github-actions bot commented Nov 22, 2024 • edited by unfurl-links bot Loading

🧪 Storybook is successfully deployed!

📊 Dashboard:

Choose a reason for hiding this comment

JaroslavTulach Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024 •

edited by unfurl-links bot

Loading