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.

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

NaN cannot be used as a key in Map #5833

Closed
1 of 4 tasks
JaroslavTulach opened this issue Mar 7, 2023 · 5 comments · Fixed by #6301
Closed
1 of 4 tasks

NaN cannot be used as a key in Map #5833

JaroslavTulach opened this issue Mar 7, 2023 · 5 comments · Fixed by #6301
Assignees
Labels
--bug Type: bug --data corruption or loss Important: data corruption or loss -compiler

Comments

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Mar 7, 2023

The primary motivation forMap existance is implementation of Vector.distinct. To make such implementation easy/fast/possible we don't want errors from Map.insert. On contrary, we want every object to provide hash code - so it can be used as a key for the Map. Constraints:

  1. it is desirable for Map to always satisfy: m.insert k v . contains_key k == True
  2. For Number we are going to use JavaScript's Equality comparisons and sameness to ensure NaN works as a key
  3. Custom types - shall have a forced reflexivity - e.g. Meta.is_same_object implies that they are Any.== which in turn satisfies the first point

Following code shows the expected behavior for custom types that violate reflexivity:

type My_Nan
    Value comment:Text

...comparator such that it always returns Nothing

k = My_Nan.Value "foo"
k2 = My_Nan.Value "foo"
(k==k).should_be_true
(k==k2).should_be_false
m = Map.empty.insert k 10
m.contains_key k . should_be_true
m.get k . should_equal 10
m.contains_key k2 . should_be_false

m2 = m.insert k2 20
m2.get k . should_equal 10
m2.get k2 . should_equal 20
m2.size . should_equal 2

m3 = m2.insert k 30
m3.size . should_equal 2
m3.get k . should_equal 30

e.g. , we will do a best effort solution. Thanks to Any.== relying on Meta.is_same_object, even if compare implementation is always incomparable, x==x will be True. Multiple separate instances of incomparable atoms will however not be.

Follow up of

Tasks

Preview Give feedback
@JaroslavTulach JaroslavTulach added --bug Type: bug -compiler --data corruption or loss Important: data corruption or loss labels Mar 7, 2023
@github-project-automation github-project-automation bot moved this to ❓New in Issues Board Mar 7, 2023
@JaroslavTulach JaroslavTulach moved this from ❓New to 📤 Backlog in Issues Board Mar 7, 2023
@jdunkerley jdunkerley moved this from 📤 Backlog to ❓New in Issues Board Mar 7, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Mar 7, 2023
@jdunkerley jdunkerley added this to the Design Partners milestone Mar 7, 2023
@radeusgd
Copy link
Member

radeusgd commented Mar 7, 2023

Probably also worth adding a test for Vector.distinct:

[Number.nan, 1, Number.nan, 2, 1].distinct.to_text . should_equal "[NaN, 1, 2]"

(using to_text as regular equality may actually yield False due to the NaNs...)

@radeusgd
Copy link
Member

Let's be sure to test the custom types properly, i.e. have tests for a custom type like My_Nan above.

As @Akirathan mentioned, this may be blocked by #5709

@jdunkerley jdunkerley moved this from 📤 Backlog to ❓New in Issues Board Mar 28, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Mar 28, 2023
@Akirathan Akirathan moved this from 📤 Backlog to 🔧 Implementation in Issues Board Apr 3, 2023
@Akirathan Akirathan moved this from 🔧 Implementation to ⚙️ Design in Issues Board Apr 3, 2023
@jdunkerley jdunkerley moved this from ⚙️ Design to 📤 Backlog in Issues Board Apr 4, 2023
@Akirathan Akirathan moved this from 📤 Backlog to 🔧 Implementation in Issues Board Apr 13, 2023
@Akirathan Akirathan linked a pull request Apr 17, 2023 that will close this issue
5 tasks
@enso-bot
Copy link

enso-bot bot commented Apr 17, 2023

Pavel Marek reports a new STANDUP for today (2023-04-17):

Progress: Updating bench results - I had broken cache locally, so I have spent some time investigating why some older bench results are not displayed. Benchmarks https://enso-org.github.io/engine-benchmark-results/ now contain results from Dec 2022, until today. NaN can now be used as a key in the map, and complies to the Same value zero equality JS specs It should be finished by 2023-04-20.

@enso-bot
Copy link

enso-bot bot commented Apr 18, 2023

Pavel Marek reports a new STANDUP for today (2023-04-18):

Progress: Polish, add some tests. Ready for review. Bookclub, review other PRs. It should be finished by 2023-04-20.

@mergify mergify bot closed this as completed in #6301 Apr 20, 2023
@github-project-automation github-project-automation bot moved this from 🔧 Implementation to 🟢 Accepted in Issues Board Apr 20, 2023
@enso-bot
Copy link

enso-bot bot commented Apr 20, 2023

Pavel Marek reports a new STANDUP for yesterday (2023-04-19):

Progress: Last nits before merging, adding some last tests. It should be finished by 2023-04-20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug --data corruption or loss Important: data corruption or loss -compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants