cmd: Warn on unused //errtrace:skip (#62)

If we find `//errtrace:skip` comments on lines
that wouldn't be instrumented, print warnings about them.

Resolves #61

---------

Co-authored-by: Prashant V <prashant@prashantv.com>

Author: abhinav

cmd/errtrace/main.go

@@ -266,6 +266,21 @@ func (cmd *mainCmd) processFile(r fileRequest) error {
 	}
 	ast.Walk(&w, f)
 
+	// Look for unused optouts and warn about them.
+	if len(w.optouts) > 0 {
+		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)
@@ -420,10 +435,11 @@ type walker struct {
 	// Inputs
 
 	fset     *token.FileSet // file set for positional information
-	optouts  map[int]struct{}
-	errtrace string // name of the errtrace package
+	errtrace string         // name of the errtrace package
 	logger   *log.Logger
 
+	optouts map[int]int // map from line to number of uses
+
 	// Outputs
 
 	// inserts is the list of inserts to make.
@@ -694,14 +710,15 @@ func (t *walker) wrapExpr(n int, expr ast.Expr) {
 	case t.isErrtraceWrap(expr):
 		return // already wrapped
 
-	case t.optout(expr.Pos()):
-		return
-
 	case isIdent(expr, "nil"):
 		// Optimization: ignore if it's "nil".
 		return
 	}
 
+	if t.optout(expr.Pos()) {
+		return
+	}
+
 	*t.inserts = append(*t.inserts,
 		&insertWrapOpen{N: n, Before: expr.Pos()},
 		&insertWrapClose{After: expr.End()},
@@ -731,10 +748,13 @@ func (t *walker) isErrtraceWrap(expr ast.Expr) bool {
 }
 
 // optout reports whether the line at the given position
-// is opted out of tracing.
+// is opted out of tracing, incrementing uses if so.
 func (t *walker) optout(pos token.Pos) bool {
 	line := t.fset.Position(pos).Line
 	_, ok := t.optouts[line]
+	if ok {
+		t.optouts[line]++
+	}
 	return ok
 }
 
@@ -857,13 +877,13 @@ func setOf[T comparable](xs []T) map[T]struct{} {
 func optoutLines(
 	fset *token.FileSet,
 	comments []*ast.CommentGroup,
-) map[int]struct{} {
-	lines := make(map[int]struct{})
+) map[int]int {
+	lines := make(map[int]int)
 	for _, cg := range comments {
 		for _, c := range cg.List {
 			if strings.Contains(c.Text, "//errtrace:skip") {
 				lineNo := fset.Position(c.Pos()).Line
-				lines[lineNo] = struct{}{}
+				lines[lineNo] = 0
 			}
 		}
 	}

cmd/errtrace/main_test.go

@@ -461,13 +461,12 @@ func extractLogs(src []byte) ([]logLine, error) {
 	var logs []logLine
 	for _, c := range f.Comments {
 		for _, l := range c.List {
-			if !strings.HasPrefix(l.Text, "// want:") {
+			_, lit, ok := strings.Cut(l.Text, "// want:")
+			if !ok {
 				continue
 			}
 
 			pos := fset.Position(l.Pos())
-			lit := strings.TrimPrefix(l.Text, "// want:")
-
 			s, err := strconv.Unquote(lit)
 			if err != nil {
 				return nil, fmt.Errorf("%s:bad string literal: %s", pos, lit)
@@ -499,11 +498,18 @@ func parseLogOutput(file, s string) ([]logLine, error) {
 		}
 
 		var msg string
-		switch len(parts) {
-		case 3:
-			msg = parts[2] // file:line:msg
-		case 4:
-			msg = parts[3] // file:line:column:msg
+		if len(parts) == 4 {
+			if _, err := strconv.Atoi(parts[2]); err == nil {
+				// file:line:column:msg
+				msg = parts[3]
+			}
+		}
+		if msg == "" && len(parts) >= 2 {
+			// file:line:msg
+			msg = strings.Join(parts[2:], ":")
+		}
+		if msg == "" {
+			return nil, fmt.Errorf("bad log line: %q", line)
 		}
 
 		lineNum, err := strconv.Atoi(parts[1])

cmd/errtrace/testdata/golden/noop.go

@@ -31,3 +31,7 @@ func immediatelyInvoked() error {
 		return errors.New("failure") //errtrace:skip
 	}()
 }
+
+func multipleReturns() (error, error) {
+	return errors.New("a"), errors.New("b") //errtrace:skip
+}

cmd/errtrace/testdata/golden/noop.go.golden

@@ -31,3 +31,7 @@ func immediatelyInvoked() error {
 		return errors.New("failure") //errtrace:skip
 	}()
 }
+
+func multipleReturns() (error, error) {
+	return errors.New("a"), errors.New("b") //errtrace:skip
+}

cmd/errtrace/testdata/golden/optout.go

@@ -23,3 +23,12 @@ func Try(problem bool) (int, error) {
 
 	return bar.Baz() //errtrace:skip // caller wants unwrapped error
 }
+
+func unused() error {
+	return nil //errtrace:skip // want:"unused errtrace:skip"
+}
+
+func multipleReturns() (a, b error) {
+	return errors.New("a"),
+		errors.New("b") //errtrace:skip
+}

cmd/errtrace/testdata/golden/optout.go.golden

@@ -23,3 +23,12 @@ func Try(problem bool) (int, error) {
 
 	return bar.Baz() //errtrace:skip // caller wants unwrapped error
 }
+
+func unused() error {
+	return nil //errtrace:skip // want:"unused errtrace:skip"
+}
+
+func multipleReturns() (a, b error) {
+	return errtrace.Wrap(errors.New("a")),
+		errors.New("b") //errtrace:skip
+}