Repository: bazelbuild/buildtools
Pull request #818 (2020-04-16 20:50:48)

Fix file packages
Currently buildifier determines a file's package as its base name (relative to the workspace root). Although that's always true for BUILD files, .bzl files may be located not necessarily in a package root but in a subdirectory.

That didn't matter before, but now with the functionality of loading additional .bzl files that's being used by some analyzers, it's important to know the current file's package to resolve relative labels correctly.

To achieve that it's enough to locate a BUILD file closest to the initial file; all other loaded files packages can be determined by analyzing their label: e.g. if "//some/package:label/name.bzl" is loaded, the package is set to "some/package" and the label to "label/name.bzl" without even looking at BUILD files.


Patch:
diff --git a/build/syntax.go b/build/syntax.go
index adbf051..c8b21d5 100644
--- a/build/syntax.go
+++ b/build/syntax.go
@@ -20,6 +20,7 @@ package build
 // Syntax data structure definitions.
 
 import (
+	"fmt"
 	"strings"
 	"unicode/utf8"
 )
@@ -93,7 +94,8 @@ func stmtsEnd(stmts []Expr) Position {
 // A File represents an entire BUILD or .bzl file.
 type File struct {
 	Path          string // absolute file path
-	Pkg           string // optional; the package of the file
+	Pkg           string // optional; the package of the file (always forward slashes)
+	Label         string // optional; file path relative to the package name (always forward slashes)
 	WorkspaceRoot string // optional; path to the directory containing the WORKSPACE file
 	Type          FileType
 	Comments
@@ -108,6 +110,14 @@ func (f *File) DisplayPath() string {
 	return f.Path
 }
 
+// CanonicalPath returns the path of a file relative to the workspace root with forward slashes only
+func (f *File) CanonicalPath() string {
+	if f.Pkg == "" {
+		return "//" + f.Label
+	}
+	return fmt.Sprintf("//%s/%s", f.Pkg, f.Label)
+}
+
 func (f *File) Span() (start, end Position) {
 	if len(f.Stmt) == 0 {
 		p := Position{Line: 1, LineRune: 1}
diff --git a/buildifier/buildifier.go b/buildifier/buildifier.go
index 0b901aa..4478df8 100644
--- a/buildifier/buildifier.go
+++ b/buildifier/buildifier.go
@@ -329,7 +329,7 @@ func processFile(filename string, data []byte, inputType, lint string, warningsL
 	}
 
 	if absoluteFilename, err := filepath.Abs(displayFilename); err == nil {
-		f.WorkspaceRoot, f.Pkg = utils.SplitFilePath(absoluteFilename)
+		f.WorkspaceRoot, f.Pkg, f.Label = utils.SplitFilePath(absoluteFilename)
 	}
 
 	warnings := utils.Lint(f, lint, warningsList, *vflag)
diff --git a/buildifier/utils/utils.go b/buildifier/utils/utils.go
index 9c84f8a..36f518e 100644
--- a/buildifier/utils/utils.go
+++ b/buildifier/utils/utils.go
@@ -5,6 +5,7 @@ package utils
 import (
 	"io/ioutil"
 	"os"
+	"path"
 	"path/filepath"
 	"strings"
 
@@ -77,12 +78,12 @@ 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)
+// containsFile checks whether a given directory contains a file called
+// <file> or <file>.bazel.
+func containsFile(dir, file string) bool {
+	for _, filename := range []string{file, file + ".bazel"} {
+		path := filepath.Join(dir, filename)
+		info, err := os.Stat(path)
 		if err == nil && !info.IsDir() {
 			return true
 		}
@@ -94,26 +95,33 @@ func hasWorkspaceFile(path string) bool {
 // Workspace root is determined as the last directory in the file path that
 // contains a WORKSPACE (or WORKSPACE.bazel) file.
 // Returns empty strings if no WORKSPACE file is found
-func SplitFilePath(filename string) (workspaceRoot, pkg string) {
-	directory := filepath.Dir(filename)
+func SplitFilePath(filename string) (workspaceRoot, pkg, label string) {
 	root := "/"
-	if volume := filepath.VolumeName(directory); volume != "" {
+	if volume := filepath.VolumeName(filename); volume != "" {
 		// Windows
 		root = volume + "\\"
 	}
-	// directory relative to the file system root
-	relPath := directory[len(root):]
+	// filename relative to the file system root
+	relPath := filename[len(root):]
 
-	dirs := append([]string{""}, strings.Split(relPath, string(os.PathSeparator))...)
+	chunks := append([]string{""}, strings.Split(relPath, string(os.PathSeparator))...)
 	parent := root
-	for i, chunk := range dirs {
+	workspaceIndex := -1
+	for i := 0; i < len(chunks)-1; i++ {
+		chunk := chunks[i]
 		parent = filepath.Join(parent, chunk)
-		if hasWorkspaceFile(parent) {
+		if containsFile(parent, "WORKSPACE") {
+			workspaceIndex = i
 			workspaceRoot = parent
-			pkg = strings.Join(dirs[i+1:], "/")
+			pkg = ""
+			label = path.Join(chunks[i+1:]...)
+		}
+		if workspaceIndex != -1 && containsFile(parent, "BUILD") {
+			pkg = path.Join(chunks[workspaceIndex+1 : i+1]...)
+			label = path.Join(chunks[i+1:]...)
 		}
 	}
-	return workspaceRoot, pkg
+	return workspaceRoot, pkg, label
 }
 
 // getFileReader returns a *FileReader object that reads files from the local
diff --git a/warn/multifile.go b/warn/multifile.go
index d8a4036..e05110c 100644
--- a/warn/multifile.go
+++ b/warn/multifile.go
@@ -1,9 +1,8 @@
 package warn
 
 import (
-	"path"
-
 	"github.com/bazelbuild/buildtools/build"
+	"strings"
 )
 
 // FileReader is a class that can read an arbitrary Starlark file
@@ -19,7 +18,7 @@ type FileReader struct {
 // (OS-independent, with forward slashes).
 func NewFileReader(readFile func(string) ([]byte, error)) *FileReader {
 	return &FileReader{
-		cache: make(map[string]*build.File),
+		cache:    make(map[string]*build.File),
 		readFile: readFile,
 	}
 }
@@ -36,19 +35,48 @@ func (fr *FileReader) retrieveFile(filename string) *build.File {
 	if err != nil {
 		return nil
 	}
+	file.Path = filename
 
-	file.Pkg = path.Dir(filename)
 	return file
 }
 
 // GetFile reads a Starlark file from the repository or the cache.
 // Returns nil if the file is not found or not valid.
-func (fr *FileReader) GetFile(filename string) *build.File {
+func (fr *FileReader) GetFile(pkg, label string) *build.File {
+	filename := label
+	if pkg != "" {
+		filename = pkg + "/" + label
+	}
+
 	// Try to retrieve from the cache
 	if file, ok := fr.cache[filename]; ok {
 		return file
 	}
 	file := fr.retrieveFile(filename)
+	if file != nil {
+		file.Pkg = pkg
+		file.Label = label
+	}
 	fr.cache[filename] = file
 	return file
 }
+
+// ResolveLabel resolves a label to a .bzl file (which may be absolute or relative to the current package)
+// to a pair (package, label) for the .bzl file
+func ResolveLabel(currentPkg, label string) (pkg, newLabel string) {
+	switch {
+	case strings.HasPrefix(label, "//"):
+		// Absolute label path
+		label = label[2:]
+		if chunks := strings.SplitN(label, ":", 2); len(chunks) == 2 {
+			return chunks[0], chunks[1]
+		}
+		return "", label
+	case strings.HasPrefix(label, ":"):
+		// Relative label path
+		return currentPkg, label[1:]
+	default:
+		// External repositories are not supported
+		return "", ""
+	}
+}
diff --git a/warn/warn_deprecated.go b/warn/warn_deprecated.go
index 9738aea..3813978 100644
--- a/warn/warn_deprecated.go
+++ b/warn/warn_deprecated.go
@@ -4,31 +4,11 @@ package warn
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/bazelbuild/buildtools/build"
 )
 
-func getPathFromLabel(label, pkg string) string {
-	switch {
-	case strings.HasPrefix(label, "//"):
-		// Absolute label path
-		return strings.ReplaceAll(label[2:], ":", "/")
-	case strings.HasPrefix(label, ":"):
-		// Relative label path
-		return pkg + "/" + label[1:]
-	default:
-		// External repositories are not supported
-		return ""
-	}
-}
-
-func readBzlFile(path string, fileReader *FileReader) (*build.File, bool) {
-	file := fileReader.GetFile(path)
-	return file, file != nil
-}
-
-func checkDeprecatedFunction(stmt build.Expr, loadedSymbols *map[string]*build.Ident, path string) *LinterFinding {
+func checkDeprecatedFunction(stmt build.Expr, loadedSymbols *map[string]*build.Ident, fullLabel string) *LinterFinding {
 	def, ok := stmt.(*build.DefStmt)
 	if !ok {
 		return nil
@@ -50,7 +30,7 @@ func checkDeprecatedFunction(stmt build.Expr, loadedSymbols *map[string]*build.I
 		return nil
 	}
 
-	return makeLinterFinding(node, fmt.Sprintf("The function %q defined in %q is deprecated.", def.Name, path))
+	return makeLinterFinding(node, fmt.Sprintf("The function %q defined in %q is deprecated.", def.Name, fullLabel))
 }
 
 func deprecatedFunctionWarning(f *build.File, fileReader *FileReader) []*LinterFinding {
@@ -64,22 +44,21 @@ func deprecatedFunctionWarning(f *build.File, fileReader *FileReader) []*LinterF
 		if !ok {
 			continue
 		}
-		path := getPathFromLabel(load.Module.Value, f.Pkg)
-		if path == "" {
+		pkg, fileLabel := ResolveLabel(f.Pkg, load.Module.Value)
+		if fileLabel == "" {
 			continue
 		}
-		loadedFile, ok := readBzlFile(path, fileReader)
-		if !ok {
+		loadedFile := fileReader.GetFile(pkg, fileLabel)
+		if loadedFile == nil {
 			continue
 		}
-
 		loadedSymbols := make(map[string]*build.Ident)
 		for _, from := range load.From {
 			loadedSymbols[from.Name] = from
 		}
 
 		for _, stmt := range loadedFile.Stmt {
-			if finding := checkDeprecatedFunction(stmt, &loadedSymbols, path); finding != nil {
+			if finding := checkDeprecatedFunction(stmt, &loadedSymbols, loadedFile.CanonicalPath()); finding != nil {
 				findings = append(findings, finding)
 			}
 		}
diff --git a/warn/warn_macro.go b/warn/warn_macro.go
index 9f84115..c4b97a9 100644
--- a/warn/warn_macro.go
+++ b/warn/warn_macro.go
@@ -13,10 +13,15 @@ const nativeModule = "<native>"
 
 // function represents a function identifier, which is a pair (module name, function name).
 type function struct {
-	filename string // .bzl file where the function is defined
+	pkg      string // package where the function is defined
+	filename string // name of a .bzl file relative to the package
 	name     string // original name of the function
 }
 
+func (f function) label() string {
+	return f.pkg + ":" + f.filename
+}
+
 // funCall represents a call to another function. It contains information of the function itself as well as some
 // information about the environment
 type funCall struct {
@@ -54,17 +59,11 @@ type fileData struct {
 	aliases   map[string]function           // all top-level aliases (e.g. `foo = bar`).
 }
 
-// externalDependency is a reference to a symbol defined in another file
-type externalDependency struct {
-	filename string
-	symbol   string
-}
-
 // resolvesExternal takes a local function definition and replaces it with an external one if it's been defined
 // in another file and loaded
-func resolveExternal(fn function, externalSymbols map[string]externalDependency) function {
+func resolveExternal(fn function, externalSymbols map[string]function) function {
 	if external, ok := externalSymbols[fn.name]; ok {
-		return function{filename: external.filename, name: external.symbol}
+		return external
 	}
 	return fn
 }
@@ -76,7 +75,7 @@ func exprLine(expr build.Expr) int {
 }
 
 // getFunCalls extracts information about functions that are being called from the given function
-func getFunCalls(def *build.DefStmt, filename string, externalSymbols map[string]externalDependency) map[string]funCall {
+func getFunCalls(def *build.DefStmt, pkg, filename string, externalSymbols map[string]function) map[string]funCall {
 	funCalls := make(map[string]funCall)
 	build.Walk(def, func(expr build.Expr, stack []build.Expr) {
 		call, ok := expr.(*build.CallExpr)
@@ -85,7 +84,7 @@ func getFunCalls(def *build.DefStmt, filename string, externalSymbols map[string
 		}
 		if ident, ok := call.X.(*build.Ident); ok {
 			funCalls[ident.Name] = funCall{
-				function:  resolveExternal(function{filename, ident.Name}, externalSymbols),
+				function:  resolveExternal(function{pkg, filename, ident.Name}, externalSymbols),
 				nameAlias: ident.Name,
 				line:      exprLine(call),
 			}
@@ -118,18 +117,18 @@ func analyzeFile(f *build.File) fileData {
 	}
 
 	// Collect loaded symbols
-	externalSymbols := make(map[string]externalDependency)
+	externalSymbols := make(map[string]function)
 	for _, stmt := range f.Stmt {
 		load, ok := stmt.(*build.LoadStmt)
 		if !ok {
 			continue
 		}
-		moduleFile := getPathFromLabel(load.Module.Value, f.Pkg)
-		if moduleFile == "" {
+		pkg, name := ResolveLabel(f.Pkg, load.Module.Value)
+		if name == "" {
 			continue
 		}
 		for i, from := range load.From {
-			externalSymbols[load.To[i].Name] = externalDependency{moduleFile, from.Name}
+			externalSymbols[load.To[i].Name] = function{pkg, name, from.Name}
 		}
 	}
 
@@ -147,7 +146,7 @@ func analyzeFile(f *build.File) fileData {
 				continue
 			}
 			if rhsIdent, ok := stmt.RHS.(*build.Ident); ok {
-				report.aliases[lhsIdent.Name] = resolveExternal(function{f.DisplayPath(), rhsIdent.Name}, externalSymbols)
+				report.aliases[lhsIdent.Name] = resolveExternal(function{f.Pkg, f.Label, rhsIdent.Name}, externalSymbols)
 				continue
 			}
 
@@ -161,7 +160,7 @@ func analyzeFile(f *build.File) fileData {
 			}
 			report.rules[lhsIdent.Name] = true
 		case *build.DefStmt:
-			report.functions[stmt.Name] = getFunCalls(stmt, f.DisplayPath(), externalSymbols)
+			report.functions[stmt.Name] = getFunCalls(stmt, f.Pkg, f.Label, externalSymbols)
 		default:
 			continue
 		}
@@ -185,7 +184,8 @@ type macroAnalyzer struct {
 
 // getFileData retrieves a file using the fileReader object and extracts information about functions and rules
 // defined in the file.
-func (ma macroAnalyzer) getFileData(filename string) fileData {
+func (ma macroAnalyzer) getFileData(pkg, label string) fileData {
+	filename := pkg + ":" + label
 	if fd, ok := ma.files[filename]; ok {
 		return fd
 	}
@@ -194,7 +194,7 @@ func (ma macroAnalyzer) getFileData(filename string) fileData {
 		ma.files[filename] = fd
 		return fd
 	}
-	f := ma.fileReader.GetFile(filename)
+	f := ma.fileReader.GetFile(pkg, label)
 	fd := analyzeFile(f)
 	ma.files[filename] = fd
 	return fd
@@ -225,7 +225,7 @@ func (ma macroAnalyzer) IsMacro(fn function) (report functionReport) {
 		return
 	}
 
-	fileData := ma.getFileData(fn.filename)
+	fileData := ma.getFileData(fn.pkg, fn.filename)
 
 	// Check whether fn.name is an alias for another function
 	if alias, ok := fileData.aliases[fn.name]; ok {
@@ -249,10 +249,9 @@ func (ma macroAnalyzer) IsMacro(fn function) (report functionReport) {
 
 	// Prioritize function calls from already loaded files. If some of the function calls are from the same file
 	// (or another file that has been loaded already), check them first.
-	knownFunCalls := []funCall{}
-	newFunCalls := []funCall{}
+	var knownFunCalls, newFunCalls []funCall
 	for _, fc := range funCalls {
-		if _, ok := ma.files[fc.function.filename]; ok || fc.function.filename == nativeModule {
+		if _, ok := ma.files[fc.function.pkg+":"+fc.function.filename]; ok || fc.function.filename == nativeModule {
 			knownFunCalls = append(knownFunCalls, fc)
 		} else {
 			newFunCalls = append(newFunCalls, fc)
@@ -285,7 +284,7 @@ func unnamedMacroWarning(f *build.File, fileReader *FileReader) []*LinterFinding
 	}
 
 	macroAnalyzer := newMacroAnalyzer(fileReader)
-	macroAnalyzer.files[f.DisplayPath()] = analyzeFile(f)
+	macroAnalyzer.files[f.Pkg+":"+f.Label] = analyzeFile(f)
 
 	findings := []*LinterFinding{}
 	for _, stmt := range f.Stmt {
@@ -298,7 +297,7 @@ func unnamedMacroWarning(f *build.File, fileReader *FileReader) []*LinterFinding
 			continue
 		}
 
-		report := macroAnalyzer.IsMacro(function{f.DisplayPath(), def.Name})
+		report := macroAnalyzer.IsMacro(function{f.Pkg, f.Label, def.Name})
 		if !report.isMacro {
 			continue
 		}
diff --git a/warn/warn_visibility.go b/warn/warn_visibility.go
index 8ae3eb7..389b825 100644
--- a/warn/warn_visibility.go
+++ b/warn/warn_visibility.go
@@ -42,19 +42,19 @@ func bzlVisibilityWarning(f *build.File) []*LinterFinding {
 			}
 		}
 
-		pkg := "//" + f.Pkg // Canonical name of the package
+		path := f.CanonicalPath() // Canonical name of the file
 		chunks := internalDirectory.Split(module, 2)
 		if len(chunks) < 2 {
 			continue
 		}
 
-		if strings.HasPrefix(pkg, chunks[0]) {
+		if strings.HasPrefix(path, chunks[0]) {
 			continue
 		}
 
 		findings = append(findings, makeLinterFinding(
 			load.Module,
-			fmt.Sprintf("Module %q can only be loaded from files located inside %q, not from %q.", load.Module.Value, chunks[0], pkg)))
+			fmt.Sprintf("Module %q can only be loaded from files located inside %q, not from %q.", load.Module.Value, chunks[0], path)))
 	}
 
 	return findings


Test patch:
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index 125a630..12e2d43 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -143,7 +143,7 @@ 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_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/to_fix_tmp.bzl\". (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: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)"
@@ -241,7 +241,7 @@ cat > golden/json_report_golden <<EOF
                     },
                     "category": "bzl-visibility",
                     "actionable": true,
-                    "message": "Module \"//foo/bar/internal/baz:module.bzl\" can only be loaded from files located inside \"//foo/bar\", not from \"//test_dir/json\".",
+                    "message": "Module \"//foo/bar/internal/baz:module.bzl\" can only be loaded from files located inside \"//foo/bar\", not from \"//test_dir/json/to_fix.bzl\".",
                     "url": "https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#bzl-visibility"
                 },
                 {
@@ -393,7 +393,7 @@ load(":nonexistent.bzl", "foo2", "bar2")
 EOF
 
 cat > report_golden <<EOF
-BUILD:1: deprecated-function: The function "bar" defined in "/lib.bzl" is deprecated. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#deprecated-function)
+BUILD:1: deprecated-function: The function "bar" defined in "//lib.bzl" is deprecated. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#deprecated-function)
 EOF
 
 $buildifier --lint=warn --warnings=deprecated-function BUILD 2> report || ret=$?
diff --git a/buildifier/utils/utils_test.go b/buildifier/utils/utils_test.go
index d61c516..e1a9091 100644
--- a/buildifier/utils/utils_test.go
+++ b/buildifier/utils/utils_test.go
@@ -121,14 +121,17 @@ func TestIsStarlarkFile(t *testing.T) {
 	}
 }
 
-func checkSplitFilePathOutput(t *testing.T, name, filename, expectedWorkspaceRoot, expectedPkg string) {
-	workspaceRoot, pkg := SplitFilePath(filename)
+func checkSplitFilePathOutput(t *testing.T, name, filename, expectedWorkspaceRoot, expectedPkg, expectedLabel string) {
+	workspaceRoot, pkg, label := SplitFilePath(filename)
 	if workspaceRoot != expectedWorkspaceRoot {
 		t.Errorf("%s: expected the workspace root to be %q, was %q instead", name, expectedWorkspaceRoot, workspaceRoot)
 	}
 	if pkg != expectedPkg {
 		t.Errorf("%s: expected the package name to be %q, was %q instead", name, expectedPkg, pkg)
 	}
+	if label != expectedLabel {
+		t.Errorf("%s: expected the label to be %q, was %q instead", name, expectedLabel, label)
+	}
 }
 
 func TestSplitFilePath(t *testing.T) {
@@ -138,22 +141,42 @@ func TestSplitFilePath(t *testing.T) {
 	}
 	defer os.RemoveAll(dir)
 
+	if err := os.MkdirAll(filepath.Join(dir, "path", "to", "package"), os.ModePerm); err != nil {
+		t.Error(err)
+	}
+
 	filename := filepath.Join(dir, "path", "to", "package", "file.bzl")
-	checkSplitFilePathOutput(t, "No WORKSPACE file", filename, "", "")
+	checkSplitFilePathOutput(t, "No WORKSPACE file", filename, "", "", "")
 
 	// Create a WORKSPACE file and try again (dir/WORKSPACE)
 	if err := ioutil.WriteFile(filepath.Join(dir, "WORKSPACE"), []byte{}, os.ModePerm); 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, "")
+	checkSplitFilePathOutput(t, "WORKSPACE file exists", filename, dir, "", "path/to/package/file.bzl")
+	checkSplitFilePathOutput(t, "WORKSPACE file exists, empty package", filepath.Join(dir, "file.bzl"), dir, "", "file.bzl")
+
+	// Add a BUILD file
+	buildPath := filepath.Join(dir, "path", "to", "BUILD")
+	if err := ioutil.WriteFile(buildPath, []byte{}, os.ModePerm); err != nil {
+		t.Error(err)
+	}
+	checkSplitFilePathOutput(t, "WORKSPACE and BUILD files exists 1", buildPath, dir, "path/to", "BUILD")
+	checkSplitFilePathOutput(t, "WORKSPACE and BUILD files exists 2", filename, dir, "path/to", "package/file.bzl")
+
+	// Add a subpackage BUILD file
+	subBuildPath := filepath.Join(dir, "path", "to", "package", "BUILD.bazel")
+	if err := ioutil.WriteFile(subBuildPath, []byte{}, os.ModePerm); err != nil {
+		t.Error(err)
+	}
+	checkSplitFilePathOutput(t, "WORKSPACE and two BUILD files exists 1", subBuildPath, dir, "path/to/package", "BUILD.bazel")
+	checkSplitFilePathOutput(t, "WORKSPACE and two BUILD files exists 2", filename, dir, "path/to/package", "file.bzl")
 
 	// 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, "")
+	checkSplitFilePathOutput(t, "WORKSPACE file exists", filename, dir, "path/to/package", "file.bzl")
+	checkSplitFilePathOutput(t, "WORKSPACE file exists, empty package", filepath.Join(dir, "file.bzl"), dir, "", "file.bzl")
 
 	// Create another WORKSPACE file and try again (dir/path/WORKSPACE)
 	newRoot := filepath.Join(dir, "path")
@@ -163,5 +186,5 @@ func TestSplitFilePath(t *testing.T) {
 	if err := ioutil.WriteFile(filepath.Join(newRoot, "WORKSPACE"), []byte{}, os.ModePerm); err != nil {
 		t.Error(err)
 	}
-	checkSplitFilePathOutput(t, "Two WORKSPACE files exist", filename, newRoot, "to/package")
+	checkSplitFilePathOutput(t, "Two WORKSPACE files exist", filename, newRoot, "to/package", "file.bzl")
 }
diff --git a/warn/warn_deprecated_test.go b/warn/warn_deprecated_test.go
index de762f8..268a20c 100644
--- a/warn/warn_deprecated_test.go
+++ b/warn/warn_deprecated_test.go
@@ -35,8 +35,8 @@ load(":invalid.bzl", "foo", "bar", "baz")
 load(":nonexistent.bzl", "foo", "bar", "baz")
 `,
 		[]string{
-			`1: The function "bar" defined in "test/package/foo.bzl" is deprecated.`,
-			`2: The function "bar" defined in "test/package/foo.bzl" is deprecated.`,
+			`1: The function "bar" defined in "//test/package/foo.bzl" is deprecated.`,
+			`2: The function "bar" defined in "//test/package/foo.bzl" is deprecated.`,
 		},
 		scopeEverywhere)
 }
diff --git a/warn/warn_macro_test.go b/warn/warn_macro_test.go
index 734f427..ca944e9 100644
--- a/warn/warn_macro_test.go
+++ b/warn/warn_macro_test.go
@@ -152,7 +152,7 @@ It is considered a macro because it calls a rule or another macro "my_rule" on l
 
 func TestUnnamedMacroWithReader(t *testing.T) {
 	defer setUpFileReader(map[string]string{
-		"test/package/foo.bzl": `
+		"test/package/subdir1/foo.bzl": `
 def foo():
   native.foo_binary()
 
@@ -161,8 +161,8 @@ def bar():
 
 my_rule = rule()
 `,
-		"test/package/baz.bzl": `
-load(":foo.bzl", "bar", your_rule = "my_rule")
+		"test/package/subdir2/baz.bzl": `
+load(":subdir1/foo.bzl", "bar", your_rule = "my_rule")
 load("//does/not:exist.bzl", "something")
 
 def baz():
@@ -178,8 +178,8 @@ def f():
 	})()
 
 	checkFindings(t, "unnamed-macro", `
-load("//test/package/foo.bzl", abc = "bar")
-load(":baz.bzl", "baz", "qux", "f")
+load("//test/package:subdir1/foo.bzl", abc = "bar")
+load(":subdir2/baz.bzl", "baz", "qux", "f")
 
 def macro1(surname):
   abc()
@@ -287,10 +287,10 @@ func TestUnnamedMacroLoadedFiles(t *testing.T) {
 	})()
 
 	checkFindings(t, "unnamed-macro", `
-load("//a.bzl", "a")
-load("//b.bzl", "b")
-load("//c.bzl", "c")
-load("//d.bzl", "d")
+load("//:a.bzl", "a")
+load("//:b.bzl", "b")
+load("//:c.bzl", "c")
+load("//:d.bzl", "d")
 
 def macro1():
   a()  # has to load a.bzl to analyze
@@ -330,8 +330,6 @@ It is considered a macro because it calls a rule or another macro "r" on line 19
 }
 
 func TestUnnamedMacroAliases(t *testing.T) {
-	// Test that not necessary files are not loaded
-
 	defer setUpFileReader(map[string]string{
 		"test/package/foo.bzl": `
 load(":bar.bzl", _bar = "bar")
diff --git a/warn/warn_test.go b/warn/warn_test.go
index ac0f53a..64d244e 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -32,7 +32,7 @@ func setUpFileReader(data map[string]string) (cleanup func()) {
 		if contents, ok := data[filename]; ok {
 			return []byte(contents), nil
 		}
-		return nil, fmt.Errorf("File not found")
+		return nil, fmt.Errorf("file not found")
 	}
 	testFileReader = NewFileReader(readFile)
 	fileReaderRequests = nil
@@ -47,25 +47,32 @@ func setUpFileReader(data map[string]string) (cleanup func()) {
 func getFilename(fileType build.FileType) string {
 	switch fileType {
 	case build.TypeBuild:
-		return "test/package/BUILD"
+		return "BUILD"
 	case build.TypeWorkspace:
-		return "test/package/WORKSPACE"
+		return "WORKSPACE"
 	case build.TypeBzl:
-		return "test/package/test_file.bzl"
+		return "test_file.bzl"
 	default:
-		return "test/package/test_file.strlrk"
+		return "test_file.strlrk"
 	}
 }
 
-func getFindings(category, input string, fileType build.FileType) []*Finding {
+func getFileForTest(input string, fileType build.FileType) *build.File {
 	input = strings.TrimLeft(input, "\n")
-	buildFile, err := build.Parse(getFilename(fileType), []byte(input))
+	filename := getFilename(fileType)
+	file, err := build.Parse("test/package/"+filename, []byte(input))
 	if err != nil {
 		panic(fmt.Sprintf("%v", err))
 	}
-	buildFile.Pkg = "test/package"
-	buildFile.WorkspaceRoot = "/home/users/foo/bar"
-	return FileWarnings(buildFile, []string{category}, nil, ModeWarn, testFileReader)
+	file.Pkg = "test/package"
+	file.Label = filename
+	file.WorkspaceRoot = "/home/users/foo/bar"
+	return file
+}
+
+func getFindings(category, input string, fileType build.FileType) []*Finding {
+	file := getFileForTest(input, fileType)
+	return FileWarnings(file, []string{category}, nil, ModeWarn, testFileReader)
 }
 
 func compareFindings(t *testing.T, category, input string, expected []string, scope, fileType build.FileType) {
@@ -104,17 +111,11 @@ func checkFix(t *testing.T, category, input, expected string, scope, fileType bu
 		expected = input
 	}
 
-	buildFile, err := build.Parse(getFilename(fileType), []byte(input))
-	if err != nil {
-		panic(fmt.Sprintf("%v", err))
-	}
-	goldenFile, err := build.Parse(getFilename(fileType), []byte(expected))
-	if err != nil {
-		panic(fmt.Sprintf("%v", err))
-	}
+	file := getFileForTest(input, fileType)
+	goldenFile := getFileForTest(expected, fileType)
 
-	FixWarnings(buildFile, []string{category}, false, testFileReader)
-	have := build.Format(buildFile)
+	FixWarnings(file, []string{category}, false, testFileReader)
+	have := build.Format(file)
 	want := build.Format(goldenFile)
 	if !bytes.Equal(have, want) {
 		t.Errorf("fixed a test (type %s) incorrectly:\ninput:\n%s\ndiff (-expected, +ours)\n",
@@ -126,15 +127,12 @@ func checkFix(t *testing.T, category, input, expected string, scope, fileType bu
 // checkFix makes sure that the file contents don't change if a fix is not requested
 // (i.e. the warning functions have no side effects modifying the AST)
 func checkNoFix(t *testing.T, category, input string, fileType build.FileType) {
-	buildFile, err := build.Parse(getFilename(fileType), []byte(input))
-	if err != nil {
-		panic(fmt.Sprintf("%v", err))
-	}
-	formatted := build.Format(buildFile)
+	file := getFileForTest(input, fileType)
+	formatted := build.Format(file)
 
 	// No fixes expected
-	FileWarnings(buildFile, []string{category}, nil, ModeWarn, testFileReader)
-	fixed := build.FormatWithoutRewriting(buildFile)
+	FileWarnings(file, []string{category}, nil, ModeWarn, testFileReader)
+	fixed := build.FormatWithoutRewriting(file)
 
 	if !bytes.Equal(formatted, fixed) {
 		t.Errorf("Modified a file (type %s) while getting warnings:\ninput:\n%s\ndiff (-before, +after)\n",
diff --git a/warn/warn_visibility_test.go b/warn/warn_visibility_test.go
index b40c607..3d301de 100644
--- a/warn/warn_visibility_test.go
+++ b/warn/warn_visibility_test.go
@@ -13,8 +13,8 @@ bar()
 baz()
 `,
 		[]string{
-			`:1: Module "//foo/bar/internal/baz:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package".`,
-			`:2: Module "//foo/bar/private/baz:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package".`,
+			`:1: Module "//foo/bar/internal/baz:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package/`,
+			`:2: Module "//foo/bar/private/baz:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package/`,
 		},
 		scopeEverywhere)
 
@@ -28,8 +28,8 @@ bar()
 baz()
 `,
 		[]string{
-			`:1: Module "//foo/bar/internal:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package".`,
-			`:2: Module "//foo/bar/private:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package".`,
+			`:1: Module "//foo/bar/internal:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package/`,
+			`:2: Module "//foo/bar/private:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package/`,
 		},
 		scopeEverywhere)
 
@@ -46,9 +46,9 @@ baz()
 qux()
 `,
 		[]string{
-			`:1: Module "@repo//foo/bar/internal:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package".`,
-			`:2: Module "@repo//foo/bar/private:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package".`,
-			`:4: Module "@repo/internal:module.bzl" can only be loaded from files located inside "@repo", not from "//test/package".`,
+			`:1: Module "@repo//foo/bar/internal:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package/`,
+			`:2: Module "@repo//foo/bar/private:module.bzl" can only be loaded from files located inside "//foo/bar", not from "//test/package/`,
+			`:4: Module "@repo/internal:module.bzl" can only be loaded from files located inside "@repo", not from "//test/package/`,
 		},
 		scopeEverywhere)