Skip to content

reflect: Value.Seq panicking on functional iterator methods #71874

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
Closed
rothelius opened this issue Feb 21, 2025 · 8 comments
Closed

reflect: Value.Seq panicking on functional iterator methods #71874

rothelius opened this issue Feb 21, 2025 · 8 comments
Assignees
@mknyszek
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.25

Comments

@rothelius
Copy link

Go version

go version go1.24.0 darwin/amd64

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE='on'
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/Users/fabianr/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/fabianr/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/5y/2cd0ydxn7nl7sz2nnk3d91xc0000gn/T/go-build15690302=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/Users/fabianr/seq/go.mod'
GOMODCACHE='/Users/fabianr/e/be/gamelogic/gohome/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/fabianr/e/be/gamelogic/gohome'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/fabianr/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

Tried to call Seq() on a reflect.Value containing a func derived from a method with signature func(func(int)bool) . See this playground code.

What did you see happen?

A panic:


goroutine 1 [running]:
reflect.Value.Seq({0x4af540, 0x5a2f40, 0x213})
	/usr/local/go-faketime/src/reflect/iter.go:107 +0x559
main.main()
	/tmp/sandbox330519453/prog.go:12 +0x2b

What did you expect to see?

No panic.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 21, 2025
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 21, 2025
@mknyszek mknyszek added this to the Backlog milestone Feb 21, 2025
@mknyszek
Copy link
Contributor

Indeed, this looks like a real issue. Oddly enough, writing the canRangeFunc logic manually does seem to work as intended. (https://go.dev/play/p/fsIm8YCwWJc)

CC @golang/runtime

@mknyszek
Copy link
Contributor

I see, there's actually special logic for obtaining the type of a value that is a method value, I think. (See the implementation of reflect.Value.Type.) And that's being skipped here.

@mknyszek
Copy link
Contributor

@gopherbot Please open backport issues for Go 1.23 and Go 1.24.

@gopherbot gopherbot mentioned this issue Feb 21, 2025
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #71875 (for 1.23), #71876 (for 1.24).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Feb 21, 2025
Closed
@mknyszek mknyszek self-assigned this Feb 21, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/651416 mentions this issue: reflect: correctly handle method values in Seq

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/651515 mentions this issue: [release-branch.go1.24] reflect: correctly handle method values in Seq

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/651498 mentions this issue: [release-branch.go1.23] reflect: correctly handle method values in Seq

callthingsoff added a commit to callthingsoff/go that referenced this issue Feb 22, 2025
@callthingsoff
reflect: add {methodIter.Seq, methodIter2.Seq2} test cases for {CanSe…
8ff8431 
…q, CanSeq2}

For golang#71874.

Change-Id: Idc834fdd9ade41ba4976ded32e5861a2dcef44bf
@callthingsoff callthingsoff mentioned this issue Feb 22, 2025
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/651775 mentions this issue: reflect: add {methodIter.Seq, methodIter2.Seq2} test cases for Type.{CanSeq, CanSeq2}

callthingsoff added a commit to callthingsoff/go that referenced this issue Feb 22, 2025
@callthingsoff
reflect: add {methodIter.Seq,methodIter2.Seq2} test cases for {CanSeq…
90d63e8 
…,CanSeq2}

For golang#71874.

Change-Id: Idc834fdd9ade41ba4976ded32e5861a2dcef44bf
@gabyhelp gabyhelp mentioned this issue Feb 22, 2025
Closed
@dmitshur dmitshur modified the milestones: Backlog, Go1.25 Feb 26, 2025
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 26, 2025
@prattmic prattmic added the Critical A critical problem that affects the availability or correctness of production systems built using Go label Feb 26, 2025
gopherbot pushed a commit that referenced this issue Feb 26, 2025
@mknyszek @prattmic
[release-branch.go1.23] reflect: correctly handle method values in Seq
d89fda2 
Currently method values aren't correctly handled in Seq because we call
canRangeFunc on the reciever type, not the method value type, when we're
handling a method value. reflect.Value.Type has the logic to obtain the
method value type from the Value.

This change slightly refactors reflect.Value.Type into a separate
function so we can obtain the correct type as an abi.Type and pass it
off to canRangeFunc (and canRangeFunc2).

For #71874.
Fixes #71875.

Change-Id: Ie62dfca2a84b8f2f816bb87ff1ed1a58a7bb8122
Reviewed-on: https://go-review.googlesource.com/c/go/+/651416
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
(cherry picked from commit d93f6df)
Reviewed-on: https://go-review.googlesource.com/c/go/+/651498
Reviewed-by: Firuze Sayan <sayanfiruze@gmail.com>
gopherbot pushed a commit that referenced this issue Feb 26, 2025
@mknyszek @prattmic
[release-branch.go1.24] reflect: correctly handle method values in Seq
5ffdb9c 
Currently method values aren't correctly handled in Seq because we call
canRangeFunc on the reciever type, not the method value type, when we're
handling a method value. reflect.Value.Type has the logic to obtain the
method value type from the Value.

This change slightly refactors reflect.Value.Type into a separate
function so we can obtain the correct type as an abi.Type and pass it
off to canRangeFunc (and canRangeFunc2).

For #71874.
Fixes #71876.

Change-Id: Ie62dfca2a84b8f2f816bb87ff1ed1a58a7bb8122
Reviewed-on: https://go-review.googlesource.com/c/go/+/651416
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
(cherry picked from commit d93f6df)
Reviewed-on: https://go-review.googlesource.com/c/go/+/651515
callthingsoff added a commit to callthingsoff/go that referenced this issue Feb 28, 2025
@callthingsoff
reflect: add {methodIter.Seq,methodIter2.Seq2} test cases for {CanSeq…
574c1ed 
…,CanSeq2}

For golang#71874.

Change-Id: Idc834fdd9ade41ba4976ded32e5861a2dcef44bf
gopherbot pushed a commit that referenced this issue Feb 28, 2025
@callthingsoff @gopherbot
reflect: add more tests for Type.{CanSeq,CanSeq2}
74ba216 
For #71874.

Change-Id: I3850edfb3104305f3bf4847a73cdd826cc99837f
GitHub-Last-Rev: 574c1ed
GitHub-Pull-Request: #71890
Reviewed-on: https://go-review.googlesource.com/c/go/+/651775
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mknyszek mknyszek

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.25
Development

No branches or pull requests
5 participants
@rothelius @mknyszek @prattmic @dmitshur @gopherbot