Retool typecheck to be simpler

Instead of walking paths ourselves, just let Go's packages library do
it.  This is a slight CLI change - it wants "./foo" rather than "foo".

This also flagged a few things which seem to be legit failures.

hack/verify-typecheck.sh

@@ -32,7 +32,7 @@ TYPECHECK_SERIAL="${TYPECHECK_SERIAL:-false}"
 go run ./test/typecheck "$@" "--serial=$TYPECHECK_SERIAL" || ret=$?
 
 if [[ $ret -ne 0 ]]; then
-  echo "!!! Type Check has failed. This may cause cross platform build failures." >&2
+  echo "!!! Typecheck has failed. This may cause cross platform build failures." >&2
   echo "!!! Please see https://git.k8s.io/kubernetes/test/typecheck for more information." >&2
   exit 1
 fi

pkg/controller/job/indexed_job_utils_test.go

@@ -904,11 +904,11 @@ func TestGetIndexFailureCount(t *testing.T) {
 			wantResult: 2,
 		},
 		"valid maxint32 value": {
-			pod:        buildPod().indexFailureCount(strconv.Itoa(math.MaxInt32)).Pod,
+			pod:        buildPod().indexFailureCount(strconv.FormatInt(math.MaxInt32, 10)).Pod,
 			wantResult: math.MaxInt32,
 		},
 		"too large value": {
-			pod:        buildPod().indexFailureCount(strconv.Itoa(math.MaxInt32 + 1)).Pod,
+			pod:        buildPod().indexFailureCount(strconv.FormatInt(math.MaxInt32+1, 10)).Pod,
 			wantResult: 0,
 		},
 		"negative value": {

pkg/kubelet/util/util_windows_test.go

@@ -21,16 +21,10 @@ package util
 
 import (
 	"fmt"
-	"math/rand"
-	"net"
-	"os"
 	"reflect"
 	"runtime"
-	"sync"
 	"testing"
-	"time"
 
-	winio "github.com/Microsoft/go-winio"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )

pkg/util/filesystem/util_windows_test.go

@@ -20,12 +20,14 @@ limitations under the License.
 package filesystem
 
 import (
+	"math/rand"
 	"net"
 	"os"
 	"sync"
 	"testing"
 	"time"
 
+	winio "github.com/Microsoft/go-winio"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )

test/typecheck/main.go

@@ -23,7 +23,6 @@ import (
 	"io"
 	"log"
 	"os"
-	"path/filepath"
 	"sort"
 	"strings"
 	"sync"
@@ -33,16 +32,16 @@ import (
 )
 
 var (
-	verbose    = flag.Bool("verbose", false, "print more information")
-	cross      = flag.Bool("cross", true, "build for all platforms")
-	platforms  = flag.String("platform", "", "comma-separated list of platforms to typecheck")
-	timings    = flag.Bool("time", false, "output times taken for each phase")
-	defuses    = flag.Bool("defuse", false, "output defs/uses")
-	serial     = flag.Bool("serial", false, "don't type check platforms in parallel (equivalent to --parallel=1)")
-	parallel   = flag.Int("parallel", 2, "limits how many platforms can be checked in parallel. 0 means no limit.")
-	skipTest   = flag.Bool("skip-test", false, "don't type check test code")
-	tags       = flag.String("tags", "", "comma-separated list of build tags to apply in addition to go's defaults")
-	ignoreDirs = flag.String("ignore-dirs", "", "comma-separated list of directories to ignore in addition to the default hardcoded list including staging, vendor, and hidden dirs")
+	verbose        = flag.Bool("verbose", false, "print more information")
+	cross          = flag.Bool("cross", true, "build for all platforms")
+	platforms      = flag.String("platform", "", "comma-separated list of platforms to typecheck")
+	timings        = flag.Bool("time", false, "output times taken for each phase")
+	defuses        = flag.Bool("defuse", false, "output defs/uses")
+	serial         = flag.Bool("serial", false, "don't type check platforms in parallel (equivalent to --parallel=1)")
+	parallel       = flag.Int("parallel", 2, "limits how many platforms can be checked in parallel. 0 means no limit.")
+	skipTest       = flag.Bool("skip-test", false, "don't type check test code")
+	tags           = flag.String("tags", "", "comma-separated list of build tags to apply in addition to go's defaults")
+	ignorePatterns = flag.String("ignore", "", "comma-separated list of Go patterns to ignore")
 
 	// When processed in order, windows and darwin are early to make
 	// interesting OS-based errors happen earlier.
@@ -54,27 +53,6 @@ var (
 		"linux/ppc64le", "linux/s390x",
 		"windows/arm64",
 	}
-
-	// directories we always ignore
-	standardIgnoreDirs = []string{
-		// Staging code is symlinked from vendor/k8s.io, and uses import
-		// paths as if it were inside of vendor/. It fails typechecking
-		// inside of staging/, but works when typechecked as part of vendor/.
-		"staging",
-		// OS-specific vendor code tends to be imported by OS-specific
-		// packages. We recursively typecheck imported vendored packages for
-		// each OS, but don't typecheck everything for every OS.
-		"vendor",
-		"_output",
-		// This is a weird one. /testdata/ is *mostly* ignored by Go,
-		// and this translates to kubernetes/vendor not working.
-		// edit/record.go doesn't compile without gopkg.in/yaml.v2
-		// in $GOSRC/$GOROOT (both typecheck and the shell script).
-		"pkg/kubectl/cmd/testdata/edit",
-		// Tools we use for maintaining the code base but not necessarily
-		// ship as part of the release
-		"hack/tools",
-	}
 )
 
 func newConfig(platform string) *packages.Config {
@@ -102,95 +80,26 @@ func newConfig(platform string) *packages.Config {
 	}
 }
 
-type collector struct {
-	dirs       []string
-	ignoreDirs []string
-}
-
-func newCollector(ignoreDirs string) collector {
-	c := collector{
-		ignoreDirs: append([]string(nil), standardIgnoreDirs...),
-	}
-	if ignoreDirs != "" {
-		c.ignoreDirs = append(c.ignoreDirs, strings.Split(ignoreDirs, ",")...)
-	}
-	return c
-}
-
-func (c *collector) walk(roots []string) error {
-	for _, root := range roots {
-		err := filepath.Walk(root, c.handlePath)
-		if err != nil {
-			return err
-		}
-	}
-	sort.Strings(c.dirs)
-	return nil
-}
-
-// handlePath walks the filesystem recursively, collecting directories,
-// ignoring some unneeded directories (hidden/vendored) that are handled
-// specially later.
-func (c *collector) handlePath(path string, info os.FileInfo, err error) error {
-	if err != nil {
-		return err
-	}
-	if info.IsDir() {
-		name := info.Name()
-		// Ignore hidden directories (.git, .cache, etc)
-		if (len(name) > 1 && (name[0] == '.' || name[0] == '_')) || name == "testdata" {
-			if *verbose {
-				fmt.Printf("DBG: skipping dir %s\n", path)
-			}
-			return filepath.SkipDir
-		}
-		for _, dir := range c.ignoreDirs {
-			if path == dir {
-				if *verbose {
-					fmt.Printf("DBG: ignoring dir %s\n", path)
-				}
-				return filepath.SkipDir
-			}
-		}
-		// Make dirs into relative pkg names.
-		// NOTE: can't use filepath.Join because it elides the leading "./"
-		pkg := path
-		if !strings.HasPrefix(pkg, "./") {
-			pkg = "./" + pkg
-		}
-		c.dirs = append(c.dirs, pkg)
-		if *verbose {
-			fmt.Printf("DBG: added dir %s\n", path)
-		}
-	}
-	return nil
-}
-
-func (c *collector) verify(plat string) ([]string, error) {
+func verify(plat string, patterns []string, ignore map[string]bool) ([]string, error) {
 	errors := []packages.Error{}
 	start := time.Now()
 	config := newConfig(plat)
 
-	rootPkgs, err := packages.Load(config, c.dirs...)
+	pkgs, err := packages.Load(config, patterns...)
 	if err != nil {
 		return nil, err
 	}
 
 	// Recursively import all deps and flatten to one list.
 	allMap := map[string]*packages.Package{}
-	for _, pkg := range rootPkgs {
+	for _, pkg := range pkgs {
+		if ignore[pkg.PkgPath] {
+			continue
+		}
 		if *verbose {
 			serialFprintf(os.Stdout, "pkg %q has %d GoFiles\n", pkg.PkgPath, len(pkg.GoFiles))
 		}
-		allMap[pkg.PkgPath] = pkg
-		if len(pkg.Imports) > 0 {
-			for _, imp := range pkg.Imports {
-				if *verbose {
-					serialFprintf(os.Stdout, "pkg %q imports %q\n", pkg.PkgPath, imp.PkgPath)
-				}
-				allMap[imp.PkgPath] = imp
-			}
-		}
+		accumulate(pkg, allMap)
 	}
 	keys := make([]string, 0, len(allMap))
 	for k := range allMap {
@@ -225,6 +134,19 @@ func (c *collector) verify(plat string) ([]string, error) {
 	return dedup(errors), nil
 }
 
+func accumulate(pkg *packages.Package, allMap map[string]*packages.Package) {
+	allMap[pkg.PkgPath] = pkg
+	for _, imp := range pkg.Imports {
+		if allMap[imp.PkgPath] != nil {
+			continue
+		}
+		if *verbose {
+			serialFprintf(os.Stdout, "pkg %q imports %q\n", pkg.PkgPath, imp.PkgPath)
+		}
+		accumulate(imp, allMap)
+	}
+}
+
 func dedup(errors []packages.Error) []string {
 	ret := []string{}
 
@@ -247,6 +169,24 @@ func serialFprintf(w io.Writer, format string, a ...interface{}) {
 	_, _ = fmt.Fprintf(w, format, a...)
 }
 
+func resolvePkgs(patterns ...string) (map[string]bool, error) {
+	config := &packages.Config{
+		Mode: packages.NeedName,
+	}
+	pkgs, err := packages.Load(config, patterns...)
+	if err != nil {
+		return nil, err
+	}
+	paths := map[string]bool{}
+	for _, p := range pkgs {
+		// ignore list errors (e.g. doesn't exist)
+		if len(p.Errors) == 0 {
+			paths[p.PkgPath] = true
+		}
+	}
+	return paths, nil
+}
+
 func main() {
 	flag.Parse()
 	args := flag.Args()
@@ -256,13 +196,16 @@ func main() {
 	}
 
 	if len(args) == 0 {
-		args = append(args, ".")
+		args = append(args, "./...")
 	}
 
-	c := newCollector(*ignoreDirs)
-
-	if err := c.walk(args); err != nil {
-		log.Fatalf("Error walking: %v", err)
+	ignore := []string{}
+	if *ignorePatterns != "" {
+		ignore = append(ignore, strings.Split(*ignorePatterns, ",")...)
+	}
+	ignorePkgs, err := resolvePkgs(ignore...)
+	if err != nil {
+		log.Fatalf("failed to resolve ignored packages: %v", err)
 	}
 
 	plats := crossPlatforms[:]
@@ -295,7 +238,7 @@ func main() {
 
 			f := false
 			serialFprintf(os.Stdout, "type-checking %s\n", plat)
-			errors, err := c.verify(plat)
+			errors, err := verify(plat, args, ignorePkgs)
 			if err != nil {
 				serialFprintf(os.Stderr, "ERROR(%s): failed to verify: %v\n", plat, err)
 				f = true

test/typecheck/main_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package main
 
 import (
-	"errors"
 	"flag"
 	"os"
 	"path/filepath"
@@ -43,12 +42,7 @@ func TestVerify(t *testing.T) {
 	}
 
 	for _, tc := range tcs {
-		c := newCollector("")
-		if err := c.walk([]string{tc.path}); err != nil {
-			t.Fatalf("error walking %s: %v", tc.path, err)
-		}
-
-		errs, err := c.verify("linux/amd64")
+		errs, err := verify("linux/amd64", []string{tc.path}, nil)
 		if err != nil {
 			t.Errorf("unexpected error: %v", err)
 		} else if len(errs) != tc.expect {
@@ -72,20 +66,6 @@ func setEnvVars(t testing.TB) {
 	}
 }
 
-func TestHandlePath(t *testing.T) {
-	c := collector{
-		ignoreDirs: standardIgnoreDirs,
-	}
-	e := errors.New("ex")
-	i, _ := os.Stat(".") // i.IsDir() == true
-	if c.handlePath("foo", nil, e) != e {
-		t.Error("handlePath not returning errors")
-	}
-	if c.handlePath("vendor", i, nil) != filepath.SkipDir {
-		t.Error("should skip vendor")
-	}
-}
-
 func TestDedup(t *testing.T) {
 	testcases := []struct {
 		input    []packages.Error