cmd/errtrace: Tighten up opt-out matching (#64)

strings.Contains is too wide for this purpose.
It trips up even if a doc comment mentions '//errtrace:skip'.

Narrow it down to a much more selective criteria:

- must be a single line comment
- must be at start of the comment (`foo() //errtrace:skip`)
  or have a space immediately before it (`foo() //nolint:bar //errtrace:skip`)
- must reach end of the comment (`foo() //errtrace:skip`)
  or have a space (`foo() //errtrace:skip //nolint:bar`)
  or an opening parenthesis (`foo() //errtrace:skip(reason)`)
  immediately after it.

---------

Co-authored-by: Prashant V <[email protected]>

Author: abhinav

README.md

@@ -352,7 +352,8 @@ on relevant lines:
 
 ```go
 //errtrace:skip
-//errtrace:skip explanation
+//errtrace:skip(explanation)
+//errtrace:skip // explanation
 ```
 
 This can be especially useful if the returned error
@@ -367,7 +368,7 @@ type myReader struct{/* ... */}
 
 func (*myReader) Read(bs []byte) (int, error) {
   // ...
-  return 0, io.EOF //errtrace:skip (io.Reader requires io.EOF)
+  return 0, io.EOF //errtrace:skip(io.Reader expects io.EOF)
 }
 ```
 

cmd/errtrace/main.go

@@ -34,6 +34,7 @@ import (
 	"io"
 	"log"
 	"os"
+	"regexp"
 	"sort"
 	"strconv"
 	"strings"
@@ -866,6 +867,8 @@ func setOf[T comparable](xs []T) map[T]struct{} {
 	return set
 }
 
+var _errtraceSkip = regexp.MustCompile(`(^| )//errtrace:skip($|[ \(])`)
+
 // optoutLines returns the line numbers
 // that have a comment in the form:
 //
@@ -880,11 +883,15 @@ func optoutLines(
 ) 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] = 0
-			}
+		if len(cg.List) > 1 {
+			// skip multiline comments which are full line comments, not tied to a return.
+			continue
+		}
+
+		c := cg.List[0]
+		if _errtraceSkip.MatchString(c.Text) {
+			lineNo := fset.Position(c.Pos()).Line
+			lines[lineNo] = 0
 		}
 	}
 	return lines

cmd/errtrace/testdata/golden/optout.go

@@ -15,7 +15,7 @@ func Try(problem bool) (int, error) {
 			return errors.New("great sadness")
 		}
 
-		return io.EOF //errtrace:skip // expects io.EOF
+		return io.EOF //nolint:errwrap //errtrace:skip(expects io.EOF)
 	})
 	if err != nil {
 		return 0, err
@@ -32,3 +32,15 @@ func multipleReturns() (a, b error) {
 	return errors.New("a"),
 		errors.New("b") //errtrace:skip
 }
+
+func multipleReturnsSkipped() (a, b error) {
+	return errors.New("a"), //errtrace:skip
+		errors.New("b") //errtrace:skip
+}
+
+// Explanation of why this function
+// is not using //errtrace:skip should not
+// trip up the warning.
+func notUsingSkip() error {
+	return nil
+}

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

@@ -15,7 +15,7 @@ func Try(problem bool) (int, error) {
 			return errtrace.Wrap(errors.New("great sadness"))
 		}
 
-		return io.EOF //errtrace:skip // expects io.EOF
+		return io.EOF //nolint:errwrap //errtrace:skip(expects io.EOF)
 	})
 	if err != nil {
 		return 0, errtrace.Wrap(err)
@@ -32,3 +32,15 @@ func multipleReturns() (a, b error) {
 	return errtrace.Wrap(errors.New("a")),
 		errors.New("b") //errtrace:skip
 }
+
+func multipleReturnsSkipped() (a, b error) {
+	return errors.New("a"), //errtrace:skip
+		errors.New("b") //errtrace:skip
+}
+
+// Explanation of why this function
+// is not using //errtrace:skip should not
+// trip up the warning.
+func notUsingSkip() error {
+	return nil
+}