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: check files not in current build context #29901

Closed
nhooyr opened this issue Jan 23, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@nhooyr
Copy link
Contributor

commented Jan 23, 2019

Lets say this file was in a package (it's used to go generate a revision file):

// +build ignore

package main

import (
	"context"
	"fmt"
	"go/format"
	"io/ioutil"
	"log"
	"os/exec"
	"strings"
	"time"
)

func main() {
	log.SetFlags(0)

	log.Println("%v", 3)
	revision := revision()
	writeRevision(revision)
}

func revision() string {
	ctx := context.Background()
	ctx, cancel := context.WithTimeout(ctx, time.Second*30)
	defer cancel()

	git := exec.CommandContext(ctx, "git", "describe", "--dirty", "--always")
	revisionBytes, err := git.Output()
	if err != nil {
		log.Fatalf("failed to run `git describe`: %v", err)
	}

	revision := string(revisionBytes)
	return strings.TrimSpace(revision)
}

func writeRevision(revision string) {
	file := revisionFile(revision)

	formattedFile, err := format.Source(file)
	if err != nil {
		log.Fatalf("failed to format revision file %q: %v", string(file), err)
	}

	err = ioutil.WriteFile("revision.go", formattedFile, 0644)
	if err != nil {
		log.Fatalf("failed to write revision.go: %v", err)
	}
}

func revisionFile(revision string) []byte {
	tmpl := `package main

//go:generate go run revision_gen.go
var revision = %q
`

	return []byte(fmt.Sprintf(tmpl, revision))
}

go veting the package would not also vet this file. You can confirm by passing the file in explicitly afterwards and seeing vet complain about line 19.

@ianlancetaylor ianlancetaylor changed the title vet: check files not in current build context cmd/vet: check files not in current build context Jan 24, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

It's generally correct for cmd/vet to ignore files with a +build ignore build constraint. vet respects build tags.

You should be able to run vet on that file via go vet ignoredfile.go.

@nhooyr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

Why is that considered correct? vet isn't building those files, its vetting them and that should happen to all files regardless of whether or not they are in the current build. Vet reports the package's correctness, which should include reporting on files that will not be built on the current platform.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

vet type checks the package, which can't be done if it reads files with ignored build tags. The type checking enables much more powerful checks.

I'm not saying that it's not useful to look at all files, just that vet is not the tool for that.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Agreed, when vet is asked to vet a package, it should look only at the files that make up that package. This is even more necessary now that vet gets complete type information.

If you want vet to look at a different set of files, you can pass the list to it just like you can do to go build. Even if we wanted to check all files, we can't know which combination of them would compile. In your example that file should be compiled on its own, while in other cases the extra files should be considered as part of the package they are in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.