feat: simplify rules for receiver with type parameters (#39)

* feat: simplify rules for receiver with type parameters Fix #38 Signed-off-by: Timon Wong <[email protected]> * add tests Signed-off-by: Timon Wong <[email protected]> * improve test coverage Signed-off-by: Timon Wong <[email protected]> Signed-off-by: Timon Wong <[email protected]>
timonwong · Sep 1, 2022 · 7395ab8 · 7395ab8
1 parent d2fadc4
commit 7395ab8
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 19 deletions.
diff --git a/internal/rules/rules.go b/internal/rules/rules.go
@@ -43,7 +43,43 @@ func (rs *Ruleset) Match(fn *types.Func) bool {
 	return false
 }
 
-func emptyQualifier(*types.Package) string { return "" }
+func receiverTypeOf(recvType types.Type) string {
+	buf := bytebufferpool.Get()
+	defer bytebufferpool.Put(buf)
+
+	var recvNamed *types.Named
+	switch recvType := recvType.(type) {
+	case *types.Pointer:
+		buf.WriteByte('*')
+		if elem, ok := recvType.Elem().(*types.Named); ok {
+			recvNamed = elem
+		}
+	case *types.Named:
+		recvNamed = recvType
+	}
+
+	if recvNamed == nil {
+		// not supported type
+		return ""
+	}
+
+	buf.WriteString(recvNamed.Obj().Name())
+	typeParams := recvNamed.TypeParams()
+	if typeParamsLen := typeParams.Len(); typeParamsLen > 0 {
+		buf.WriteByte('[')
+		for i := 0; i < typeParamsLen; i++ {
+			if i > 0 {
+				// comma as separator
+				buf.WriteByte(',')
+			}
+			p := typeParams.At(i)
+			buf.WriteString(p.Obj().Name())
+		}
+		buf.WriteByte(']')
+	}
+
+	return buf.String()
+}
 
 func matchRule(p *FuncRule, sig *types.Signature) bool {
 	// we do not check package import here since it's already checked in Match()
@@ -55,10 +91,8 @@ func matchRule(p *FuncRule, sig *types.Signature) bool {
 
 	if isReceiver {
 		recvType := recv.Type()
-		recvTypeBuf := bytebufferpool.Get()
-		defer bytebufferpool.Put(recvTypeBuf)
-		types.WriteType(recvTypeBuf, recvType, emptyQualifier)
-		if recvTypeBuf.String() != p.ReceiverType {
+		receiverType := receiverTypeOf(recvType)
+		if receiverType != p.ReceiverType {
 			return false
 		}
 	}

diff --git a/internal/rules/rules_test.go b/internal/rules/rules_test.go
@@ -2,6 +2,7 @@ package rules
 
 import (
 	"errors"
+	"go/types"
 	"testing"
 	"testing/iotest"
 
@@ -84,3 +85,10 @@ func TestParseFuncRule(t *testing.T) {
 		})
 	}
 }
+
+func TestReceiverTypeOf_InvalidType(t *testing.T) {
+	t.Parallel()
+
+	basicType := types.Universe.Lookup("byte").Type()
+	assert.Equal(t, "", receiverTypeOf(basicType))
+}
diff --git a/loggercheck_test.go b/loggercheck_test.go
@@ -1,7 +1,6 @@
 package loggercheck_test
 
 import (
-	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -25,7 +24,7 @@ func TestLinter(t *testing.T) {
 		name      string
 		patterns  string
 		flags     []string
-		wantError error
+		wantError string
 	}{
 		{
 			name:     "all",
@@ -71,7 +70,16 @@ func TestLinter(t *testing.T) {
 				"-rulefile",
 				"testdata/wrong-rules.txt",
 			},
-			wantError: rules.ErrInvalidRule,
+			wantError: rules.ErrInvalidRule.Error(),
+		},
+		{
+			name:     "not-found-rules",
+			patterns: "a/customonly",
+			flags: []string{
+				"-rulefile",
+				"testdata/xxxxx-wrong-rules-xxxxx.txt",
+			},
+			wantError: "failed to open rule file",
 		},
 	}
 
@@ -83,16 +91,16 @@ func TestLinter(t *testing.T) {
 			require.NoError(t, err)
 
 			var result []*analysistest.Result
-			if tc.wantError != nil {
+			if tc.wantError != "" {
 				result = analysistest.Run(&dummyTestingErrorf{t}, testdata, a, tc.patterns)
 			} else {
 				result = analysistest.Run(t, testdata, a, tc.patterns)
 			}
 			require.Len(t, result, 1)
 
-			if tc.wantError != nil {
+			if tc.wantError != "" {
 				assert.Error(t, result[0].Err)
-				assert.True(t, errors.Is(result[0].Err, tc.wantError))
+				assert.ErrorContains(t, result[0].Err, tc.wantError)
 			}
 		})
 	}
@@ -117,11 +125,25 @@ func TestOptions(t *testing.T) {
 		"a/customonly.With",
 	}
 
+	wrongCustomRules := []string{
+		"# Wrong rule file",
+		"(*a/wrong.Method.Rule",
+	}
+
 	testCases := []struct {
-		name     string
-		options  []loggercheck.Option
-		patterns string
+		name      string
+		options   []loggercheck.Option
+		patterns  string
+		wantError string
 	}{
+		{
+			name: "wrong-rules",
+			options: []loggercheck.Option{
+				loggercheck.WithRules(wrongCustomRules),
+			},
+			patterns:  "a/customonly",
+			wantError: "failed to parse rules: ",
+		},
 		{
 			name: "disable-all-then-enable-mylogger",
 			options: []loggercheck.Option{
@@ -158,7 +180,19 @@ func TestOptions(t *testing.T) {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			a := loggercheck.NewAnalyzer(tc.options...)
-			analysistest.Run(t, testdata, a, tc.patterns)
+
+			var result []*analysistest.Result
+			if tc.wantError != "" {
+				result = analysistest.Run(&dummyTestingErrorf{t}, testdata, a, tc.patterns)
+			} else {
+				result = analysistest.Run(t, testdata, a, tc.patterns)
+			}
+			require.Len(t, result, 1)
+
+			if tc.wantError != "" {
+				assert.Error(t, result[0].Err)
+				assert.ErrorContains(t, result[0].Err, tc.wantError)
+			}
 		})
 	}
 }
diff --git a/testdata/custom-rules-generic.txt b/testdata/custom-rules-generic.txt
@@ -1,2 +1,3 @@
 # Generic methods
-(*a/custom-generic.Logger[any]).Infow
+(*a/custom-generic.Logger[T,W]).Infow
+(*a/custom-generic.Logger[T,W]).Infox
diff --git a/testdata/src/a/custom-generic/example.go b/testdata/src/a/custom-generic/example.go
@@ -2,18 +2,38 @@ package custom_generic
 
 import "go.uber.org/zap"
 
-type Logger[T any] struct {
+type Signed interface {
+	~int | ~int8 | ~int16 | ~int32 | ~int64
+}
+
+type Unsigned interface {
+	~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 | ~uintptr
+}
+
+type Integer interface {
+	Signed | Unsigned
+}
+type Logger[T, W Integer] struct {
 	*zap.SugaredLogger
 }
 
-func (l *Logger[T]) Infow(message string, keysAndValues ...any) {
+func (l *Logger[T, W]) Infow(message string, keysAndValues ...any) {
 	l.SugaredLogger.Infow(message, keysAndValues...)
 }
 
+func (l *Logger[T, W]) Infox(message string, keysAndValues ...string) {
+	kvs := make([]any, 0, len(keysAndValues))
+	for _, v := range keysAndValues {
+		kvs = append(kvs, v)
+	}
+	l.SugaredLogger.Infow(message, kvs...)
+}
+
 func ExampleGenericCustomOnly() {
 	l := zap.NewExample().Sugar()
-	logger := &Logger[any]{l}
+	logger := &Logger[int8, int]{l}
 
+	logger.Infox("will not check this", "hello", "world")
 	logger.Infow("custom message", "hello")                                     // want `odd number of arguments passed as key-value pairs for logging`
 	logger.Debugw("embedded sugar log also works without custom rules", "key1") // want `odd number of arguments passed as key-value pairs for logging`
 }