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

cmd/vet: mishandling of top block comment #46169

Closed
yrashk opened this issue May 14, 2021 · 5 comments
Closed

cmd/vet: mishandling of top block comment #46169

yrashk opened this issue May 14, 2021 · 5 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@yrashk
Copy link

yrashk commented May 14, 2021

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

$ go version
go version go1.16.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/yrashk/.cache/go-build"
GOENV="/home/yrashk/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/yrashk/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/yrashk/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/nix/store/lm96gcrii9xgyh66c01y21hp95rrk8zp-go-1.16.2/share/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/nix/store/lm96gcrii9xgyh66c01y21hp95rrk8zp-go-1.16.2/share/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/yrashk/Projects/bpxe/bpxe/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/nix-shell.x8yA87/go-build891825899=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Running go vet on this file:

/*
 License header here
*/

// +build sometag

package main

What did you expect to see?

Nothing

What did you see instead?

./example.go:5:1: +build comment must appear before package clause and be followed by a blank line

My experimentation shows that if I convert the license header to the single-line form (//), it works fine -- but it doesn't
with block comments, which I think it should, unless I am missing something.

@yrashk yrashk changed the title cmd/vet: cmd/vet: mishandling of top block comment May 14, 2021
yrashk added a commit to yrashk/bpxe that referenced this issue May 14, 2021
BSL license stipulates that every source file should have one:

> All source files under BSL have a Change Date and the name of an Open
Source license in the header. The Change Date specifies when the source
file changes from BSL to the specified Open Source license. On the
Change Date, or the fourth anniversary of the first publicly available
distribution of the code under the BSL, whichever comes first, the code
automatically becomes available under the Change License (i.e., an Open
Source license).

Solution: use an automatic header insertion tool (go-license)
and [go-header](https://github.com/denis-tingaikin/go-header) lint
(golangci-run includes it)

Originally, I wanted to have empty comment lines between clauses of the
license header (copyright, reference to LICENSE and Change Date clause),
but go-license and go-header didn't quite agree on how to handle empty
lines and go-header was complaining and failing the check. They did
agree when I used a block comment to insert the license, but `go vet`
didn't: golang/go#46169

So I just removed the empty comment lines to get this done for now.

Closes yrashk-archived#13
@heschi heschi added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 14, 2021
@heschi heschi added this to the Backlog milestone May 14, 2021
@heschi
Copy link
Contributor

heschi commented May 14, 2021

I believe @rsc wrote this analysis.

@guodongli-google
Copy link

The "buildtag" checker seems to process only line comments starting with "//" when determining the cutoff point. It needs to be extent to support block comments.
Isn't copyright comment assumed to use line comments by default? e.g.

// Copyright 2020 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// ...

// +build

BTW, you may be using the old "buildtag" checker, although the new one will have the same issue.

@yrashk
Copy link
Author

yrashk commented May 17, 2021

I tried to use block comments becausr https://github.com/denis-tingajkin/go-header lint and https://github.com/palantir/go-license couldn't agree in how to treat empty lines in those headers. They agreed when block comments were used. But go vet doesn't agree with the build tag in this case.

So I currently settled on a header without new lines. Not perfect, but good enough for now.

But I found this behavior to be off as it's specified to be a comment, and not limited to line comments

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label May 17, 2021
@zpavlinovic
Copy link
Contributor

zpavlinovic commented May 24, 2021

The documentation says

Constraints may appear in any kind of source file (not just Go), but they must appear near the top of the file, preceded only by blank lines and other **line** comments.

@yrashk Did you perhaps encounter some other documentation that did not specify the exact type of comments in question?

@yrashk
Copy link
Author

yrashk commented May 24, 2021 via email

@yrashk yrashk closed this as completed May 24, 2021
@golang golang locked and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants