From dc30eb118203f247e50e69a88f6868df00104b72 Mon Sep 17 00:00:00 2001 From: Ivan Trubach Date: Sat, 18 Jun 2022 19:47:53 +0300 Subject: [PATCH] fix(receiver-naming): distinguish types with parameters (#692) * fix(receiver-naming): distinguish types with parameters * chore: run tests using supported Go versions matrix --- .../workflows/{build-test.yaml => build.yaml} | 10 +---- .github/workflows/lint.yaml | 1 + .github/workflows/test.yaml | 28 ++++++++++++++ internal/typeparams/typeparams.go | 29 +++++++++++++++ internal/typeparams/typeparams_go117.go | 12 ++++++ internal/typeparams/typeparams_go118.go | 20 ++++++++++ lint/package.go | 19 ++-------- rule/exported.go | 3 +- rule/receiver-naming.go | 3 +- rule/unexported-return.go | 3 +- rule/utils.go | 13 ------- test/receiver-naming_test.go | 15 ++++++++ testdata/receiver-naming-issue-669.go | 37 +++++++++++++++++++ 13 files changed, 153 insertions(+), 40 deletions(-) rename .github/workflows/{build-test.yaml => build.yaml} (58%) create mode 100644 .github/workflows/test.yaml create mode 100644 internal/typeparams/typeparams.go create mode 100644 internal/typeparams/typeparams_go117.go create mode 100644 internal/typeparams/typeparams_go118.go create mode 100644 test/receiver-naming_test.go create mode 100644 testdata/receiver-naming-issue-669.go diff --git a/.github/workflows/build-test.yaml b/.github/workflows/build.yaml similarity index 58% rename from .github/workflows/build-test.yaml rename to .github/workflows/build.yaml index b6ca02a..751d8ee 100644 --- a/.github/workflows/build-test.yaml +++ b/.github/workflows/build.yaml @@ -1,5 +1,6 @@ -name: Build and Test +name: Build on: + workflow_dispatch: pull_request: types: [opened, edited, synchronize, reopened] @@ -11,10 +12,3 @@ jobs: - uses: actions/checkout@v2 - name: build run: make build - test: - name: Test - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - - name: test - run: make test diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index 7dc3def..b6b6d0b 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -1,5 +1,6 @@ name: Lint on: + workflow_dispatch: pull_request: types: [opened, edited, synchronize, reopened] diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml new file mode 100644 index 0000000..bea1ab1 --- /dev/null +++ b/.github/workflows/test.yaml @@ -0,0 +1,28 @@ +name: Test +on: + workflow_dispatch: + pull_request: + types: [opened, edited, synchronize, reopened] + +jobs: + test: + name: Test + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + go-version: + - 1.16.x + - 1.17.x + - 1.18.x + steps: + - name: Checkout code + uses: actions/checkout@v3.0.2 + - name: Set up Go + uses: actions/setup-go@v3.2.0 + with: + go-version: ${{ matrix.go-version }} + cache: true + cache-dependency-path: '**/go.sum' + - name: Run tests + run: go test -race ./... diff --git a/internal/typeparams/typeparams.go b/internal/typeparams/typeparams.go new file mode 100644 index 0000000..33c4f20 --- /dev/null +++ b/internal/typeparams/typeparams.go @@ -0,0 +1,29 @@ +// Package typeparams provides utilities for working with Go ASTs with support +// for type parameters when built with Go 1.18 and higher. +package typeparams + +import ( + "go/ast" +) + +// Enabled reports whether type parameters are enabled in the current build +// environment. +func Enabled() bool { + return enabled +} + +// ReceiverType returns the named type of the method receiver, sans "*" and type +// parameters, or "invalid-type" if fn.Recv is ill formed. +func ReceiverType(fn *ast.FuncDecl) string { + e := fn.Recv.List[0].Type + if s, ok := e.(*ast.StarExpr); ok { + e = s.X + } + if enabled { + e = unpackIndexExpr(e) + } + if id, ok := e.(*ast.Ident); ok { + return id.Name + } + return "invalid-type" +} diff --git a/internal/typeparams/typeparams_go117.go b/internal/typeparams/typeparams_go117.go new file mode 100644 index 0000000..913a731 --- /dev/null +++ b/internal/typeparams/typeparams_go117.go @@ -0,0 +1,12 @@ +//go:build !go1.18 +// +build !go1.18 + +package typeparams + +import ( + "go/ast" +) + +const enabled = false + +func unpackIndexExpr(e ast.Expr) ast.Expr { return e } diff --git a/internal/typeparams/typeparams_go118.go b/internal/typeparams/typeparams_go118.go new file mode 100644 index 0000000..0f7fd88 --- /dev/null +++ b/internal/typeparams/typeparams_go118.go @@ -0,0 +1,20 @@ +//go:build go1.18 +// +build go1.18 + +package typeparams + +import ( + "go/ast" +) + +const enabled = true + +func unpackIndexExpr(e ast.Expr) ast.Expr { + switch e := e.(type) { + case *ast.IndexExpr: + return e.X + case *ast.IndexListExpr: + return e.X + } + return e +} diff --git a/lint/package.go b/lint/package.go index 7cfcab3..52505f3 100644 --- a/lint/package.go +++ b/lint/package.go @@ -7,6 +7,8 @@ import ( "sync" "golang.org/x/tools/go/gcexportdata" + + "github.com/mgechev/revive/internal/typeparams" ) // Package represents a package in the project. @@ -146,7 +148,7 @@ func (w *walker) Visit(n ast.Node) ast.Visitor { return w } // TODO(dsymonds): We could check the signature to be more precise. - recv := receiverType(fn) + recv := typeparams.ReceiverType(fn) if i, ok := w.nmap[fn.Name.Name]; ok { w.has[recv] |= i } @@ -174,21 +176,6 @@ func (p *Package) scanSortable() { } } -// receiverType returns the named type of the method receiver, sans "*", -// or "invalid-type" if fn.Recv is ill formed. -func receiverType(fn *ast.FuncDecl) string { - switch e := fn.Recv.List[0].Type.(type) { - case *ast.Ident: - return e.Name - case *ast.StarExpr: - if id, ok := e.X.(*ast.Ident); ok { - return id.Name - } - } - // The parser accepts much more than just the legal forms. - return "invalid-type" -} - func (p *Package) lint(rules []Rule, config Config, failures chan Failure) { p.scanSortable() var wg sync.WaitGroup diff --git a/rule/exported.go b/rule/exported.go index ed24396..cf2915d 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -9,6 +9,7 @@ import ( "unicode" "unicode/utf8" + "github.com/mgechev/revive/internal/typeparams" "github.com/mgechev/revive/lint" ) @@ -116,7 +117,7 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) { if fn.Recv != nil && len(fn.Recv.List) > 0 { // method kind = "method" - recv := receiverType(fn) + recv := typeparams.ReceiverType(fn) if !w.checkPrivateReceivers && !ast.IsExported(recv) { // receiver is unexported return diff --git a/rule/receiver-naming.go b/rule/receiver-naming.go index fee5777..d79bb9f 100644 --- a/rule/receiver-naming.go +++ b/rule/receiver-naming.go @@ -4,6 +4,7 @@ import ( "fmt" "go/ast" + "github.com/mgechev/revive/internal/typeparams" "github.com/mgechev/revive/lint" ) @@ -65,7 +66,7 @@ func (w lintReceiverName) Visit(n ast.Node) ast.Visitor { }) return w } - recv := receiverType(fn) + recv := typeparams.ReceiverType(fn) if prev, ok := w.typeReceiver[recv]; ok && prev != name { w.onFailure(lint.Failure{ Node: n, diff --git a/rule/unexported-return.go b/rule/unexported-return.go index a4b9422..10f8e3f 100644 --- a/rule/unexported-return.go +++ b/rule/unexported-return.go @@ -5,6 +5,7 @@ import ( "go/ast" "go/types" + "github.com/mgechev/revive/internal/typeparams" "github.com/mgechev/revive/lint" ) @@ -55,7 +56,7 @@ func (w lintUnexportedReturn) Visit(n ast.Node) ast.Visitor { thing := "func" if fn.Recv != nil && len(fn.Recv.List) > 0 { thing = "method" - if !ast.IsExported(receiverType(fn)) { + if !ast.IsExported(typeparams.ReceiverType(fn)) { // Don't report exported methods of unexported types, // such as private implementations of sort.Interface. return nil diff --git a/rule/utils.go b/rule/utils.go index 689988e..dca1674 100644 --- a/rule/utils.go +++ b/rule/utils.go @@ -26,19 +26,6 @@ var commonMethods = map[string]bool{ "Unwrap": true, } -func receiverType(fn *ast.FuncDecl) string { - switch e := fn.Recv.List[0].Type.(type) { - case *ast.Ident: - return e.Name - case *ast.StarExpr: - if id, ok := e.X.(*ast.Ident); ok { - return id.Name - } - } - // The parser accepts much more than just the legal forms. - return "invalid-type" -} - var knownNameExceptions = map[string]bool{ "LastInsertId": true, // must match database/sql "kWh": true, diff --git a/test/receiver-naming_test.go b/test/receiver-naming_test.go new file mode 100644 index 0000000..1d530cb --- /dev/null +++ b/test/receiver-naming_test.go @@ -0,0 +1,15 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/internal/typeparams" + "github.com/mgechev/revive/rule" +) + +func TestReceiverNamingTypeParams(t *testing.T) { + if !typeparams.Enabled() { + t.Skip("type parameters are not enabled in the current build environment") + } + testRule(t, "receiver-naming-issue-669", &rule.ReceiverNamingRule{}) +} diff --git a/testdata/receiver-naming-issue-669.go b/testdata/receiver-naming-issue-669.go new file mode 100644 index 0000000..58c5301 --- /dev/null +++ b/testdata/receiver-naming-issue-669.go @@ -0,0 +1,37 @@ +package fixtures + +type gen1[T any] struct{} + +func (g gen1[T]) f1() {} + +func (g gen1[U]) f2() {} + +func (n gen1[T]) f3() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen1/ + +func (n gen1[U]) f4() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen1/ + +func (n gen1[V]) f5() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen1/ + +func (n *gen1[T]) f6() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen1/ + +func (n *gen1[U]) f7() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen1/ + +func (n *gen1[V]) f8() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen1/ + +type gen2[T1, T2 any] struct{} + +func (g gen2[T1, T2]) f1() {} + +func (g gen2[U1, U2]) f2() {} + +func (n gen2[T1, T2]) f3() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen2/ + +func (n gen2[U1, U2]) f4() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen2/ + +func (n gen2[V1, V2]) f5() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen2/ + +func (n *gen2[T1, T2]) f6() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen2/ + +func (n *gen2[U1, U2]) f7() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen2/ + +func (n *gen2[V1, V2]) f8() {} // MATCH /receiver name n should be consistent with previous receiver name g for gen2/