Repository: bazelbuild/buildtools
Pull request #597 (2019-04-03 16:17:47)

Update warning messages
Make warning messages for some categories more readable:

For already loaded symbols mention line numbers where the load occurs.
For not used loaded symbols provide a workaround for reexporting symbols.


Patch:
diff --git a/warn/warn_control_flow.go b/warn/warn_control_flow.go
index 48aee97..ff48e56 100644
--- a/warn/warn_control_flow.go
+++ b/warn/warn_control_flow.go
@@ -275,7 +275,10 @@ func redefinedVariableWarning(f *build.File, fix bool) []*Finding {
 
 func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
-	loaded := make(map[string]struct{ label, from string })
+	loaded := make(map[string]struct {
+		label, from string
+		line        int
+	})
 
 	symbols := edit.UsedSymbols(f)
 	for stmtIndex := 0; stmtIndex < len(f.Stmt); stmtIndex++ {
@@ -283,24 +286,29 @@ func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 		if !ok {
 			continue
 		}
+
 		for i := 0; i < len(load.To); i++ {
 			from := load.From[i]
 			to := load.To[i]
 			// Check if the symbol was already loaded
 			origin, alreadyLoaded := loaded[to.Name]
-			loaded[to.Name] = struct{ label, from string }{load.Module.Token, from.Name}
+			start, _ := from.Span()
+			loaded[to.Name] = struct {
+				label, from string
+				line        int
+			}{load.Module.Token, from.Name, start.Line}
 
 			if alreadyLoaded {
 				if fix && origin.label == load.Module.Token && origin.from == from.Name {
-					// Only fix if it's loaded from the label and variable
+					// Only fix if it's loaded from the same label and variable
 					load.To = append(load.To[:i], load.To[i+1:]...)
 					load.From = append(load.From[:i], load.From[i+1:]...)
 					i--
 				} else {
 					start, end := to.Span()
+					message := fmt.Sprintf("Symbol %q has already been loaded on line %d. Please remove it.", to.Name, origin.line)
 					findings = append(findings,
-						makeFinding(f, start, end, "load",
-							"Symbol \""+to.Name+"\" has already been loaded. Please remove it.", true, nil))
+						makeFinding(f, start, end, "load", message, true, nil))
 				}
 				continue
 			}
@@ -313,10 +321,18 @@ func unusedLoadWarning(f *build.File, fix bool) []*Finding {
 					i--
 				} else {
 					start, end := to.Span()
+					message := fmt.Sprintf(`Loaded symbol %q is unused. Please remove it.
+To disable the warning, add '@unused' in a comment.`, to.Name)
+					if f.Type == build.TypeDefault || f.Type == build.TypeBzl {
+						message += fmt.Sprintf(`
+If you want to re-export a symbol, use the following pattern:
+
+    load(..., _%s = %q, ...)
+    %s = _%s
+`, to.Name, from.Name, to.Name, to.Name)
+					}
 					findings = append(findings,
-						makeFinding(f, start, end, "load",
-							"Loaded symbol \""+to.Name+"\" is unused. Please remove it.\n"+
-								"To disable the warning, add '@unused' in a comment.", true, nil))
+						makeFinding(f, start, end, "load", message, true, nil))
 
 				}
 			}
diff --git a/warn/warn_cosmetic.go b/warn/warn_cosmetic.go
index af120c6..da86b48 100644
--- a/warn/warn_cosmetic.go
+++ b/warn/warn_cosmetic.go
@@ -3,6 +3,7 @@
 package warn
 
 import (
+	"fmt"
 	"sort"
 	"strings"
 
@@ -46,9 +47,14 @@ func sameOriginLoadWarning(f *build.File, fix bool) []*Finding {
 			stmtIndex--
 		} else {
 			start, end := load.Module.Span()
+			previousStart, _ := previousLoad.Span()
+			message := fmt.Sprintf(
+				"There is already a load from %q on line %d. Please merge all loads from the same origin into a single one.",
+				load.Module.Value,
+				previousStart.Line,
+			)
 			findings = append(findings,
-				makeFinding(f, start, end, "same-origin-load",
-					"There is already a load from \""+load.Module.Value+"\". Please merge all loads from the same origin into a single one.", true, nil))
+				makeFinding(f, start, end, "same-origin-load", message, true, nil))
 		}
 	}
 	return findings


Test patch:
diff --git a/warn/warn_control_flow_test.go b/warn/warn_control_flow_test.go
index 9b7e629..ac124f9 100644
--- a/warn/warn_control_flow_test.go
+++ b/warn/warn_control_flow_test.go
@@ -363,7 +363,7 @@ load(":bar.bzl", "s1")
 foo(name = s1)`,
 		[]string{
 			":1: Loaded symbol \"s2\" is unused.",
-			":2: Symbol \"s1\" has already been loaded.",
+			":2: Symbol \"s1\" has already been loaded on line 1.",
 		},
 		scopeEverywhere)
 
@@ -378,7 +378,7 @@ load("foo", "d")
 z = a + b + d`,
 		[]string{
 			":1: Loaded symbol \"c\" is unused.",
-			":2: Symbol \"a\" has already been loaded.",
+			":2: Symbol \"a\" has already been loaded on line 1.",
 			":2: Loaded symbol \"e\" is unused.",
 		},
 		scopeEverywhere)
@@ -413,12 +413,12 @@ 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.",
+			":3: Symbol \"a\" has already been loaded on line 1.",
+			":5: Symbol \"a\" has already been loaded on line 3.",
+			":7: Symbol \"a\" has already been loaded on line 5.",
+			":9: Symbol \"a\" has already been loaded on line 7.",
+			":11: Symbol \"a\" has already been loaded on line 9.",
+			":13: Symbol \"a\" has already been loaded on line 11.",
 		},
 		scopeEverywhere)
 
diff --git a/warn/warn_cosmetic_test.go b/warn/warn_cosmetic_test.go
index fd99e03..63fd211 100644
--- a/warn/warn_cosmetic_test.go
+++ b/warn/warn_cosmetic_test.go
@@ -21,11 +21,13 @@ func TestWarnSameOriginLoad(t *testing.T) {
 		"s2"
 	)
 	load(":t.bzl", "s3")`,
-		[]string{":7: There is already a load from \":f.bzl\". Please merge all loads from the same origin into a single one."},
+		[]string{`:7: There is already a load from ":f.bzl" on line 1. Please merge all loads from the same origin into a single one.`},
 		scopeEverywhere,
 	)
 
 	checkFindingsAndFix(t, category, `
+	"""Module"""
+
 	load(
 		":f.bzl",
 		"s1"
@@ -38,14 +40,16 @@ func TestWarnSameOriginLoad(t *testing.T) {
 		":f.bzl",
 		"s3"
 	)`, `
+	"""Module"""
+
 	load(
 		":f.bzl",
 		"s1",
 		"s2",
 		"s3"
 	)`,
-		[]string{":6: There is already a load from \":f.bzl\". Please merge all loads from the same origin into a single one.",
-			":10: There is already a load from \":f.bzl\". Please merge all loads from the same origin into a single one."},
+		[]string{`:8: There is already a load from ":f.bzl" on line 3. Please merge all loads from the same origin into a single one.`,
+			`:12: There is already a load from ":f.bzl" on line 3. Please merge all loads from the same origin into a single one.`},
 		scopeEverywhere,
 	)
 
@@ -55,23 +59,25 @@ func TestWarnSameOriginLoad(t *testing.T) {
 	`, `
 	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."},
+		[]string{`:2: There is already a load from ":f.bzl" on line 1. Please merge all loads from the same origin into a single one.`},
 		scopeEverywhere,
 	)
 
 	checkFindingsAndFix(t, category, `
+	load(":g.bzl", "s0")
 	load(":f.bzl", "s1")
 	load(":f.bzl",
     "s2",
     "s3")
 	`, `
+	load(":g.bzl", "s0")
 	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."},
+		[]string{`:3: There is already a load from ":f.bzl" on line 2. Please merge all loads from the same origin into a single one.`},
 		scopeEverywhere,
 	)
 
@@ -89,8 +95,8 @@ func TestWarnSameOriginLoad(t *testing.T) {
       "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.",
+			`:2: There is already a load from ":f.bzl" on line 1. Please merge all loads from the same origin into a single one.`,
+			`:3: There is already a load from ":f.bzl" on line 1. Please merge all loads from the same origin into a single one.`,
 		}, scopeEverywhere,
 	)
 }