NaN can be used as a key in Map #6301

Akirathan · 2023-04-17T08:55:04Z

Pull Request Description

Number.nan can be used as a key in Map. This PR basically implements the support for JavaScript's Same Value Zero Equality so that Number.nan can be used as a key in Map.

Important Notes

For NaN, it holds that Meta.is_same_object Number.nan Number.nan, and Number.nan != Number.nan - inspired by JS spec.
Meta.is_same_object x y implies Any.== x y, except for Number.nan.

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,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

mwu-tow · 2023-04-17T12:38:46Z

@Akirathan

Number.nan can be used as a key in Map.

Why do we want this? For me NaN is the value that is not equal to itself. That by definition kind of prevents it from being useful as a key in the map.
Even if we wanted this, I'd rather expect some kind of byte-representation-comparison rather than just NaN==NaN assumption. There are different values (byte-wise) that are considered to be a 'NaN'. Do we want to disregard the differences?

Akirathan · 2023-04-17T13:37:43Z

@Akirathan

Number.nan can be used as a key in Map.

Why do we want this? For me NaN is the value that is not equal to itself. That by definition kind of prevents it from being useful as a key in the map. Even if we wanted this, I'd rather expect some kind of byte-representation-comparison rather than just NaN==NaN assumption. There are different values (byte-wise) that are considered to be a 'NaN'. Do we want to disregard the differences?

As noted in the issue description #5833, we want to comply with JavaScript's same-value-zero equality. The reason for that is because we want every value in Enso, including polyglot values, to have some hash code, and thus be used as a key in the map.

…ity. Spec at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness#same-value-zero_equality

This reverts commit 32d81f4.

# Conflicts: # distribution/lib/Standard/Base/0.0.0-dev/src/Any.enso

This is not according to the JS spec.

Akirathan · 2023-04-17T17:09:31Z

test/Tests/src/Data/Map_Spec.enso

+            Map.singleton (Map.singleton Number.nan 1) 42 . keys . at 0 . get Number.nan . should_equal 1
+            Map.singleton (Map.singleton Number.nan 1) 42 . at (Map.singleton Number.nan 1) . should_equal 42
+
+        Test.specify "should support atoms with custom comparators that violate reflexivity as keys" <|


This test was copied from the issue description

test/Tests/src/Semantic/Meta_Spec.enso

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

JaroslavTulach · 2023-04-18T10:01:30Z

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

+      Atom other,
+      @Cached EqualsAtomNode equalsNode,
+      @Cached IsSameObjectNode isSameObjectNode) {
+    return isSameObjectNode.execute(self, other) || equalsNode.execute(self, other);


This is the fix for #6065, right?

Yes, #6065 is addressed in these commits:

3507484

55ceaba

JaroslavTulach · 2023-04-18T10:03:53Z

...untime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/IsSameObjectNode.java

@@ -23,6 +25,15 @@ public static IsSameObjectNode build() {

  public abstract boolean execute(@AcceptsError Object left, @AcceptsError Object right);

+  @Specialization
+  boolean isSameDouble(double left, double right) {
+    if (Double.isNaN(left) && Double.isNaN(right)) {


This is the handling of NaN as being "same".

JaroslavTulach · 2023-04-18T10:06:24Z

test/Tests/src/Semantic/Meta_Spec.enso


        Test.specify "type_of Nothing is Nothing" <|
            Meta.type_of Nothing . should_equal Nothing

+        Test.specify "NaN and NaN should be the same object" <|
+            Meta.is_same_object Number.nan Number.nan . should_be_true
+            (Number.nan == Number.nan) . should_be_false


nan != nan, but Meta.is_same_object gives true. OK.

hubertp

Do we want/need any tests for polyglot handling of NaNs?

...untime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/IsSameObjectNode.java

tools/performance/engine-benchmarks/bench_download.py

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

radeusgd · 2023-04-19T16:12:37Z

test/Tests/src/Data/Vector_Spec.enso

+    Test.specify "should be able to handle distinct on different primitive values" <|
+        alter [1, "a"] . distinct . should_equal [1, "a"]
+        alter ["a", 1] . distinct . should_equal ["a", 1]
+        alter [Nothing, Nothing] . distinct . should_equal [Nothing]
+        alter [Number.nan, Number.nan] . distinct . to_text . should_equal "[NaN]"
+        alter [Nothing, Number.nan, Nothing, Number.nan] . distinct . to_text . should_equal "[Nothing, NaN]"
+        alter [1, Nothing] . distinct . should_equal [1, Nothing]
+        alter [Nothing, 1, Nothing] . distinct . should_equal [Nothing, 1]
+        alter [1, Number.nan] . distinct . to_text . should_equal "[1, NaN]"
+        alter [Number.nan, 1, Number.nan] . distinct . to_text . should_equal "[NaN, 1]"
+        alter [1, Number.nan, 1] . distinct . to_text . should_equal "[1, NaN]"


How does distinct work with other non-reflexive values, like My_Nan? Can we get a test for it too?

Added in 6bae728

radeusgd

Looks good, I'd just appreciate a test for My_Nan in distinct.

This also should at some point work consistently in Table.aggregate - Aggregate_Column.Group_By. I guess no need to add these tests now, but maybe we should add a note to #6292 to not forget about it? It is quite a brittle feature and easy to get wrong so it will be good to have tests for this case once available.

Check for Meta.is_same_object in Any.==

ff88018

Akirathan linked an issue Apr 17, 2023 that may be closed by this pull request

NaN cannot be used as a key in Map #5833

Closed

Add some tests for Vector.distinct

6302281

Akirathan self-assigned this Apr 17, 2023

Meta.is_same_object NaN NaN returns True

32d81f4

Akirathan added 10 commits April 17, 2023 16:02

Add Map tests for various incomparable keys

061cb79

Test that Meta.is_same_object complies with "Same Value Zero" equal…

5f35024

…ity. Spec at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness#same-value-zero_equality

Revert "Meta.is_same_object NaN NaN returns True"

6963198

This reverts commit 32d81f4.

Add docs to Map

aebbf0b

Merge branch 'develop' into wip/akirathan/nan-in-map-5833

c5b5696

# Conflicts: # distribution/lib/Standard/Base/0.0.0-dev/src/Any.enso

Any.== checks for Meta.is_same_object (#6065)

3507484

Meta.is_same_object NaN NaN returns True

cb520ed

Remove "should support vector with NaNs as keys".

268c5f3

This is not according to the JS spec.

Fix typos in tests

7720f14

bench_download tool: fail if there are no job reports

4b3baa3

Akirathan commented Apr 17, 2023

View reviewed changes

test/Tests/src/Semantic/Meta_Spec.enso Show resolved Hide resolved

Akirathan linked an issue Apr 17, 2023 that may be closed by this pull request

Any.== should check for Meta.is_same_object as the first step #6065

Closed

Akirathan added 2 commits April 18, 2023 11:31

Use IsSameObject instead of reference comparison

55ceaba

Merge branch 'develop' into wip/akirathan/nan-in-map-5833

d3b1aa7

Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 18, 2023

Akirathan marked this pull request as ready for review April 18, 2023 09:36

Akirathan requested review from jdunkerley, radeusgd, 4e6, JaroslavTulach and hubertp as code owners April 18, 2023 09:36

Small fixes after merge

a91e985

JaroslavTulach approved these changes Apr 18, 2023

View reviewed changes

JaroslavTulach mentioned this pull request Apr 18, 2023

Any.== should check for Meta.is_same_object as the first step #6065

Closed

hubertp approved these changes Apr 18, 2023

View reviewed changes

...untime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/IsSameObjectNode.java Show resolved Hide resolved

tools/performance/engine-benchmarks/bench_download.py Show resolved Hide resolved

enso-bot bot mentioned this pull request Apr 19, 2023

An Atom is recognized as a Primitive when WithWarnings wrap another WithWarnings #6258

Closed

Use markdown in docs

fc5aace

radeusgd reviewed Apr 19, 2023

View reviewed changes

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

Fix implication in docs

d65e5e9

radeusgd reviewed Apr 19, 2023

View reviewed changes

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

radeusgd reviewed Apr 19, 2023

View reviewed changes

radeusgd approved these changes Apr 19, 2023

View reviewed changes

Add more incomparable tests to Vector_Spec

6bae728

Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Apr 20, 2023

mergify bot merged commit 4076a64 into develop Apr 20, 2023

mergify bot deleted the wip/akirathan/nan-in-map-5833 branch April 20, 2023 09:25

JaroslavTulach mentioned this pull request Apr 25, 2023

Get rid of TruffleBoundary in Map.insert and Map.get #5233

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NaN can be used as a key in Map #6301

NaN can be used as a key in Map #6301

Akirathan commented Apr 17, 2023 •

edited

Loading

mwu-tow commented Apr 17, 2023

Akirathan commented Apr 17, 2023

Akirathan Apr 17, 2023

JaroslavTulach Apr 18, 2023

Akirathan Apr 18, 2023

JaroslavTulach Apr 18, 2023

JaroslavTulach Apr 18, 2023

hubertp left a comment

radeusgd Apr 19, 2023

Akirathan Apr 20, 2023

radeusgd left a comment

NaN can be used as a key in Map #6301

NaN can be used as a key in Map #6301

Conversation

Akirathan commented Apr 17, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

mwu-tow commented Apr 17, 2023

Akirathan commented Apr 17, 2023

Akirathan Apr 17, 2023

Choose a reason for hiding this comment

JaroslavTulach Apr 18, 2023

Choose a reason for hiding this comment

Akirathan Apr 18, 2023

Choose a reason for hiding this comment

JaroslavTulach Apr 18, 2023

Choose a reason for hiding this comment

JaroslavTulach Apr 18, 2023

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

radeusgd Apr 19, 2023

Choose a reason for hiding this comment

Akirathan Apr 20, 2023

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

Akirathan commented Apr 17, 2023 •

edited

Loading