Repository: bazelbuild/buildtools
Pull request #815 (2020-04-09 18:03:04)

Refactor rewrites
Currently to format a file one should call two separate functions:

  * `build.Rewrite(file, &rewriteInfo)`: to modify the AST in-place (e.g. to sort the load arguments or to replace the division operator `/` with the integer division operator `//`). This function also takes an output argument of the type `*build.RewriteInfo` which is actually not useful (the diagnostics are being printed to the console but users are usually interested in whether the file has been reformatted or not, rather than how many division operators were fixed).
  * `build.Format(f)`: to actually convert the AST to bytes.

This is confusing for users who use buildifier as a library. If they don't know that they need to call `Rewrite` they may end up with incorrectly sorted load arguments if they add new loads.

The PR refactors the pipeline in the following way:

  * `Rewrite()` doesn't take the output argument anymore.
  * `Format()` rewrites and formats a file.
  *  In some rare cases (mostly in testing) it's required to format a file without rewriting (e.g. because some of the rewrites are not idempotent and it's required to format the same file twice). For such case a `FormatWithoutRewriting()` function is created (it's equal to the former `Format()`).


Patch:
diff --git a/build/print.go b/build/print.go
index 04964de..79868e7 100644
--- a/build/print.go
+++ b/build/print.go
@@ -29,13 +29,20 @@ const (
 	defIndentation    = 8 // Indentation of multiline function definitions
 )
 
-// Format returns the formatted form of the given BUILD or bzl file.
-func Format(f *File) []byte {
+// FormatWithoutRewriting returns the formatted form of the given Starlark file.
+// This function is mostly useful for tests only, please consider using `Format` instead.
+func FormatWithoutRewriting(f *File) []byte {
 	pr := &printer{fileType: f.Type}
 	pr.file(f)
 	return pr.Bytes()
 }
 
+// Format rewrites the file and returns the formatted form of it.
+func Format(f *File) []byte {
+	Rewrite(f)
+	return FormatWithoutRewriting(f)
+}
+
 // FormatString returns the string form of the given expression.
 func FormatString(x Expr) string {
 	fileType := TypeBuild // for compatibility
@@ -356,7 +363,7 @@ func (p *printer) expr(v Expr, outerPrec int) {
 	// However, even then we can't emit line comments since that would
 	// end the expression. This is only a concern if we have rewritten
 	// the parse tree. If comments were okay before this expression in
-	// the original input they're still okay now, in the absense of rewrites.
+	// the original input they're still okay now, in the absence of rewrites.
 	//
 	// TODO(bazel-team): Check whether it is valid to emit comments right now,
 	// and if not, insert them earlier in the output instead, at the most
@@ -514,7 +521,7 @@ func (p *printer) expr(v Expr, outerPrec int) {
 
 	case *BinaryExpr:
 		// Precedence: use the precedence of the operator.
-		// Since all binary expressions format left-to-right,
+		// Since all binary expressions FormatWithoutRewriting left-to-right,
 		// it is okay for the left side to reuse the same operator
 		// without parentheses, so we use prec for v.X.
 		// For the same reason, the right side cannot reuse the same
diff --git a/build/rewrite.go b/build/rewrite.go
index 2a81741..f7be55e 100644
--- a/build/rewrite.go
+++ b/build/rewrite.go
@@ -53,51 +53,16 @@ func allowedSort(name string) bool {
 }
 
 // Rewrite applies the high-level Buildifier rewrites to f, modifying it in place.
-// If info is non-nil, Rewrite updates it with information about the rewrite.
-func Rewrite(f *File, info *RewriteInfo) {
-	// Allocate an info so that helpers can assume it's there.
-	if info == nil {
-		info = new(RewriteInfo)
-	}
-
+func Rewrite(f *File) {
 	for _, r := range rewrites {
 		if !disabled(r.name) {
 			if f.Type&r.scope != 0 {
-				r.fn(f, info)
+				r.fn(f)
 			}
 		}
 	}
 }
 
-// RewriteInfo collects information about what Rewrite did.
-type RewriteInfo struct {
-	EditLabel        int      // number of label strings edited
-	NameCall         int      // number of calls with argument names added
-	SortCall         int      // number of call argument lists sorted
-	SortStringList   int      // number of string lists sorted
-	UnsafeSort       int      // number of unsafe string lists sorted
-	SortLoad         int      // number of load argument lists sorted
-	FormatDocstrings int      // number of reindented docstrings
-	ReorderArguments int      // number of reordered function call arguments
-	EditOctal        int      // number of edited octals
-	Log              []string // log entries - may change
-}
-
-// Stats returns a map with statistics about applied rewrites
-func (info *RewriteInfo) Stats() map[string]int {
-	return map[string]int{
-		"label":            info.EditLabel,
-		"callname":         info.NameCall,
-		"callsort":         info.SortCall,
-		"listsort":         info.SortStringList,
-		"unsafesort":       info.UnsafeSort,
-		"sortload":         info.SortLoad,
-		"formatdocstrings": info.FormatDocstrings,
-		"reorderarguments": info.ReorderArguments,
-		"editoctal":        info.EditOctal,
-	}
-}
-
 // Each rewrite function can be either applied for BUILD files, other files (such as .bzl),
 // or all files.
 const (
@@ -111,7 +76,7 @@ const (
 // before sorting lists of strings.
 var rewrites = []struct {
 	name  string
-	fn    func(*File, *RewriteInfo)
+	fn    func(*File)
 	scope FileType
 }{
 	{"callsort", sortCallArgs, scopeBuild},
@@ -176,7 +141,7 @@ func keepSorted(x Expr) bool {
 // "//third_party/m4:m4" into "//third_party/m4" as well as ones like
 // "@foo//:foo" into "@foo".
 //
-func fixLabels(f *File, info *RewriteInfo) {
+func fixLabels(f *File) {
 	joinLabel := func(p *Expr) {
 		add, ok := (*p).(*BinaryExpr)
 		if !ok || add.Op != "+" {
@@ -190,7 +155,6 @@ func fixLabels(f *File, info *RewriteInfo) {
 		if !ok || strings.Contains(str2.Value, " ") {
 			return
 		}
-		info.EditLabel++
 		str1.Value += str2.Value
 
 		// Deleting nodes add and str2.
@@ -220,11 +184,9 @@ func fixLabels(f *File, info *RewriteInfo) {
 		if !ok {
 			return
 		}
-		editPerformed := false
 
 		if tables.StripLabelLeadingSlashes && strings.HasPrefix(str.Value, "//") {
 			if filepath.Dir(f.Path) == "." || !strings.HasPrefix(str.Value, "//:") {
-				editPerformed = true
 				str.Value = str.Value[2:]
 			}
 		}
@@ -237,10 +199,8 @@ func fixLabels(f *File, info *RewriteInfo) {
 			}
 
 			if str.Value == thisPackage {
-				editPerformed = true
 				str.Value = ":" + path.Base(str.Value)
 			} else if strings.HasPrefix(str.Value, thisPackage+":") {
-				editPerformed = true
 				str.Value = str.Value[len(thisPackage):]
 			}
 		}
@@ -250,15 +210,10 @@ func fixLabels(f *File, info *RewriteInfo) {
 			return
 		}
 		if m[4] != "" && m[4] == m[5] { // e.g. //foo:foo
-			editPerformed = true
 			str.Value = m[1]
 		} else if m[3] != "" && m[4] == "" && m[3] == m[5] { // e.g. @foo//:foo
-			editPerformed = true
 			str.Value = "@" + m[3]
 		}
-		if editPerformed {
-			info.EditLabel++
-		}
 	}
 
 	Walk(f, func(v Expr, stk []Expr) {
@@ -319,7 +274,7 @@ func callName(call *CallExpr) string {
 }
 
 // sortCallArgs sorts lists of named arguments to a call.
-func sortCallArgs(f *File, info *RewriteInfo) {
+func sortCallArgs(f *File) {
 	Walk(f, func(v Expr, stk []Expr) {
 		call, ok := v.(*CallExpr)
 		if !ok {
@@ -350,7 +305,6 @@ func sortCallArgs(f *File, info *RewriteInfo) {
 		if sort.IsSorted(args) {
 			return
 		}
-		info.SortCall++
 		sort.Sort(args)
 		for i, x := range args {
 			call.List[start+i] = x.expr
@@ -420,7 +374,7 @@ func (x namedArgs) Less(i, j int) bool {
 }
 
 // sortStringLists sorts lists of string literals used as specific rule arguments.
-func sortStringLists(f *File, info *RewriteInfo) {
+func sortStringLists(f *File) {
 	Walk(f, func(v Expr, stk []Expr) {
 		switch v := v.(type) {
 		case *CallExpr:
@@ -447,7 +401,7 @@ func sortStringLists(f *File, info *RewriteInfo) {
 				if disabled("unsafesort") && !tables.SortableWhitelist[context] && !allowedSort(context) {
 					continue
 				}
-				sortStringList(as.RHS, info, context)
+				sortStringList(as.RHS, context)
 			}
 		case *AssignExpr:
 			if disabled("unsafesort") {
@@ -456,7 +410,7 @@ func sortStringLists(f *File, info *RewriteInfo) {
 			// "keep sorted" comment on x = list forces sorting of list.
 			as := v
 			if keepSorted(as) {
-				sortStringList(as.RHS, info, "?")
+				sortStringList(as.RHS, "?")
 			}
 		case *KeyValueExpr:
 			if disabled("unsafesort") {
@@ -464,7 +418,7 @@ func sortStringLists(f *File, info *RewriteInfo) {
 			}
 			// "keep sorted" before key: list also forces sorting of list.
 			if keepSorted(v) {
-				sortStringList(v.Value, info, "?")
+				sortStringList(v.Value, "?")
 			}
 		case *ListExpr:
 			if disabled("unsafesort") {
@@ -472,7 +426,7 @@ func sortStringLists(f *File, info *RewriteInfo) {
 			}
 			// "keep sorted" comment above first list element also forces sorting of list.
 			if len(v.List) > 0 && (keepSorted(v) || keepSorted(v.List[0])) {
-				sortStringList(v, info, "?")
+				sortStringList(v, "?")
 			}
 		}
 	})
@@ -480,13 +434,13 @@ func sortStringLists(f *File, info *RewriteInfo) {
 
 // SortStringList sorts x, a list of strings.
 func SortStringList(x Expr) {
-	sortStringList(x, nil, "")
+	sortStringList(x, "")
 }
 
 // sortStringList sorts x, a list of strings.
 // The list is broken by non-strings and by blank lines and comments into chunks.
 // Each chunk is sorted in place.
-func sortStringList(x Expr, info *RewriteInfo, context string) {
+func sortStringList(x Expr, context string) {
 	list, ok := x.(*ListExpr)
 	if !ok || len(list.List) < 2 || doNotSort(list.List[0]) {
 		return
@@ -524,13 +478,6 @@ func sortStringList(x Expr, info *RewriteInfo, context string) {
 			chunk = append(chunk, makeSortKey(index, x.(*StringExpr)))
 		}
 		if !sort.IsSorted(byStringExpr(chunk)) || !isUniq(chunk) {
-			if info != nil {
-				info.SortStringList++
-				if !tables.SortableWhitelist[context] {
-					info.UnsafeSort++
-					info.Log = append(info.Log, "sort:"+context)
-				}
-			}
 			before := chunk[0].x.Comment().Before
 			chunk[0].x.Comment().Before = nil
 
@@ -671,7 +618,7 @@ func (x byStringExpr) Less(i, j int) bool {
 //	)
 //
 // which typically works better with our aggressively compact formatting.
-func fixMultilinePlus(f *File, info *RewriteInfo) {
+func fixMultilinePlus(f *File) {
 
 	// List manipulation helpers.
 	// As a special case, we treat f([...]) as a list, mainly
@@ -811,12 +758,10 @@ func fixMultilinePlus(f *File, info *RewriteInfo) {
 }
 
 // sortAllLoadArgs sorts all load arguments in the file
-func sortAllLoadArgs(f *File, info *RewriteInfo) {
+func sortAllLoadArgs(f *File) {
 	Walk(f, func(v Expr, stk []Expr) {
 		if load, ok := v.(*LoadStmt); ok {
-			if SortLoadArgs(load) {
-				info.SortLoad++
-			}
+			SortLoadArgs(load)
 		}
 	})
 }
@@ -883,7 +828,7 @@ func SortLoadArgs(load *LoadStmt) bool {
 }
 
 // formatDocstrings fixes the indentation and trailing whitespace of docstrings
-func formatDocstrings(f *File, info *RewriteInfo) {
+func formatDocstrings(f *File) {
 	Walk(f, func(v Expr, stk []Expr) {
 		def, ok := v.(*DefStmt)
 		if !ok || len(def.Body) == 0 {
@@ -904,7 +849,6 @@ func formatDocstrings(f *File, info *RewriteInfo) {
 			docstring.Token = updatedToken
 			// Update the value to keep it consistent with Token
 			docstring.Value, _, _ = Unquote(updatedToken)
-			info.FormatDocstrings++
 		}
 	})
 }
@@ -958,7 +902,7 @@ func argumentType(expr Expr) int {
 
 // reorderArguments fixes the order of arguments of a function call
 // (positional, named, *args, **kwargs)
-func reorderArguments(f *File, info *RewriteInfo) {
+func reorderArguments(f *File) {
 	Walk(f, func(expr Expr, stack []Expr) {
 		call, ok := expr.(*CallExpr)
 		if !ok {
@@ -969,14 +913,13 @@ func reorderArguments(f *File, info *RewriteInfo) {
 		}
 		if !sort.SliceIsSorted(call.List, compare) {
 			sort.SliceStable(call.List, compare)
-			info.ReorderArguments++
 		}
 	})
 }
 
 // editOctals inserts 'o' into octal numbers to make it more obvious they are octal
 // 0123 -> 0o123
-func editOctals(f *File, info *RewriteInfo) {
+func editOctals(f *File) {
 	Walk(f, func(expr Expr, stack []Expr) {
 		l, ok := expr.(*LiteralExpr)
 		if !ok {
@@ -984,7 +927,6 @@ func editOctals(f *File, info *RewriteInfo) {
 		}
 		if len(l.Token) > 1 && l.Token[0] == '0' && l.Token[1] >= '0' && l.Token[1] <= '9' {
 			l.Token = "0o" + l.Token[1:]
-			info.EditOctal++
 		}
 	})
 }
diff --git a/buildifier/buildifier.go b/buildifier/buildifier.go
index 3d58673..0b901aa 100644
--- a/buildifier/buildifier.go
+++ b/buildifier/buildifier.go
@@ -338,9 +338,6 @@ func processFile(filename string, data []byte, inputType, lint string, warningsL
 	}
 	fileDiagnostics := utils.NewFileDiagnostics(f.DisplayPath(), warnings)
 
-	var info build.RewriteInfo
-	build.Rewrite(f, &info)
-
 	ndata := build.Format(f)
 
 	switch *mode {
@@ -348,7 +345,6 @@ func processFile(filename string, data []byte, inputType, lint string, warningsL
 		// check mode: print names of files that need formatting.
 		if !bytes.Equal(data, ndata) {
 			fileDiagnostics.Formatted = false
-			fileDiagnostics.SetRewrites(info.Stats())
 			return fileDiagnostics, 4
 		}
 
diff --git a/buildifier/utils/diagnostics.go b/buildifier/utils/diagnostics.go
index ea7e021..6aee1aa 100644
--- a/buildifier/utils/diagnostics.go
+++ b/buildifier/utils/diagnostics.go
@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"github.com/bazelbuild/buildtools/build"
 	"github.com/bazelbuild/buildtools/warn"
-	"sort"
 	"strings"
 )
 
@@ -33,20 +32,6 @@ func (d *Diagnostics) Format(format string, verbose bool) string {
 					w.Message,
 					w.URL))
 			}
-			if !f.Formatted {
-				rewrites := []string{}
-				for category, count := range f.Rewrites {
-					if count > 0 {
-						rewrites = append(rewrites, category)
-					}
-				}
-				log := ""
-				if len(rewrites) > 0 {
-					sort.Strings(rewrites)
-					log = " " + strings.Join(rewrites, " ")
-				}
-				output.WriteString(fmt.Sprintf("%s # reformat%s\n", f.Filename, log))
-			}
 		}
 		return output.String()
 	case "json":
@@ -63,20 +48,10 @@ func (d *Diagnostics) Format(format string, verbose bool) string {
 
 // FileDiagnostics contains diagnostics information for a file
 type FileDiagnostics struct {
-	Filename  string         `json:"filename"`
-	Formatted bool           `json:"formatted"`
-	Valid     bool           `json:"valid"`
-	Warnings  []*warning     `json:"warnings"`
-	Rewrites  map[string]int `json:"rewrites,omitempty"`
-}
-
-// SetRewrites adds information about rewrites to the diagnostics
-func (fd *FileDiagnostics) SetRewrites(categories map[string]int) {
-	for category, count := range categories {
-		if count > 0 {
-			fd.Rewrites[category] = count
-		}
-	}
+	Filename  string     `json:"filename"`
+	Formatted bool       `json:"formatted"`
+	Valid     bool       `json:"valid"`
+	Warnings  []*warning `json:"warnings"`
 }
 
 type warning struct {
@@ -115,7 +90,6 @@ func NewFileDiagnostics(filename string, warnings []*warn.Finding) *FileDiagnost
 		Formatted: true,
 		Valid:     true,
 		Warnings:  []*warning{},
-		Rewrites:  map[string]int{},
 	}
 
 	for _, w := range warnings {
@@ -139,7 +113,6 @@ func InvalidFileDiagnostics(filename string) *FileDiagnostics {
 		Formatted: false,
 		Valid:     false,
 		Warnings:  []*warning{},
-		Rewrites:  map[string]int{},
 	}
 	if filename == "" {
 		fileDiagnostics.Filename = "<stdin>"
diff --git a/edit/buildozer.go b/edit/buildozer.go
index 6016aab..0ed3391 100644
--- a/edit/buildozer.go
+++ b/edit/buildozer.go
@@ -990,7 +990,6 @@ var EditFile = func(fi os.FileInfo, name string) error {
 // opts.Buildifier is useful to force consistency with other tools that call Buildifier.
 func runBuildifier(opts *Options, f *build.File) ([]byte, error) {
 	if opts.Buildifier == "" {
-		build.Rewrite(f, nil)
 		return build.Format(f), nil
 	}
 
diff --git a/edit/edit.go b/edit/edit.go
index 5164898..1cabb63 100644
--- a/edit/edit.go
+++ b/edit/edit.go
@@ -21,7 +21,6 @@ import (
 	"path"
 	"path/filepath"
 	"regexp"
-	"sort"
 	"strconv"
 	"strings"
 
@@ -1026,12 +1025,7 @@ func AppendToLoad(load *build.LoadStmt, from, to []string) bool {
 	}
 
 	// Append the remaining loads to the load statement.
-	sortedSymbols := []string{}
 	for s := range symbolsToLoad {
-		sortedSymbols = append(sortedSymbols, s)
-	}
-	sort.Strings(sortedSymbols)
-	for _, s := range sortedSymbols {
 		load.From = append(load.From, &build.Ident{Name: symbolsToLoad[s]})
 		load.To = append(load.To, &build.Ident{Name: s})
 	}


Test patch:
diff --git a/build/print_test.go b/build/print_test.go
index 07a7c15..4edb5f0 100644
--- a/build/print_test.go
+++ b/build/print_test.go
@@ -175,8 +175,7 @@ func findTests(t *testing.T, suffix string) ([]string, func()) {
 
 // testPrint is a helper for testing the printer.
 // It reads the file named in, reformats it, and compares
-// the result to the file named out. If rewrite is true, the
-// reformatting includes buildifier's higher-level rewrites.
+// the result to the file named out.
 func testPrint(t *testing.T, in, out string, isBuild bool) {
 	data, err := ioutil.ReadFile(in)
 	if err != nil {
@@ -208,8 +207,6 @@ func testPrint(t *testing.T, in, out string, isBuild bool) {
 			return
 		}
 
-		Rewrite(file, nil)
-
 		ndata := Format(file)
 
 		if !bytes.Equal(ndata, golden) {
@@ -244,7 +241,7 @@ func TestPrintParse(t *testing.T) {
 			t.Errorf("parsing original: %v", err)
 		}
 
-		ndata := Format(f)
+		ndata := FormatWithoutRewriting(f)
 
 		f2, err := Parse(base, ndata)
 		if err != nil {
@@ -374,8 +371,6 @@ func TestPrintNewSequences(t *testing.T) {
 				Stmt: newSequences,
 			}
 
-			Rewrite(file, nil)
-
 			ndata := Format(file)
 
 			if !bytes.Equal(ndata, golden) {
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index 99f9dbc..125a630 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -284,10 +284,7 @@ cat > golden/json_report_golden <<EOF
             "filename": "to_fix_3.bzl",
             "formatted": false,
             "valid": true,
-            "warnings": [],
-            "rewrites": {
-                "editoctal": 1
-            }
+            "warnings": []
         },
         {
             "filename": "to_fix_4.bzl",
diff --git a/warn/types_test.go b/warn/types_test.go
index 1bd0fc0..16226b4 100644
--- a/warn/types_test.go
+++ b/warn/types_test.go
@@ -36,7 +36,7 @@ func checkTypes(t *testing.T, input, output string) {
 	build.Edit(f, edit)
 
 	want := []byte(strings.TrimLeft(output, "\n"))
-	have := build.Format(f)
+	have := build.FormatWithoutRewriting(f)
 	if !bytes.Equal(have, want) {
 		t.Errorf("detected types incorrectly: diff shows -expected, +ours")
 		testutils.Tdiff(t, want, have)
diff --git a/warn/warn_docstring_test.go b/warn/warn_docstring_test.go
index ec5c125..2d7ef2e 100644
--- a/warn/warn_docstring_test.go
+++ b/warn/warn_docstring_test.go
@@ -123,35 +123,35 @@ def f():
 		scopeEverywhere)
 
 	checkFindings(t, "function-docstring-header", `
-	def _f(x):
-	  """Long private function
-	  with a docstring"""
-	  x += 1
-	  x *= 2
-	  x /= 3
-	  x -= 4
-	  x %= 5
-	  return x
-	`,
+def _f(x):
+  """Long private function
+  with a docstring"""
+  x += 1
+  x *= 2
+  x /= 3
+  x -= 4
+  x %= 5
+  return x
+`,
 		[]string{
 			`:2: The docstring for the function "_f" should start with a one-line summary.`,
 		},
 		scopeEverywhere)
 
 	checkFindings(t, "function-docstring-header", `
-	def f(x):
-	  """Long function with a docstring
-
-		Docstring
-		body
-		"""
-	  x += 1
-	  x *= 2
-	  x /= 3
-	  x -= 4
-	  x %= 5
-	  return x
-	`,
+def f(x):
+  """Long function with a docstring
+
+	Docstring
+	body
+	"""
+  x += 1
+  x *= 2
+  x /= 3
+  x -= 4
+  x %= 5
+  return x
+`,
 		[]string{},
 		scopeEverywhere)
 
diff --git a/warn/warn_test.go b/warn/warn_test.go
index 1521349..f0c1037 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -128,7 +128,7 @@ func checkNoFix(t *testing.T, category, input string, fileType build.FileType) {
 
 	// No fixes expected
 	FileWarnings(buildFile, []string{category}, nil, ModeWarn, testFileReader)
-	fixed := build.Format(buildFile)
+	fixed := build.FormatWithoutRewriting(buildFile)
 
 	if !bytes.Equal(formatted, fixed) {
 		t.Errorf("Modified a file (type %s) while getting warnings:\ninput:\n%s\ndiff (-before, +after)\n",