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.

Sign up for GitHub

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

Jump to bottom

Expression update contains full multi-value info #12195

Merged
merged 11 commits into from
Feb 10, 2025
Merged

Expression update contains full multi-value info #12195

merged 11 commits into from
Feb 10, 2025

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Jan 30, 2025 •
edited
Loading

Pull Request Description

close #12153

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • 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.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 30, 2025
@4e6 4e6 self-assigned this Jan 30, 2025
JaroslavTulach
JaroslavTulach reviewed Jan 31, 2025
View reviewed changes
...runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionCallbacks.java Outdated

if (publicTypes != null) {
final Type[] allTypes = typeOfNode.findAllTypesOrNull(value, true);
// Relies on the fact that allTypes appends extra types to the end of publicTypes.
Copy link
Member

@JaroslavTulach JaroslavTulach Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Make this safer:

engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala Show resolved Hide resolved
JaroslavTulach
JaroslavTulach reviewed Jan 31, 2025
View reviewed changes
engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala
* @param conversionType the available conversions
*/
case class ExpressionType(
visibleType: Vector[String],
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I like the terminology:

  • maybe we use visible type somewhere
  • but we certainly never used conversion type!

The documentation is using following terms:

  • "has been cast to (the visible part)"
  • "hidden types .... can be cast to."

I understand that the current terminology isn't perfect. We can change it, but consistently. We shouldn't be inventing conversionType term in the protocol only. If we want new term, we should update (at least) docs as well.

@4e6 4e6 force-pushed the wip/db/12153-multi-value-update branch 2 times, most recently from ffb1fe2 to e9b98b5 Compare February 6, 2025 16:56
@4e6 4e6 force-pushed the wip/db/12153-multi-value-update branch from e9b98b5 to 6de087f Compare February 6, 2025 17:07
@4e6 4e6 marked this pull request as ready for review February 6, 2025 17:13
@4e6 4e6 requested review from hubertp and Akirathan as code owners February 6, 2025 17:13
JaroslavTulach
JaroslavTulach approved these changes Feb 7, 2025
View reviewed changes
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 don't understand why the protocol gets new expressionType?: ExpressionType field. Otherwise kinda OK.

docs/language-server/protocol-language-server.md Outdated Show resolved Hide resolved
docs/language-server/protocol-language-server.md Show resolved Hide resolved
docs/language-server/protocol-language-server.md Show resolved Hide resolved
engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/TypeInfo.java Outdated Show resolved Hide resolved
...runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionCallbacks.java Outdated Show resolved Hide resolved
...runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionCallbacks.java Outdated Show resolved Hide resolved
...runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionCallbacks.java Outdated Show resolved Hide resolved
engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/TypeInfo.java
* @param visibleType the public type of the value visible to the user
* @param hiddenType the list of types the value can be converted to
*/
public record TypeInfo(String[] visibleType, String[] hiddenType) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also use plural?

...e/runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionService.java
cachedHiddenType = cachedTypeInfo.hiddenType();
}

return !Arrays.equals(visibleType, cachedVisibleType)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this delegate to typeInfo.equals(...) method? Once properly implemented to deal with String[] or once we use List<String> instead?

engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/TypeInfo.java
* @param visibleType the public type of the value visible to the user
* @param hiddenType the list of types the value can be converted to
*/
public record TypeInfo(String[] visibleType, String[] hiddenType) {
Copy link
Member

Choose a reason for hiding this comment

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

record is immutable structure, but it references mutable String[] arrays. Isn't it better to use List<String> and make it immutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is close to runtime, so I decided that arrays are a better choice than lists

@enso-bot enso-bot bot mentioned this pull request Feb 7, 2025
Open
2 tasks
@4e6 4e6 added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Feb 10, 2025
hubertp
hubertp approved these changes Feb 10, 2025
View reviewed changes
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

nit: please move equality check test to TypeInfo, as Jaroslav suggested.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Feb 10, 2025
@mergify mergify bot merged commit 7d5bee6 into develop Feb 10, 2025
48 of 50 checks passed
@mergify mergify bot deleted the wip/db/12153-multi-value-update branch February 10, 2025 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hubertp hubertp hubertp approved these changes

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@Akirathan Akirathan Awaiting requested review from Akirathan Akirathan is a code owner

Assignees

@4e6 4e6

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expression update should contain full multi-value info
3 participants
@4e6 @hubertp @JaroslavTulach