Repository: bazelbuild/buildtools
Pull request #792 (2020-02-20 18:30:08)

Fix .bzl visibility warnings
The warning didn't work correctly with relative file paths.

vladmos: That's Windows- (case-insensitive) and MacOS- (different interface of `sed`) specific in the shell test. I had to update the shell test because the unit test for the warning didn't catch everything, i.e. the location of the WORKSPACE file is supposed to be detected not by the warning function but by the main buildifier binary.

I'll fix the test soon.
vladmos: The tests are passing now.

Patch:
diff --git a/build/syntax.go b/build/syntax.go
index b768821..adbf051 100644
--- a/build/syntax.go
+++ b/build/syntax.go
@@ -92,9 +92,10 @@ func stmtsEnd(stmts []Expr) Position {
 
 // A File represents an entire BUILD or .bzl file.
 type File struct {
-	Path string // file path, relative to workspace directory
-	Pkg  string // optional; the package of the file
-	Type FileType
+	Path          string // absolute file path
+	Pkg           string // optional; the package of the file
+	WorkspaceRoot string // optional; path to the directory containing the WORKSPACE file
+	Type          FileType
 	Comments
 	Stmt []Expr
 }
diff --git a/buildifier/buildifier.go b/buildifier/buildifier.go
index 56fdd95..3d58673 100644
--- a/buildifier/buildifier.go
+++ b/buildifier/buildifier.go
@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"runtime"
 	"strings"
 
@@ -327,7 +328,10 @@ func processFile(filename string, data []byte, inputType, lint string, warningsL
 		return utils.InvalidFileDiagnostics(displayFilename), exitCode
 	}
 
-	_, f.Pkg = utils.SplitFilePath(displayFilename)
+	if absoluteFilename, err := filepath.Abs(displayFilename); err == nil {
+		f.WorkspaceRoot, f.Pkg = utils.SplitFilePath(absoluteFilename)
+	}
+
 	warnings := utils.Lint(f, lint, warningsList, *vflag)
 	if len(warnings) > 0 {
 		exitCode = 4
diff --git a/buildifier/utils/BUILD.bazel b/buildifier/utils/BUILD.bazel
index 6947f92..5b61390 100644
--- a/buildifier/utils/BUILD.bazel
+++ b/buildifier/utils/BUILD.bazel
@@ -16,13 +16,6 @@ go_library(
     ],
 )
 
-go_test(
-    name = "utils_test",
-    size = "small",
-    srcs = ["utils_test.go"],
-    embed = [":go_default_library"],
-)
-
 go_test(
     name = "go_default_test",
     srcs = ["utils_test.go"],
diff --git a/buildifier/utils/utils.go b/buildifier/utils/utils.go
index 2d62510..61fc98b 100644
--- a/buildifier/utils/utils.go
+++ b/buildifier/utils/utils.go
@@ -4,7 +4,6 @@ package utils
 
 import (
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 
@@ -77,12 +76,25 @@ func GetParser(inputType string) func(filename string, data []byte) (*build.File
 	}
 }
 
+// hasWorkspaceFile checks whether a given directory contains a file called
+// WORKSPACE or WORKSPACE.bazel.
+func hasWorkspaceFile(path string) bool {
+	for _, filename := range []string{"WORKSPACE", "WORKSPACE.bazel"} {
+		workspace := filepath.Join(path, filename)
+		info, err := os.Stat(workspace)
+		if err == nil && !info.IsDir() {
+			return true
+		}
+	}
+	return false
+}
+
 // SplitFilePath splits a file path into the workspace root and package name.
 // Workspace root is determined as the last directory in the file path that
-// contains a WORKSPACE file.
+// contains a WORKSPACE (or WORKSPACE.bazel) file.
 // Returns empty strings if no WORKSPACE file is found
 func SplitFilePath(filename string) (workspaceRoot, pkg string) {
-	directory := path.Dir(filename)
+	directory := filepath.Dir(filename)
 	root := "/"
 	if volume := filepath.VolumeName(directory); volume != "" {
 		// Windows
@@ -95,8 +107,7 @@ func SplitFilePath(filename string) (workspaceRoot, pkg string) {
 	parent := root
 	for i, chunk := range dirs {
 		parent = filepath.Join(parent, chunk)
-		workspace := filepath.Join(parent, "WORKSPACE")
-		if _, err := os.Stat(workspace); !os.IsNotExist(err) {
+		if hasWorkspaceFile(parent) {
 			workspaceRoot = parent
 			pkg = strings.Join(dirs[i+1:], "/")
 		}
diff --git a/warn/warn.go b/warn/warn.go
index 537af1e..5c3281b 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -106,7 +106,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"attr-output-default":       attrOutputDefaultWarning,
 	"attr-single-file":          attrSingleFileWarning,
 	"build-args-kwargs":         argsKwargsInBuildFilesWarning,
-	"bzl-visibility":            deprecatedBzlLoadWarning,
+	"bzl-visibility":            bzlVisibilityWarning,
 	"confusing-name":            confusingNameWarning,
 	"constant-glob":             constantGlobWarning,
 	"ctx-actions":               ctxActionsWarning,
diff --git a/warn/warn_visibility.go b/warn/warn_visibility.go
index 5331a1d..6973a8f 100644
--- a/warn/warn_visibility.go
+++ b/warn/warn_visibility.go
@@ -12,9 +12,16 @@ import (
 
 var internalDirectory = regexp.MustCompile("/(internal|private)[/:]")
 
-func deprecatedBzlLoadWarning(f *build.File) []*LinterFinding {
+func bzlVisibilityWarning(f *build.File) []*LinterFinding {
 	var findings []*LinterFinding
 
+	if f.WorkspaceRoot == "" {
+		// Empty workspace root means buildifier doesn't know the location of
+		// the file relative to the workspace directory and can't warn about .bzl
+		// file visibility correctly.
+		return findings
+	}
+
 	for _, stmt := range f.Stmt {
 		load, ok := stmt.(*build.LoadStmt)
 		if !ok || load.Module == nil {


Test patch:
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index 9019ec7..57cfa8e 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -33,6 +33,8 @@ die () {
 buildifier="$(rlocation "$buildifier")"
 buildifier2="$(rlocation "$buildifier2")"
 
+touch WORKSPACE.bazel
+rm -r test_dir || true
 mkdir -p test_dir/subdir
 mkdir -p golden
 INPUT="load(':foo.bzl', 'foo'); foo(tags=['b', 'a'],srcs=['d', 'c'])"  # formatted differently in build and bzl modes
@@ -97,50 +99,61 @@ fi
 # Test the linter
 
 cat > test_dir/to_fix.bzl <<EOF
+load("//foo/bar/internal/baz:module.bzl", "b")
+
 a = b / c
 d = {"b": 2, "a": 1}
 attr.foo(bar, cfg = "data")
 EOF
 
 cat > test_dir/fixed_golden.bzl <<EOF
+load("//foo/bar/internal/baz:module.bzl", "b")
+
 a = b // c
 d = {"b": 2, "a": 1}
 attr.foo(bar)
 EOF
 
 cat > test_dir/fixed_golden_all.bzl <<EOF
+load("//foo/bar/internal/baz:module.bzl", "b")
+
 a = b // c
 d = {"a": 1, "b": 2}
 attr.foo(bar)
 EOF
 
 cat > test_dir/fixed_golden_dict_cfg.bzl <<EOF
+load("//foo/bar/internal/baz:module.bzl", "b")
+
 a = b / c
 d = {"a": 1, "b": 2}
 attr.foo(bar)
 EOF
 
 cat > test_dir/fixed_golden_cfg.bzl <<EOF
+load("//foo/bar/internal/baz:module.bzl", "b")
+
 a = b / c
 d = {"b": 2, "a": 1}
 attr.foo(bar)
 EOF
 
 cat > test_dir/fix_report_golden <<EOF
-test_dir/to_fix_tmp.bzl: applied fixes, 1 warnings left
+test_dir/to_fix_tmp.bzl: applied fixes, 2 warnings left
 fixed test_dir/to_fix_tmp.bzl
 EOF
 
+error_bzl="test_dir/to_fix_tmp.bzl:1: bzl-visibility: Module \"//foo/bar/internal/baz:module.bzl\" can only be loaded from files located inside \"//foo/bar\", not from \"//test_dir\". (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#bzl-visibility)"
 error_docstring="test_dir/to_fix_tmp.bzl:1: module-docstring: The file has no module docstring."$'\n'"A module docstring is a string literal (not a comment) which should be the first statement of a file (it may follow comment lines). (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#module-docstring)"
-error_integer="test_dir/to_fix_tmp.bzl:1: integer-division: The \"/\" operator for integer division is deprecated in favor of \"//\". (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#integer-division)"
-error_dict="test_dir/to_fix_tmp.bzl:2: unsorted-dict-items: Dictionary items are out of their lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#unsorted-dict-items)"
-error_cfg="test_dir/to_fix_tmp.bzl:3: attr-cfg: cfg = \"data\" for attr definitions has no effect and should be removed. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#attr-cfg)"
+error_integer="test_dir/to_fix_tmp.bzl:3: integer-division: The \"/\" operator for integer division is deprecated in favor of \"//\". (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#integer-division)"
+error_dict="test_dir/to_fix_tmp.bzl:4: unsorted-dict-items: Dictionary items are out of their lexicographical order. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#unsorted-dict-items)"
+error_cfg="test_dir/to_fix_tmp.bzl:5: attr-cfg: cfg = \"data\" for attr definitions has no effect and should be removed. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#attr-cfg)"
 
 test_lint () {
   ret=0
   cp test_dir/to_fix.bzl test_dir/to_fix_tmp.bzl
   echo "$4" > golden/error_golden
-  echo "${4//test_dir\/to_fix_tmp.bzl/foo.bzl}" > golden/error_golden_foo
+  echo "${4//test_dir/another_test_dir}" > golden/error_golden_another
 
   cat > golden/fix_report_golden <<EOF
 test_dir/to_fix_tmp.bzl: applied fixes, $5 warnings left
@@ -163,11 +176,11 @@ EOF
   diff test_dir/error golden/error_golden || die "$1: wrong console output for --lint=warn"
 
   # --lint=warn with --path
-  $buildifier --lint=warn --path=foo.bzl $2 test_dir/to_fix_tmp.bzl 2> test_dir/error || ret=$?
+  $buildifier --lint=warn --path=another_test_dir/to_fix_tmp.bzl $2 test_dir/to_fix_tmp.bzl 2> test_dir/error || ret=$?
   if [[ $ret -ne 4 ]]; then
     die "$1: warn: Expected buildifier to exit with 4, actual: $ret"
   fi
-  diff test_dir/error golden/error_golden_foo || die "$1: wrong console output for --lint=warn and --path"
+  diff test_dir/error golden/error_golden_another || die "$1: wrong console output for --lint=warn and --path"
 
   # --lint=fix
   $buildifier --lint=fix $2 -v test_dir/to_fix_tmp.bzl 2> test_dir/fix_report || ret=$?
@@ -178,8 +191,8 @@ EOF
   diff test_dir/fix_report golden/fix_report_golden || die "$1: wrong console output for --lint=fix"
 }
 
-test_lint "default" "" "test_dir/fixed_golden.bzl" "$error_integer"$'\n'"$error_docstring"$'\n'"$error_cfg" 1
-test_lint "all" "--warnings=all" "test_dir/fixed_golden_all.bzl" "$error_integer"$'\n'"$error_docstring"$'\n'"$error_dict"$'\n'"$error_cfg" 1
+test_lint "default" "" "test_dir/fixed_golden.bzl" "$error_docstring"$'\n'"$error_integer"$'\n'"$error_cfg" 1
+test_lint "all" "--warnings=all" "test_dir/fixed_golden_all.bzl" "$error_bzl"$'\n'"$error_docstring"$'\n'"$error_integer"$'\n'"$error_dict"$'\n'"$error_cfg" 2
 test_lint "cfg" "--warnings=attr-cfg" "test_dir/fixed_golden_cfg.bzl" "$error_cfg" 0
 test_lint "custom" "--warnings=-integer-division,+unsorted-dict-items" "test_dir/fixed_golden_dict_cfg.bzl" "$error_docstring"$'\n'"$error_dict"$'\n'"$error_cfg" 1
 
@@ -219,11 +232,11 @@ cat > golden/json_report_golden <<EOF
             "warnings": [
                 {
                     "start": {
-                        "line": 1,
+                        "line": 3,
                         "column": 5
                     },
                     "end": {
-                        "line": 1,
+                        "line": 3,
                         "column": 10
                     },
                     "category": "integer-division",
@@ -233,11 +246,11 @@ cat > golden/json_report_golden <<EOF
                 },
                 {
                     "start": {
-                        "line": 3,
+                        "line": 5,
                         "column": 15
                     },
                     "end": {
-                        "line": 3,
+                        "line": 5,
                         "column": 27
                     },
                     "category": "attr-cfg",
diff --git a/buildifier/utils/utils_test.go b/buildifier/utils/utils_test.go
index 7043ead..d61c516 100644
--- a/buildifier/utils/utils_test.go
+++ b/buildifier/utils/utils_test.go
@@ -148,6 +148,13 @@ func TestSplitFilePath(t *testing.T) {
 	checkSplitFilePathOutput(t, "WORKSPACE file exists", filename, dir, "path/to/package")
 	checkSplitFilePathOutput(t, "WORKSPACE file exists, empty package", filepath.Join(dir, "file.bzl"), dir, "")
 
+	// Rename WORKSPACE to WORKSPACE.bazel and try again (dir/WORKSPACE.bazel)
+	if err := os.Rename(filepath.Join(dir, "WORKSPACE"), filepath.Join(dir, "WORKSPACE.bazel")); err != nil {
+		t.Error(err)
+	}
+	checkSplitFilePathOutput(t, "WORKSPACE file exists", filename, dir, "path/to/package")
+	checkSplitFilePathOutput(t, "WORKSPACE file exists, empty package", filepath.Join(dir, "file.bzl"), dir, "")
+
 	// Create another WORKSPACE file and try again (dir/path/WORKSPACE)
 	newRoot := filepath.Join(dir, "path")
 	if err := os.MkdirAll(newRoot, os.ModePerm); err != nil {
diff --git a/warn/warn_test.go b/warn/warn_test.go
index 735a679..54f8aa5 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -39,6 +39,7 @@ func getFindings(category, input string, fileType build.FileType) []*Finding {
 		panic(fmt.Sprintf("%v", err))
 	}
 	buildFile.Pkg = "test/package"
+	buildFile.WorkspaceRoot = "/home/users/foo/bar"
 	return FileWarnings(buildFile, []string{category}, nil, ModeWarn)
 }