From 32a33968dec0bdb856c5dbc847008f6386664327 Mon Sep 17 00:00:00 2001 From: Bob Glickstein Date: Thu, 20 Nov 2025 14:36:34 -0800 Subject: [PATCH 1/6] Prefer some named interfaces over others. --- .github/workflows/go.yml | 3 +- Readme.md | 5 +- _testdata/foo.go | 1 - cmd/decouple/decouple_test.go | 10 ++-- cmd/decouple/main.go | 13 +++-- debug.go | 4 +- decouple.go | 95 ++++++++++++++++++++++++++--------- decouple_test.go | 18 ++++--- go.mod | 4 +- go.sum | 4 ++ 10 files changed, 111 insertions(+), 46 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 17240c5..ffc8a1f 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -27,10 +27,11 @@ jobs: uses: shogo82148/actions-goveralls@v1 with: path-to-profile: cover.out + continue-on-error: true - name: Modver if: ${{ github.event_name == 'pull_request' }} - uses: bobg/modver@v2.7.0 + uses: bobg/modver@v2.12.1 with: github_token: ${{ secrets.GITHUB_TOKEN }} pull_request_url: https://github.com/${{ github.repository }}/pull/${{ github.event.number }} diff --git a/Readme.md b/Readme.md index 2bdf2d4..f110c6c 100644 --- a/Readme.md +++ b/Readme.md @@ -3,7 +3,7 @@ [![Go Reference](https://pkg.go.dev/badge/github.com/bobg/decouple.svg)](https://pkg.go.dev/github.com/bobg/decouple) [![Go Report Card](https://goreportcard.com/badge/github.com/bobg/decouple)](https://goreportcard.com/report/github.com/bobg/decouple) [![Tests](https://github.com/bobg/decouple/actions/workflows/go.yml/badge.svg)](https://github.com/bobg/decouple/actions/workflows/go.yml) -[![Coverage Status](https://coveralls.io/repos/github/bobg/decouple/badge.svg?branch=main)](https://coveralls.io/github/bobg/decouple?branch=main) +[![Coverage Status](https://coveralls.io/repos/github.com/bobg/decouple/badge.svg?branch=main)](https://coveralls.io/github.com/bobg/decouple?branch=main) [![Mentioned in Awesome Go](https://awesome.re/mentioned-badge.svg)](https://github.com/avelino/awesome-go) This is decouple, @@ -135,7 +135,8 @@ The same report with `-json` specified looks like this: "Methods": [ "Write" ], - "InterfaceName": "io.Writer" + "InterfaceName": "Writer", + "InterfacePkg": "io" } ] } diff --git a/_testdata/foo.go b/_testdata/foo.go index 0efb51f..49530ea 100644 --- a/_testdata/foo.go +++ b/_testdata/foo.go @@ -274,7 +274,6 @@ func F34(r *os.File, ch chan<- *os.File) ([]byte, error) { } // {"x": {"foo": "func()"}} -// {"x": ""} func F35(x interface { foo() bar() diff --git a/cmd/decouple/decouple_test.go b/cmd/decouple/decouple_test.go index 13b89db..8c41ed6 100644 --- a/cmd/decouple/decouple_test.go +++ b/cmd/decouple/decouple_test.go @@ -5,10 +5,11 @@ import ( "encoding/json" "path/filepath" "reflect" + "slices" "strings" "testing" - "github.com/bobg/go-generics/v3/iter" + "github.com/bobg/seqs" ) func TestRunJSON(t *testing.T) { @@ -33,7 +34,7 @@ func TestRunJSON(t *testing.T) { want := []jtuple{{ PackageName: "main", FileName: "main.go", - Line: 100, + Line: 105, Column: 6, FuncName: "showJSON", Params: []jparam{{ @@ -55,8 +56,9 @@ func TestRunPlain(t *testing.T) { t.Fatal(err) } - lines, err := iter.ToSlice(iter.Lines(buf)) - if err != nil { + linesSeq, errptr := seqs.Lines(buf) + lines := slices.Collect(linesSeq) + if err := *errptr; err != nil { t.Fatal(err) } diff --git a/cmd/decouple/main.go b/cmd/decouple/main.go index ad3da39..7b70c69 100644 --- a/cmd/decouple/main.go +++ b/cmd/decouple/main.go @@ -7,6 +7,7 @@ import ( "io" "os" "sort" + "strings" "github.com/bobg/errors" "github.com/bobg/go-generics/v3/maps" @@ -83,8 +84,12 @@ func run(w io.Writer, verbose, doJSON bool, args []string) error { showedFuncName = true } - if intfName := checker.NameForMethods(mm); intfName != "" { - fmt.Fprintf(w, " %s: %s\n", param, intfName) + if pkg, intfName := checker.NameForMethods(mm); intfName != "" { + pkgpath := pkg.PkgPath + if strings.ContainsAny(pkgpath, "./") { + pkgpath = fmt.Sprintf(`"%s"`, pkgpath) + } + fmt.Fprintf(w, " %s: %s.%s\n", param, pkgpath, intfName) continue } @@ -119,7 +124,8 @@ func showJSON(w io.Writer, checker decouple.Checker, tuples []decouple.Tuple) er Methods: maps.Keys(mm), } sort.Strings(jp.Methods) - if intfName := checker.NameForMethods(mm); intfName != "" { + if pkg, intfName := checker.NameForMethods(mm); intfName != "" { + jp.InterfacePkg = pkg.PkgPath jp.InterfaceName = intfName } jt.Params = append(jt.Params, jp) @@ -149,5 +155,6 @@ type jtuple struct { type jparam struct { Name string Methods []string `json:",omitempty"` + InterfacePkg string `json:",omitempty"` InterfaceName string `json:",omitempty"` } diff --git a/debug.go b/debug.go index 9841b57..f3d48da 100644 --- a/debug.go +++ b/debug.go @@ -11,9 +11,9 @@ func (a *analyzer) debugf(format string, args ...any) { return } s := fmt.Sprintf(format, args...) - strings.TrimRight(s, "\r\n") + s = strings.TrimRight(s, "\r\n") if a.level > 0 { - fmt.Fprintf(os.Stderr, strings.Repeat(" ", a.level)) + fmt.Fprint(os.Stderr, strings.Repeat(" ", a.level)) } fmt.Fprintln(os.Stderr, s) } diff --git a/decouple.go b/decouple.go index bf8976e..c587981 100644 --- a/decouple.go +++ b/decouple.go @@ -5,11 +5,11 @@ import ( "go/ast" "go/token" "go/types" + "slices" + "sort" "strings" "github.com/bobg/errors" - "github.com/bobg/go-generics/v3/set" - "github.com/bobg/go-generics/v3/slices" "golang.org/x/tools/go/packages" ) @@ -27,7 +27,7 @@ type Checker struct { Verbose bool pkgs []*packages.Package - namedInterfaces map[string]MethodMap // maps a package-qualified interface-type name to its method set + namedInterfaces map[*packages.Package]map[string]MethodMap // pkg -> named interface type -> method set } // NewCheckerFromDir creates a new Checker containing packages loaded @@ -54,24 +54,24 @@ func NewCheckerFromDir(dir string) (Checker, error) { // which should be the result of calling "golang.org/x/go/packages".Load // with at least the bits in PkgMode set in the Config.Mode field. func NewCheckerFromPackages(pkgs []*packages.Package) Checker { - var ( - namedInterfaces = make(map[string]MethodMap) - seen = set.New[*packages.Package]() - ) + namedInterfaces := make(map[*packages.Package]map[string]MethodMap) for _, pkg := range pkgs { - findNamedInterfaces(pkg, seen, namedInterfaces) + findNamedInterfaces(pkg, namedInterfaces) } return Checker{pkgs: pkgs, namedInterfaces: namedInterfaces} } -func findNamedInterfaces(pkg *packages.Package, seen set.Of[*packages.Package], namedInterfaces map[string]MethodMap) { - if seen.Has(pkg) { +func findNamedInterfaces(pkg *packages.Package, namedInterfaces map[*packages.Package]map[string]MethodMap) { + if _, ok := namedInterfaces[pkg]; ok { + // Already visited this package. return } - seen.Add(pkg) + + namedInterfacesForPkg := make(map[string]MethodMap) + namedInterfaces[pkg] = namedInterfacesForPkg for _, ipkg := range pkg.Imports { - findNamedInterfaces(ipkg, seen, namedInterfaces) + findNamedInterfaces(ipkg, namedInterfaces) } if isInternal(pkg.PkgPath) { @@ -107,16 +107,10 @@ func findNamedInterfaces(pkg *packages.Package, seen set.Of[*packages.Package], } mm := make(MethodMap) addMethodsToMap(intf, mm) - name := pkg.PkgPath - if strings.ContainsAny(name, "./") { - name = `"` + name + `"` - } - name += "." + typespec.Name.Name - namedInterfaces[name] = mm + namedInterfacesForPkg[typespec.Name.Name] = mm } } } - } // Check checks all the packages in the Checker. @@ -274,14 +268,65 @@ func (ch Checker) CheckParam(pkg *packages.Package, fndecl *ast.FuncDecl, name * // and returns the name of an interface defining exactly the methods in it, // if it can find one among the packages in the Checker. // If there are multiple such interfaces, -// one is chosen arbitrarily. -func (ch Checker) NameForMethods(inp MethodMap) string { - for name, mm := range ch.namedInterfaces { - if sameMethodMaps(mm, inp) { - return name +// one is chosen arbitrarily from the Go standard library, if any exist, +// otherwise from the "main module," if any exist there, +// otherwise from any modules directly depended on by the main module, +// and finally from any other module. +func (ch Checker) NameForMethods(inp MethodMap) (*packages.Package, string) { + type pkgNamePair struct { + pkg *packages.Package + name string + } + var pairs []pkgNamePair + + for pkg, namedInterfaces := range ch.namedInterfaces { + for name, methodMap := range namedInterfaces { + if sameMethodMaps(methodMap, inp) { + pairs = append(pairs, pkgNamePair{pkg: pkg, name: name}) + } } } - return "" + + if len(pairs) == 0 { + return nil, "" + } + + sort.Slice(pairs, func(i, j int) bool { + a, b := pairs[i], pairs[j] + if isStdlib(a.pkg.PkgPath) && !isStdlib(b.pkg.PkgPath) { + return true + } + if !isStdlib(a.pkg.PkgPath) && isStdlib(b.pkg.PkgPath) { + return false + } + if isMainModulePackage(a.pkg) && !isMainModulePackage(b.pkg) { + return true + } + if !isMainModulePackage(a.pkg) && isMainModulePackage(b.pkg) { + return false + } + if isDirectDependencyOfMainModule(a.pkg) && !isDirectDependencyOfMainModule(b.pkg) { + return true + } + if !isDirectDependencyOfMainModule(a.pkg) && isDirectDependencyOfMainModule(b.pkg) { + return false + } + return a.pkg.PkgPath < b.pkg.PkgPath + }) + + return pairs[0].pkg, pairs[0].name +} + +func isStdlib(pkgPath string) bool { + return !strings.Contains(pkgPath, ".") +} + +func isMainModulePackage(pkg *packages.Package) bool { + return pkg.Module != nil && pkg.Module.Main +} + +func isDirectDependencyOfMainModule(pkg *packages.Package) bool { + return pkg.Module != nil && !pkg.Module.Main && !pkg.Module.Indirect } type funcDeclOrLit struct { diff --git a/decouple_test.go b/decouple_test.go index 7c98971..7b30fa9 100644 --- a/decouple_test.go +++ b/decouple_test.go @@ -6,11 +6,11 @@ import ( "go/ast" "go/token" "go/types" + "maps" "strings" "testing" - "github.com/bobg/go-generics/v3/maps" - "github.com/bobg/go-generics/v3/set" + "github.com/bobg/go-generics/v4/set" // "github.com/davecgh/go-spew/spew" ) @@ -49,8 +49,8 @@ func TestCheck(t *testing.T) { } var ( - gotParamNames = set.New(maps.Keys(tuple.M)...) - wantParamNames = set.New(maps.Keys(pre)...) + gotParamNames = set.Collect(maps.Keys(tuple.M)) + wantParamNames = set.Collect(maps.Keys(pre)) ) if !gotParamNames.Equal(wantParamNames) { t.Fatalf("got param names %v, want %v", gotParamNames.Slice(), wantParamNames.Slice()) @@ -59,8 +59,8 @@ func TestCheck(t *testing.T) { for paramName, methods := range pre { t.Run(paramName, func(t *testing.T) { var ( - gotMethodNames = set.New(maps.Keys(tuple.M[paramName])...) - wantMethodNames = set.New(maps.Keys(methods)...) + gotMethodNames = set.Collect(maps.Keys(tuple.M[paramName])) + wantMethodNames = set.Collect(maps.Keys(methods)) ) if !gotMethodNames.Equal(wantMethodNames) { t.Fatalf("got method names %v, want %v", gotMethodNames.Slice(), wantMethodNames.Slice()) @@ -91,7 +91,11 @@ func TestCheck(t *testing.T) { for paramName, intfname := range intfnames { t.Run(paramName, func(t *testing.T) { - got := checker.NameForMethods(tuple.M[paramName]) + gotPkg, gotName := checker.NameForMethods(tuple.M[paramName]) + if gotName == "" { + t.Fatalf("no named interface found for param %s", paramName) + } + got := gotPkg.PkgPath + "." + gotName if got != intfname { t.Errorf("got %s, want %s", got, intfname) } diff --git a/go.mod b/go.mod index 9375a4f..8597153 100644 --- a/go.mod +++ b/go.mod @@ -1,12 +1,14 @@ module github.com/bobg/decouple -go 1.22.0 +go 1.23 toolchain go1.24.0 require ( github.com/bobg/errors v1.1.0 github.com/bobg/go-generics/v3 v3.7.0 + github.com/bobg/go-generics/v4 v4.2.0 + github.com/bobg/seqs v1.8.0 golang.org/x/tools v0.30.0 ) diff --git a/go.sum b/go.sum index 28122b7..e690002 100644 --- a/go.sum +++ b/go.sum @@ -2,6 +2,10 @@ github.com/bobg/errors v1.1.0 h1:gsVanPzJMpZQpwY+27/GQYElZez5CuMYwiIpk2A3RGw= github.com/bobg/errors v1.1.0/go.mod h1:Q4775qBZpnte7EGFJqmvnlB1U4pkI1XmU3qxqdp7Zcc= github.com/bobg/go-generics/v3 v3.7.0 h1:4SJHDWqONTRcA8al6491VW/ys6061bPCcTcI7YnIHPc= github.com/bobg/go-generics/v3 v3.7.0/go.mod h1:wGlMLQER92clsh3cJoQjbUtUEJ03FoxnGhZjaWhf4fM= +github.com/bobg/go-generics/v4 v4.2.0 h1:c3eX8rlFCRrxFnUepwQIA174JK7WuckbdRHf5ARCl7w= +github.com/bobg/go-generics/v4 v4.2.0/go.mod h1:KVwpxEYErjvcqjJSJqVNZd/JEq3SsQzb9t01+82pZGw= +github.com/bobg/seqs v1.8.0 h1:UfmjNlR3PIWfu+7ok4oVuekzlXWDL12g99fyMngNuT4= +github.com/bobg/seqs v1.8.0/go.mod h1:Iw4ESqX24EovuZ+0UHrnPmHYK1UyO9jcAZpPIzlNMa0= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/mattn/go-sqlite3 v1.14.22 h1:2gZY6PC6kBnID23Tichd1K+Z0oS6nE/XwU+Vz/5o4kU= From af4a0e050507fd85ce920cb60dbdb544aaec6c15 Mon Sep 17 00:00:00 2001 From: Bob Glickstein Date: Thu, 20 Nov 2025 14:55:31 -0800 Subject: [PATCH 2/6] Instead of collecting and sorting all matching named interfaces, just keep the best one seen so far. --- _testdata/foo.go | 6 ++++ _testdata/go.mod | 4 +-- decouple.go | 73 +++++++++++++++++++++++------------------------- 3 files changed, 43 insertions(+), 40 deletions(-) diff --git a/_testdata/foo.go b/_testdata/foo.go index 49530ea..7b46542 100644 --- a/_testdata/foo.go +++ b/_testdata/foo.go @@ -7,6 +7,12 @@ import ( "os" ) +// This named type should never be suggested, +// since it is superseded by an identical one in the stdlib. +type JankyReader interface { + Read([]byte) (int, error) +} + // {"r": {"Read": "func([]byte) (int, error)"}} // {"r": "io.Reader"} func F1(r *os.File, n int) ([]byte, error) { diff --git a/_testdata/go.mod b/_testdata/go.mod index 0031805..8a034ce 100644 --- a/_testdata/go.mod +++ b/_testdata/go.mod @@ -1,3 +1,3 @@ -module m +module github.com/bobg/decouple/testdata -go 1.19 +go 1.23 diff --git a/decouple.go b/decouple.go index c587981..51daab1 100644 --- a/decouple.go +++ b/decouple.go @@ -6,7 +6,6 @@ import ( "go/token" "go/types" "slices" - "sort" "strings" "github.com/bobg/errors" @@ -268,53 +267,51 @@ func (ch Checker) CheckParam(pkg *packages.Package, fndecl *ast.FuncDecl, name * // and returns the name of an interface defining exactly the methods in it, // if it can find one among the packages in the Checker. // If there are multiple such interfaces, -// one is chosen arbitrarily from the Go standard library, if any exist, -// otherwise from the "main module," if any exist there, +// one is chosen arbitrarily from the Go standard library if possible, +// otherwise from the "main module" if possible, // otherwise from any modules directly depended on by the main module, // and finally from any other module. func (ch Checker) NameForMethods(inp MethodMap) (*packages.Package, string) { - type pkgNamePair struct { - pkg *packages.Package - name string - } - var pairs []pkgNamePair + var ( + bestPkg *packages.Package + bestName string + bestIsMainModule bool + bestIsDirectDependency bool + ) for pkg, namedInterfaces := range ch.namedInterfaces { for name, methodMap := range namedInterfaces { - if sameMethodMaps(methodMap, inp) { - pairs = append(pairs, pkgNamePair{pkg: pkg, name: name}) + if !sameMethodMaps(methodMap, inp) { + continue + } + if isStdlib(pkg.PkgPath) { + // First match found in stdlib wins. + return pkg, name + } + if bestPkg == nil { + bestPkg, bestName = pkg, name + bestIsMainModule = isMainModulePackage(pkg) + bestIsDirectDependency = isDirectDependencyOfMainModule(pkg) + continue + } + if isMainModulePackage(pkg) { + if !bestIsMainModule { + bestPkg, bestName = pkg, name + bestIsMainModule = true + } + continue + } + if isDirectDependencyOfMainModule(pkg) { + if !bestIsDirectDependency { + bestPkg, bestName = pkg, name + bestIsDirectDependency = true + } + continue } } } - if len(pairs) == 0 { - return nil, "" - } - - sort.Slice(pairs, func(i, j int) bool { - a, b := pairs[i], pairs[j] - if isStdlib(a.pkg.PkgPath) && !isStdlib(b.pkg.PkgPath) { - return true - } - if !isStdlib(a.pkg.PkgPath) && isStdlib(b.pkg.PkgPath) { - return false - } - if isMainModulePackage(a.pkg) && !isMainModulePackage(b.pkg) { - return true - } - if !isMainModulePackage(a.pkg) && isMainModulePackage(b.pkg) { - return false - } - if isDirectDependencyOfMainModule(a.pkg) && !isDirectDependencyOfMainModule(b.pkg) { - return true - } - if !isDirectDependencyOfMainModule(a.pkg) && isDirectDependencyOfMainModule(b.pkg) { - return false - } - return a.pkg.PkgPath < b.pkg.PkgPath - }) - - return pairs[0].pkg, pairs[0].name + return bestPkg, bestName } func isStdlib(pkgPath string) bool { From 5854b72e5ba6f57a779042c081a8ebdedd7141c4 Mon Sep 17 00:00:00 2001 From: Bob Glickstein Date: Sat, 22 Nov 2025 09:04:36 -0800 Subject: [PATCH 3/6] Create bestChooser type and rewrite in terms of that. Add unit tests. --- cmd/decouple/main.go | 2 +- decouple.go | 153 +++++++++++++++++++------- decouple_test.go | 253 +++++++++++++++++++++++++++++++++++++++++++ types.go | 2 + 4 files changed, 371 insertions(+), 39 deletions(-) diff --git a/cmd/decouple/main.go b/cmd/decouple/main.go index 7b70c69..688ff31 100644 --- a/cmd/decouple/main.go +++ b/cmd/decouple/main.go @@ -38,7 +38,7 @@ func run(w io.Writer, verbose, doJSON bool, args []string) error { case 1: dir = args[0] default: - return fmt.Errorf("Usage: %s [-v] [-json] [DIR]", os.Args[0]) + return fmt.Errorf("usage: %s [-v] [-json] [DIR]", os.Args[0]) } checker, err := decouple.NewCheckerFromDir(dir) diff --git a/decouple.go b/decouple.go index 51daab1..d0d3c67 100644 --- a/decouple.go +++ b/decouple.go @@ -14,7 +14,7 @@ import ( // PkgMode is the minimal set of bit flags needed for the Config.Mode field of golang.org/x/go/packages // for the result to be usable by a Checker. -const PkgMode = packages.NeedName | packages.NeedFiles | packages.NeedImports | packages.NeedDeps | packages.NeedTypes | packages.NeedSyntax | packages.NeedTypesInfo +const PkgMode = packages.NeedName | packages.NeedFiles | packages.NeedImports | packages.NeedDeps | packages.NeedTypes | packages.NeedSyntax | packages.NeedTypesInfo | packages.NeedModule // Checker is the object that can analyze a directory tree of Go code, // or a set of packages loaded with "golang.org/x/go/packages".Load, @@ -26,7 +26,13 @@ type Checker struct { Verbose bool pkgs []*packages.Package - namedInterfaces map[*packages.Package]map[string]MethodMap // pkg -> named interface type -> method set + namedInterfaces map[*packages.Package]map[string]NamedInterfacePair // pkg -> named interface type -> (method map, is alias) +} + +// NamedInterfacePair is a MethodMap and an is-alias flag. +type NamedInterfacePair struct { + Map MethodMap + IsAlias bool } // NewCheckerFromDir creates a new Checker containing packages loaded @@ -53,20 +59,20 @@ func NewCheckerFromDir(dir string) (Checker, error) { // which should be the result of calling "golang.org/x/go/packages".Load // with at least the bits in PkgMode set in the Config.Mode field. func NewCheckerFromPackages(pkgs []*packages.Package) Checker { - namedInterfaces := make(map[*packages.Package]map[string]MethodMap) + namedInterfaces := make(map[*packages.Package]map[string]NamedInterfacePair) for _, pkg := range pkgs { findNamedInterfaces(pkg, namedInterfaces) } return Checker{pkgs: pkgs, namedInterfaces: namedInterfaces} } -func findNamedInterfaces(pkg *packages.Package, namedInterfaces map[*packages.Package]map[string]MethodMap) { +func findNamedInterfaces(pkg *packages.Package, namedInterfaces map[*packages.Package]map[string]NamedInterfacePair) { if _, ok := namedInterfaces[pkg]; ok { // Already visited this package. return } - namedInterfacesForPkg := make(map[string]MethodMap) + namedInterfacesForPkg := make(map[string]NamedInterfacePair) namedInterfaces[pkg] = namedInterfacesForPkg for _, ipkg := range pkg.Imports { @@ -106,7 +112,7 @@ func findNamedInterfaces(pkg *packages.Package, namedInterfaces map[*packages.Pa } mm := make(MethodMap) addMethodsToMap(intf, mm) - namedInterfacesForPkg[typespec.Name.Name] = mm + namedInterfacesForPkg[typespec.Name.Name] = NamedInterfacePair{Map: mm, IsAlias: typespec.Assign != token.NoPos} } } } @@ -266,58 +272,129 @@ func (ch Checker) CheckParam(pkg *packages.Package, fndecl *ast.FuncDecl, name * // NameForMethods takes a MethodMap // and returns the name of an interface defining exactly the methods in it, // if it can find one among the packages in the Checker. +// // If there are multiple such interfaces, // one is chosen arbitrarily from the Go standard library if possible, // otherwise from the "main module" if possible, // otherwise from any modules directly depended on by the main module, // and finally from any other module. +// +// Within each category, +// if two packages are from the same module, +// we prefer the one closer to the module root, +// and we prefer a non-alias type over an alias type. func (ch Checker) NameForMethods(inp MethodMap) (*packages.Package, string) { - var ( - bestPkg *packages.Package - bestName string - bestIsMainModule bool - bestIsDirectDependency bool - ) + var chooser bestChooser for pkg, namedInterfaces := range ch.namedInterfaces { - for name, methodMap := range namedInterfaces { - if !sameMethodMaps(methodMap, inp) { - continue - } - if isStdlib(pkg.PkgPath) { - // First match found in stdlib wins. - return pkg, name - } - if bestPkg == nil { - bestPkg, bestName = pkg, name - bestIsMainModule = isMainModulePackage(pkg) - bestIsDirectDependency = isDirectDependencyOfMainModule(pkg) + for name, pair := range namedInterfaces { + if !sameMethodMaps(pair.Map, inp) { continue } - if isMainModulePackage(pkg) { - if !bestIsMainModule { - bestPkg, bestName = pkg, name - bestIsMainModule = true - } - continue - } - if isDirectDependencyOfMainModule(pkg) { - if !bestIsDirectDependency { - bestPkg, bestName = pkg, name - bestIsDirectDependency = true - } - continue + chooser.choose(pkg, name, pair.IsAlias) + if chooser.isStdlib && !chooser.isAlias { + // Instant winner. + return chooser.pkg, chooser.name } } } - return bestPkg, bestName + return chooser.pkg, chooser.name +} + +type bestChooser struct { + pkg *packages.Package + name string + isAlias bool + isStdlib bool + isMainModule bool + isDirectDependency bool +} + +func (b *bestChooser) choose(pkg *packages.Package, name string, isAlias bool) { + switch { + case b.pkg == nil: + b.pkg = pkg + b.name = name + b.isStdlib = isStdlib(pkg.PkgPath) + b.isMainModule = isMainModulePackage(pkg) + b.isDirectDependency = isDirectDependencyOfMainModule(pkg) + b.isAlias = isAlias + + case isStdlib(pkg.PkgPath): + if !b.isStdlib || (b.isAlias && !isAlias) { + b.pkg = pkg + b.name = name + b.isStdlib = true + b.isMainModule = false + b.isDirectDependency = false + b.isAlias = isAlias + } + + case isMainModulePackage(pkg): + if b.isStdlib { + return + } + if b.isMainModule && sameModuleButHigher(b.pkg, pkg) { + return + } + if !b.isMainModule || sameModuleButHigher(pkg, b.pkg) || (b.isAlias && !isAlias) { + b.pkg = pkg + b.name = name + b.isStdlib = false + b.isMainModule = true + b.isDirectDependency = false + b.isAlias = isAlias + } + + case isDirectDependencyOfMainModule(pkg): + if b.isStdlib || b.isMainModule { + return + } + if b.isDirectDependency && sameModuleButHigher(b.pkg, pkg) { + return + } + if !b.isDirectDependency || sameModuleButHigher(pkg, b.pkg) || (b.isAlias && !isAlias) { + b.pkg = pkg + b.name = name + b.isStdlib = false + b.isMainModule = false + b.isDirectDependency = true + b.isAlias = isAlias + } + + case !b.isStdlib && !b.isMainModule && !b.isDirectDependency: + if sameModuleButHigher(pkg, b.pkg) || (!sameModuleButHigher(b.pkg, pkg) && b.isAlias && !isAlias) { + b.pkg = pkg + b.name = name + b.isStdlib = false + b.isMainModule = false + b.isDirectDependency = false + b.isAlias = isAlias + } + } } func isStdlib(pkgPath string) bool { return !strings.Contains(pkgPath, ".") } +// Return true if a and b are in the same module and a is closer to the module root than b. +// If a and b are at the same height in the same module, +// this function considers the length of the package paths. +// Shorter one wins. +func sameModuleButHigher(a, b *packages.Package) bool { + modpath := a.Module.Path + if b.Module.Path != modpath { + return false + } + acount, bcount := strings.Count(a.PkgPath, "/"), strings.Count(b.PkgPath, "/") + if acount != bcount { + return acount < bcount + } + return len(a.PkgPath) < len(b.PkgPath) +} + func isMainModulePackage(pkg *packages.Package) bool { return pkg.Module != nil && pkg.Module.Main } diff --git a/decouple_test.go b/decouple_test.go index 7b30fa9..9c395d4 100644 --- a/decouple_test.go +++ b/decouple_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/bobg/go-generics/v4/set" + "golang.org/x/tools/go/packages" // "github.com/davecgh/go-spew/spew" ) @@ -125,3 +126,255 @@ func TestGetIdent(t *testing.T) { t.Errorf("got %v, want foo", ident) } } + +func TestBestChooser(t *testing.T) { + cases := []struct { + name string + pkgpath1, pkgpath2 string + modpath1, modpath2 string + isAlias1, isAlias2 bool + isMain1, isMain2 bool + isDirect1, isDirect2 bool + want1 bool + }{{ + name: "stdlib vs non-stdlib", + pkgpath1: "fmt", + pkgpath2: "github.com/bobg/decouple", + modpath2: "github.com/bobg/decouple", + isAlias1: false, + isAlias2: false, + isMain1: false, + isMain2: true, + isDirect1: true, + isDirect2: true, + want1: true, + }, { + name: "non-stdlib vs stdlib", + pkgpath1: "github.com/bobg/decouple", + modpath1: "github.com/bobg/decouple", + pkgpath2: "fmt", + isAlias1: false, + isAlias2: false, + isMain1: true, + isMain2: false, + isDirect1: true, + isDirect2: true, + want1: false, + }, { + name: "higher alias vs lower non-alias in main module", + pkgpath1: "google.golang.org/protobuf/proto", + modpath1: "google.golang.org/protobuf", + pkgpath2: "google.golang.org/protobuf/reflect/protoreflect", + modpath2: "google.golang.org/protobuf", + isMain1: true, + isMain2: true, + isAlias1: true, + isAlias2: false, + want1: true, + }, { + name: "lower non-alias vs higher alias in main module", + pkgpath1: "google.golang.org/protobuf/reflect/protoreflect", + modpath1: "google.golang.org/protobuf", + pkgpath2: "google.golang.org/protobuf/proto", + modpath2: "google.golang.org/protobuf", + isMain1: true, + isMain2: true, + isAlias1: false, + isAlias2: true, + want1: false, + }, { + name: "higher alias vs lower non-alias in direct dependency", + pkgpath1: "google.golang.org/protobuf/proto", + modpath1: "google.golang.org/protobuf", + pkgpath2: "google.golang.org/protobuf/reflect/protoreflect", + modpath2: "google.golang.org/protobuf", + isDirect1: true, + isDirect2: true, + isAlias1: true, + isAlias2: false, + want1: true, + }, { + name: "lower non-alias vs higher alias in direct dependency", + pkgpath1: "google.golang.org/protobuf/reflect/protoreflect", + modpath1: "google.golang.org/protobuf", + pkgpath2: "google.golang.org/protobuf/proto", + modpath2: "google.golang.org/protobuf", + isDirect1: true, + isDirect2: true, + isAlias1: false, + isAlias2: true, + want1: false, + }, { + name: "higher alias vs lower non-alias in other dependency", + pkgpath1: "google.golang.org/protobuf/proto", + modpath1: "google.golang.org/protobuf", + pkgpath2: "google.golang.org/protobuf/reflect/protoreflect", + modpath2: "google.golang.org/protobuf", + isAlias1: true, + isAlias2: false, + want1: true, + }, { + name: "lower non-alias vs higher alias in other dependency", + pkgpath1: "google.golang.org/protobuf/reflect/protoreflect", + modpath1: "google.golang.org/protobuf", + pkgpath2: "google.golang.org/protobuf/proto", + modpath2: "google.golang.org/protobuf", + isAlias1: false, + isAlias2: true, + want1: false, + }, { + name: "same height alias vs non-alias in same other-dependency module", + pkgpath1: "google.golang.org/protobuf/proto", + modpath1: "google.golang.org/protobuf", + pkgpath2: "google.golang.org/protobuf/protoadapt", + modpath2: "google.golang.org/protobuf", + isAlias1: true, + isAlias2: false, + want1: true, + }, { + name: "same height non-alias vs alias in same other-dependency module", + pkgpath1: "google.golang.org/protobuf/proto", + modpath1: "google.golang.org/protobuf", + pkgpath2: "google.golang.org/protobuf/protoadapt", + modpath2: "google.golang.org/protobuf", + isAlias1: false, + isAlias2: true, + want1: true, + }, { + name: "alias vs non-alias in different direct dependency modules", + pkgpath1: "github.com/foo/pkg", + modpath1: "github.com/foo/pkg", + pkgpath2: "github.com/bar/pkg", + modpath2: "github.com/bar/pkg", + isDirect1: true, + isDirect2: true, + isAlias1: true, + isAlias2: false, + want1: false, + }, { + name: "non-alias vs alias in different direct dependency modules", + pkgpath1: "github.com/foo/pkg", + modpath1: "github.com/foo/pkg", + pkgpath2: "github.com/bar/pkg", + modpath2: "github.com/bar/pkg", + isDirect1: true, + isDirect2: true, + isAlias1: false, + isAlias2: true, + want1: true, + }, { + name: "alias vs non-alias in different non-direct-dependency modules", + pkgpath1: "github.com/foo/pkg", + modpath1: "github.com/foo/pkg", + pkgpath2: "github.com/bar/pkg", + modpath2: "github.com/bar/pkg", + isAlias1: true, + isAlias2: false, + want1: false, + }, { + name: "non-alias vs alias in different non-direct-dependency modules", + pkgpath1: "github.com/foo/pkg", + modpath1: "github.com/foo/pkg", + pkgpath2: "github.com/bar/pkg", + modpath2: "github.com/bar/pkg", + isAlias1: false, + isAlias2: true, + want1: true, + }, { + name: "alias vs non-alias in main vs direct-dependency module", + pkgpath1: "github.com/bobg/decouple", + modpath1: "github.com/bobg/decouple", + pkgpath2: "github.com/some/dependency", + modpath2: "github.com/some/dependency", + isMain1: true, + isDirect2: true, + isAlias1: true, + isAlias2: false, + want1: true, + }, { + name: "non-alias vs alias in main vs direct-dependency module", + pkgpath1: "github.com/bobg/decouple", + modpath1: "github.com/bobg/decouple", + pkgpath2: "github.com/some/dependency", + modpath2: "github.com/some/dependency", + isMain1: true, + isDirect2: true, + isAlias1: false, + isAlias2: true, + want1: true, + }, { + name: "alias vs non-alias in direct-dependency vs main module", + pkgpath1: "github.com/bobg/decouple", + modpath1: "github.com/bobg/decouple", + pkgpath2: "github.com/some/dependency", + modpath2: "github.com/some/dependency", + isDirect1: true, + isMain2: true, + isAlias1: true, + isAlias2: false, + want1: false, + }, { + name: "non-alias vs alias in direct-dependency vs main module", + pkgpath1: "github.com/bobg/decouple", + modpath1: "github.com/bobg/decouple", + pkgpath2: "github.com/some/dependency", + modpath2: "github.com/some/dependency", + isDirect1: true, + isMain2: true, + isAlias1: false, + isAlias2: true, + want1: false, + }, { + name: "main module vs direct dependency", + pkgpath1: "github.com/bobg/decouple", + modpath1: "github.com/bobg/decouple", + pkgpath2: "github.com/some/dependency", + modpath2: "github.com/some/dependency", + isMain1: true, + isDirect2: true, + want1: true, + }, { + name: "direct dependency vs main module", + pkgpath1: "github.com/some/dependency", + modpath1: "github.com/some/dependency", + pkgpath2: "github.com/bobg/decouple", + modpath2: "github.com/bobg/decouple", + isDirect1: true, + isMain2: true, + want1: false, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + pkg1 := &packages.Package{ + PkgPath: tc.pkgpath1, + Module: &packages.Module{ + Main: tc.isMain1, + Indirect: !tc.isDirect1, + }, + } + var chooser bestChooser + chooser.choose(pkg1, "x", tc.isAlias1) + + pkg2 := &packages.Package{ + PkgPath: tc.pkgpath2, + Module: &packages.Module{ + Main: tc.isMain2, + Indirect: !tc.isDirect2, + }, + } + + chooser.choose(pkg2, "y", tc.isAlias2) + + if tc.want1 { + if chooser.name != "x" { + t.Errorf("got name %q, want x", chooser.name) + } + } else { + if chooser.name != "y" { + t.Errorf("got name %q, want y", chooser.name) + } + } + }) + } +} diff --git a/types.go b/types.go index b81fe01..b02eccc 100644 --- a/types.go +++ b/types.go @@ -6,6 +6,8 @@ func getType[T types.Type](typ types.Type) T { switch typ := typ.(type) { case T: return typ + case *types.Alias: + return getType[T](typ.Rhs()) case *types.Named: return getType[T](typ.Underlying()) default: From a3ef065991e9099461cfed106f19753a60845d90 Mon Sep 17 00:00:00 2001 From: Bob Glickstein Date: Sat, 22 Nov 2025 09:06:23 -0800 Subject: [PATCH 4/6] Do not need to export namedInterfacePair. --- decouple.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/decouple.go b/decouple.go index d0d3c67..e8a0229 100644 --- a/decouple.go +++ b/decouple.go @@ -26,13 +26,12 @@ type Checker struct { Verbose bool pkgs []*packages.Package - namedInterfaces map[*packages.Package]map[string]NamedInterfacePair // pkg -> named interface type -> (method map, is alias) + namedInterfaces map[*packages.Package]map[string]namedInterfacePair // pkg -> named interface type -> (method map, is alias) } -// NamedInterfacePair is a MethodMap and an is-alias flag. -type NamedInterfacePair struct { - Map MethodMap - IsAlias bool +type namedInterfacePair struct { + mmap MethodMap + isAlias bool } // NewCheckerFromDir creates a new Checker containing packages loaded @@ -59,20 +58,20 @@ func NewCheckerFromDir(dir string) (Checker, error) { // which should be the result of calling "golang.org/x/go/packages".Load // with at least the bits in PkgMode set in the Config.Mode field. func NewCheckerFromPackages(pkgs []*packages.Package) Checker { - namedInterfaces := make(map[*packages.Package]map[string]NamedInterfacePair) + namedInterfaces := make(map[*packages.Package]map[string]namedInterfacePair) for _, pkg := range pkgs { findNamedInterfaces(pkg, namedInterfaces) } return Checker{pkgs: pkgs, namedInterfaces: namedInterfaces} } -func findNamedInterfaces(pkg *packages.Package, namedInterfaces map[*packages.Package]map[string]NamedInterfacePair) { +func findNamedInterfaces(pkg *packages.Package, namedInterfaces map[*packages.Package]map[string]namedInterfacePair) { if _, ok := namedInterfaces[pkg]; ok { // Already visited this package. return } - namedInterfacesForPkg := make(map[string]NamedInterfacePair) + namedInterfacesForPkg := make(map[string]namedInterfacePair) namedInterfaces[pkg] = namedInterfacesForPkg for _, ipkg := range pkg.Imports { @@ -112,7 +111,7 @@ func findNamedInterfaces(pkg *packages.Package, namedInterfaces map[*packages.Pa } mm := make(MethodMap) addMethodsToMap(intf, mm) - namedInterfacesForPkg[typespec.Name.Name] = NamedInterfacePair{Map: mm, IsAlias: typespec.Assign != token.NoPos} + namedInterfacesForPkg[typespec.Name.Name] = namedInterfacePair{mmap: mm, isAlias: typespec.Assign != token.NoPos} } } } @@ -288,10 +287,10 @@ func (ch Checker) NameForMethods(inp MethodMap) (*packages.Package, string) { for pkg, namedInterfaces := range ch.namedInterfaces { for name, pair := range namedInterfaces { - if !sameMethodMaps(pair.Map, inp) { + if !sameMethodMaps(pair.mmap, inp) { continue } - chooser.choose(pkg, name, pair.IsAlias) + chooser.choose(pkg, name, pair.isAlias) if chooser.isStdlib && !chooser.isAlias { // Instant winner. return chooser.pkg, chooser.name From cfbf541e931c512fa498558c5e7c0d58450fe986 Mon Sep 17 00:00:00 2001 From: Bob Glickstein Date: Sat, 22 Nov 2025 09:11:45 -0800 Subject: [PATCH 5/6] Remove dependency on v3 of bobg/go-generics. --- cmd/decouple/main.go | 5 +++-- go.mod | 1 - go.sum | 2 -- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/cmd/decouple/main.go b/cmd/decouple/main.go index 688ff31..7104bda 100644 --- a/cmd/decouple/main.go +++ b/cmd/decouple/main.go @@ -5,12 +5,13 @@ import ( "flag" "fmt" "io" + "maps" "os" + "slices" "sort" "strings" "github.com/bobg/errors" - "github.com/bobg/go-generics/v3/maps" "github.com/bobg/decouple" ) @@ -71,7 +72,7 @@ func run(w io.Writer, verbose, doJSON bool, args []string) error { for _, tuple := range tuples { var showedFuncName bool - params := maps.Keys(tuple.M) + params := slices.Collect(maps.Keys(tuple.M)) sort.Strings(params) for _, param := range params { mm := tuple.M[param] diff --git a/go.mod b/go.mod index 8597153..05d64ca 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,6 @@ toolchain go1.24.0 require ( github.com/bobg/errors v1.1.0 - github.com/bobg/go-generics/v3 v3.7.0 github.com/bobg/go-generics/v4 v4.2.0 github.com/bobg/seqs v1.8.0 golang.org/x/tools v0.30.0 diff --git a/go.sum b/go.sum index e690002..650694d 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,5 @@ github.com/bobg/errors v1.1.0 h1:gsVanPzJMpZQpwY+27/GQYElZez5CuMYwiIpk2A3RGw= github.com/bobg/errors v1.1.0/go.mod h1:Q4775qBZpnte7EGFJqmvnlB1U4pkI1XmU3qxqdp7Zcc= -github.com/bobg/go-generics/v3 v3.7.0 h1:4SJHDWqONTRcA8al6491VW/ys6061bPCcTcI7YnIHPc= -github.com/bobg/go-generics/v3 v3.7.0/go.mod h1:wGlMLQER92clsh3cJoQjbUtUEJ03FoxnGhZjaWhf4fM= github.com/bobg/go-generics/v4 v4.2.0 h1:c3eX8rlFCRrxFnUepwQIA174JK7WuckbdRHf5ARCl7w= github.com/bobg/go-generics/v4 v4.2.0/go.mod h1:KVwpxEYErjvcqjJSJqVNZd/JEq3SsQzb9t01+82pZGw= github.com/bobg/seqs v1.8.0 h1:UfmjNlR3PIWfu+7ok4oVuekzlXWDL12g99fyMngNuT4= From d4146fb3d50178bf62703d4653cd3df7864365b8 Mon Sep 17 00:00:00 2001 From: Bob Glickstein Date: Sat, 22 Nov 2025 09:14:24 -0800 Subject: [PATCH 6/6] Oops. --- cmd/decouple/decouple_test.go | 2 +- cmd/decouple/main.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/decouple/decouple_test.go b/cmd/decouple/decouple_test.go index 8c41ed6..b26bd98 100644 --- a/cmd/decouple/decouple_test.go +++ b/cmd/decouple/decouple_test.go @@ -34,7 +34,7 @@ func TestRunJSON(t *testing.T) { want := []jtuple{{ PackageName: "main", FileName: "main.go", - Line: 105, + Line: 106, Column: 6, FuncName: "showJSON", Params: []jparam{{ diff --git a/cmd/decouple/main.go b/cmd/decouple/main.go index 7104bda..4e50f88 100644 --- a/cmd/decouple/main.go +++ b/cmd/decouple/main.go @@ -94,7 +94,7 @@ func run(w io.Writer, verbose, doJSON bool, args []string) error { continue } - methods := maps.Keys(tuple.M[param]) + methods := slices.Collect(maps.Keys(tuple.M[param])) sort.Strings(methods) fmt.Fprintf(w, " %s: %v\n", param, methods) } @@ -122,7 +122,7 @@ func showJSON(w io.Writer, checker decouple.Checker, tuples []decouple.Tuple) er } jp := jparam{ Name: param, - Methods: maps.Keys(mm), + Methods: slices.Collect(maps.Keys(mm)), } sort.Strings(jp.Methods) if pkg, intfName := checker.NameForMethods(mm); intfName != "" {