Repository: bazelbuild/buildtools
Pull request #511 (2019-01-10 18:59:46)

Merge two single-line load statements into a single-line one
When the `same_origin_load` fix merges two load statements it currently creates a multi-line load statement in any case (because the loaded symbols were originally located on different lines).

With the update they will be merged into a single-line load statement if both original load statements were also single-line.


Patch:
diff --git a/build/print.go b/build/print.go
index 49a0067..9cc7816 100644
--- a/build/print.go
+++ b/build/print.go
@@ -541,7 +541,7 @@ func (p *printer) expr(v Expr, outerPrec int) {
 			}
 			args = append(args, arg)
 		}
-		p.seq("()", &v.Load, &args, &v.Rparen, modeCall, v.ForceCompact, false)
+		p.seq("()", &v.Load, &args, &v.Rparen, modeLoad, v.ForceCompact, false)
 
 	case *ListExpr:
 		p.seq("[]", &v.Start, &v.List, &v.End, modeList, false, v.ForceMultiLine)
@@ -674,6 +674,7 @@ const (
 	modeDict  // {x:y}
 	modeSeq   // x, y
 	modeDef   // def f(x, y)
+	modeLoad  // load(a, b, c)
 )
 
 // useCompactMode reports whether a sequence should be formatted in a compact mode
@@ -697,7 +698,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.fileType == TypeDefault || mode == modeDef {
+	if (p.level != 0 || p.fileType == TypeDefault || mode == modeDef) && mode != modeLoad {
 		// 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/warn/warn.go b/warn/warn.go
index 9ab4211..a51a52b 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -182,6 +182,13 @@ func sameOriginLoadWarning(f *build.File, fix bool) []*Finding {
 			continue
 		}
 
+		if fix {
+			start, end := load.Span()
+			if start.Line == end.Line {
+				load.ForceCompact = true
+			}
+		}
+
 		previousLoad := loaded[load.Module.Value]
 		if previousLoad == nil {
 			loaded[load.Module.Value] = load
@@ -191,6 +198,12 @@ func sameOriginLoadWarning(f *build.File, fix bool) []*Finding {
 		if fix {
 			previousLoad.To = append(previousLoad.To, load.To...)
 			previousLoad.From = append(previousLoad.From, load.From...)
+
+			// Force the merged load statement to be compact if both previous and current load statements are compact
+			if !load.ForceCompact {
+				previousLoad.ForceCompact = false
+			}
+
 			f.Stmt = append(f.Stmt[:stmtIndex], f.Stmt[stmtIndex+1:]...)
 			stmtIndex--
 		} else {


Test patch:
diff --git a/warn/warn_test.go b/warn/warn_test.go
index ba8d7db..f74bc4f 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -316,8 +316,53 @@ func TestWarnSameOriginLoad(t *testing.T) {
 			":10: There is already a load from \":f.bzl\". Please merge all loads from the same origin into a single one."},
 		scopeEverywhere,
 	)
-}
 
+	checkFindingsAndFix(t, category, `
+	load(":f.bzl", "s1")
+	load(":f.bzl", "s2", "s3")
+	`, `
+	load(":f.bzl", "s1", "s2", "s3")
+  `,
+		[]string{":2: There is already a load from \":f.bzl\". Please merge all loads from the same origin into a single one."},
+		scopeEverywhere,
+	)
+
+	checkFindingsAndFix(t, category, `
+	load(":f.bzl", "s1")
+	load(":f.bzl",
+    "s2",
+    "s3")
+	`, `
+	load(
+      ":f.bzl",
+      "s1",
+      "s2",
+      "s3",
+  )`,
+		[]string{":2: There is already a load from \":f.bzl\". Please merge all loads from the same origin into a single one."},
+		scopeEverywhere,
+	)
+
+	checkFindingsAndFix(t, category, `
+	load(":f.bzl", "s1")
+	load(":f.bzl", "s2", "s3")
+	load(":f.bzl",
+    "s4")
+	`, `
+	load(
+      ":f.bzl",
+      "s1",
+      "s2",
+      "s3",
+      "s4",
+  )`,
+		[]string{
+			":2: There is already a load from \":f.bzl\". Please merge all loads from the same origin into a single one.",
+			":3: There is already a load from \":f.bzl\". Please merge all loads from the same origin into a single one.",
+		}, scopeEverywhere,
+	)
+
+}
 func TestWarnUnusedVariables(t *testing.T) {
 	checkFindings(t, "unused-variable", `
 load(":f.bzl", "x")