bug: `redundant-import-alias` rule check for import path instead of package name

[candiduslynx]

Describe the bug

When checking redundant-import-alias the code will only look at the import path, whereas the proper way would've been to check the imported package name. I've seen this with import apiv2 "cloud.google.com/go/run/apiv2" (the code is under apiv2 directory, but the package name is run: https://pkg.go.dev/cloud.google.com/go/run/apiv2)

To Reproduce Steps to reproduce the behavior:

1. Create a package in a dir named redundant with the following content (note that the package name must be different):

   package relevant
   
   var Lovely = int(123)
   

2. Import it via import redundant "path/to/redundant"
3. Run check

Expected behavior No issue is presented, as the alias is used to override the package name, not the path.

Logs

redundant-import-alias: Import alias "apiv2" is redundant (revive)

This is printed when importing:

import apiv2 "cloud.google.com/go/run/apiv2"

[chavacava]

Hi @candiduslynx, thank you for reporting the issue.

[denisvmedia]

The only viable solution for that seems to be using

	import "golang.org/x/tools/go/packages"

	// ...

	// Load the package information
	pkgs, err := packages.Load(cfg, importPath)
	if err != nil {
		return "", fmt.Errorf("Error loading package: %w", err)
	}

	// Ensure that the package information is available
	if len(pkgs) == 0 || pkgs[0].Name == "" {
		return "", fmt.Errorf("Package information not found for import path: %s", importPath)
	}

	return pkgs[0].Name, nil // <-- here we'll return `run` for https://pkg.go.dev/cloud.google.com/go/run/apiv2

The problem of this solution is that package.Load uses exec.Command internally. And thus it's very slow on big code bases. For such cases I'd use importas linter, where you can strictly define an alias (or an absence of such). I'm not sure if revive has a built-in alternative.

[candiduslynx]

Another possible solution would be to load only package name when traversing the vendored imports, but this will also require an additional effort:

• How the vendor directory is defined based on the place where linter was executed from?
• Are the dependencies required to be vendored now? If not, this will be a major change to simple code scanning

[denisvmedia]

There are two ways of having the deps: vendor dir or in $GOPATH/pkg/mod. This would mean that the tool will have to understand, which mode is used in the project. While technically possible (first check for vendor dir, then - if not found - look in $GOPATH/pkg/mod), that would require the rule to go way beyond what other rules do. It will also mean that the rule will have to parse the first found non-test file using "go/parser" (this will slow down the processing as well, I guess). I'm not sure if it's a good idea.

[candiduslynx]

IMO this check just needs to be retired, as it's not usable with some major SDKs.

Or, at the very least, the package name should be determined by an updated process (it will still be a hack and won't work for the example in the issue description, where the package name doesn't match the directory name):

1. Split name parts
2. iterating from the end, check if the part is actually v%d
3. if it's v%d, go to the next one
4. if not - here's the (assumed) package name

I'm not entirely sure what to do with

import apiv2 "cloud.google.com/go/run/apiv2"

[denisvmedia]

Note that the actual package name can be v1: https://pkg.go.dev/k8s.io/api/core/v1.

[natefinch]

Note that this linter directly conflicts with what goimports does for packages like the one denisvmedia posted above.

if the package name is literally v1, then goimports will change

import "github.com/foo/bar/v1"

to

import v1 "gitub.com/foo/bar/v1"

...and then revive will tell you that's redundant.

Now, I don't 100% agree with goimports aliasing a thing to the same name... and I think it's generally dumb to call a package literally v1. BUT, if I have to pick who has to comply with who, I gotta say that revive should not call anything that goimports does "incorrect".