Repository: bazelbuild/buildtools
Pull request #450 (2018-11-22 17:31:00)

Support WORKSPACE files
Currently, a boolean flag is used to distinguish between .bzl and BUILD files, and WORKSPACE have been treated like BUILD files. However there are differences, certain refactoring tools (such as moving all load statements on the top) shouldn't be apllied to WORKSPACE files, but can be applied to BUILD files.

This PR creates a new type FileType to represent multiple file types (currently there are three types available).

Also the `--type` flag now accepts a new value (`workspace`).


Patch:
diff --git a/build/lex.go b/build/lex.go
index f698e26..c64d1da 100644
--- a/build/lex.go
+++ b/build/lex.go
@@ -26,9 +26,24 @@ import (
 	"unicode/utf8"
 )
 
-// BuildFilenames is a collection of filenames in lowercase that are treated as BUILD files.
-var BuildFilenames = map[string]bool{
-	"stdin": true,
+// FileType represents a type of a file (default (for .bzl files), BUILD, or WORKSPACE).
+// Certain formatting or refactoring rules can be applied to several file types, so they support
+// bitwise operations: `type1 | type2` can represent a scope (e.g. BUILD and WORKSPACE files) and
+// `scope & fileType` can be used to check whether a file type belongs to a scope.
+type FileType int
+
+const (
+	TypeDefault FileType = 1 << iota
+	TypeBuild
+	TypeWorkspace
+)
+
+func (t FileType) String() string {
+	return [...]string{
+		".bzl",
+		"BUILD",
+		"WORKSPACE",
+	}[t]
 }
 
 // ParseBuild parses a file, marks it as a BUILD file and returns the corresponding parse tree.
@@ -38,7 +53,19 @@ func ParseBuild(filename string, data []byte) (*File, error) {
 	in := newInput(filename, data)
 	f, err := in.parse()
 	if f != nil {
-		f.Build = true
+		f.Type = TypeBuild
+	}
+	return f, err
+}
+
+// ParseBuild parses a file, marks it as a WORKSPACE file and returns the corresponding parse tree.
+//
+// The filename is used only for generating error messages.
+func ParseWorkspace(filename string, data []byte) (*File, error) {
+	in := newInput(filename, data)
+	f, err := in.parse()
+	if f != nil {
+		f.Type = TypeWorkspace
 	}
 	return f, err
 }
@@ -50,32 +77,41 @@ func ParseDefault(filename string, data []byte) (*File, error) {
 	in := newInput(filename, data)
 	f, err := in.parse()
 	if f != nil {
-		f.Build = false
+		f.Type = TypeDefault
 	}
 	return f, err
 }
 
-func isBuildFilename(basename string) bool {
+func getFileType(basename string) FileType {
 	basename = strings.ToLower(basename)
-	if isBuild, ok := BuildFilenames[basename]; ok {
-		return isBuild
+	if basename == "stdin" {
+		return TypeBuild // For compatibility
 	}
 	ext := filepath.Ext(basename)
 	if ext == ".bzl" || ext == ".sky" {
-		return false
+		return TypeDefault
 	}
 	base := basename[:len(basename)-len(ext)]
-	return ext == ".build" || ext == ".workspace" || base == "build" || base == "workspace"
+	switch {
+	case ext == ".build" || base == "build":
+		return TypeBuild
+	case ext == ".workspace" || base == "workspace":
+		return TypeWorkspace
+	}
+	return TypeDefault
 }
 
 // Parse parses the input data and returns the corresponding parse tree.
 //
-// Uses the filename to detect the formatting type (either build or default) and calls
-// either ParseBuild or to ParseDefault correspondingly.
+// Uses the filename to detect the formatting type (build, workspace, or default) and calls
+// ParseBuild, ParseWorkspace, or ParseDefault correspondingly.
 func Parse(filename string, data []byte) (*File, error) {
 	basename := filepath.Base(filename)
-	if isBuildFilename(basename) {
+	switch getFileType(basename) {
+	case TypeBuild:
 		return ParseBuild(filename, data)
+	case TypeWorkspace:
+		return ParseWorkspace(filename, data)
 	}
 	return ParseDefault(filename, data)
 }
diff --git a/build/print.go b/build/print.go
index 131dc78..49a0067 100644
--- a/build/print.go
+++ b/build/print.go
@@ -31,19 +31,19 @@ const (
 
 // Format returns the formatted form of the given BUILD or bzl file.
 func Format(f *File) []byte {
-	pr := &printer{buildMode: f.Build}
+	pr := &printer{fileType: f.Type}
 	pr.file(f)
 	return pr.Bytes()
 }
 
 // FormatString returns the string form of the given expression.
 func FormatString(x Expr) string {
-	buildMode := true // for compatibility
+	fileType := TypeBuild // for compatibility
 	if file, ok := x.(*File); ok {
-		buildMode = file.Build
+		fileType = file.Type
 	}
 
-	pr := &printer{buildMode: buildMode}
+	pr := &printer{fileType: fileType}
 	switch x := x.(type) {
 	case *File:
 		pr.file(x)
@@ -55,7 +55,7 @@ func FormatString(x Expr) string {
 
 // A printer collects the state during printing of a file or expression.
 type printer struct {
-	buildMode    bool      // whether should be printed in a build mode.
+	fileType     FileType  // different rules can be applied to different file types.
 	bytes.Buffer           // output buffer
 	comment      []Comment // pending end-of-line comments
 	margin       int       // left margin (indent), a number of spaces
@@ -226,8 +226,8 @@ func (p *printer) compactStmt(s1, s2 Expr) bool {
 	} else if isCommentBlock(s1) || isCommentBlock(s2) {
 		// Standalone comment blocks shouldn't be attached to other statements
 		return false
-	} else if p.buildMode && p.level == 0 {
-		// Top-level statements in a BUILD file
+	} else if p.fileType != TypeDefault && p.level == 0 {
+		// Top-level statements in a BUILD or WORKSPACE file
 		return false
 	} else if isFunctionDefinition(s1) || isFunctionDefinition(s2) {
 		// On of the statements is a function definition
@@ -697,7 +697,7 @@ func (p *printer) useCompactMode(start *Position, list *[]Expr, end *End, mode s
 	// In the Default printing mode try to keep the original printing style.
 	// Non-top-level statements and lists of arguments of a function definition
 	// should also keep the original style regardless of the mode.
-	if p.level != 0 || !p.buildMode || mode == modeDef {
+	if p.level != 0 || p.fileType == TypeDefault || mode == modeDef {
 		// If every element (including the brackets) ends on the same line where the next element starts,
 		// use the compact mode, otherwise use multiline mode.
 		// If an node's line number is 0, it means it doesn't appear in the original file,
diff --git a/build/rewrite.go b/build/rewrite.go
index ff61852..43a0426 100644
--- a/build/rewrite.go
+++ b/build/rewrite.go
@@ -62,9 +62,7 @@ func Rewrite(f *File, info *RewriteInfo) {
 
 	for _, r := range rewrites {
 		if !disabled(r.name) {
-			if r.scope == scopeBoth ||
-				(f.Build && r.scope == scopeBuild) ||
-				(!f.Build && r.scope == scopeDefault) {
+			if f.Type & r.scope != 0 {
 				r.fn(f, info)
 			}
 		}
@@ -115,9 +113,9 @@ func (info *RewriteInfo) String() string {
 // Each rewrite function can be either applied for BUILD files, other files (such as .bzl),
 // or all files.
 const (
-	scopeDefault = iota
-	scopeBuild
-	scopeBoth
+	scopeDefault = TypeDefault
+	scopeBuild = TypeBuild | TypeWorkspace // BUILD and WORKSPACE files
+	scopeBoth = scopeDefault | scopeBuild
 )
 
 // rewrites is the list of all Buildifier rewrites, in the order in which they are applied.
@@ -126,7 +124,7 @@ const (
 var rewrites = []struct {
 	name  string
 	fn    func(*File, *RewriteInfo)
-	scope int
+	scope FileType
 }{
 	{"callsort", sortCallArgs, scopeBuild},
 	{"label", fixLabels, scopeBuild},
@@ -460,7 +458,7 @@ func sortStringLists(f *File, info *RewriteInfo) {
 					continue
 				}
 				context := rule + "." + key.Name
-				if !tables.IsSortableListArg[key.Name] || tables.SortableBlacklist[context] || !f.Build {
+				if !tables.IsSortableListArg[key.Name] || tables.SortableBlacklist[context] || f.Type == TypeDefault {
 					continue
 				}
 				if disabled("unsafesort") && !tables.SortableWhitelist[context] && !allowedSort(context) {
diff --git a/build/syntax.go b/build/syntax.go
index 1c007e0..06de35f 100644
--- a/build/syntax.go
+++ b/build/syntax.go
@@ -81,8 +81,8 @@ func (c *Comments) Comment() *Comments {
 
 // A File represents an entire BUILD file.
 type File struct {
-	Path  string // file path, relative to workspace directory
-	Build bool
+	Path string // file path, relative to workspace directory
+	Type FileType
 	Comments
 	Stmt []Expr
 }
diff --git a/buildifier/buildifier.go b/buildifier/buildifier.go
index d17d8b3..91ecc3a 100644
--- a/buildifier/buildifier.go
+++ b/buildifier/buildifier.go
@@ -51,7 +51,7 @@ var (
 	tablesPath    = flag.String("tables", "", "path to JSON file with custom table definitions which will replace the built-in tables")
 	addTablesPath = flag.String("add_tables", "", "path to JSON file with custom table definitions which will be merged with the built-in tables")
 	version       = flag.Bool("version", false, "Print the version of buildifier")
-	inputType     = flag.String("type", "auto", "Input file type: build (for BUILD files), bzl (for .bzl files) or auto (default, based on the filename)")
+	inputType     = flag.String("type", "auto", "Input file type: build (for BUILD files), bzl (for .bzl files), workspace (for WORKSPACE files), or auto (default, based on the filename)")
 
 	// Debug flags passed through to rewrite.go
 	allowSort = stringList("allowsort", "additional sort contexts to treat as safe")
@@ -110,11 +110,11 @@ func main() {
 
 	// Check input type.
 	switch *inputType {
-	case "build", "bzl", "auto":
+	case "build", "bzl", "workspace", "auto":
 		// ok
 
 	default:
-		fmt.Fprintf(os.Stderr, "buildifier: unrecognized input type %s; valid types are build, bzl, auto\n", *inputType)
+		fmt.Fprintf(os.Stderr, "buildifier: unrecognized input type %s; valid types are build, bzl, workspace, auto\n", *inputType)
 		os.Exit(2)
 	}
 
@@ -419,6 +419,8 @@ func getParser(inputType string) func(filename string, data []byte) (*build.File
 		return build.ParseBuild
 	case "auto":
 		return build.Parse
+	case "workspace":
+		return build.ParseWorkspace
 	default:
 		return build.ParseDefault
 	}
diff --git a/convertast/convert_ast.go b/convertast/convert_ast.go
index 2a0898a..74e44d1 100644
--- a/convertast/convert_ast.go
+++ b/convertast/convert_ast.go
@@ -20,6 +20,7 @@ func ConvFile(f *syntax.File) *build.File {
 	}
 
 	return &build.File{
+		Type:     build.TypeDefault,
 		Stmt:     stmts,
 		Comments: convComments(f.Comments()),
 	}
diff --git a/edit/edit.go b/edit/edit.go
index 995af3b..23439d6 100644
--- a/edit/edit.go
+++ b/edit/edit.go
@@ -230,7 +230,7 @@ func RemoveEmptyPackage(f *build.File) *build.File {
 		}
 		all = append(all, stmt)
 	}
-	return &build.File{Path: f.Path, Comments: f.Comments, Stmt: all, Build: true}
+	return &build.File{Path: f.Path, Comments: f.Comments, Stmt: all, Type: build.TypeBuild}
 }
 
 // InsertAfter inserts an expression after index i.
@@ -339,7 +339,7 @@ func DeleteRule(f *build.File, rule *build.Rule) *build.File {
 		}
 		all = append(all, stmt)
 	}
-	return &build.File{Path: f.Path, Comments: f.Comments, Stmt: all, Build: true}
+	return &build.File{Path: f.Path, Comments: f.Comments, Stmt: all, Type: build.TypeBuild}
 }
 
 // DeleteRuleByName returns the AST without the rules that have the
@@ -357,7 +357,7 @@ func DeleteRuleByName(f *build.File, name string) *build.File {
 			all = append(all, stmt)
 		}
 	}
-	return &build.File{Path: f.Path, Comments: f.Comments, Stmt: all, Build: true}
+	return &build.File{Path: f.Path, Comments: f.Comments, Stmt: all, Type: build.TypeBuild}
 }
 
 // DeleteRuleByKind removes the rules of the specified kind from the AST.
@@ -375,7 +375,7 @@ func DeleteRuleByKind(f *build.File, kind string) *build.File {
 			all = append(all, stmt)
 		}
 	}
-	return &build.File{Path: f.Path, Comments: f.Comments, Stmt: all, Build: true}
+	return &build.File{Path: f.Path, Comments: f.Comments, Stmt: all, Type: build.TypeBuild}
 }
 
 // AllLists returns all the lists concatenated in an expression.
diff --git a/warn/warn.go b/warn/warn.go
index dd8762d..d060a28 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -235,8 +235,8 @@ func unusedVariableWarning(f *build.File, fix bool) []*Finding {
 
 func duplicatedNameWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
-	if !f.Build {
-		// Not applicable to non-BUILD files.
+	if f.Type == build.TypeDefault {
+		// Not applicable to .bzl files.
 		return findings
 	}
 	names := make(map[string]int) // map from name to line number
@@ -293,7 +293,7 @@ func packageOnTopWarning(f *build.File, fix bool) []*Finding {
 func loadOnTopWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
 
-	if f.Build {
+	if f.Type != build.TypeDefault {
 		// Not applicable for BUILD or WORKSPACE files
 		return findings
 	}
@@ -452,8 +452,8 @@ func noEffectStatementsCheck(f *build.File, body []build.Expr, isTopLevel, isFun
 // unusedVariableCheck checks for unused variables inside a given node `stmt` (either *build.File or
 // *build.DefStmt) and reports unused and already defined variables.
 func unusedVariableCheck(f *build.File, stmts []build.Expr, findings []*Finding) []*Finding {
-	if !f.Build {
-		// Not applicable to non-BUILD files, unused symbols may be loaded and used in other files.
+	if f.Type == build.TypeDefault {
+		// Not applicable to .bzl files, unused symbols may be loaded and used in other files.
 		return findings
 	}
 	usedSymbols := make(map[string]bool)
@@ -769,7 +769,7 @@ func argumentsOrderWarning(f *build.File, fix bool) []*Finding {
 func nativeInBuildFilesWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
 
-	if !f.Build {
+	if f.Type != build.TypeBuild {
 		return findings
 	}
 
@@ -804,7 +804,7 @@ func nativeInBuildFilesWarning(f *build.File, fix bool) []*Finding {
 func nativePackageWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
 
-	if f.Build {
+	if f.Type != build.TypeDefault {
 		return findings
 	}
 
@@ -941,7 +941,7 @@ func FileWarnings(f *build.File, pkg string, enabledWarnings []string, fix bool)
 			if fn == nil {
 				log.Fatalf("unexpected warning %q", warn)
 			}
-			if !f.Build {
+			if f.Type == build.TypeDefault {
 				continue
 			}
 			for _, stmt := range f.Stmt {


Test patch:
diff --git a/build/lex_test.go b/build/lex_test.go
index 801bbf6..ce35af3 100644
--- a/build/lex_test.go
+++ b/build/lex_test.go
@@ -21,26 +21,28 @@ import (
 )
 
 func TestIsBuildFilename(t *testing.T) {
-	cases := map[string]bool{
-		"BUILD":           true,
-		"build":           true,
-		"bUIld":           true,
-		"BUILD.bazel":     true,
-		"build.bzl":       false,
-		"build.sky":       false,
-		"WORKSPACE":       true,
-		"external.BUILD":  true,
-		"BUILD.external":  true,
-		"aBUILD":          false,
-		"thing.sky":       false,
-		"my.WORKSPACE":    true,
-		"thing.bzl":       false,
-		"workspace.bazel": true,
+	cases := map[string]FileType{
+		"BUILD":           TypeBuild,
+		"build":           TypeBuild,
+		"bUIld":           TypeBuild,
+		"BUILD.bazel":     TypeBuild,
+		"build.bzl":       TypeDefault,
+		"build.sky":       TypeDefault,
+		"WORKSPACE":       TypeWorkspace,
+		"external.BUILD":  TypeBuild,
+		"BUILD.external":  TypeBuild,
+		"aBUILD":          TypeDefault,
+		"thing.sky":       TypeDefault,
+		"my.WORKSPACE":    TypeWorkspace,
+		"thing.bzl":       TypeDefault,
+		"workspace.bazel": TypeWorkspace,
+		"workspace.bzl":   TypeDefault,
+		"foo.bar":         TypeDefault,
 	}
-	for name, isBuild := range cases {
-		res := isBuildFilename(name)
-		if res != isBuild {
-			t.Errorf("isBuildFilename(%q) should be %v but was %v", name, isBuild, res)
+	for name, fileType := range cases {
+		res := getFileType(name)
+		if res != fileType {
+			t.Errorf("isBuildFilename(%q) should be %v but was %v", name, fileType, res)
 		}
 	}
 }
diff --git a/build/parse_test.go b/build/parse_test.go
index 30e9024..de38fe2 100644
--- a/build/parse_test.go
+++ b/build/parse_test.go
@@ -96,8 +96,8 @@ var parseTests = []struct {
 )
 `,
 		out: &File{
-			Path:  "BUILD",
-			Build: true,
+			Path: "BUILD",
+			Type: TypeBuild,
 			Stmt: []Expr{
 				&CallExpr{
 					X: &Ident{
@@ -130,8 +130,8 @@ var parseTests = []struct {
 	{
 		in: `foo.bar.baz(name = "x")`,
 		out: &File{
-			Path:  "test",
-			Build: false,
+			Path: "test",
+			Type: TypeDefault,
 			Stmt: []Expr{
 				&CallExpr{
 					X: &DotExpr{
@@ -192,8 +192,8 @@ var parseTests = []struct {
 		in: `load(":foo.bzl", "foo", """bar""", baz="foo", foo="""baz""")
 `,
 		out: &File{
-			Path:  "BUILD",
-			Build: true,
+			Path: "BUILD",
+			Type: TypeBuild,
 			Stmt: []Expr{
 				&LoadStmt{
 					Load: Position{1, 1, 0},
diff --git a/warn/warn_test.go b/warn/warn_test.go
index 2457944..833bfbd 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -10,37 +10,39 @@ import (
 	"github.com/bazelbuild/buildtools/testutils"
 )
 
-type testScope int
-
 const (
-	scopeBuild testScope = 1 << iota
-	scopeBzl
+	scopeBuild      = build.TypeBuild
+	scopeBzl        = build.TypeDefault
 	scopeEverywhere = scopeBuild | scopeBzl
 )
 
-func getFilename(isBuildFile bool) string {
-	if isBuildFile {
+func getFilename(fileType build.FileType) string {
+	switch fileType {
+	case build.TypeBuild:
 		return "BUILD"
+	case build.TypeWorkspace:
+		return "WORKSPACE"
+	default:
+		return "test_file.bzl"
 	}
-	return "test_file.bzl"
 }
 
-func getFindings(category, input string, isBuildFile bool) []*Finding {
+func getFindings(category, input string, fileType build.FileType) []*Finding {
 	input = strings.TrimLeft(input, "\n")
-	buildFile, err := build.Parse(getFilename(isBuildFile), []byte(input))
+	buildFile, err := build.Parse(getFilename(fileType), []byte(input))
 	if err != nil {
 		panic(fmt.Sprintf("%v", err))
 	}
 	return FileWarnings(buildFile, "the_package", []string{category}, false)
 }
 
-func compareFindings(t *testing.T, category, input string, expected []string, scope testScope, isBuildFile bool) {
+func compareFindings(t *testing.T, category, input string, expected []string, scope, fileType build.FileType) {
 	// If scope doesn't match the file type, no warnings are expected
-	if (scope&scopeBuild == 0 && isBuildFile) || (scope&scopeBzl == 0 && !isBuildFile) {
+	if scope&fileType == 0 {
 		expected = []string{}
 	}
 
-	findings := getFindings(category, input, isBuildFile)
+	findings := getFindings(category, input, fileType)
 	// We ensure that there is the expected number of warnings.
 	// At the moment, we check only the line numbers.
 	if len(expected) != len(findings) {
@@ -63,17 +65,17 @@ func compareFindings(t *testing.T, category, input string, expected []string, sc
 	}
 }
 
-func checkFix(t *testing.T, category, input, expected string, scope testScope, isBuildFile bool) {
+func checkFix(t *testing.T, category, input, expected string, scope, fileType build.FileType) {
 	// If scope doesn't match the file type, no changes are expected
-	if (scope&scopeBuild == 0 && isBuildFile) || (scope&scopeBzl == 0 && !isBuildFile) {
+	if scope&fileType == 0 {
 		expected = input
 	}
 
-	buildFile, err := build.Parse(getFilename(isBuildFile), []byte(input))
+	buildFile, err := build.Parse(getFilename(fileType), []byte(input))
 	if err != nil {
 		panic(fmt.Sprintf("%v", err))
 	}
-	goldenFile, err := build.Parse(getFilename(isBuildFile), []byte(expected))
+	goldenFile, err := build.Parse(getFilename(fileType), []byte(expected))
 	if err != nil {
 		panic(fmt.Sprintf("%v", err))
 	}
@@ -82,29 +84,25 @@ func checkFix(t *testing.T, category, input, expected string, scope testScope, i
 	have := build.Format(buildFile)
 	want := build.Format(goldenFile)
 	if !bytes.Equal(have, want) {
-		fileType := "bzl"
-		if isBuildFile {
-			fileType = "BUILD"
-		}
 		t.Errorf("fixed a test (type %s) incorrectly:\ninput:\n%s\ndiff (-expected, +ours)\n",
 			fileType, input)
 		testutils.Tdiff(t, want, have)
 	}
 }
 
-func checkFindings(t *testing.T, category, input string, expected []string, scope testScope) {
+func checkFindings(t *testing.T, category, input string, expected []string, scope build.FileType) {
 	// The same as checkFindingsAndFix but ensure that fixes don't change the file (except for formatting)
 	checkFindingsAndFix(t, category, input, input, expected, scope)
 }
 
-func checkFindingsAndFix(t *testing.T, category, input, output string, expected []string, scope testScope) {
+func checkFindingsAndFix(t *testing.T, category, input, output string, expected []string, scope build.FileType) {
 	// BUILD file
-	compareFindings(t, category, input, expected, scope, true)
-	checkFix(t, category, input, output, scope, true)
+	compareFindings(t, category, input, expected, scope, build.TypeBuild)
+	checkFix(t, category, input, output, scope, build.TypeBuild)
 
 	// Bzl file
-	compareFindings(t, category, input, expected, scope, false)
-	checkFix(t, category, input, output, scope, false)
+	compareFindings(t, category, input, expected, scope, build.TypeDefault)
+	checkFix(t, category, input, output, scope, build.TypeDefault)
 }
 
 func TestNoEffect(t *testing.T) {