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

x/tools/gopls: Incorrect warnings that yield maybe called again #70598

Closed
renthraysk opened this issue Nov 27, 2024 · 6 comments
Closed

x/tools/gopls: Incorrect warnings that yield maybe called again #70598

renthraysk opened this issue Nov 27, 2024 · 6 comments
Assignees
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@renthraysk
Copy link

Go version

go version go1.23.3 linux/amd64 & gopls version golang.org/x/tools/gopls v0.17.0-pre.3

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/ren/.cache/go-build'
GOENV='/home/ren/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/ren/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/ren/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.3'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/ren/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/ren/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3925782217=/tmp/go-build -gno-record-gcc-switches'

What did you do?

package main

import "iter"

func D() iter.Seq[uint8] {
    return func(yield func(uint8) bool) {
        ok := yield(0x01)
        ok = ok && yield(0x01)
        ok = ok && yield(0x01)
        ok = ok && yield(0x01)
    }
}

What did you see happen?

Get 3 warnings that yield maybe called again.

What did you expect to see?

No warnings.

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Nov 27, 2024
@gopherbot gopherbot added this to the Unreleased milestone Nov 27, 2024
@adonovan
Copy link
Member

Smaller repro:

package main

func f(yield func(int) bool) {
	ok := yield(1)      // yield may be called again (on L5) after returning false
	ok = ok && yield(1) // yield may be called again (on L6) after returning false
	ok = ok && yield(1)
}

The first call to yield is unconditional, and the second depends on the result of the first, so there's no problem there. But the third depends on the phi(false, t1) node representing the two alternatives of ok && yield(1):

// func f(yield func(int) bool):
// 0:                                                                entry P:0 S:2
// 	t0 = yield(1:int)                                                  bool
// 	if t0 goto 1 else 2
// 1:                                                            binop.rhs P:1 S:1
// 	t1 = yield(1:int)                                                  bool
// 	jump 2
// 2:                                                           binop.done P:2 S:2
// 	t2 = phi [0: false:bool, 1: t1] #&&                                bool
// 	if t2 goto 3 else 4
// 3:                                                            binop.rhs P:1 S:1
// 	t3 = yield(1:int)                                                  bool
// 	jump 4
// 4:                                                           binop.done P:2 S:0
// 	t4 = phi [2: false:bool, 3: t3] #&&                                bool
// 	return

The analyzer should work harder to recursively analyze phi nodes used as conditions, especially when the phi operands are constants.

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.17.0 Nov 27, 2024
@adonovan adonovan self-assigned this Nov 27, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/632117 mentions this issue: gopls/internal/analysis/yield: peephole-optimize phi(false, x)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633196 mentions this issue: gopls/internal/analysis/yield: add comment about dataflow

gopherbot pushed a commit to golang/tools that referenced this issue Dec 3, 2024
This comment is the residue of futile couple of hours
last week playing with more sophisticated approaches
to fixing golang/go#70598

Updates golang/go#70598

Change-Id: I92fc5433189ae4558aa615bdafb7e680e8636b2e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633196
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
dennypenta pushed a commit to dennypenta/tools that referenced this issue Dec 3, 2024
This comment is the residue of futile couple of hours
last week playing with more sophisticated approaches
to fixing golang/go#70598

Updates golang/go#70598

Change-Id: I92fc5433189ae4558aa615bdafb7e680e8636b2e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/633196
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633709 mentions this issue: gopls/internal/analysis/yield: add comment about dataflow

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/633702 mentions this issue: gopls/internal/analysis/yield: peephole-optimize phi(false, x)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants