Disable bzl-visibility lint if the target file has a visibility() declaration (#1439)

* Disable bzl-visibility lint if the target file has a `visibility()` declaration.

The official docs for this lint say:

> This lint predates the load visibility feature and is unnecessary in workspaces where .bzl files declare visibilities.

However the lint can sometimes get in the way, by warning about instances where the usage is actually intended.

Since `visibility()` is more explicit, and more strictly enforced, there is no need for the lint to return any warnings if the target file has a visibility declaration.

MARKDOWN=true

* Addressed edge case where call function is not an ident.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* Added a test that files without visibility() still enforce the check.

MARKDOWN=true

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Author: haberman

warn/warn.go

@@ -124,7 +124,6 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"attr-output-default":       attrOutputDefaultWarning,
 	"attr-single-file":          attrSingleFileWarning,
 	"build-args-kwargs":         argsKwargsInBuildFilesWarning,
-	"bzl-visibility":            bzlVisibilityWarning,
 	"canonical-repository":      canonicalRepositoryWarning,
 	"confusing-name":            confusingNameWarning,
 	"constant-glob":             constantGlobWarning,
@@ -172,6 +171,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 
 // MultiFileWarningMap lists the warnings that run on the whole file, but may use other files.
 var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader) []*LinterFinding{
+	"bzl-visibility":                     bzlVisibilityWarning,
 	"deprecated-function":                deprecatedFunctionWarning,
 	"git-repository":                     nativeGitRepositoryWarning,
 	"http-archive":                       nativeHTTPArchiveWarning,

warn/warn_visibility.go

@@ -24,11 +24,38 @@ import (
 	"strings"
 
 	"github.com/bazelbuild/buildtools/build"
+	"github.com/bazelbuild/buildtools/labels"
 )
 
 var internalDirectory = regexp.MustCompile("/(internal|private)[/:]")
 
-func bzlVisibilityWarning(f *build.File) []*LinterFinding {
+func hasVisibilityStatement(f *build.File, load *build.LoadStmt, fileReader *FileReader) bool {
+	if fileReader == nil {
+		return false
+	}
+	label := labels.ParseRelative(load.Module.Value, f.Pkg)
+	if label.Repository != "" || label.Target == "" {
+		return false
+	}
+	loadedFile := fileReader.GetFile(label.Package, label.Target)
+	if loadedFile == nil {
+		return false
+	}
+	for _, stmt := range loadedFile.Stmt {
+		call, ok := stmt.(*build.CallExpr)
+		if !ok {
+			continue
+		}
+		// We don't try to be exhaustive here, but rather only catch the most
+		// common cases of visibility declarations.
+        if ident, ok := call.X.(*build.Ident); ok && ident.Name == "visibility" {
+            return true
+        }
+	}
+	return false
+}
+
+func bzlVisibilityWarning(f *build.File, fileReader *FileReader) []*LinterFinding {
 	var findings []*LinterFinding
 
 	if f.WorkspaceRoot == "" {
@@ -58,6 +85,13 @@ func bzlVisibilityWarning(f *build.File) []*LinterFinding {
 			}
 		}
 
+		if hasVisibilityStatement(f, load, fileReader) {
+			// The module has a visibility statement, which is a more explicit
+			// (and strongly-enforced) mechanism to specify visibility. No need
+			// to issue a warning using the older and less explicit mechanism.
+			continue
+		}
+
 		path := f.CanonicalPath() // Canonical name of the file
 		chunks := internalDirectory.Split(module, 2)
 		if len(chunks) < 2 {

warn/warn_visibility_test.go

@@ -108,3 +108,40 @@ bar1()
 		[]string{},
 		scopeEverywhere)
 }
+
+// Test that the warning is not issued if the module has a visibility statement.
+func TestBzlVisibilityWithVisibilityStatement(t *testing.T) {
+	defer setUpFileReader(map[string]string{
+		"foo/private/bar.bzl": `
+visibility([
+    "//test/package:__subpackages__",
+])
+
+abc = 1
+`,
+	})()
+
+	checkFindings(t, "bzl-visibility", `
+load("//foo/private:bar.bzl", "abc")
+`,
+		[]string{},
+		scopeEverywhere)
+}
+
+// Test that the warning *is* issued if the module does *not* have a visibility statement.
+func TestBzlVisibilityWithoutVisibilityStatement(t *testing.T) {
+	defer setUpFileReader(map[string]string{
+		"foo/private/bar.bzl": `
+
+abc = 1
+`,
+	})()
+
+	checkFindings(t, "bzl-visibility", `
+load("//foo/private:bar.bzl", "abc")
+`,
+		[]string{
+			`Module "//foo/private:bar.bzl" can only be loaded from files located inside`,
+		},
+		scopeEverywhere)
+}