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.

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

x/tools/go/analysis/unitchecker: does not exit 1 when json output is enabled #53509

Closed
rvignesh89 opened this issue Jun 23, 2022 · 3 comments
Closed

x/tools/go/analysis/unitchecker: does not exit 1 when json output is enabled #53509

rvignesh89 opened this issue Jun 23, 2022 · 3 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. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@rvignesh89
Copy link

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

$ go version
go version go1.18.3 darwin/arm64

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="arm64"
GOBIN=""
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPRIVATE=""
GOROOT="/opt/homebrew/Cellar/go/1.18.3/libexec"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.18.3/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/w_/gzh0xkz92yng_xbzrtm7b1lr0000gq/T/go-build2812371200=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Ran go vet on a test file,

$ (go vet -json test.go &>/dev/null && echo "ok") || echo "failed"
// test.go
package main

import (
	"encoding/json"
	"fmt"
)

type Event struct {
	Id int `json:"id",omitempty`
}

func main() {
	byt := []byte(`{"id":42}`)
	var event Event
	if err := json.Unmarshal(byt, &event); err != nil {
		panic(err)
	}
	fmt.Println(event)
}

What did you expect to see?

go vet fails because Id field tag is not formatted correctly and prints "failed" in the shell.

What did you see instead?

It prints ok.

I think this is because if the json flag is enabled the command exits gracefully in this branch.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jun 23, 2022
@gopherbot gopherbot added this to the Unreleased milestone Jun 23, 2022
@timothy-king
Copy link
Contributor

My inclination is to stick with exitcode of 0 when json is enabled. x/tools/go/analysis/internal/checker.go states:

// printDiagnostics prints the diagnostics for the root packages in either
// plain text or JSON format. JSON format also includes errors for any
// dependencies.
//
// It returns the exitcode: in plain mode, 0 for success, 1 for analysis
// errors, and 3 for diagnostics. We avoid 2 since the flag package uses
// it. JSON mode always succeeds at printing errors and diagnostics in a
// structured form to stdout.

In this view unitchecker.Run succeeds when it has printed errors and diagnostics when in json mode. This makes sense [to me] as json mode is for building tools and that interpret the json output itself. In this setting exitcode!=0 is a problem with running the tool.

The change that may be appropriate is that the documentation for unitchecker.Run is maybe too vague about what "appropriate error code" means.

// Run reads the *.cfg file, runs the analysis,
// and calls os.Exit with an appropriate error code.
// It assumes flags have already been set.
func Run(configFile string, analyzers []*analysis.Analyzer) {

@adonovan @zpavlinovic thoughts?

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Jun 23, 2022
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 24, 2022
@zpavlinovic
Copy link
Contributor

Sticking with exitcode 0 looks correct to me.

@rvignesh89
Copy link
Author

I agree with json mode always exiting with 0 is better as it helps build on top of the result. I didn't read the comment for the unitchecker.run before don't think it's incorrect in what it's saying. So I'll close this issue. @timothy-king if you'd like to re-open to fix the comment feel free!

@golang golang locked and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@cagedmantis @timothy-king @zpavlinovic @rvignesh89 @gopherbot