v23/verror: fix how default verror package paths are calculated (#164)

It was a mistake to use go.mod files at runtime since they are likely not present when apps are actually run!

v23/verror/pkgpath.go

@@ -5,37 +5,19 @@
 package verror
 
 import (
-	"fmt"
-	"io/ioutil"
-	"os"
 	"path"
 	"path/filepath"
+	"reflect"
 	"runtime"
 	"strings"
 	"sync"
-
-	"golang.org/x/mod/modfile"
 )
 
 type pathCache struct {
 	sync.Mutex
 	paths map[string]string
 }
 
-func enclosingGoMod(dir string) (string, error) {
-	for {
-		gomodfile := filepath.Join(dir, "go.mod")
-		if fi, err := os.Stat(gomodfile); err == nil && !fi.IsDir() {
-			return dir, nil
-		}
-		d := filepath.Dir(dir)
-		if d == dir {
-			return "", fmt.Errorf("failed to find enclosing go.mod for dir %v", dir)
-		}
-		dir = d
-	}
-}
-
 var pkgPathCache = pathCache{
 	paths: make(map[string]string),
 }
@@ -53,40 +35,94 @@ func (pc *pathCache) set(dir, pkg string) {
 	pc.paths[dir] = pkg
 }
 
-func (pc *pathCache) pkgPath(file string) (string, error) {
-	dir := filepath.Clean(filepath.Dir(file))
-	if p, ok := pc.has(dir); ok {
-		return p, nil
-	}
-	root, err := enclosingGoMod(dir)
-	if err != nil {
-		return "", err
-	}
-	gomodfile := filepath.Join(root, "go.mod")
-	gomod, err := ioutil.ReadFile(gomodfile)
-	if err != nil {
-		return "", err
-	}
-	module := modfile.ModulePath(gomod)
-	if len(module) == 0 {
-		return "", fmt.Errorf("failed to read module path from %v", gomodfile)
+// IDPath returns a string of the form <package-path>.<name>
+// where <package-path> is derived from the type of the supplied
+// value. Typical usage would be except that dummy can be replaced
+// by an existing type defined in the package.
+//
+//  type dummy int
+//  verror.ID(verror.IDPath(dummy(0), "MyError"))
+//
+func IDPath(val interface{}, id string) ID {
+	return ID(reflect.TypeOf(val).PkgPath() + "." + id)
+}
+
+// longestCommonSuffix for a package path and filename.
+func longestCommonSuffix(pkgPath, filename string) (string, string) {
+	longestPkg, longestFilePath := "", ""
+	for {
+		fl := filepath.Base(filename)
+		pl := path.Base(pkgPath)
+		if fl == pl {
+			longestPkg = path.Join(fl, longestPkg)
+			longestFilePath = filepath.Join(fl, longestFilePath)
+			filename = filepath.Dir(filename)
+			pkgPath = path.Dir(pkgPath)
+			if fl == "/" {
+				break
+			}
+			continue
+		}
+		break
 	}
+	return longestPkg, longestFilePath
+}
+
+type pathState struct {
+	pkg string // pkg path for the value passed to init
+	dir string // the directory component for the file passed to init
+	// The portion of the local file path that is outside of the go module,
+	// e.g. for /a/b/c/core/v23/verror it would be /a/b/c/core.
+	filePrefix string
+	// the portion of the package path that does not appear in the file name,
+	// e.g. for /a/b/c/core/v23/verror and v.io/v23/verror it would be v.io.
+	pkgPrefix string
+}
+
+func (ps *pathState) init(pkgPath string, file string) {
+	ps.pkg = pkgPath
+	ps.dir = filepath.Dir(file)
+	pkgLCS, fileLCS := longestCommonSuffix(ps.pkg, ps.dir)
+	ps.filePrefix = filepath.Clean(strings.TrimSuffix(ps.dir, fileLCS))
+	ps.pkgPrefix = path.Clean(strings.TrimSuffix(ps.pkg, pkgLCS))
+}
+
+var (
+	ps       = &pathState{}
+	initOnce sync.Once
+)
 
-	pkgPath := strings.TrimPrefix(dir, root)
-	if !strings.HasPrefix(pkgPath, module) {
-		pkgPath = path.Join(module, pkgPath)
+func convertFileToPkgName(filename string) string {
+	return path.Clean(strings.ReplaceAll(filename, string(filepath.Separator), "/"))
+}
+
+func (pc *pathCache) pkgPath(file string) string {
+	initOnce.Do(func() {
+		type dummy int
+		_, file, _, _ := runtime.Caller(0)
+		ps.init(reflect.TypeOf(dummy(0)).PkgPath(), file)
+	})
+	pdir := filepath.Dir(file)
+	rel := strings.TrimPrefix(pdir, ps.filePrefix)
+	if rel == pdir {
+		return ""
 	}
-	pc.set(dir, pkgPath)
-	return pkgPath, nil
+	relPkg := convertFileToPkgName(rel)
+	pkgPath := path.Join(ps.pkgPrefix, relPkg)
+	pc.set(filepath.Dir(file), pkgPath)
+	return pkgPath
 }
 
 func ensurePackagePath(id ID) ID {
+	sid := string(id)
+	if strings.Contains(sid, ".") && sid[0] != '.' {
+		return id
+	}
 	_, file, _, _ := runtime.Caller(2)
-	pkg, err := pkgPathCache.pkgPath(file)
-	if err != nil {
-		panic(fmt.Sprintf("failed to determine package name for %v: %v", file, err))
+	pkg := pkgPathCache.pkgPath(file)
+	if len(pkg) == 0 {
+		return id
 	}
-	sid := string(id)
 	if strings.HasPrefix(sid, pkg) {
 		return id
 	}

v23/verror/pkgpath_test.go

@@ -0,0 +1,51 @@
+package verror
+
+import (
+	"path/filepath"
+	"reflect"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+func TestLCS(t *testing.T) {
+	for i, tc := range []struct {
+		pkg, path       string
+		lcsPkg, lcsPath string
+	}{
+		{"a", "b", "", ""},
+		{"/a/b/c", "/a/b/c", "/a/b/c", "/a/b/c"},
+		{"/a/b/c/d", "/x/y/c/d", "c/d", "c/d"},
+	} {
+		lcsPkg, lcsPath := longestCommonSuffix(tc.pkg, tc.path)
+		if got, want := lcsPkg, tc.lcsPkg; got != want {
+			t.Errorf("%v: got %v, want %v", i, got, want)
+		}
+		if got, want := lcsPath, tc.lcsPath; got != want {
+			t.Errorf("%v: got %v, want %v", i, got, want)
+		}
+	}
+}
+
+func TestPkgPath(t *testing.T) {
+	type dummy int
+	ps := &pathState{}
+	_, file, _, _ := runtime.Caller(0)
+	ps.init(reflect.TypeOf(dummy(0)).PkgPath(), file)
+	if got, want := ps.pkg, "v.io/v23/verror"; got != want {
+		t.Errorf("got %v, want %v", got, want)
+	}
+	if got, want := ps.dir, filepath.Dir(file); got != want {
+		t.Errorf("got %v, want %v", got, want)
+	}
+	if got, want := strings.HasPrefix(file, ps.filePrefix), true; got != want {
+		t.Errorf("got %v, want %v", got, want)
+	}
+	if got, want := ps.pkgPrefix, "v.io"; got != want {
+		t.Errorf("got %v, want %v", got, want)
+	}
+	ps.init("github.com/grailbio/base", "/a/b/src/base/file")
+	if got, want := ps.pkgPrefix, "github.com/grailbio"; got != want {
+		t.Errorf("got %v, want %v", got, want)
+	}
+}

v23/verror/verror.go

@@ -178,9 +178,8 @@ type IDAction struct {
 
 // Register returns a IDAction with the given ID and Action fields, and
 // inserts a message into the default i18n Catalogue in US English.
-// Other languages can be added by adding to the Catalogue. Register
-// will internally prepend the caller's package path to id if it's not
-// already present.
+// Other languages can be added by adding to the Catalogue.
+// IDPath can be used to generate an appropriate ID.
 func Register(id ID, action ActionCode, englishText string) IDAction {
 	id = ensurePackagePath(id)
 	i18n.Cat().SetWithBase(defaultLangID(i18n.NoLangID), i18n.MsgID(id), englishText)
@@ -189,8 +188,7 @@ func Register(id ID, action ActionCode, englishText string) IDAction {
 
 // NewIDAction creates a new instance of IDAction with the given ID and Action
 // field. It should be used when localization support is not required instead
-// of Register.  NewIDAction will internally prepend the caller's package path
-// to id if it's not already present.
+// of Register. IDPath can be used to
 func NewIDAction(id ID, action ActionCode) IDAction {
 	return IDAction{ensurePackagePath(id), action}
 }

v23/verror/verror_id_test.go

@@ -0,0 +1,24 @@
+package verror_test
+
+import (
+	"testing"
+
+	"v.io/v23/flow"
+	"v.io/v23/verror"
+	"v.io/x/ref/lib/security"
+)
+
+func TestPackageErrorIDs(t *testing.T) {
+	for i, tc := range []struct {
+		err error
+		id  string
+	}{
+		{verror.ErrInternal.Errorf(nil, "x"), "v.io/v23/verror.Internal"},
+		{flow.ErrAuth.Errorf(nil, "x"), "v.io/v23/flow.Auth"},
+		{security.ErrBadPassphrase.Errorf(nil, "x"), "v.io/x/ref/lib/security.errBadPassphrase"},
+	} {
+		if got, want := verror.ErrorID(tc.err), verror.ID(tc.id); got != want {
+			t.Errorf("%v: got %v, want %v", i, got, want)
+		}
+	}
+}

x/ref/runtime/internal/flow/util_test.go

@@ -24,7 +24,7 @@ func TestMaybeWrapError(t *testing.T) {
 		{verror.ErrUnknown.Errorf(ctx, "some random thing"), false},
 		{flow.ErrAuth.Errorf(ctx, ""), false},
 	}
-	for _, test := range tests {
+	for i, test := range tests {
 		werr := MaybeWrapError(flow.ErrAuth, ctx, test.err)
 		// If the returned error is not equal to the original error it was wrapped.
 		msg := ""
@@ -33,9 +33,9 @@ func TestMaybeWrapError(t *testing.T) {
 		}
 		if wasWrapped := werr.Error() != msg; wasWrapped != test.wrap {
 			if test.wrap {
-				t.Errorf("wanted %v to be wrapped", test.err)
+				t.Errorf("%v: wanted %v to be wrapped", i, test.err)
 			} else {
-				t.Errorf("did not want %v to be wrapped", test.err)
+				t.Errorf("%v: did not want %v to be wrapped", i, test.err)
 			}
 		}
 	}