Repository: bazelbuild/buildtools
Pull request #510 (2019-01-10 15:17:21)

Safer duplicated load fix
If a symbol is loaded twice from the same label, it's safe to delete the second time it's loaded. It may be not safe to delete the first time because it may be used in between.
If a symbol is loaded twice from different labels, it's not safe to automatically delete it, instead just a warning should be shown. For instance, the following code should be fixed manually by renaming the loaded symbols and eliminating the confusion:

    load("foo", "a")

    a()  # from foo

    load("bar", "a")

    a()  # from bar

laurentlb: You may want to add a test with aliases?
vladmos: Added a test, also improved the logic to also take the original variable name into account, e.g. the following loaded symbols are now considered different:

    load("foo", "a")
    load("foo", a = "b")

Patch:
diff --git a/build/lex.go b/build/lex.go
index 801192f..665e4a7 100644
--- a/build/lex.go
+++ b/build/lex.go
@@ -42,11 +42,15 @@ const (
 )
 
 func (t FileType) String() string {
-	return [...]string{
-		".bzl",
-		"BUILD",
-		"WORKSPACE",
-	}[t]
+	switch t {
+	case TypeDefault:
+		return ".bzl"
+	case TypeBuild:
+		return "BUILD"
+	case TypeWorkspace:
+		return "WORKSPACE"
+	}
+	return "unknown"
 }
 
 // ParseBuild parses a file, marks it as a BUILD file and returns the corresponding parse tree.
diff --git a/warn/warn.go b/warn/warn.go
index a51a52b..74b6bce 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -119,13 +119,10 @@ func constantGlobWarning(f *build.File, fix bool) []*Finding {
 
 func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
-	loaded := make(map[string]bool)
+	loaded := make(map[string]struct{label, from string})
 
 	symbols := edit.UsedSymbols(f)
-	// 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-- {
+	for stmtIndex := 0; stmtIndex < len(f.Stmt); stmtIndex++ {
 		load, ok := f.Stmt[stmtIndex].(*build.LoadStmt)
 		if !ok {
 			continue
@@ -134,8 +131,12 @@ func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 			from := load.From[i]
 			to := load.To[i]
 			// Check if the symbol was already loaded
-			if loaded[to.Name] {
-				if fix {
+			origin, alreadyLoaded := loaded[to.Name]
+			loaded[to.Name] = struct{label, from string}{load.Module.Token, from.Name}
+
+			if alreadyLoaded {
+				if fix && origin.label == load.Module.Token && origin.from == from.Name {
+					// Only fix if it's loaded from the label and variable
 					load.To = append(load.To[:i], load.To[i+1:]...)
 					load.From = append(load.From[:i], load.From[i+1:]...)
 					i--
@@ -163,7 +164,6 @@ func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 
 				}
 			}
-			loaded[to.Name] = true
 		}
 		// If there are no loaded symbols left remove the entire load statement
 		if fix && len(load.To) == 0 {


Test patch:
diff --git a/warn/warn_test.go b/warn/warn_test.go
index f74bc4f..a15deb1 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -212,26 +212,67 @@ func TestWarnUnusedLoad(t *testing.T) {
 load(":f.bzl", "s1", "s2")
 load(":bar.bzl", "s1")
 foo(name = s1)`, `
+load(":f.bzl", "s1")
 load(":bar.bzl", "s1")
 foo(name = s1)`,
 		[]string{
-			":1: Symbol \"s1\" has already been loaded.",
-			":1: Loaded symbol \"s2\" is unused."},
+			":1: Loaded symbol \"s2\" is unused.",
+			":2: Symbol \"s1\" has already been loaded.",
+		},
 		scopeEverywhere)
 
 	checkFindingsAndFix(t, "load", `
 load("foo", "a", "b", "c")
-load("bar", "a", "d", "e")
+load("foo", "a", "d", "e")
 
 z = a + b + d`, `
-load("foo", "b")
-load("bar", "a", "d")
+load("foo", "a", "b")
+load("foo", "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.",
+			":2: Symbol \"a\" has already been loaded.",
+			":2: Loaded symbol \"e\" is unused.",
+		},
+		scopeEverywhere)
+
+	checkFindingsAndFix(t, "load", `
+load("foo", "a")
+a(1)
+load("bar", "a")
+a(2)
+load("bar", a = "a")
+a(3)
+load("bar", a = "b")
+a(4)
+load("foo", "a")
+a(5)
+load("foo", "a")
+a(6)
+load("foo", a = "a")
+a(7)`, `
+load("foo", "a")
+a(1)
+load("bar", "a")
+a(2)
+
+a(3)
+load("bar", a = "b")
+a(4)
+load("foo", "a")
+a(5)
+
+a(6)
+
+a(7)`,
+		[]string{
+			":3: Symbol \"a\" has already been loaded.",
+			":5: Symbol \"a\" has already been loaded.",
+			":7: Symbol \"a\" has already been loaded.",
+			":9: Symbol \"a\" has already been loaded.",
+			":11: Symbol \"a\" has already been loaded.",
+			":13: Symbol \"a\" has already been loaded.",
 		},
 		scopeEverywhere)