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/go/analysis/passes/directive: spurious requirement for blank line between //go:debug and package declaration #66046

Open
deltamualpha opened this issue Mar 1, 2024 · 6 comments
Assignees
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@deltamualpha
Copy link

deltamualpha commented Mar 1, 2024

Go version

go version go1.22.0 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/me/Library/Caches/go-build'
GOENV='/Users/me/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/me/go/pkg/mod'
GONOPROXY='private/*'
GONOSUMDB='private/*'
GOOS='darwin'
GOPATH='/Users/me/go'
GOPRIVATE='private/*'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.0/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.0/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
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 -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/vl/k9dw37lx0x5bbtzj98plsqbh0000gp/T/go-build1408921875=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Prompted by a question from Sean Sorrell in the #general channel in the Gophers Slack about using go:debug directives in test files, I wrote the following file called main_test.go::

//go:debug panicnil=1
package main_test

import "testing"

func TestFoo(t *testing.T) {
	t.Errorf("oops!")
}

According to the documentation, this should pass go vet.

What did you see happen?

go vet . returns:

./main_test.go:1:1: //go:debug directive only valid before package declaration

What did you expect to see?

A clean go vet output.

@seankhliao seankhliao changed the title govet: go:debug directives in test files are flagged as incorrectly placed cmd/vet: spurious requirement for blank line between //go:debug and package declaration Mar 1, 2024
@seankhliao seankhliao changed the title cmd/vet: spurious requirement for blank line between //go:debug and package declaration x/tools/go/analysis/passes/directive: spurious requirement for blank line between //go:debug and package declaration Mar 1, 2024
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 1, 2024
@gopherbot gopherbot added this to the Unreleased milestone Mar 1, 2024
@seankhliao
Copy link
Member

Not just tests,

fails: https://go.dev/play/p/rM-aiRMn4rd

//go:debug panicnil=1
package main

func main() {
	panic(nil)
}

vs

passes: https://go.dev/play/p/BZY9qp1ilAI

//go:debug panicnil=1

package main

func main() {
	panic(nil)
}

cc @timothy-king

@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Analysis Issues related to static analysis (vet, x/tools/go/analysis) labels Mar 1, 2024
@adonovan
Copy link
Member

adonovan commented Mar 4, 2024

I suspect the directive analyzer handles //go:debug comments this way for consistency with //go:build comments, which are "ignored adjoining the package declaration":

		// A +build comment is ignored after or adjoining the package declaration.
		if group.End()+1 >= f.Package {
			check.inHeader = false
		}

Assigning to @rsc to adjudicate the desired behavior.

@gopherbot
Copy link

Change https://go.dev/cl/569360 mentions this issue: go/analysis/passes/directive: do not report adjoining //go:debug

@ianlancetaylor
Copy link
Contributor

//go:debug comments are parsed by the same code that parses //go:build comments. So the vet warning seems consistent.

That said, unless my testing is broken the parsing does currently permit the //go:build or //go:debug comment to appear as part of the package comment. The only requirement is that the comment appear before the package clause. That may be a bug.

@deltamualpha
Copy link
Author

@ianlancetaylor I don't interpret "(preceding the package statement)" on the documentation page on go:debug as requiring the comment having a newline between it and package main, but perhaps I'm misunderstanding what "package clause"/"package statement"/"package comment" means in this context? The spec makes no reference to requiring white space between any comment and the package clause, but I suppose this is a toolchain-specific question and not a spec one.

@rsc
Copy link
Contributor

rsc commented Apr 26, 2024

// +build comments need a blank line, because otherwise a package doc comment talking about the +build feature might
accidentally turn into a semantically important comment after an unfortunate line wrap.

Newer directives of the //tool:thing form do not need that blank line and do not have the wrapping problem. As part of the richer doc comment syntax, we updated gofmt and friends to move such directives to the end of doc comments and not to print them as documentation.

Empirically, new-style directives, both //go:debug and //go:build, are recognized in package doc comments:

% cat x.go
// Hello world
//
//go:debug panicnil=2
package main
% go list -f '{{.DefaultGODEBUG}}' x.go
panicnil=2
% 
% cat x.go
// Hello world
//go:build linux
package main
% gofmt x.go | gofmt
//go:build linux

// Hello world
//
package main
% 

Since the standard tools recognize them in the package doc comment, x/tools/go/analysis should recognize them there too. But be sure not to start recognizing // +build in the package doc comment.

It is possible that the code has always been wrong but it didn't matter because gofmt moves //go:build lines out of package doc comments into stanzas of their own. So it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants