Skip to content

Commit

Permalink
fix: enforce-repeated-arg-type-style rule finds false positives in ca…
Browse files Browse the repository at this point in the history
…se of invalid types (#1046)

* fix: enforce-repeated-arg-type-style rule finds false positives in case of invalid types

* Fix oversight issue
  • Loading branch information
denisvmedia authored Sep 23, 2024
1 parent 53a111d commit 3249a5e
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 2 deletions.
3 changes: 3 additions & 0 deletions RULES_DESCRIPTIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,9 @@ declaration. The 'short' style encourages omitting repeated types for concisenes
whereas the 'full' style mandates explicitly stating the type for each argument
and return value, even if they are repeated, promoting clarity.

_IMPORTANT_: When `short` style is used, the rule will not flag the arguments that use
imported types. This is because the rule cannot efficiently determine the imported type.

_Configuration (1)_: (string) as a single string, it configures both argument
and return value styles. Accepts 'any', 'short', or 'full' (default: 'any').

Expand Down
14 changes: 12 additions & 2 deletions rule/enforce-repeated-arg-type-style.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"go/ast"
"go/types"
"strings"
"sync"

"github.com/mgechev/revive/lint"
Expand Down Expand Up @@ -134,7 +135,8 @@ func (r *EnforceRepeatedArgTypeStyleRule) Apply(file *lint.File, arguments lint.
var prevType ast.Expr
if fn.Type.Params != nil {
for _, field := range fn.Type.Params.List {
if types.Identical(typesInfo.Types[field.Type].Type, typesInfo.Types[prevType].Type) {
// TODO: For invalid types we could have compared raw import names (import package alias + selector), but will it work properly in all the cases?
if !r.isInvalidType(typesInfo.Types[field.Type].Type) && types.Identical(typesInfo.Types[field.Type].Type, typesInfo.Types[prevType].Type) {
failures = append(failures, lint.Failure{
Confidence: 1,
Node: field,
Expand Down Expand Up @@ -166,7 +168,8 @@ func (r *EnforceRepeatedArgTypeStyleRule) Apply(file *lint.File, arguments lint.
var prevType ast.Expr
if fn.Type.Results != nil {
for _, field := range fn.Type.Results.List {
if field.Names != nil && types.Identical(typesInfo.Types[field.Type].Type, typesInfo.Types[prevType].Type) {
// TODO: For invalid types we could have compared raw import names (import package alias + selector), but will it work properly in all the cases?
if !r.isInvalidType(typesInfo.Types[field.Type].Type) && field.Names != nil && types.Identical(typesInfo.Types[field.Type].Type, typesInfo.Types[prevType].Type) {
failures = append(failures, lint.Failure{
Confidence: 1,
Node: field,
Expand All @@ -189,3 +192,10 @@ func (r *EnforceRepeatedArgTypeStyleRule) Apply(file *lint.File, arguments lint.
func (*EnforceRepeatedArgTypeStyleRule) Name() string {
return "enforce-repeated-arg-type-style"
}

// Invalid types are imported from other packages, and we can't compare them.
// Note, we check the string suffix to cover all the cases: non-pointer, pointer, double pointer, etc.
// See: https://github.com/mgechev/revive/issues/1032
func (*EnforceRepeatedArgTypeStyleRule) isInvalidType(t types.Type) bool {
return strings.HasSuffix(t.String(), "invalid type")
}

0 comments on commit 3249a5e

Please sign in to comment.