[release-branch.go1.12] go/analysis: disable embedded struct tag check

This disables the enhancement added in golang.org/cl/115677. The
original change was done in the old cmd/vet location, so it would be
non-trivial to port a full revert of all the code changes. Simply
skipping anonymous struct fields is a simpler way to undo the effects of
the CL.

The reason we want to disable this enhancement of the check in the Go
1.12 release branch is because a false positive was uncovered, which we
want fixed for Go 1.12.1. It's possible that the check will instead be
tweaked for 1.13, but for 1.12.1 we want to play it safe.

Updates golang/go#30465.

Change-Id: I379b4547a560723b8023dad45ab26556b10ee308
Reviewed-on: https://go-review.googlesource.com/c/tools/+/164659
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

cmd/godoc/godoc_test.go

@@ -134,6 +134,46 @@ func killAndWait(cmd *exec.Cmd) {
 	cmd.Wait()
 }
 
+func TestURL(t *testing.T) {
+	if runtime.GOOS == "plan9" {
+		t.Skip("skipping on plan9; fails to start up quickly enough")
+	}
+	bin, cleanup := buildGodoc(t)
+	defer cleanup()
+
+	testcase := func(url string, contents string) func(t *testing.T) {
+		return func(t *testing.T) {
+			stdout, stderr := new(bytes.Buffer), new(bytes.Buffer)
+
+			args := []string{fmt.Sprintf("-url=%s", url)}
+			cmd := exec.Command(bin, args...)
+			cmd.Stdout = stdout
+			cmd.Stderr = stderr
+			cmd.Args[0] = "godoc"
+
+			// Set GOPATH variable to non-existing path
+			// and GOPROXY=off to disable module fetches.
+			// We cannot just unset GOPATH variable because godoc would default it to ~/go.
+			// (We don't want the indexer looking at the local workspace during tests.)
+			cmd.Env = append(os.Environ(),
+				"GOPATH=does_not_exist",
+				"GOPROXY=off",
+				"GO111MODULE=off")
+
+			if err := cmd.Run(); err != nil {
+				t.Fatalf("failed to run godoc -url=%q: %s\nstderr:\n%s", url, err, stderr)
+			}
+
+			if !strings.Contains(stdout.String(), contents) {
+				t.Errorf("did not find substring %q in output of godoc -url=%q:\n%s", contents, url, stdout)
+			}
+		}
+	}
+
+	t.Run("index", testcase("/", "Go is an open source programming language"))
+	t.Run("fmt", testcase("/pkg/fmt", "Package fmt implements formatted I/O"))
+}
+
 // Basic integration test for godoc HTTP interface.
 func TestWeb(t *testing.T) {
 	testWeb(t, false)
@@ -150,7 +190,7 @@ func TestWebIndex(t *testing.T) {
 // Basic integration test for godoc HTTP interface.
 func testWeb(t *testing.T, withIndex bool) {
 	if runtime.GOOS == "plan9" {
-		t.Skip("skipping on plan9; files to start up quickly enough")
+		t.Skip("skipping on plan9; fails to start up quickly enough")
 	}
 	bin, cleanup := buildGodoc(t)
 	defer cleanup()

cmd/godoc/main.go

@@ -94,7 +94,7 @@ type httpResponseRecorder struct {
 }
 
 func (w *httpResponseRecorder) Header() http.Header         { return w.header }
-func (w *httpResponseRecorder) Write(b []byte) (int, error) { return len(b), nil }
+func (w *httpResponseRecorder) Write(b []byte) (int, error) { return w.body.Write(b) }
 func (w *httpResponseRecorder) WriteHeader(code int)        { w.code = code }
 
 func usage() {
@@ -168,8 +168,8 @@ func main() {
 		fmt.Fprintln(os.Stderr, `Unexpected arguments. Use "go doc" for command-line help output instead. For example, "go doc fmt.Printf".`)
 		usage()
 	}
-	if *httpAddr != "" && *urlFlag != "" && !*writeIndex {
-		fmt.Fprintln(os.Stderr, "Missing args.")
+	if *httpAddr == "" && *urlFlag == "" && !*writeIndex {
+		fmt.Fprintln(os.Stderr, "At least one of -http, -url, or -write_index must be set to a non-zero value.")
 		usage()
 	}
 
@@ -228,7 +228,7 @@ func main() {
 	corpus.IndexDirectory = indexDirectoryDefault
 	corpus.IndexThrottle = *indexThrottle
 	corpus.IndexInterval = *indexInterval
-	if *writeIndex {
+	if *writeIndex || *urlFlag != "" {
 		corpus.IndexThrottle = 1.0
 		corpus.IndexEnabled = true
 		initCorpus(corpus)

go/analysis/passes/printf/printf.go

@@ -453,15 +453,23 @@ func printfNameAndKind(pass *analysis.Pass, call *ast.CallExpr) (fn *types.Func,
 }
 
 // isFormatter reports whether t satisfies fmt.Formatter.
-// Unlike fmt.Stringer, it's impossible to satisfy fmt.Formatter without importing fmt.
-func isFormatter(pass *analysis.Pass, t types.Type) bool {
-	for _, imp := range pass.Pkg.Imports() {
-		if imp.Path() == "fmt" {
-			formatter := imp.Scope().Lookup("Formatter").Type().Underlying().(*types.Interface)
-			return types.Implements(t, formatter)
-		}
+// The only interface method to look for is "Format(State, rune)".
+func isFormatter(typ types.Type) bool {
+	obj, _, _ := types.LookupFieldOrMethod(typ, false, nil, "Format")
+	fn, ok := obj.(*types.Func)
+	if !ok {
+		return false
 	}
-	return false
+	sig := fn.Type().(*types.Signature)
+	return sig.Params().Len() == 2 &&
+		sig.Results().Len() == 0 &&
+		isNamed(sig.Params().At(0).Type(), "fmt", "State") &&
+		types.Identical(sig.Params().At(1).Type(), types.Typ[types.Rune])
+}
+
+func isNamed(T types.Type, pkgpath, name string) bool {
+	named, ok := T.(*types.Named)
+	return ok && named.Obj().Pkg().Path() == pkgpath && named.Obj().Name() == name
 }
 
 // formatState holds the parsed representation of a printf directive such as "%3.*[4]d".
@@ -753,7 +761,7 @@ func okPrintfArg(pass *analysis.Pass, call *ast.CallExpr, state *formatState) (o
 	formatter := false
 	if state.argNum < len(call.Args) {
 		if tv, ok := pass.TypesInfo.Types[call.Args[state.argNum]]; ok {
-			formatter = isFormatter(pass, tv.Type)
+			formatter = isFormatter(tv.Type)
 		}
 	}
 
@@ -831,7 +839,7 @@ func recursiveStringer(pass *analysis.Pass, e ast.Expr) bool {
 	typ := pass.TypesInfo.Types[e].Type
 
 	// It's unlikely to be a recursive stringer if it has a Format method.
-	if isFormatter(pass, typ) {
+	if isFormatter(typ) {
 		return false
 	}
 

go/analysis/passes/printf/printf_test.go

@@ -10,5 +10,5 @@ import (
 func Test(t *testing.T) {
 	testdata := analysistest.TestData()
 	printf.Analyzer.Flags.Set("funcs", "Warn,Warnf")
-	analysistest.Run(t, testdata, printf.Analyzer, "a", "b")
+	analysistest.Run(t, testdata, printf.Analyzer, "a", "b", "nofmt")
 }

go/analysis/passes/printf/testdata/src/nofmt/nofmt.go

@@ -0,0 +1,10 @@
+package b
+
+import (
+	"math/big"
+	"testing"
+)
+
+func formatBigInt(t *testing.T) {
+	t.Logf("%d\n", big.NewInt(4))
+}

go/analysis/passes/printf/types.go

@@ -38,7 +38,7 @@ func matchArgTypeInternal(pass *analysis.Pass, t printfArgType, typ types.Type,
 		}
 	}
 	// If the type implements fmt.Formatter, we have nothing to check.
-	if isFormatter(pass, typ) {
+	if isFormatter(typ) {
 		return true
 	}
 	// If we can use a string, might arg (dynamically) implement the Stringer or Error interface?

go/analysis/passes/structtag/structtag.go

@@ -96,6 +96,11 @@ func checkTagDuplicates(pass *analysis.Pass, tag, key string, nearest, field *ty
 	}
 	if val == "" || val[0] == ',' {
 		if field.Anonymous() {
+			// Disable this check enhancement in Go 1.12.1; some
+			// false positives were spotted in the initial 1.12
+			// release. See https://golang.org/issues/30465.
+			return
+
 			typ, ok := field.Type().Underlying().(*types.Struct)
 			if !ok {
 				return

go/analysis/passes/structtag/testdata/src/a/a.go

@@ -75,7 +75,7 @@ type DuplicateJSONFields struct {
 	}
 	AnonymousJSON `json:"a"` // want "struct field AnonymousJSON repeats json tag .a. also at a.go:64"
 
-	AnonymousJSONField // want "struct field DuplicateAnonJSON repeats json tag .a. also at a.go:64"
+	AnonymousJSONField
 
 	XML              int `xml:"a"`
 	DuplicateXML     int `xml:"a"` // want "struct field DuplicateXML repeats xml tag .a. also at a.go:80"
@@ -129,5 +129,5 @@ type DuplicateWithAnotherPackage struct {
 
 	// The "also at" position is in a different package and directory. Use
 	// "b.b" instead of "b/b" to match the relative path on Windows easily.
-	DuplicateJSON int `json:"a"` // want "struct field DuplicateJSON repeats json tag .a. also at b.b.go:8"
+	DuplicateJSON int `json:"a"`
 }