Skip to content

proposal: slices: Add Find #71536

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

Closed
icholy opened this issue Feb 3, 2025 · 9 comments
Closed

proposal: slices: Add Find #71536

icholy opened this issue Feb 3, 2025 · 9 comments
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool Proposal
Milestone

Comments

@icholy
Copy link

icholy commented Feb 3, 2025

Proposal Details

A function for finding the first value in a slice which satisfies a predicate function.

package slices

// Find returns the first value satisfying f.
// The second return value reports whether a value was found.
func Find[S ~[]E, E any](s S, f func(E) bool) (E, bool) {
	for _, v := range s {
		if f(v) {
			return v, true
		}
	}
	var z E
	return z, false
}
@icholy icholy added the Proposal label Feb 3, 2025
@gopherbot gopherbot added this to the Proposal milestone Feb 3, 2025
@ianlancetaylor
Copy link
Contributor

We already have https://pkg.go.dev/slices#IndexFunc.

@icholy
Copy link
Author

icholy commented Feb 3, 2025

I have this code:

var prev *github.CheckRun
for _, r := range res.CheckRuns {
	if r.GetName() == "mergelock" {
		prev = r
		break
	}
}

Using slices.IndexFunc looks like this (which is the same number of lines and harder to read):

var prev *github.CheckRun
prevIdx := slices.IndexFunc(res.CheckRuns, func(r *github.CheckRun) bool {
	return r.GetName() == "mergelock"
})
if prevIdx >= 0 {
	prev = res.CheckRuns[prevIdx]
}

It would be nice if I could write:

prev, _ := slices.Find(res.CheckRuns, func(r *github.CheckRun) bool {
	return r.GetName() == "mergelock"
})

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Feb 3, 2025
@gabyhelp
Copy link

gabyhelp commented Feb 3, 2025

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@ianlancetaylor
Copy link
Contributor

To make any argument for adding this function when we already have IndexFunc, it would be good to see examples in existing Go code (code that you didn't write) where it could be used to simplify the code. Thanks.

@gabyhelp gabyhelp added the LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool label Feb 3, 2025
@icholy
Copy link
Author

icholy commented Feb 3, 2025

Here's an example:

var curveID CurveID
for _, c := range clientHello.supportedCurves {
if config.supportsCurve(ka.version, c) {
curveID = c
break
}
}
if curveID == 0 {
return nil, errors.New("tls: no supported elliptic curves offered")
}

This could be rewritten as follows:

curveID, ok := slices.Find(clientHello.supportedCurves, func(c CurveID) bool {
	return config.supportsCurve(ka.version, c)
})
if !ok {
	return nil, errors.New("tls: no supported elliptic curves offered")
}

Here are some others:

var bss *Section
for _, sect := range f.Sections {
if sect.Name == ".bss" {
bss = sect
break
}
}
if bss == nil {
t.Fatal("could not find .bss section")
}

var s *profile.Sample
for _, sample := range p.Sample {
if sampleToString(sample) == expectedSample {
s = sample
break
}
}
if s == nil {
t.Fatalf("expected \n%s\ngot\n%s", expectedSample, strings.Join(profileToStrings(p), "\n"))
}

go/src/os/stat_test.go

Lines 99 to 109 in 37f27fb

var lsfi2 fs.FileInfo
for _, fi2 := range fis {
if fi2.Name() == base {
lsfi2 = fi2
break
}
}
if lsfi2 == nil {
t.Errorf("failed to find %q in its parent", path)
return
}

var wsro *elf.Section
for _, s := range elfFile.Sections {
if s.Name == wsroname {
wsro = s
break
}
}
if wsro == nil {
if k == 0 {
t.Fatalf("test %s: can't locate %q section",
test.name, wsroname)
}
continue
}

var iEntry *dwarf.Entry
for _, child := range childDies {
if child.Tag == dwarf.TagVariable && child.Val(dwarf.AttrName).(string) == "i" {
iEntry = child
break
}
}
if iEntry == nil {
t.Fatalf("didn't find DW_TAG_variable for i in main.main")
}

var arch *sys.Arch
for _, a := range sys.Archs {
if a.Name == e.Obj.Arch {
arch = a
break
}
}

compiler := ""
for _, line := range lines {
if strings.HasPrefix(line, " ") && !strings.HasPrefix(line, " (in-process)") {
compiler = line
break
}
}
if compiler == "" {
return "", "", fmt.Errorf("%s: can not find compilation command in %q", name, out)
}

@Agent-Hellboy
Copy link

I don't have much experience, but I don't think we should bloat the standard libraries or add new built-in function, just for cleaner code. Look at the heap library, I'm becoming fan of Go because of its simplicity.

All of the example code you have shared from go code base is pleasing to my eye. There is no need of Find

@seankhliao
Copy link
Member

see previously #52006 (comment)

@icholy
Copy link
Author

icholy commented Feb 3, 2025

That is, there is still a required result check, and it still is fundamentally a statement. The win is entirely not repeating x[ ] around the result.

This is not correct. In a lot of cases the check is unnecessary and may be omitted.

@ianlancetaylor
Copy link
Contributor

@icholy Thanks for the example, but I have to say that if somebody sent me a patch to change the current code in key_agreement.go to use your suggested replacement, I would reject the patch. The original code is longer but to me it seems clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool Proposal
Projects
None yet
Development

No branches or pull requests

6 participants