From fd0ca168486851f315e701994b1956c7858a5b71 Mon Sep 17 00:00:00 2001 From: Prashant V Date: Tue, 13 Feb 2024 23:18:06 -0800 Subject: [PATCH 01/14] cmd/errtrace: Split parse and rewrite out for toolexec --- cmd/errtrace/main.go | 111 ++++++++++++++++++++++++++++--------------- 1 file changed, 72 insertions(+), 39 deletions(-) diff --git a/cmd/errtrace/main.go b/cmd/errtrace/main.go index d34342a..744dda5 100644 --- a/cmd/errtrace/main.go +++ b/cmd/errtrace/main.go @@ -346,18 +346,65 @@ type fileRequest struct { // The collected information is used to pick a package name, // whether we need an import, etc. and *then* the edits are applied. func (cmd *mainCmd) processFile(r fileRequest) error { - fset := token.NewFileSet() - src, err := cmd.readFile(r) if err != nil { return errtrace.Wrap(err) } - f, err := parser.ParseFile(fset, r.Filename, src, parser.ParseComments) + parsed, err := cmd.parseFile(r.Filename, src) if err != nil { return errtrace.Wrap(err) } + for _, line := range parsed.unusedOptouts { + cmd.log.Printf("%s:%d:unused errtrace:skip", r.Filename, line) + } + if r.List { + if len(parsed.inserts) > 0 { + _, err = fmt.Fprintf(cmd.Stdout, "%s\n", r.Filename) + } + return errtrace.Wrap(err) + } + + out := bytes.NewBuffer(nil) + if err := cmd.rewriteFile(parsed, out); err != nil { + return errtrace.Wrap(err) + } + + outSrc := out.Bytes() + if r.Format { + outSrc, err = gofmt.Source(outSrc) + if err != nil { + return errtrace.Wrap(fmt.Errorf("format: %w", err)) + } + } + + if r.Write { + err = os.WriteFile(r.Filename, outSrc, 0o644) + } else { + _, err = cmd.Stdout.Write(outSrc) + } + return errtrace.Wrap(err) +} + +type file struct { + src []byte + fset *token.FileSet + file *ast.File + + errtracePkg string + importsErrtrace bool + inserts []insert + unusedOptouts []int +} + +func (cmd *mainCmd) parseFile(filename string, src []byte) (file, error) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, filename, src, parser.ParseComments) + if err != nil { + return file{}, errtrace.Wrap(err) + } + errtracePkg := "errtrace" // name to use for errtrace package var importsErrtrace bool // whether the file imports errtrace already for _, imp := range f.Imports { @@ -409,25 +456,15 @@ func (cmd *mainCmd) processFile(r fileRequest) error { ast.Walk(&w, f) // Look for unused optouts and warn about them. + var unusedOptouts []int if len(w.optouts) > 0 { - unusedOptouts := make([]int, 0, len(w.optouts)) + unusedOptouts = make([]int, 0, len(w.optouts)) for line, used := range w.optouts { if used == 0 { unusedOptouts = append(unusedOptouts, line) } } sort.Ints(unusedOptouts) - - for _, line := range unusedOptouts { - cmd.log.Printf("%s:%d:unused errtrace:skip", r.Filename, line) - } - } - - if r.List { - if len(inserts) > 0 { - _, err = fmt.Fprintf(cmd.Stdout, "%s\n", r.Filename) - } - return errtrace.Wrap(err) } // If errtrace isn't imported, but at least one insert was made, @@ -487,13 +524,23 @@ func (cmd *mainCmd) processFile(r fileRequest) error { return inserts[i].Pos() < inserts[j].Pos() }) - out := bytes.NewBuffer(nil) + return file{ + src: src, + fset: fset, + file: f, + errtracePkg: errtracePkg, + importsErrtrace: importsErrtrace, + inserts: inserts, + unusedOptouts: unusedOptouts, + }, nil +} +func (cmd *mainCmd) rewriteFile(f file, out *bytes.Buffer) error { var lastOffset int - filePos := fset.File(f.Pos()) // position information for this file - for _, it := range inserts { + filePos := f.fset.File(f.file.Pos()) // position information for this file + for _, it := range f.inserts { offset := filePos.Offset(it.Pos()) - _, _ = out.Write(src[lastOffset:offset]) + _, _ = out.Write(f.src[lastOffset:offset]) lastOffset = offset switch it := it.(type) { @@ -503,15 +550,15 @@ func (cmd *mainCmd) processFile(r fileRequest) error { _, _ = io.WriteString(out, "import ") } - if errtracePkg == "errtrace" { + if f.errtracePkg == "errtrace" { // Don't use named imports if we're using the default name. fmt.Fprintf(out, "%q", "braces.dev/errtrace") } else { - fmt.Fprintf(out, "%s %q", errtracePkg, "braces.dev/errtrace") + fmt.Fprintf(out, "%s %q", f.errtracePkg, "braces.dev/errtrace") } case *insertWrapOpen: - fmt.Fprintf(out, "%s.Wrap", errtracePkg) + fmt.Fprintf(out, "%s.Wrap", f.errtracePkg) if it.N > 1 { fmt.Fprintf(out, "%d", it.N) } @@ -536,7 +583,7 @@ func (cmd *mainCmd) processFile(r fileRequest) error { if i > 0 { _, _ = out.WriteString(", ") } - fmt.Fprintf(out, "%s.Wrap(%s)", errtracePkg, name) + fmt.Fprintf(out, "%s.Wrap(%s)", f.errtracePkg, name) } _, _ = out.WriteString("; ") @@ -544,22 +591,8 @@ func (cmd *mainCmd) processFile(r fileRequest) error { cmd.log.Panicf("unhandled insertion type %T", it) } } - _, _ = out.Write(src[lastOffset:]) // flush remaining - - outSrc := out.Bytes() - if r.Format { - outSrc, err = gofmt.Source(outSrc) - if err != nil { - return errtrace.Wrap(fmt.Errorf("format: %w", err)) - } - } - - if r.Write { - err = os.WriteFile(r.Filename, outSrc, 0o644) - } else { - _, err = cmd.Stdout.Write(outSrc) - } - return errtrace.Wrap(err) + _, _ = out.Write(f.src[lastOffset:]) // flush remaining + return nil } func (cmd *mainCmd) readFile(r fileRequest) ([]byte, error) { From a2016a3d48c85b52705092ed0d0786f420b97b71 Mon Sep 17 00:00:00 2001 From: Prashant V Date: Tue, 13 Feb 2024 21:53:17 -0800 Subject: [PATCH 02/14] cmd/errtrace: Add initial toolexec mode Use TOOLEXEC_IMPORTPATH to determine if we're running in toolexec mode. In toolexec mode, all go tool executions are intercepted, though only compile commands interacting with .go files are modified. Only .go files that import errtrace are rewritten to a temporary file and passed to compile instead of the original path. --- cmd/errtrace/main.go | 9 +- cmd/errtrace/testdata/main/foo/foo.go | 7 + cmd/errtrace/testdata/main/go.mod | 5 + cmd/errtrace/testdata/main/go.sum | 2 + cmd/errtrace/testdata/main/main.go | 13 ++ cmd/errtrace/toolexec.go | 180 ++++++++++++++++++++++++++ 6 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 cmd/errtrace/testdata/main/foo/foo.go create mode 100644 cmd/errtrace/testdata/main/go.mod create mode 100644 cmd/errtrace/testdata/main/go.sum create mode 100644 cmd/errtrace/testdata/main/main.go create mode 100644 cmd/errtrace/toolexec.go diff --git a/cmd/errtrace/main.go b/cmd/errtrace/main.go index 744dda5..f23c1f8 100644 --- a/cmd/errtrace/main.go +++ b/cmd/errtrace/main.go @@ -57,7 +57,9 @@ func main() { Stdin: os.Stdin, Stderr: os.Stderr, Stdout: os.Stdout, + Getenv: os.Getenv, } + os.Exit(cmd.Run(os.Args[1:])) } @@ -98,8 +100,6 @@ func (p *mainParams) Parse(w io.Writer, args []string) error { flag.BoolVar(&p.List, "l", false, "list files that would be modified without making any changes.") - // TODO: toolexec mode - if err := flag.Parse(args); err != nil { return errtrace.Wrap(err) } @@ -176,6 +176,7 @@ type mainCmd struct { Stdin io.Reader Stdout io.Writer Stderr io.Writer + Getenv func(string) string log *log.Logger } @@ -183,6 +184,10 @@ type mainCmd struct { func (cmd *mainCmd) Run(args []string) (exitCode int) { cmd.log = log.New(cmd.Stderr, "", 0) + if exitCode, ok := cmd.handleToolExec(args); ok { + return exitCode + } + var p mainParams if err := p.Parse(cmd.Stderr, args); err != nil { if errors.Is(err, flag.ErrHelp) { diff --git a/cmd/errtrace/testdata/main/foo/foo.go b/cmd/errtrace/testdata/main/foo/foo.go new file mode 100644 index 0000000..b64af2b --- /dev/null +++ b/cmd/errtrace/testdata/main/foo/foo.go @@ -0,0 +1,7 @@ +package foo + +import "errors" + +func Foo() error { + return errors.New("test") +} diff --git a/cmd/errtrace/testdata/main/go.mod b/cmd/errtrace/testdata/main/go.mod new file mode 100644 index 0000000..7be1ff3 --- /dev/null +++ b/cmd/errtrace/testdata/main/go.mod @@ -0,0 +1,5 @@ +module braces.dev/errtrace/cmd/errtrace/testdata/main + +go 1.21.4 + +require braces.dev/errtrace v0.3.0 diff --git a/cmd/errtrace/testdata/main/go.sum b/cmd/errtrace/testdata/main/go.sum new file mode 100644 index 0000000..983e90b --- /dev/null +++ b/cmd/errtrace/testdata/main/go.sum @@ -0,0 +1,2 @@ +braces.dev/errtrace v0.3.0 h1:pzfd6LcWgfWtXLaNFWRnxV/7NP+FSOlIjRLwDuHfPxs= +braces.dev/errtrace v0.3.0/go.mod h1:YQpXdo+u5iimgQdZzFoic8AjedEDncXGpp6/2SfazzI= diff --git a/cmd/errtrace/testdata/main/main.go b/cmd/errtrace/testdata/main/main.go new file mode 100644 index 0000000..1ca803f --- /dev/null +++ b/cmd/errtrace/testdata/main/main.go @@ -0,0 +1,13 @@ +package main + +import ( + "fmt" + + "braces.dev/errtrace/cmd/errtrace/testdata/main/foo" +) + +func main() { + if err := foo.Foo(); err != nil { + fmt.Printf("%+v\n", err) + } +} diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go new file mode 100644 index 0000000..b6eb1a8 --- /dev/null +++ b/cmd/errtrace/toolexec.go @@ -0,0 +1,180 @@ +package main + +import ( + "bytes" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + + "braces.dev/errtrace" +) + +func (cmd *mainCmd) handleToolExec(args []string) (exitCode int, handled bool) { + // In toolexec mode, we're passed the original command + arguments. + if len(args) == 0 { + return -1, false + } + + for _, arg := range args { + if arg == "-V=full" { + // compile is run first with "-V=full" to get a version number + // for caching build IDs. + // No TOOLEXEC_IMPORTPATH is set in this case. + return cmd.toolExecVersion(args), true + } + } + + if cmd.Getenv == nil { + cmd.Getenv = os.Getenv + } + // When "-toolexec" is used, the go cmd sets the package being compiled in the env. + if pkg := cmd.Getenv("TOOLEXEC_IMPORTPATH"); pkg != "" { + return cmd.toolExecRewrite(pkg, args), true + } + + return -1, false +} + +func (cmd *mainCmd) toolExecVersion(args []string) int { + tool := exec.Command(args[0], args[1:]...) + var stdout bytes.Buffer + tool.Stdout = &stdout + tool.Stderr = cmd.Stderr + tool.Env = os.Environ() + if err := tool.Run(); err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + return exitErr.ExitCode() + } + + fmt.Fprintf(cmd.Stderr, "%v failed: %v", args[0], err) + return 1 + } + + // TODO: This version number should change whenever the rewriting changes. + fmt.Fprintf(cmd.Stdout, "%s-errtrace0\n", strings.TrimSpace(stdout.String())) + return 0 +} + +func (cmd *mainCmd) toolExecRewrite(pkg string, args []string) (exitCode int) { + // We only need to modify the arguments for "compile" calls which work with .go files. + if !isCompile(args[0]) { + return cmd.runOriginal(args) + } + + // We only modify files that import errtrace, and have "errtrace.ToolExecInstrument" + // However, that requires parsing files to determine, which we want to avoid for stdlib + // so use a heuristic to detect stdlib packages -- whether the name contains "."". + if !strings.Contains(pkg, ".") { + return cmd.runOriginal(args) + } + + exitCode, err := cmd.rewriteCompile(pkg, args) + if err != nil { + cmd.log.Print(err) + return 1 + } + + return exitCode +} + +func (cmd *mainCmd) rewriteCompile(pkg string, args []string) (exitCode int, _ error) { + parsed := make(map[string]file) + var canRewrite, needRewrite bool + for _, arg := range args { + if !isGoFile(arg) { + continue + } + + contents, err := os.ReadFile(arg) + if err != nil { + return -1, errtrace.Wrap(err) + } + + f, err := cmd.parseFile(arg, contents) + if err != nil { + return -1, errtrace.Wrap(err) + } + parsed[arg] = f + + // TODO: Support an "unsafe" mode to rewrite packages without errtrace imports. + if f.importsErrtrace { + canRewrite = true + } + if len(f.inserts) > 0 { + needRewrite = true + } + } + + if !canRewrite || !needRewrite { + return cmd.runOriginal(args), nil + } + + // Use a temporary directory per-package that is rewritten. + tempDir, err := os.MkdirTemp("", filepath.Base(pkg)) + if err != nil { + return -1, errtrace.Wrap(err) + } + + newArgs := make([]string, 0, len(args)) + for _, arg := range args { + if !isGoFile(arg) { + newArgs = append(newArgs, arg) + continue + } + + f, ok := parsed[arg] + if !ok || len(f.inserts) == 0 { + newArgs = append(newArgs, arg) + continue + } + + // Add a //line directive so the original filepath is used in errors and panics. + out := bytes.NewBuffer(nil) + _, _ = fmt.Fprintf(out, "//line %v:1\n", arg) + + if err := cmd.rewriteFile(f, out); err != nil { + return -1, errtrace.Wrap(err) + } + + newFile := filepath.Join(tempDir, filepath.Base(arg)) + if err := os.WriteFile(newFile, out.Bytes(), 0o666); err != nil { + return -1, errtrace.Wrap(err) + } + + newArgs = append(newArgs, newFile) + } + + return cmd.runOriginal(newArgs), nil +} + +func isCompile(arg string) bool { + if runtime.GOOS == "windows" { + arg = strings.TrimSuffix(arg, ".exe") + } + return strings.HasSuffix(arg, "compile") +} + +func isGoFile(arg string) bool { + return strings.HasSuffix(arg, ".go") +} + +func (cmd *mainCmd) runOriginal(args []string) (exitCode int) { + tool := exec.Command(args[0], args[1:]...) + tool.Stdin = cmd.Stdin + tool.Stdout = cmd.Stdout + tool.Stderr = cmd.Stderr + tool.Env = os.Environ() + + if err := tool.Run(); err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + return exitErr.ExitCode() + } + fmt.Fprintf(cmd.Stderr, "tool failed: %v", err) + return 1 + } + + return 0 +} From 2fe0af197a292f77093e3ebfbd9fd71f86e56f1c Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 18:01:13 -0800 Subject: [PATCH 03/14] cmd/errtrace: Add toolexec test --- .../testdata/toolexec-test/errtrace.go | 4 + cmd/errtrace/testdata/toolexec-test/main.go | 17 +++ cmd/errtrace/testdata/toolexec-test/p1/p1.go | 12 ++ .../testdata/toolexec-test/p2/errtrace.go | 4 + cmd/errtrace/testdata/toolexec-test/p2/p2.go | 12 ++ cmd/errtrace/toolexec_test.go | 117 ++++++++++++++++++ 6 files changed, 166 insertions(+) create mode 100644 cmd/errtrace/testdata/toolexec-test/errtrace.go create mode 100644 cmd/errtrace/testdata/toolexec-test/main.go create mode 100644 cmd/errtrace/testdata/toolexec-test/p1/p1.go create mode 100644 cmd/errtrace/testdata/toolexec-test/p2/errtrace.go create mode 100644 cmd/errtrace/testdata/toolexec-test/p2/p2.go create mode 100644 cmd/errtrace/toolexec_test.go diff --git a/cmd/errtrace/testdata/toolexec-test/errtrace.go b/cmd/errtrace/testdata/toolexec-test/errtrace.go new file mode 100644 index 0000000..3df1b34 --- /dev/null +++ b/cmd/errtrace/testdata/toolexec-test/errtrace.go @@ -0,0 +1,4 @@ +package main + +// Opt-in to errtrace wrapping with toolexec. +import _ "braces.dev/errtrace" diff --git a/cmd/errtrace/testdata/toolexec-test/main.go b/cmd/errtrace/testdata/toolexec-test/main.go new file mode 100644 index 0000000..056c7cf --- /dev/null +++ b/cmd/errtrace/testdata/toolexec-test/main.go @@ -0,0 +1,17 @@ +package main + +import ( + "fmt" + + "braces.dev/errtrace/cmd/errtrace/testdata/toolexec-test/p1" +) + +func main() { + if err := callP1(); err != nil { + fmt.Printf("%+v\n", err) + } +} + +func callP1() error { + return p1.WrapP2() // @trace +} diff --git a/cmd/errtrace/testdata/toolexec-test/p1/p1.go b/cmd/errtrace/testdata/toolexec-test/p1/p1.go new file mode 100644 index 0000000..100cf01 --- /dev/null +++ b/cmd/errtrace/testdata/toolexec-test/p1/p1.go @@ -0,0 +1,12 @@ +package p1 + +import ( + "fmt" + + "braces.dev/errtrace/cmd/errtrace/testdata/toolexec-test/p2" +) + +// WrapP2 wraps an error return from p2. +func WrapP2() error { + return fmt.Errorf("test2: %w", p2.ReturnErr()) +} diff --git a/cmd/errtrace/testdata/toolexec-test/p2/errtrace.go b/cmd/errtrace/testdata/toolexec-test/p2/errtrace.go new file mode 100644 index 0000000..d5e4057 --- /dev/null +++ b/cmd/errtrace/testdata/toolexec-test/p2/errtrace.go @@ -0,0 +1,4 @@ +package p2 + +// Opt-in to errtrace wrapping with toolexec. +import _ "braces.dev/errtrace" diff --git a/cmd/errtrace/testdata/toolexec-test/p2/p2.go b/cmd/errtrace/testdata/toolexec-test/p2/p2.go new file mode 100644 index 0000000..b976078 --- /dev/null +++ b/cmd/errtrace/testdata/toolexec-test/p2/p2.go @@ -0,0 +1,12 @@ +package p2 + +import ( + "errors" + + "braces.dev/errtrace" +) + +// ReturnErr returns an error. +func ReturnErr() error { + return errtrace.Wrap(errors.New("test")) // @trace +} diff --git a/cmd/errtrace/toolexec_test.go b/cmd/errtrace/toolexec_test.go new file mode 100644 index 0000000..3d7705f --- /dev/null +++ b/cmd/errtrace/toolexec_test.go @@ -0,0 +1,117 @@ +package main + +import ( + "bufio" + "bytes" + "fmt" + "io/fs" + "os" + "os/exec" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" + + "braces.dev/errtrace" + "braces.dev/errtrace/internal/diff" +) + +func TestToolExec(t *testing.T) { + const testProg = "./testdata/toolexec-test" + + errTraceCmd := filepath.Join(t.TempDir(), "errtrace") + runGo(t, ".", "build", "-o", errTraceCmd, ".") + + var wantTraces []string + err := filepath.Walk(testProg, func(path string, info fs.FileInfo, err error) error { + if err != nil { + return errtrace.Wrap(err) + } + if info.IsDir() { + return nil + } + + for _, line := range findTraceLines(t, path) { + absPath, err := filepath.Abs(path) + if err != nil { + t.Fatalf("abspath: %v", err) + } + + wantTraces = append(wantTraces, fmt.Sprintf("%v:%v", absPath, line)) + } + return nil + }) + if err != nil { + t.Fatal("Walk failed", err) + } + sort.Strings(wantTraces) + + t.Run("no toolexec", func(t *testing.T) { + stdout, _ := runGo(t, testProg, "run", ".") + if lines := fileLines(stdout); len(lines) > 0 { + t.Errorf("expected no file:line, got %v", lines) + } + }) + + t.Run("with toolexec", func(t *testing.T) { + stdout, _ := runGo(t, testProg, "run", "-toolexec", errTraceCmd, ".") + gotLines := fileLines(stdout) + + sort.Strings(gotLines) + if d := diff.Diff(wantTraces, gotLines); d != "" { + t.Errorf("diff in traces:\n%s", d) + t.Errorf("go run output:\n%s", stdout) + } + }) +} + +func findTraceLines(t testing.TB, file string) []int { + f, err := os.Open(file) + if err != nil { + t.Fatal(err) + } + defer f.Close() //nolint:errcheck + + var traces []int + scanner := bufio.NewScanner(f) + var lineNum int + for scanner.Scan() { + lineNum++ + line := scanner.Text() + if strings.Contains(line, "// @trace") { + traces = append(traces, lineNum) + } + } + if err := scanner.Err(); err != nil { + t.Fatal(err) + } + + return traces +} + +var fileLineRegex = regexp.MustCompile(`^\s*(.*:[0-9]+)$`) + +func fileLines(out string) []string { + var fileLines []string + for _, line := range strings.Split(out, "\n") { + if fileLineRegex.MatchString(line) { + fileLines = append(fileLines, strings.TrimSpace(line)) + } + } + return fileLines +} + +func runGo(t testing.TB, dir string, args ...string) (stdout, stderr string) { + var stdoutBuf, stderrBuf bytes.Buffer + cmd := exec.Command("go", args...) + cmd.Dir = dir + cmd.Stdin = nil + cmd.Stdout = &stdoutBuf + cmd.Stderr = &stderrBuf + if err := cmd.Run(); err != nil { + t.Fatalf("run failed: %v", err) + } + + return stdoutBuf.String(), stderrBuf.String() +} From 4ffa91402005194d2a014ac2c83b4c13fe938cec Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 18:26:30 -0800 Subject: [PATCH 04/14] Windows support for toolexec tests --- cmd/errtrace/toolexec_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cmd/errtrace/toolexec_test.go b/cmd/errtrace/toolexec_test.go index 3d7705f..5e590d9 100644 --- a/cmd/errtrace/toolexec_test.go +++ b/cmd/errtrace/toolexec_test.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "regexp" + "runtime" "sort" "strings" "testing" @@ -21,6 +22,9 @@ func TestToolExec(t *testing.T) { const testProg = "./testdata/toolexec-test" errTraceCmd := filepath.Join(t.TempDir(), "errtrace") + if runtime.GOOS == "windows" { + errTraceCmd += ".exe" // can't run binaries on Windows otherwise. + } runGo(t, ".", "build", "-o", errTraceCmd, ".") var wantTraces []string @@ -37,6 +41,11 @@ func TestToolExec(t *testing.T) { if err != nil { t.Fatalf("abspath: %v", err) } + if runtime.GOOS == "windows" { + // On Windows, absPath uses windows path separators, e.g., "c:\foo" + // but the paths reported in traces contain '/'. + absPath = filepath.ToSlash(absPath) + } wantTraces = append(wantTraces, fmt.Sprintf("%v:%v", absPath, line)) } From fd2b20564f86f58e62c900714423cb7c8afe2eee Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 21:43:29 -0800 Subject: [PATCH 05/14] Use `var out bytes.Buffer` instead of `NewBuffer(nil)` --- cmd/errtrace/main.go | 4 ++-- cmd/errtrace/toolexec.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/errtrace/main.go b/cmd/errtrace/main.go index f23c1f8..da02727 100644 --- a/cmd/errtrace/main.go +++ b/cmd/errtrace/main.go @@ -371,8 +371,8 @@ func (cmd *mainCmd) processFile(r fileRequest) error { return errtrace.Wrap(err) } - out := bytes.NewBuffer(nil) - if err := cmd.rewriteFile(parsed, out); err != nil { + var out bytes.Buffer + if err := cmd.rewriteFile(parsed, &out); err != nil { return errtrace.Wrap(err) } diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go index b6eb1a8..fa9ea73 100644 --- a/cmd/errtrace/toolexec.go +++ b/cmd/errtrace/toolexec.go @@ -132,10 +132,10 @@ func (cmd *mainCmd) rewriteCompile(pkg string, args []string) (exitCode int, _ e } // Add a //line directive so the original filepath is used in errors and panics. - out := bytes.NewBuffer(nil) - _, _ = fmt.Fprintf(out, "//line %v:1\n", arg) + var out bytes.Buffer + _, _ = fmt.Fprintf(&out, "//line %v:1\n", arg) - if err := cmd.rewriteFile(f, out); err != nil { + if err := cmd.rewriteFile(f, &out); err != nil { return -1, errtrace.Wrap(err) } From c5d6e22d422e89393897933761f812021af0a5c2 Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 21:52:35 -0800 Subject: [PATCH 06/14] Rename `file` type to `parsedFile` --- cmd/errtrace/main.go | 10 +++++----- cmd/errtrace/toolexec.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/errtrace/main.go b/cmd/errtrace/main.go index da02727..93ab526 100644 --- a/cmd/errtrace/main.go +++ b/cmd/errtrace/main.go @@ -392,7 +392,7 @@ func (cmd *mainCmd) processFile(r fileRequest) error { return errtrace.Wrap(err) } -type file struct { +type parsedFile struct { src []byte fset *token.FileSet file *ast.File @@ -403,11 +403,11 @@ type file struct { unusedOptouts []int } -func (cmd *mainCmd) parseFile(filename string, src []byte) (file, error) { +func (cmd *mainCmd) parseFile(filename string, src []byte) (parsedFile, error) { fset := token.NewFileSet() f, err := parser.ParseFile(fset, filename, src, parser.ParseComments) if err != nil { - return file{}, errtrace.Wrap(err) + return parsedFile{}, errtrace.Wrap(err) } errtracePkg := "errtrace" // name to use for errtrace package @@ -529,7 +529,7 @@ func (cmd *mainCmd) parseFile(filename string, src []byte) (file, error) { return inserts[i].Pos() < inserts[j].Pos() }) - return file{ + return parsedFile{ src: src, fset: fset, file: f, @@ -540,7 +540,7 @@ func (cmd *mainCmd) parseFile(filename string, src []byte) (file, error) { }, nil } -func (cmd *mainCmd) rewriteFile(f file, out *bytes.Buffer) error { +func (cmd *mainCmd) rewriteFile(f parsedFile, out *bytes.Buffer) error { var lastOffset int filePos := f.fset.File(f.file.Pos()) // position information for this file for _, it := range f.inserts { diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go index fa9ea73..e97f408 100644 --- a/cmd/errtrace/toolexec.go +++ b/cmd/errtrace/toolexec.go @@ -81,7 +81,7 @@ func (cmd *mainCmd) toolExecRewrite(pkg string, args []string) (exitCode int) { } func (cmd *mainCmd) rewriteCompile(pkg string, args []string) (exitCode int, _ error) { - parsed := make(map[string]file) + parsed := make(map[string]parsedFile) var canRewrite, needRewrite bool for _, arg := range args { if !isGoFile(arg) { From a6f6876d7363b37bef3787a60a6009d193976d37 Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 21:53:10 -0800 Subject: [PATCH 07/14] Add comment to `file.unusedOptouts` --- cmd/errtrace/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/errtrace/main.go b/cmd/errtrace/main.go index 93ab526..9f778f5 100644 --- a/cmd/errtrace/main.go +++ b/cmd/errtrace/main.go @@ -400,7 +400,7 @@ type parsedFile struct { errtracePkg string importsErrtrace bool inserts []insert - unusedOptouts []int + unusedOptouts []int // list of line numbers } func (cmd *mainCmd) parseFile(filename string, src []byte) (parsedFile, error) { From 2f78b3aa431d2f89f6b6eab6ad3f17b33480b4ae Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 21:54:17 -0800 Subject: [PATCH 08/14] Drop `tool.Env = os.Environ()` -- same as default --- cmd/errtrace/toolexec.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go index e97f408..c0971b8 100644 --- a/cmd/errtrace/toolexec.go +++ b/cmd/errtrace/toolexec.go @@ -43,7 +43,6 @@ func (cmd *mainCmd) toolExecVersion(args []string) int { var stdout bytes.Buffer tool.Stdout = &stdout tool.Stderr = cmd.Stderr - tool.Env = os.Environ() if err := tool.Run(); err != nil { if exitErr, ok := err.(*exec.ExitError); ok { return exitErr.ExitCode() @@ -166,7 +165,6 @@ func (cmd *mainCmd) runOriginal(args []string) (exitCode int) { tool.Stdin = cmd.Stdin tool.Stdout = cmd.Stdout tool.Stderr = cmd.Stderr - tool.Env = os.Environ() if err := tool.Run(); err != nil { if exitErr, ok := err.(*exec.ExitError); ok { From d243b3821a234df94052ba6efed3d63232431c5c Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 21:58:29 -0800 Subject: [PATCH 09/14] Update stale comment, no more errtrace.ToolExecInstrument --- cmd/errtrace/toolexec.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go index c0971b8..a75b173 100644 --- a/cmd/errtrace/toolexec.go +++ b/cmd/errtrace/toolexec.go @@ -63,9 +63,9 @@ func (cmd *mainCmd) toolExecRewrite(pkg string, args []string) (exitCode int) { return cmd.runOriginal(args) } - // We only modify files that import errtrace, and have "errtrace.ToolExecInstrument" - // However, that requires parsing files to determine, which we want to avoid for stdlib - // so use a heuristic to detect stdlib packages -- whether the name contains "."". + // We only modify files that import errtrace, so stdlib is never eliglble. + // To avoid unnecessary parsing, use a heuristic to detect stdlib packages -- + // whether the name contains ".". if !strings.Contains(pkg, ".") { return cmd.runOriginal(args) } From 926320b0f7970ccbd1b4c93bec36f999b2ed4243 Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 22:02:25 -0800 Subject: [PATCH 10/14] Remove `rewriteCompile` temp dir after execution --- cmd/errtrace/toolexec.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go index a75b173..30f417a 100644 --- a/cmd/errtrace/toolexec.go +++ b/cmd/errtrace/toolexec.go @@ -116,6 +116,7 @@ func (cmd *mainCmd) rewriteCompile(pkg string, args []string) (exitCode int, _ e if err != nil { return -1, errtrace.Wrap(err) } + defer os.RemoveAll(tempDir) //nolint:errcheck // best-effort removal of temp files. newArgs := make([]string, 0, len(args)) for _, arg := range args { From ee68e2954f34027412e8ff49bbfcfa6b5c769b4f Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 22:13:16 -0800 Subject: [PATCH 11/14] No need to recheck `isGoFile` before rewriting --- cmd/errtrace/toolexec.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go index 30f417a..26cdec0 100644 --- a/cmd/errtrace/toolexec.go +++ b/cmd/errtrace/toolexec.go @@ -120,11 +120,6 @@ func (cmd *mainCmd) rewriteCompile(pkg string, args []string) (exitCode int, _ e newArgs := make([]string, 0, len(args)) for _, arg := range args { - if !isGoFile(arg) { - newArgs = append(newArgs, arg) - continue - } - f, ok := parsed[arg] if !ok || len(f.inserts) == 0 { newArgs = append(newArgs, arg) From 7d5a41f3b7d2d1de62608853d0d1ea9d8383c268 Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 22:16:51 -0800 Subject: [PATCH 12/14] Add TODO for non-unique base names for .go files (thanks bazel) --- cmd/errtrace/toolexec.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go index 26cdec0..9595562 100644 --- a/cmd/errtrace/toolexec.go +++ b/cmd/errtrace/toolexec.go @@ -134,6 +134,7 @@ func (cmd *mainCmd) rewriteCompile(pkg string, args []string) (exitCode int, _ e return -1, errtrace.Wrap(err) } + // TODO: Handle clashes with the same base name in different directories (E.g., with bazel). newFile := filepath.Join(tempDir, filepath.Base(arg)) if err := os.WriteFile(newFile, out.Bytes(), 0o666); err != nil { return -1, errtrace.Wrap(err) From dc084a01a813c2a3e57d4f6ce47ebdc4fa0d8b91 Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 22:18:07 -0800 Subject: [PATCH 13/14] Consistent tool failure messages with tool name --- cmd/errtrace/toolexec.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/errtrace/toolexec.go b/cmd/errtrace/toolexec.go index 9595562..3197f8e 100644 --- a/cmd/errtrace/toolexec.go +++ b/cmd/errtrace/toolexec.go @@ -48,7 +48,7 @@ func (cmd *mainCmd) toolExecVersion(args []string) int { return exitErr.ExitCode() } - fmt.Fprintf(cmd.Stderr, "%v failed: %v", args[0], err) + fmt.Fprintf(cmd.Stderr, "tool %v failed: %v", args[0], err) return 1 } @@ -167,7 +167,7 @@ func (cmd *mainCmd) runOriginal(args []string) (exitCode int) { if exitErr, ok := err.(*exec.ExitError); ok { return exitErr.ExitCode() } - fmt.Fprintf(cmd.Stderr, "tool failed: %v", err) + fmt.Fprintf(cmd.Stderr, "tool %v failed: %v", args[0], err) return 1 } From d2760afce06e4f441ac97b7b8c97cf9c9f510fa4 Mon Sep 17 00:00:00 2001 From: Prashant V Date: Sat, 17 Feb 2024 22:22:26 -0800 Subject: [PATCH 14/14] Update toolexec-test * main: toolexec rewrite expected * p1: no import, no rewrite * p2: already wrapped * p3: toolexec rewrite expected --- cmd/errtrace/testdata/toolexec-test/p1/p1.go | 2 +- cmd/errtrace/testdata/toolexec-test/p2/p2.go | 10 +++++----- .../testdata/toolexec-test/{p2 => p3}/errtrace.go | 2 +- cmd/errtrace/testdata/toolexec-test/p3/p3.go | 10 ++++++++++ 4 files changed, 17 insertions(+), 7 deletions(-) rename cmd/errtrace/testdata/toolexec-test/{p2 => p3}/errtrace.go (87%) create mode 100644 cmd/errtrace/testdata/toolexec-test/p3/p3.go diff --git a/cmd/errtrace/testdata/toolexec-test/p1/p1.go b/cmd/errtrace/testdata/toolexec-test/p1/p1.go index 100cf01..217045c 100644 --- a/cmd/errtrace/testdata/toolexec-test/p1/p1.go +++ b/cmd/errtrace/testdata/toolexec-test/p1/p1.go @@ -8,5 +8,5 @@ import ( // WrapP2 wraps an error return from p2. func WrapP2() error { - return fmt.Errorf("test2: %w", p2.ReturnErr()) + return fmt.Errorf("test2: %w", p2.CallP3()) } diff --git a/cmd/errtrace/testdata/toolexec-test/p2/p2.go b/cmd/errtrace/testdata/toolexec-test/p2/p2.go index b976078..32e99ee 100644 --- a/cmd/errtrace/testdata/toolexec-test/p2/p2.go +++ b/cmd/errtrace/testdata/toolexec-test/p2/p2.go @@ -1,12 +1,12 @@ package p2 import ( - "errors" - "braces.dev/errtrace" + + "braces.dev/errtrace/cmd/errtrace/testdata/toolexec-test/p3" ) -// ReturnErr returns an error. -func ReturnErr() error { - return errtrace.Wrap(errors.New("test")) // @trace +// CallP3 calls p3, and wraps the error. +func CallP3() error { + return errtrace.Wrap(p3.ReturnErr()) // @trace } diff --git a/cmd/errtrace/testdata/toolexec-test/p2/errtrace.go b/cmd/errtrace/testdata/toolexec-test/p3/errtrace.go similarity index 87% rename from cmd/errtrace/testdata/toolexec-test/p2/errtrace.go rename to cmd/errtrace/testdata/toolexec-test/p3/errtrace.go index d5e4057..4366786 100644 --- a/cmd/errtrace/testdata/toolexec-test/p2/errtrace.go +++ b/cmd/errtrace/testdata/toolexec-test/p3/errtrace.go @@ -1,4 +1,4 @@ -package p2 +package p3 // Opt-in to errtrace wrapping with toolexec. import _ "braces.dev/errtrace" diff --git a/cmd/errtrace/testdata/toolexec-test/p3/p3.go b/cmd/errtrace/testdata/toolexec-test/p3/p3.go new file mode 100644 index 0000000..b60a12f --- /dev/null +++ b/cmd/errtrace/testdata/toolexec-test/p3/p3.go @@ -0,0 +1,10 @@ +package p3 + +import ( + "errors" +) + +// ReturnErr returns an error. +func ReturnErr() error { + return errors.New("test") // @trace +}