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

maps: document that DeleteFunc fails in presence of NaN keys #63312

Open
candlerb opened this issue Sep 30, 2023 · 7 comments
Open

maps: document that DeleteFunc fails in presence of NaN keys #63312

candlerb opened this issue Sep 30, 2023 · 7 comments
Labels
Documentation FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@candlerb
Copy link

What version of Go are you using (go version)?

Playground 1.21: https://go.dev/play/p/-NzVmrPm-ln

Does this issue reproduce with the latest release?

Yes: https://go.dev/play/p/-NzVmrPm-ln?v=gotip

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

package main

import (
	"fmt"
	"maps"
	"math"
)

func build(n int) map[float64]int {
	m := map[float64]int{}
	for i := 0; i < n; i++ {
		m[math.NaN()] = i
		//m[float64(i)] = i
	}
	return m
}

func main() {
	m := build(10)
	for k, v := range m {
		fmt.Printf("%f %d\n", k, v)
	}
	fmt.Printf("-----\n")
	maps.DeleteFunc(m, func(k float64, v int) bool {
		return v == 7
	})
	for k, v := range m {
		fmt.Printf("%f %d\n", k, v)
	}
}

(Code based on this from @rsc)

What did you expect to see?

One element deleted from the map: the one where DeleteFunc's predicate returns true (v == 7).

What did you see instead?

No elements are deleted from the map.

However, if I replace m[math.NaN()] = i with m[float64(i)] = i, so that the map has all distinct non-NaN keys, it works.
@seankhliao
Copy link
Member

I believe this is unfortunate but working as intended.
clear is the only way NaN keys can be removed.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Sep 30, 2023
@candlerb
Copy link
Author

clear is the only way NaN keys can be removed.

I didn't see this documented either with maps.DeleteFunc, or with map types or clear. Maybe it should be?

Aside: there's some possibly unfortunate language relating to min/max here: "assuming all NaNs are equal". NaNs are not equal - not even to themselves. But I believe that all NaNs are equivalent or fungible.

@seankhliao
Copy link
Member

The inequality of NaNs come from:

Two floating-point values are compared as defined by the IEEE-754 standard.

@candlerb
Copy link
Author

Yes I understand that. I was querying the language of the golang spec which says "assuming all NaNs are equal"

@randall77
Copy link
Contributor

We should document this behavior, if nothing else. I'll reopen for that.

It is possible we could let DeleteFunc to dive into the runtime to delete entries which have NaN keys. Since DeleteFunc is already released with the current behavior, changing its behavior is a breaking change. Although I think it's pretty easy to justify this as a bug fix, at the same time I'm not seeing any strong reason to fix it, and compatibility argues for keeping the behavior as is.

@randall77 randall77 reopened this Sep 30, 2023
@seankhliao seankhliao added Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 1, 2023
@bcmills
Copy link
Contributor

bcmills commented Oct 13, 2023

(Compare #20660.)

@bcmills bcmills mentioned this issue Oct 13, 2023
Open
@gopherbot
Copy link

Change https://go.dev/cl/553157 mentions this issue: maps: document that DeleteFunc can't remove NaN keys

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. labels Jan 4, 2024
@dmitshur dmitshur added this to the Backlog milestone Jan 4, 2024
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 4, 2024
@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Jan 4, 2024
@dmitshur dmitshur changed the title maps: DeleteFunc fails in presence of NaN keys maps: document that DeleteFunc fails in presence of NaN keys Jan 4, 2024
@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
6 participants
@candlerb @dmitshur @bcmills @randall77 @gopherbot @seankhliao