Support custom forbid reason messages (#11)

In order to allow users to communicate intent to collaborators,
optionally embed custom messages into each forbidden pattern.

The syntax is as follows: `identifier(# message goes here)?`

Example: `^fmt\.Errorf(# Please don't use this!)?$`

Regular expressions containing custom messages are effectively identical
to ones that don't, because the sub-expression containing it is marked
optional with a `?`. All this commit does is parse out any recognized
custom message, and place it prominently in the tool's output. The
regular expression itself is omitted from the tool's output when a
custom message is specified.

Author: craigfurman

README.md

@@ -22,6 +22,7 @@ A larger set of interesting patterns might include:
 * `^fmt\.Errorf$` -- forbid Errorf in favor of using github.com/pkg/errors
 * `^ginkgo\.F[A-Z].*$` -- forbid ginkgo focused commands (used for debug issues)
 * `^spew\.Dump$` -- forbid dumping detailed data to stdout
+* `^fmt\.Errorf(# please use github.com/pkg/errors)?$` -- forbid Errorf, with a custom message
 
 Note that the linter has no knowledge of what packages were actually imported, so aliased imports will match these patterns.
 

forbidigo/forbidigo.go

@@ -24,10 +24,15 @@ type UsedIssue struct {
 	identifier string
 	pattern    string
 	position   token.Position
+	customMsg  string
 }
 
 func (a UsedIssue) Details() string {
-	return fmt.Sprintf("use of `%s` forbidden by pattern `%s`", a.identifier, a.pattern)
+	explanation := fmt.Sprintf(` because %q`, a.customMsg)
+	if a.customMsg == "" {
+		explanation = fmt.Sprintf(" by pattern `%s`", a.pattern)
+	}
+	return fmt.Sprintf("use of `%s` forbidden", a.identifier) + explanation
 }
 
 func (a UsedIssue) Position() token.Position {
@@ -36,13 +41,13 @@ func (a UsedIssue) Position() token.Position {
 
 func (a UsedIssue) String() string { return toString(a) }
 
-func toString(i Issue) string {
+func toString(i UsedIssue) string {
 	return fmt.Sprintf("%s at %s", i.Details(), i.Position())
 }
 
 type Linter struct {
 	cfg      config
-	patterns []*regexp.Regexp
+	patterns []*pattern
 }
 
 func DefaultPatterns() []string {
@@ -65,13 +70,13 @@ func NewLinter(patterns []string, options ...Option) (*Linter, error) {
 	if len(patterns) == 0 {
 		patterns = DefaultPatterns()
 	}
-	compiledPatterns := make([]*regexp.Regexp, 0, len(patterns))
-	for _, p := range patterns {
-		re, err := regexp.Compile(p)
+	compiledPatterns := make([]*pattern, 0, len(patterns))
+	for _, ptrn := range patterns {
+		p, err := parse(ptrn)
 		if err != nil {
-			return nil, fmt.Errorf("unable to compile pattern `%s`: %s", p, err)
+			return nil, err
 		}
-		compiledPatterns = append(compiledPatterns, re)
+		compiledPatterns = append(compiledPatterns, p)
 	}
 	return &Linter{
 		cfg:      cfg,
@@ -158,11 +163,12 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor {
 		return v
 	}
 	for _, p := range v.linter.patterns {
-		if p.MatchString(v.textFor(node)) && !v.permit(node) {
+		if p.pattern.MatchString(v.textFor(node)) && !v.permit(node) {
 			v.issues = append(v.issues, UsedIssue{
 				identifier: v.textFor(node),
-				pattern:    p.String(),
+				pattern:    p.pattern.String(),
 				position:   v.fset.Position(node.Pos()),
+				customMsg:  p.msg,
 			})
 		}
 	}

forbidigo/forbidigo_test.go

@@ -19,6 +19,16 @@ func foo() {
 }`, "use of `fmt.Printf` forbidden by pattern `fmt\\.Printf` at testing.go:5:2")
 	})
 
+	t.Run("displays custom messages", func(t *testing.T) {
+		linter, _ := NewLinter([]string{`^fmt\.Printf(# a custom message)?$`})
+		expectIssues(t, linter, `
+package bar
+
+func foo() {
+	fmt.Printf("here i am")
+}`, "use of `fmt.Printf` forbidden because \"a custom message\" at testing.go:5:2")
+	})
+
 	t.Run("it doesn't require a package on the identifier", func(t *testing.T) {
 		linter, _ := NewLinter([]string{`Printf`})
 		expectIssues(t, linter, `

forbidigo/patterns.go

@@ -0,0 +1,43 @@
+package forbidigo
+
+import (
+	"fmt"
+	"regexp"
+	"regexp/syntax"
+	"strings"
+)
+
+type pattern struct {
+	pattern *regexp.Regexp
+	msg     string
+}
+
+func parse(ptrn string) (*pattern, error) {
+	ptrnRe, err := regexp.Compile(ptrn)
+	if err != nil {
+		return nil, fmt.Errorf("unable to compile pattern `%s`: %s", ptrn, err)
+	}
+	re, err := syntax.Parse(ptrn, syntax.Perl)
+	if err != nil {
+		return nil, fmt.Errorf("unable to parse pattern `%s`: %s", ptrn, err)
+	}
+	msg := extractComment(re)
+	return &pattern{pattern: ptrnRe, msg: msg}, nil
+}
+
+// Traverse the leaf submatches in the regex tree and extract a comment, if any
+// is present.
+func extractComment(re *syntax.Regexp) string {
+	for _, sub := range re.Sub {
+		if len(sub.Sub) > 0 {
+			if comment := extractComment(sub); comment != "" {
+				return comment
+			}
+		}
+		subStr := sub.String()
+		if strings.HasPrefix(subStr, "#") {
+			return strings.TrimSpace(strings.TrimPrefix(subStr, "#"))
+		}
+	}
+	return ""
+}

forbidigo/patterns_test.go

@@ -0,0 +1,57 @@
+package forbidigo
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseValidPatterns(t *testing.T) {
+	for _, tc := range []struct {
+		name            string
+		ptrn            string
+		expectedComment string
+	}{
+		{
+			name: "simple expression, no comment",
+			ptrn: `fmt\.Errorf`,
+		},
+		{
+			name: "anchored expression, no comment",
+			ptrn: `^fmt\.Errorf$`,
+		},
+		{
+			name:            "contains multiple subexpression, with comment",
+			ptrn:            `(f)mt\.Errorf(# a comment)?`,
+			expectedComment: "a comment",
+		},
+		{
+			name:            "simple expression with comment",
+			ptrn:            `fmt\.Println(# Please don't use this!)?`,
+			expectedComment: "Please don't use this!",
+		},
+		{
+			name:            "deeply nested expression with comment",
+			ptrn:            `fmt\.Println((((# Please don't use this!))))?`,
+			expectedComment: "Please don't use this!",
+		},
+		{
+			name:            "anchored expression with comment",
+			ptrn:            `^fmt\.Println(# Please don't use this!)?$`,
+			expectedComment: "Please don't use this!",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			ptrn, err := parse(tc.ptrn)
+			require.Nil(t, err)
+			assert.Equal(t, tc.ptrn, ptrn.pattern.String())
+			assert.Equal(t, tc.expectedComment, ptrn.msg)
+		})
+	}
+}
+
+func TestParseInvalidPattern_ReturnsError(t *testing.T) {
+	_, err := parse(`fmt\`)
+	assert.NotNil(t, err)
+}