cmd/vet/lowerell, drive/driveimpl: forbid variables named "l" or "I"

Add a new vet checker that rejects variables, parameters, named
return values, receivers, range/type-switch bindings, type
parameters, struct fields, and constants named "l" (lowercase ell)
or "I" (uppercase i). Both are hard to distinguish from the digit
"1" and from each other in too many fonts.

Rename the two pre-existing struct fields named "l" (both of type
net.Listener) in drive/driveimpl/drive_test.go to "ln", matching the
convention used elsewhere for net.Listener locals.

Rename the test-fixture struct fields "I" (single int label) to
"Int" in metrics/multilabelmap_test.go and util/deephash/deephash_test.go,
preserving the "first letters of types" convention used alongside
neighboring fields like I8/I16/U/U8.

Also teach pkgdoc_test.go to skip testdata/ directories, which
the go tool ignores; they are not real packages.

Fixes #19631

Change-Id: I71ad2fa990705f7a070406ebcdb8cefa7487d849
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/vet/lowerell/analyzer.go

@@ -0,0 +1,132 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+// Package lowerell forbids variables named "l" (lowercase ell) or "I"
+// (uppercase i), because they are hard to distinguish from the digit
+// "1" and from each other in too many fonts.
+package lowerell
+
+import (
+	"go/ast"
+	"go/token"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+// Analyzer reports variables named "l" (lowercase ell) or "I" (uppercase i).
+var Analyzer = &analysis.Analyzer{
+	Name: "lowerell",
+	Doc:  `forbid variables named "l" (lowercase ell) or "I" (uppercase i), which are hard to distinguish from "1"`,
+	Run:  run,
+}
+
+// messages maps a banned identifier name to the diagnostic shown to users.
+// Each message names the specific symbol that triggered it, so the
+// reader does not have to guess which of "l" or "I" they typed.
+var messages = map[string]string{
+	"l": `do not use "l" (lowercase ell) as a variable name; it is hard to distinguish from "1" and "I" in too many fonts; see https://github.com/tailscale/tailscale/issues/19631`,
+	"I": `do not use "I" (uppercase i) as a variable name; it is hard to distinguish from "1" and "l" in too many fonts; see https://github.com/tailscale/tailscale/issues/19631`,
+}
+
+// reported tracks identifier positions already reported, to avoid duplicate
+// diagnostics when the same declaration is reachable from multiple AST nodes.
+type reportedSet map[token.Pos]bool
+
+func (rs reportedSet) check(pass *analysis.Pass, ident *ast.Ident) {
+	if ident == nil {
+		return
+	}
+	msg, ok := messages[ident.Name]
+	if !ok {
+		return
+	}
+	if rs[ident.Pos()] {
+		return
+	}
+	rs[ident.Pos()] = true
+	pass.Reportf(ident.Pos(), "%s", msg)
+}
+
+func (rs reportedSet) checkFieldList(pass *analysis.Pass, fl *ast.FieldList) {
+	if fl == nil {
+		return
+	}
+	for _, f := range fl.List {
+		for _, n := range f.Names {
+			rs.check(pass, n)
+		}
+	}
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	rs := reportedSet{}
+
+	for _, file := range pass.Files {
+		ast.Inspect(file, func(n ast.Node) bool {
+			switch n := n.(type) {
+			case *ast.FuncDecl:
+				// Receiver name.
+				rs.checkFieldList(pass, n.Recv)
+				// Parameters, results, and type parameters
+				// are checked via the FuncType case below.
+
+			case *ast.FuncType:
+				rs.checkFieldList(pass, n.TypeParams)
+				rs.checkFieldList(pass, n.Params)
+				rs.checkFieldList(pass, n.Results)
+
+			case *ast.StructType:
+				rs.checkFieldList(pass, n.Fields)
+
+			case *ast.GenDecl:
+				if n.Tok != token.VAR && n.Tok != token.CONST {
+					return true
+				}
+				for _, spec := range n.Specs {
+					vs, ok := spec.(*ast.ValueSpec)
+					if !ok {
+						continue
+					}
+					for _, name := range vs.Names {
+						rs.check(pass, name)
+					}
+				}
+
+			case *ast.AssignStmt:
+				if n.Tok != token.DEFINE {
+					return true
+				}
+				for _, lhs := range n.Lhs {
+					if id, ok := lhs.(*ast.Ident); ok {
+						rs.check(pass, id)
+					}
+				}
+
+			case *ast.RangeStmt:
+				if n.Tok != token.DEFINE {
+					return true
+				}
+				if id, ok := n.Key.(*ast.Ident); ok {
+					rs.check(pass, id)
+				}
+				if id, ok := n.Value.(*ast.Ident); ok {
+					rs.check(pass, id)
+				}
+
+			case *ast.TypeSwitchStmt:
+				// switch l := x.(type) { ... }
+				as, ok := n.Assign.(*ast.AssignStmt)
+				if !ok || as.Tok != token.DEFINE {
+					return true
+				}
+				for _, lhs := range as.Lhs {
+					if id, ok := lhs.(*ast.Ident); ok {
+						rs.check(pass, id)
+					}
+				}
+			}
+			return true
+		})
+	}
+	return nil, nil
+}

cmd/vet/lowerell/analyzer_test.go

@@ -0,0 +1,15 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+package lowerell
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestAnalyzer(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, Analyzer, "example")
+}

cmd/vet/lowerell/testdata/src/example/example.go

@@ -0,0 +1,100 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+package example
+
+import "sync"
+
+// Bad: var declarations.
+var l int // want `do not use "l"`
+var I int // want `do not use "I"`
+
+// OK: variables named "ll", "II", "i" are fine.
+var (
+	ll int
+	II int
+	i  int
+)
+
+// Bad: const declaration in a function scope.
+func F0() {
+	const l = 3 // want `do not use "l"`
+	const I = 4 // want `do not use "I"`
+	_ = l
+	_ = I
+}
+
+// Bad: function parameters.
+func F1a(l int) {} // want `do not use "l"`
+func F1b(I int) {} // want `do not use "I"`
+
+// Bad: named return values.
+func F2a() (l int) { return } // want `do not use "l"`
+func F2b() (I int) { return } // want `do not use "I"`
+
+// Bad: receiver names.
+type T struct{}
+
+func (l *T) Ml() {} // want `do not use "l"`
+func (I *T) MI() {} // want `do not use "I"`
+
+// Bad: struct fields.
+type S struct {
+	l int // want `do not use "l"`
+	I int // want `do not use "I"`
+}
+
+// Bad: short variable declarations.
+func F3() {
+	l := 1 // want `do not use "l"`
+	I := 2 // want `do not use "I"`
+	_ = l
+	_ = I
+}
+
+// Bad: var statement inside a function.
+func F4() {
+	var l int // want `do not use "l"`
+	var I int // want `do not use "I"`
+	_ = l
+	_ = I
+}
+
+// Bad: range key/value.
+func F5(xs []int) {
+	for l, v := range xs { // want `do not use "l"`
+		_ = l
+		_ = v
+	}
+	for _, I := range xs { // want `do not use "I"`
+		_ = I
+	}
+}
+
+// Bad: type parameters.
+func F6a[l any](x l) l { return x } // want `do not use "l"`
+func F6b[I any](x I) I { return x } // want `do not use "I"`
+
+// Bad: type switch guards.
+func F7(x any) {
+	switch l := x.(type) { // want `do not use "l"`
+	case int:
+		_ = l
+	}
+	switch I := x.(type) { // want `do not use "I"`
+	case int:
+		_ = I
+	}
+}
+
+// OK: clean code with no banned variables.
+func F8() {
+	count := 0
+	for i := 0; i < 10; i++ {
+		count++
+	}
+	_ = count
+}
+
+// OK: sync.Mutex named "mu".
+var mu sync.Mutex

cmd/vet/vet.go

@@ -9,6 +9,7 @@ import (
 
 	"golang.org/x/tools/go/analysis/unitchecker"
 	"tailscale.com/cmd/vet/jsontags"
+	"tailscale.com/cmd/vet/lowerell"
 	"tailscale.com/cmd/vet/subtestnames"
 )
 
@@ -21,5 +22,5 @@ func init() {
 }
 
 func main() {
-	unitchecker.Main(jsontags.Analyzer, subtestnames.Analyzer)
+	unitchecker.Main(jsontags.Analyzer, lowerell.Analyzer, subtestnames.Analyzer)
 }

drive/driveimpl/drive_test.go

@@ -239,7 +239,7 @@ func TestLOCK(t *testing.T) {
 	}
 
 	u := fmt.Sprintf("http://%s/%s/%s/%s/%s",
-		s.local.l.Addr(),
+		s.local.ln.Addr(),
 		url.PathEscape(domain),
 		url.PathEscape(remote1),
 		url.PathEscape(share11),
@@ -365,7 +365,7 @@ func TestUNLOCK(t *testing.T) {
 	}
 
 	u := fmt.Sprintf("http://%s/%s/%s/%s/%s",
-		s.local.l.Addr(),
+		s.local.ln.Addr(),
 		url.PathEscape(domain),
 		url.PathEscape(remote1),
 		url.PathEscape(share11),
@@ -428,12 +428,12 @@ func TestUNLOCK(t *testing.T) {
 }
 
 type local struct {
-	l  net.Listener
+	ln net.Listener
 	fs *FileSystemForLocal
 }
 
 type remote struct {
-	l           net.Listener
+	ln          net.Listener
 	fs          *FileSystemForRemote
 	fileServer  *FileServer
 	shares      map[string]string
@@ -487,7 +487,7 @@ func newSystem(t *testing.T) *system {
 	client.SetTransport(&http.Transport{DisableKeepAlives: true})
 	s := &system{
 		t:       t,
-		local:   &local{l: ln, fs: fs},
+		local:   &local{ln: ln, fs: fs},
 		client:  client,
 		remotes: make(map[string]*remote),
 	}
@@ -510,7 +510,7 @@ func (s *system) addRemote(name string) string {
 	s.t.Logf("FileServer for %v listening at %s", name, fileServer.Addr())
 
 	r := &remote{
-		l:           ln,
+		ln:          ln,
 		fileServer:  fileServer,
 		fs:          NewFileSystemForRemote(log.Printf),
 		shares:      make(map[string]string),
@@ -524,7 +524,7 @@ func (s *system) addRemote(name string) string {
 	for name, r := range s.remotes {
 		remotes = append(remotes, &drive.Remote{
 			Name: name,
-			URL:  func() string { return fmt.Sprintf("http://%s", r.l.Addr()) },
+			URL:  func() string { return fmt.Sprintf("http://%s", r.ln.Addr()) },
 		})
 	}
 	s.local.fs.SetRemotes(
@@ -683,7 +683,7 @@ func (s *system) stop() {
 		s.t.Fatalf("failed to Close fs: %s", err)
 	}
 
-	err = s.local.l.Close()
+	err = s.local.ln.Close()
 	if err != nil {
 		s.t.Fatalf("failed to Close listener: %s", err)
 	}
@@ -694,7 +694,7 @@ func (s *system) stop() {
 			s.t.Fatalf("failed to Close remote fs: %s", err)
 		}
 
-		err = r.l.Close()
+		err = r.ln.Close()
 		if err != nil {
 			s.t.Fatalf("failed to Close remote listener: %s", err)
 		}

metrics/multilabelmap_test.go

@@ -86,10 +86,10 @@ metricname{foo="si",bar="si"} 5
 
 func TestMultiLabelMapTypes(t *testing.T) {
 	type LabelTypes struct {
-		S string
-		B bool
-		I int
-		U uint
+		S   string
+		B   bool
+		Int int
+		U   uint
 	}
 
 	m := new(MultiLabelMap[LabelTypes])
@@ -100,7 +100,7 @@ func TestMultiLabelMapTypes(t *testing.T) {
 	m.WritePrometheus(&buf, "metricname")
 	const want = `# TYPE metricname counter
 # HELP metricname some good stuff
-metricname{s="a",b="true",i="-1",u="2"} 3
+metricname{s="a",b="true",int="-1",u="2"} 3
 `
 	if got := buf.String(); got != want {
 		t.Errorf("got %q; want %q", got, want)

pkgdoc_test.go

@@ -41,6 +41,9 @@ func TestPackageDocs(t *testing.T) {
 		if fi.Mode().IsDir() && path != "." && strings.HasPrefix(filepath.Base(path), ".") {
 			return filepath.SkipDir // No documentation lives in dot directories (.git, .claude, etc)
 		}
+		if fi.Mode().IsDir() && filepath.Base(path) == "testdata" {
+			return filepath.SkipDir // testdata is ignored by the go tool; not real packages
+		}
 		if fi.Mode().IsRegular() && strings.HasSuffix(path, ".go") {
 			if strings.HasSuffix(path, "_test.go") {
 				return nil

util/deephash/deephash_test.go

@@ -59,7 +59,7 @@ func TestHash(t *testing.T) {
 		I16  int16
 		I32  int32
 		I64  int64
-		I    int
+		Int  int
 		U8   uint8
 		U16  uint16
 		U32  uint32
@@ -92,7 +92,7 @@ func TestHash(t *testing.T) {
 		{in: tuple{scalars{I16: math.MinInt16}, scalars{I16: math.MinInt16 / 2}}, wantEq: false},
 		{in: tuple{scalars{I32: math.MinInt32}, scalars{I32: math.MinInt32 / 2}}, wantEq: false},
 		{in: tuple{scalars{I64: math.MinInt64}, scalars{I64: math.MinInt64 / 2}}, wantEq: false},
-		{in: tuple{scalars{I: -1234}, scalars{I: -1234 / 2}}, wantEq: false},
+		{in: tuple{scalars{Int: -1234}, scalars{Int: -1234 / 2}}, wantEq: false},
 		{in: tuple{scalars{U8: math.MaxUint8}, scalars{U8: math.MaxUint8 / 2}}, wantEq: false},
 		{in: tuple{scalars{U16: math.MaxUint16}, scalars{U16: math.MaxUint16 / 2}}, wantEq: false},
 		{in: tuple{scalars{U32: math.MaxUint32}, scalars{U32: math.MaxUint32 / 2}}, wantEq: false},