Implement `type_of` #3722

hubertp · 2022-09-19T21:24:43Z

Pull Request Description

This change implements a simple type_of method that returns a type of a given value, including for polyglot objects.

The change also allows for pattern matching on various time-related instances. It is a nice-to-have on its own, but it was primarily needed here to write some tests. For equality checks on types we currently can't use == due to a known feature which essentially does wrong dispatching. This will be improved in the upcoming statics PR so we agreed that there is no point in duplicating that work and we can replace it later.

Also, note that this PR changes Meta.is_same_object. Comparing types revealed that it was wrong when comparing polyglot wrappers over the same value.

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.

JaroslavTulach

I like it. Few comments and opinions left inline.

...time/src/main/java/org/enso/interpreter/node/expression/builtin/meta/TypeOfPolyglotNode.java

...untime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/TimeOfDayBranchNode.java

JaroslavTulach · 2022-09-20T04:01:19Z

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Time/Time_Zone.enso

@@ -140,4 +140,7 @@ type Time_Zone

    ## Compares two Zones for equality.
    == : Time_Zone -> Boolean
-    == self that = Time_Utils.equals_zone self that
+    == self that =


I believe this can be written as:

== self that = case Meta.type_of that of Time_Zone -> Time_Utils.equals_zone self that _ -> False

that would mean less changes in the engine now. Moroever this kind of code would remain valid in the future even with by type pattern matches

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso

test/Tests/src/Semantic/Meta_Spec.enso

radeusgd

Looks good to me overall.

I don't like the is_of_type function - the name is pretty confusing to me. I'd love if we could just fix equality on types. If we cannot, can we at least rename the function to something indicating that it is just checking type equality? And updating the docstring to say that also (currently "matching" of two types is a concept that is not very well-defined and can be associated with other concepts like subtyping, so it is just confusing). May be worth adding a note in the docstring why this function exists and == cannot just be used.

If possible, would be cool if the builtin could handle numeric types and errors too - for uniformity. If not - can we get a descritption in a comment somewhere - why? So that we don't waste time revisiting this in the future if we forget the 'why'.

Lastly, I think we absolutely need a test suite for the case-of pattern matching.

Currently, I'm pretty sure

case Date_Time.now of
    Time_Zone -> "TZ!"
    _ -> "..."

will match and return TZ!. Is this what we want? I'm not sure but intuition tells me that nope.
So I think we should add proper test cases to show it works correctly and amend the branch nodes too.

CHANGELOG.md

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Time/Date.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso

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

test/Tests/src/Semantic/Meta_Spec.enso

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

hubertp · 2022-09-26T10:50:31Z

I changed type_of tests to use Meta.is_same_object rather than the new function. I reused some of the logic from it though (see isIdentical) to fix wrong semantics for polyglot values. Added a bunch of tests, changed the subtyping for date/time to what was requested.

I couldn't use ==, as we all wished, to test types because that is currently broken and it is a known feature. Should be fixed once work on statics is finalized.

radeusgd

Looks good to me, just a few suggestions and a question.

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso

...runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/DateTimeBranchNode.java

...runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/TimeZoneBranchNode.java

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

test/Tests/src/Data/Time/Date_Spec.enso

test/Tests/src/Semantic/Case_Spec.enso

test/Tests/src/Semantic/Meta_Spec.enso

This change implements a simple `type_of` method that returns a type of a given value, including for polyglot objects. The change also allows for patterning matching on various time-related instances. It is a nice-to-have on its own, but it was primarily needed here to write some tests.

Addressed PR comments - eliminated `Meta.type_of_polyglot_builtin` and merged its implementation into a single node. Added `is_of_type` to avoid using `==` checks on types, which doesn't work as expected, half the time.

Removed `is_of_type` and moved some of the logic to `is_same_object`. Turns out the latter was actually broken when comparing polyglot values with the same underlying java class. Additionally, discovered why I can't simply use `==` - the dispatch for types is broken and it's a known "feature". This will be improved in the upcoming statics change so I'm not going to duplicate that work. Once in, we can most likely get rid of `should_equal_type` and `should_not_equal_type`. Addressed comments re date/time-related branches in pattern matching so that it doesn't try to do semi-subtyping checks but rather checks for equality. Moved all logic of `type_of` to the builtin node. Added tests for `type_of` of Date, Date_Time, Time_Of_Day and Time_Zone. Might still change `type_of` to ensure that java classes aren't leaking.

We want to make sure that the type of Java's LocalTime or LocalDate etc is a corresponding Enso type, Enso's Time_Of_Day or Date, respectively. Added more tests, including those that ensure lack of subtyping between Enso's Date, Date_Time, Time_Of_Day and Time_Zone.

Co-authored-by: Radosław Waśko <[email protected]>

hubertp force-pushed the wip/hubert/typeof-183188858 branch from aba7f00 to d66e6e9 Compare September 19, 2022 21:26

JaroslavTulach approved these changes Sep 20, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/typeof-183188858 branch from 24fe232 to 00d449d Compare September 22, 2022 13:46

hubertp changed the title ~~Implement type_of method~~ Implement type_of Sep 22, 2022

hubertp marked this pull request as ready for review September 22, 2022 14:08

hubertp requested review from jdunkerley, radeusgd and 4e6 as code owners September 22, 2022 14:08

radeusgd reviewed Sep 22, 2022

View reviewed changes

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

hubertp force-pushed the wip/hubert/typeof-183188858 branch from 7cb1a7a to 2f9307e Compare September 23, 2022 19:20

hubertp requested review from radeusgd and JaroslavTulach September 26, 2022 10:45

hubertp force-pushed the wip/hubert/typeof-183188858 branch from 46fdc62 to 65cadeb Compare September 26, 2022 10:51

radeusgd approved these changes Sep 26, 2022

View reviewed changes

hubertp and others added 10 commits September 26, 2022 16:48

Added is_of_type method to easier test types

0b2c25a

Addressed PR comments - eliminated `Meta.type_of_polyglot_builtin` and merged its implementation into a single node. Added `is_of_type` to avoid using `==` checks on types, which doesn't work as expected, half the time.

Update changelog

e42ffe4

formatting

639487a

PR comment

bb6560e

PR review

097bfbb

Apply suggestions from code review

228500e

Co-authored-by: Radosław Waśko <[email protected]>

Remove is_an completely

ba4f1bf

hubertp force-pushed the wip/hubert/typeof-183188858 branch from 951fcf6 to ba4f1bf Compare September 26, 2022 14:48

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Sep 26, 2022

mergify bot merged commit 7a6ee0c into develop Sep 26, 2022

mergify bot deleted the wip/hubert/typeof-183188858 branch September 26, 2022 16:01

wdanilo mentioned this pull request Feb 6, 2023

Test Rust based IR in CI #4568

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `type_of` #3722

Implement `type_of` #3722

hubertp commented Sep 19, 2022 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Sep 20, 2022

radeusgd left a comment

hubertp commented Sep 26, 2022

radeusgd left a comment

Implement type_of #3722

Implement type_of #3722

Conversation

hubertp commented Sep 19, 2022 • edited Loading

Pull Request Description

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Sep 20, 2022

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

hubertp commented Sep 26, 2022

radeusgd left a comment

Choose a reason for hiding this comment

Implement `type_of` #3722

Implement `type_of` #3722

hubertp commented Sep 19, 2022 •

edited

Loading