Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix redunant-import-alias rule - unused alias is also redundant #950

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions RULES_DESCRIPTIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -672,9 +672,18 @@ _Configuration_: N/A

## redundant-import-alias

_Description_: This rule warns on redundant import aliases. This happens when the alias used on the import statement matches the imported package name.
_Description_: This rule warns on redundant import aliases. This happens when the alias used on the import statement matches the imported package name.
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved

_Configuration_: N/A
_Configuration_:

* `ignoreUsed` : (bool) ignore aliases used in code (useful when using popular sdk packages).
Copy link
Collaborator

@denisvmedia denisvmedia Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please improve the wording? It's not really clear for me what exactly is to be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import apiv2 "cloud.google.com/go/run/apiv2"
import v1 "https://pkg.go.dev/k8s.io/api/core/v1"

technically above aliases are correct if they are used min one time later in code. What is your sugestion to describe that flag?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that if it's not used, the code won't compile.

package main

import apiv2 "cloud.google.com/go/run/apiv2"
import v1 "k8s.io/api/core/v1"
import "fmt"

func main() {
    fmt.Println("dummy")
}
$ go build .
# poc
./main.go:3:8: "cloud.google.com/go/run/apiv2" imported and not used
./main.go:4:8: "k8s.io/api/core/v1" imported and not used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no no no, your case is not that case, because this code is not used and make "noise" after compile

but in case mentioned in #936:

package main

import (
	"fmt"
	redundant "example.com/app/redundant"
)

func main() {
	fmt.Println(redundant.Lovely)
}

=$ ./app 
123

above code is working well, but linter shows warning:

=$ cat ~/revive-aliases.toml 
ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 0
warningCode = 0

[rule.blank-imports]
[rule.dot-imports]
[rule.redundant-import-alias]
revive -config ~/revive-aliases.toml -formatter stylish ./...
main.go
  (5, 2)  https://revive.run/r#redundant-import-alias  Import alias "redundant" is redundant  


 ✖ 1 problem (0 errors) (1 warnings)

after set ignoreUsed = true

[rule.redundant-import-alias]
  arguments = [{ignoreUsed = true}]

linter dont produce warnings that is the case :)

Copy link
Collaborator

@denisvmedia denisvmedia Dec 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed redundant. This is the purpose of the linter. If you think it's fine for your project, you should just disable this linter rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i know, it is specific case, because in one way you should think how you use packages and aliases for them. On the other side we can leave this rule with that argument for more flexibility, but this is not my decision, what you think @chavacava

Copy link
Collaborator

@denisvmedia denisvmedia Dec 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is specific about this case? ignoreUsed seems to be essentially disabling the rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@denisvmedia ok, I think that issue #936 and this pull request is to close, because in standard scenario this rule is used to detect redundant aliases, and results should used to fix/cleanup codebase of your project (change redundant aliases to proper form)


Example:

```toml
[rule.redundant-import-alias]
arguments = [{ ignoreUsed = true}]
```

## string-format

Expand Down
90 changes: 77 additions & 13 deletions rule/redundant-import-alias.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,35 @@ package rule
import (
"fmt"
"go/ast"
"strings"
"sync"

"github.com/mgechev/revive/lint"
)

const (
defaultIgnoreUsed = false
)

// RedundantImportAlias lints given else constructs.
type RedundantImportAlias struct{}
type RedundantImportAlias struct {
ignoreUsed bool
sync.Mutex
}

// Apply applies the rule to given file.
func (*RedundantImportAlias) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
func (r *RedundantImportAlias) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configure(arguments)

var failures []lint.Failure

redundants := r.checkRedundantAliases(file.AST)

for _, imp := range file.AST.Imports {
if imp.Name == nil {
continue
}

if getImportPackageName(imp) == imp.Name.Name {
if _, exists := redundants[imp.Name.Name]; exists {
failures = append(failures, lint.Failure{
Confidence: 1,
Failure: fmt.Sprintf("Import alias \"%s\" is redundant", imp.Name.Name),
Expand All @@ -29,7 +40,6 @@ func (*RedundantImportAlias) Apply(file *lint.File, _ lint.Arguments) []lint.Fai
})
}
}

return failures
}

Expand All @@ -38,15 +48,69 @@ func (*RedundantImportAlias) Name() string {
return "redundant-import-alias"
}

func getImportPackageName(imp *ast.ImportSpec) string {
const pathSep = "/"
const strDelim = `"`
func (r *RedundantImportAlias) checkRedundantAliases(node ast.Node) map[string]string {

var aliasedPackages = make(map[string]string)

// First pass: Identify all aliases and their usage - by default alias is redundant
ast.Inspect(node, func(n ast.Node) bool {
imp, ok := n.(*ast.ImportSpec)
if !ok {
return true
}

if imp.Name != nil && imp.Path != nil {
aliasedPackages[imp.Name.Name] = "redundant"
}
return true
})

if r.ignoreUsed {
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
// Second pass: remove one time used aliases
ast.Inspect(node, func(n ast.Node) bool {
sel, ok := n.(*ast.SelectorExpr)
if !ok {
return true
}

x, ok := sel.X.(*ast.Ident)
if !ok {
return true
}

// This alias is being used; it's not redundant
if _, exists := aliasedPackages[x.Name]; exists {
delete(aliasedPackages, x.Name)
}

path := imp.Path.Value
i := strings.LastIndex(path, pathSep)
if i == -1 {
return strings.Trim(path, strDelim)
return true
})
}

return strings.Trim(path[i+1:], strDelim)
return aliasedPackages
}

func (r *RedundantImportAlias) configure(arguments lint.Arguments) {
r.Lock()
defer r.Unlock()

r.ignoreUsed = defaultIgnoreUsed
if len(arguments) > 0 {
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved

args, ok := arguments[0].(map[string]any)
if !ok {
panic(fmt.Sprintf("Invalid argument to the redundant-import-alias rule. Expecting a k,v map, got %T", arguments[0]))
}

for k, v := range args {
switch k {
case "ignoreUsed":
value, ok := v.(bool)
if !ok {
panic(fmt.Sprintf("Invalid argument to the redundant-import-alias rule, expecting string representation of an bool. Got '%v' (%T)", v, v))
}
r.ignoreUsed = value
}
}
}
}
26 changes: 24 additions & 2 deletions test/redundant-import-alias_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,33 @@
package test

import (
"github.com/mgechev/revive/rule"
"testing"

"github.com/mgechev/revive/lint"
"github.com/mgechev/revive/rule"
)

// TestRedundantImportAlias rule.
func TestRedundantImportIgnoredAliases(t *testing.T) {

args := []any{map[string]any{
"ignoreUsed": true,
}}

testRule(t, "redundant-import-alias-ignored", &rule.RedundantImportAlias{}, &lint.RuleConfig{
Arguments: args,
})

}

func TestRedundantImportAlias(t *testing.T) {
testRule(t, "redundant-import-alias", &rule.RedundantImportAlias{})

args := []any{map[string]any{
"ignoreUsed": false,
}}

testRule(t, "redundant-import-alias", &rule.RedundantImportAlias{}, &lint.RuleConfig{
Arguments: args,
})

}
22 changes: 22 additions & 0 deletions testdata/redundant-import-alias-ignored.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package fixtures

import (
runpb "cloud.google.com/go/run/apiv2/runpb"
md5 "crypto/md5"
strings "strings" // MATCH /Import alias "strings" is redundant/

"crypto/md5"
_ "crypto/md5" // MATCH /Import alias "_" is redundant/

"strings"
str "strings" // MATCH /Import alias "str" is redundant/

)

func UseRunpb() {
runpb.RegisterTasksServer()
}

func UseMd5() {
fmt.PrintLn(md5.Size)
}
26 changes: 19 additions & 7 deletions testdata/redundant-import-alias.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
package fixtures

import(
import (
runpb "cloud.google.com/go/run/apiv2/runpb" // MATCH /Import alias "runpb" is redundant/

md5 "crypto/md5" // MATCH /Import alias "md5" is redundant/

strings "strings" // MATCH /Import alias "strings" is redundant/

"crypto/md5"
"strings"
_ "crypto/md5"
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
str "strings"
strings "strings" // MATCH /Import alias "strings" is redundant/
crypto "crypto/md5"
md5 "crypto/md5" // MATCH /Import alias "md5" is redundant/
_ "crypto/md5" // MATCH /Import alias "_" is redundant/
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved

"strings"
str "strings" // MATCH /Import alias "str" is redundant/

)

func UseRunpb() {
runpb.RegisterTasksServer()
}

func UseMd5() {
fmt.PrintLn(md5.Size)
}
Loading