diff --git a/cmd/vet/lowerell/analyzer.go b/cmd/vet/lowerell/analyzer.go new file mode 100644 index 000000000..a62f79bdc --- /dev/null +++ b/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 +} diff --git a/cmd/vet/lowerell/analyzer_test.go b/cmd/vet/lowerell/analyzer_test.go new file mode 100644 index 000000000..c566c2ec4 --- /dev/null +++ b/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") +} diff --git a/cmd/vet/lowerell/testdata/src/example/example.go b/cmd/vet/lowerell/testdata/src/example/example.go new file mode 100644 index 000000000..c67c19781 --- /dev/null +++ b/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 diff --git a/cmd/vet/vet.go b/cmd/vet/vet.go index 4087f7073..38ffdb6d0 100644 --- a/cmd/vet/vet.go +++ b/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) } diff --git a/drive/driveimpl/drive_test.go b/drive/driveimpl/drive_test.go index 8f9b43d6b..185ae2a9c 100644 --- a/drive/driveimpl/drive_test.go +++ b/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) } diff --git a/metrics/multilabelmap_test.go b/metrics/multilabelmap_test.go index 70554c63e..0fa730992 100644 --- a/metrics/multilabelmap_test.go +++ b/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) diff --git a/pkgdoc_test.go b/pkgdoc_test.go index c5e50ee63..d0f0d66bd 100644 --- a/pkgdoc_test.go +++ b/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 diff --git a/util/deephash/deephash_test.go b/util/deephash/deephash_test.go index 7ea83566f..a82203d50 100644 --- a/util/deephash/deephash_test.go +++ b/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},