From 527f65a4687161ebce4bde8474b8990a10770785 Mon Sep 17 00:00:00 2001 From: Craig Furman <craig.furman89@gmail.com> Date: Wed, 17 Nov 2021 18:24:50 +0000 Subject: [PATCH 1/3] Support custom forbid reason messages 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. --- README.md | 1 + forbidigo/forbidigo.go | 26 ++++++++++++------- forbidigo/forbidigo_test.go | 10 +++++++ forbidigo/patterns.go | 41 +++++++++++++++++++++++++++++ forbidigo/patterns_test.go | 52 +++++++++++++++++++++++++++++++++++++ 5 files changed, 120 insertions(+), 10 deletions(-) create mode 100644 forbidigo/patterns.go create mode 100644 forbidigo/patterns_test.go diff --git a/README.md b/README.md index 793924b..baadf53 100644 --- a/README.md +++ b/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. diff --git a/forbidigo/forbidigo.go b/forbidigo/forbidigo.go index 2337404..d062ce4 100644 --- a/forbidigo/forbidigo.go +++ b/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 "%s"`, 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, }) } } diff --git a/forbidigo/forbidigo_test.go b/forbidigo/forbidigo_test.go index 9b370ab..d8b0f8d 100644 --- a/forbidigo/forbidigo_test.go +++ b/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, ` diff --git a/forbidigo/patterns.go b/forbidigo/patterns.go new file mode 100644 index 0000000..c13c36b --- /dev/null +++ b/forbidigo/patterns.go @@ -0,0 +1,41 @@ +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 { + return extractComment(sub) + } + subStr := sub.String() + if strings.HasPrefix(subStr, "#") { + return strings.TrimSpace(strings.TrimPrefix(subStr, "#")) + } + } + return "" +} diff --git a/forbidigo/patterns_test.go b/forbidigo/patterns_test.go new file mode 100644 index 0000000..6fc82a2 --- /dev/null +++ b/forbidigo/patterns_test.go @@ -0,0 +1,52 @@ +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: "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) +} From 23a130587acc4c1f8bdc64c28803257514481fe0 Mon Sep 17 00:00:00 2001 From: Craig Furman <craig.furman89@gmail.com> Date: Tue, 14 Dec 2021 18:20:15 +0000 Subject: [PATCH 2/3] Support comments with multiple subexpressions --- forbidigo/patterns.go | 4 +++- forbidigo/patterns_test.go | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/forbidigo/patterns.go b/forbidigo/patterns.go index c13c36b..c236488 100644 --- a/forbidigo/patterns.go +++ b/forbidigo/patterns.go @@ -30,7 +30,9 @@ func parse(ptrn string) (*pattern, error) { func extractComment(re *syntax.Regexp) string { for _, sub := range re.Sub { if len(sub.Sub) > 0 { - return extractComment(sub) + if comment := extractComment(sub); comment != "" { + return comment + } } subStr := sub.String() if strings.HasPrefix(subStr, "#") { diff --git a/forbidigo/patterns_test.go b/forbidigo/patterns_test.go index 6fc82a2..ce4e617 100644 --- a/forbidigo/patterns_test.go +++ b/forbidigo/patterns_test.go @@ -21,6 +21,11 @@ func TestParseValidPatterns(t *testing.T) { 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!)?`, From 2e7a2ba6ec33677d2f65bd1c835ce77c9583d818 Mon Sep 17 00:00:00 2001 From: Andrew Shannon Brown <ashanbrown@users.noreply.github.com> Date: Mon, 20 Dec 2021 07:10:26 -0800 Subject: [PATCH 3/3] Update forbidigo/forbidigo.go --- forbidigo/forbidigo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forbidigo/forbidigo.go b/forbidigo/forbidigo.go index d062ce4..17740fa 100644 --- a/forbidigo/forbidigo.go +++ b/forbidigo/forbidigo.go @@ -28,7 +28,7 @@ type UsedIssue struct { } func (a UsedIssue) Details() string { - explanation := fmt.Sprintf(` because "%s"`, a.customMsg) + explanation := fmt.Sprintf(` because %q`, a.customMsg) if a.customMsg == "" { explanation = fmt.Sprintf(" by pattern `%s`", a.pattern) }