Repository: bazelbuild/buildtools
Pull request #433 (2018-11-16 01:44:57)

[buildifier] Preserve symbol semantics when removing unused loads.
When the same symbol is loaded multiple times, during evalution the
last symbol wins and all previous loads are ignored, but `buildifier`
would preserve the first instance and remove all other usages
which breaks semantics unless all loads are from the same file.

In other words

```
load(":defs1.bzl", "s1")
load(":defs2.bzl", "s1")
```

is equivalent to

```
load(":defs2.bzl", "s1")
```

but `buildifier` would produce

```
load(":defs1.bzl", "s1")
```

To address this, all statements are analyzed in a reverse order, so the very
last (in terms of line numbers) is preserved and all preceding loads are
removed. This also slightly simplifies `load` removal logic, since we no longer
need to adjust index.

laurentlb: The new behavior is not perfect either (what if the symbol is used between the two loads?).

But the interpreter should be stricter in the future: we'll forbid symbol redefinition in that case (as in bzl files, you shouldn't be able define twice the same symbol at top-level).
vladmos: Please also add a more complex test case, where both `load`s don't need to be removed entirely, e.g.:

    load("foo", a, b, c)
    load("bar", a, d, e)

    a + b + d

Should be fixed as

    load("foo", b)
    load("bar", a, d)

    a + b + d

ttsugriy: @laurentlb, you're right that this behavior is not 100% correct too, but fortunately it plays well with another lint rule - `load-on-top` - https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#load-must-appear-at-top-of-file which should make sure that no symbol is used until all loads happen :)

Patch:
diff --git a/warn/warn.go b/warn/warn.go
index 0881a26..4adf6ae 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -120,7 +120,10 @@ func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 	loaded := make(map[string]bool)
 
 	symbols := edit.UsedSymbols(f)
-	for stmtIndex := 0; stmtIndex < len(f.Stmt); stmtIndex++ {
+	// statements are considered in reserve order to make sure that the last
+	// load wins, just like what happens during evaluation, which is very
+	// important if same symbol is loaded from different files.
+	for stmtIndex := len(f.Stmt) - 1; stmtIndex >= 0; stmtIndex-- {
 		load, ok := f.Stmt[stmtIndex].(*build.LoadStmt)
 		if !ok {
 			continue
@@ -163,7 +166,6 @@ func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 		// If there are no loaded symbols left remove the entire load statement
 		if fix && len(load.To) == 0 {
 			f.Stmt = append(f.Stmt[:stmtIndex], f.Stmt[stmtIndex+1:]...)
-			stmtIndex--
 		}
 	}
 	return findings


Test patch:
diff --git a/warn/warn_test.go b/warn/warn_test.go
index d962259..5badeb2 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -207,12 +207,29 @@ php_library(name = "x")`,
 func TestWarnUnusedLoad(t *testing.T) {
 	checkFindingsAndFix(t, "load", `
 load(":f.bzl", "s1", "s2")
-load("f", "s1")
+load(":bar.bzl", "s1")
 foo(name = s1)`, `
-load(":f.bzl", "s1")
+load(":bar.bzl", "s1")
 foo(name = s1)`,
-		[]string{":1: Loaded symbol \"s2\" is unused.",
-			":2: Symbol \"s1\" has already been loaded."},
+		[]string{
+			":1: Symbol \"s1\" has already been loaded.",
+			":1: Loaded symbol \"s2\" is unused."},
+		scopeEverywhere)
+
+	checkFindingsAndFix(t, "load", `
+load("foo", "a", "b", "c")
+load("bar", "a", "d", "e")
+
+z = a + b + d`, `
+load("foo", "b")
+load("bar", "a", "d")
+
+z = a + b + d`,
+		[]string{
+			":2: Loaded symbol \"e\" is unused.",
+			":1: Symbol \"a\" has already been loaded.",
+			":1: Loaded symbol \"c\" is unused.",
+		},
 		scopeEverywhere)
 
 	checkFindingsAndFix(t, "load", `