Repository: bazelbuild/buildtools
Pull request #658 (2019-06-19 17:38:35)

Refactor the remaining warn functions
The remaining warning functions are converted to the new signature allowing automatic suggestions for all findings that can be automatically fixed.

There'll be another follow-up PR to cleanup the legacy code (e.g. the empty `LegacyFileWarningMap`) because it's still being used internally.


Patch:
diff --git a/build/rewrite.go b/build/rewrite.go
index 2bfbdc1..50fb701 100644
--- a/build/rewrite.go
+++ b/build/rewrite.go
@@ -118,7 +118,7 @@ var rewrites = []struct {
 	{"label", fixLabels, scopeBuild},
 	{"listsort", sortStringLists, scopeBoth},
 	{"multiplus", fixMultilinePlus, scopeBuild},
-	{"loadsort", sortLoadArgs, scopeBoth},
+	{"loadsort", sortAllLoadArgs, scopeBoth},
 	{"formatdocstrings", formatDocstrings, scopeBoth},
 	{"reorderarguments", reorderArguments, scopeBoth},
 	{"editoctal", editOctals, scopeBoth},
@@ -822,16 +822,13 @@ func fixMultilinePlus(f *File, info *RewriteInfo) {
 	})
 }
 
-func sortLoadArgs(f *File, info *RewriteInfo) {
+// sortAllLoadArgs sorts all load arguments in the file
+func sortAllLoadArgs(f *File, info *RewriteInfo) {
 	Walk(f, func(v Expr, stk []Expr) {
-		load, ok := v.(*LoadStmt)
-		if !ok {
-			return
-		}
-		args := loadArgs{From: load.From, To: load.To}
-		sort.Sort(args)
-		if args.modified {
-			info.SortLoad++
+		if load, ok := v.(*LoadStmt); ok {
+			if SortLoadArgs(load) {
+				info.SortLoad++
+			}
 		}
 	})
 }
@@ -890,6 +887,13 @@ func (args loadArgs) Less(i, j int) bool {
 	return args.To[i].Name < args.To[j].Name
 }
 
+// SortLoadArgs sorts a load statement arguments (lexicographically, but positional first)
+func SortLoadArgs(load *LoadStmt) bool {
+	args := loadArgs{From: load.From, To: load.To}
+	sort.Sort(args)
+	return args.modified
+}
+
 // formatDocstrings fixes the indentation and trailing whitespace of docstrings
 func formatDocstrings(f *File, info *RewriteInfo) {
 	Walk(f, func(v Expr, stk []Expr) {
diff --git a/warn/warn.go b/warn/warn.go
index df77b14..dcb4d61 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -94,58 +94,57 @@ var RuleWarningMap = map[string]func(f *build.File, pkg string, expr build.Expr)
 
 // FileWarningMap lists the warnings that run on the whole file.
 var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
-	"attr-cfg":            attrConfigurationWarning,
-	"attr-license":        attrLicenseWarning,
-	"attr-non-empty":      attrNonEmptyWarning,
-	"attr-output-default": attrOutputDefaultWarning,
-	"attr-single-file":    attrSingleFileWarning,
-	"build-args-kwargs":   argsKwargsInBuildFilesWarning,
-	"constant-glob":       constantGlobWarning,
-	"ctx-actions":         ctxActionsWarning,
-	"ctx-args":            contextArgsAPIWarning,
-	"duplicated-name":     duplicatedNameWarning,
-	"filetype":            fileTypeWarning,
-	"git-repository":      nativeGitRepositoryWarning,
-	"http-archive":        nativeHTTPArchiveWarning,
-	"load":                unusedLoadWarning,
-	"native-android":      nativeAndroidRulesWarning,
-	"native-build":        nativeInBuildFilesWarning,
-	"native-package":      nativePackageWarning,
-	"no-effect":           noEffectWarning,
-	"output-group":        outputGroupWarning,
-	"package-name":        packageNameWarning,
-	"positional-args":     RuleWarning(positionalArgumentsWarning),
-	"print":               printWarning,
-	"redefined-variable":  redefinedVariableWarning,
-	"repository-name":     repositoryNameWarning,
-	"rule-impl-return":    ruleImplReturnWarning,
-	"return-value":        missingReturnValueWarning,
-	"uninitialized":       uninitializedVariableWarning,
-	"unreachable":         unreachableStatementWarning,
-	"unused-variable":     unusedVariableWarning,
-}
-
-// LegacyFileWarningMap lists the warnings that run on the whole file with legacy interface.
-var LegacyFileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
+	"attr-cfg":                  attrConfigurationWarning,
+	"attr-license":              attrLicenseWarning,
+	"attr-non-empty":            attrNonEmptyWarning,
+	"attr-output-default":       attrOutputDefaultWarning,
+	"attr-single-file":          attrSingleFileWarning,
+	"build-args-kwargs":         argsKwargsInBuildFilesWarning,
 	"confusing-name":            confusingNameWarning,
+	"constant-glob":             constantGlobWarning,
+	"ctx-actions":               ctxActionsWarning,
+	"ctx-args":                  contextArgsAPIWarning,
 	"depset-iteration":          depsetIterationWarning,
 	"depset-union":              depsetUnionWarning,
 	"dict-concatenation":        dictionaryConcatenationWarning,
+	"duplicated-name":           duplicatedNameWarning,
+	"filetype":                  fileTypeWarning,
 	"function-docstring":        functionDocstringWarning,
 	"function-docstring-header": functionDocstringHeaderWarning,
 	"function-docstring-args":   functionDocstringArgsWarning,
 	"function-docstring-return": functionDocstringReturnWarning,
+	"git-repository":            nativeGitRepositoryWarning,
+	"http-archive":              nativeHTTPArchiveWarning,
 	"integer-division":          integerDivisionWarning,
+	"load":                      unusedLoadWarning,
 	"load-on-top":               loadOnTopWarning,
 	"module-docstring":          moduleDocstringWarning,
 	"name-conventions":          nameConventionsWarning,
+	"native-android":            nativeAndroidRulesWarning,
+	"native-build":              nativeInBuildFilesWarning,
+	"native-package":            nativePackageWarning,
+	"no-effect":                 noEffectWarning,
+	"output-group":              outputGroupWarning,
 	"out-of-order-load":         outOfOrderLoadWarning,
+	"package-name":              packageNameWarning,
 	"package-on-top":            packageOnTopWarning,
+	"positional-args":           RuleWarning(positionalArgumentsWarning),
+	"print":                     printWarning,
+	"redefined-variable":        redefinedVariableWarning,
+	"repository-name":           repositoryNameWarning,
+	"rule-impl-return":          ruleImplReturnWarning,
+	"return-value":              missingReturnValueWarning,
 	"same-origin-load":          sameOriginLoadWarning,
 	"string-iteration":          stringIterationWarning,
+	"uninitialized":             uninitializedVariableWarning,
+	"unreachable":               unreachableStatementWarning,
 	"unsorted-dict-items":       unsortedDictItemsWarning,
+	"unused-variable":           unusedVariableWarning,
 }
 
+// LegacyFileWarningMap lists the warnings that run on the whole file with legacy interface.
+var LegacyFileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{}
+
 // nonDefaultWarnings contains warnings that are enabled by default because they're not applicable
 // for all files and cause too much diff noise when applied.
 var nonDefaultWarnings = map[string]bool{
@@ -203,7 +202,7 @@ func DisabledWarning(f *build.File, findingLine int, warning string) bool {
 				// Is the whole rule or this specific line as a comment
 				// to disable this warning?
 				if edit.ContainsComments(rule, format) ||
-						edit.ContainsComments(stmt, format) {
+					edit.ContainsComments(stmt, format) {
 					return true
 				}
 			}
diff --git a/warn/warn_bazel_operation.go b/warn/warn_bazel_operation.go
index 00aeffd..282942a 100644
--- a/warn/warn_bazel_operation.go
+++ b/warn/warn_bazel_operation.go
@@ -4,13 +4,11 @@ package warn
 
 import "github.com/bazelbuild/buildtools/build"
 
-func depsetUnionWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func depsetUnionWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 	addWarning := func(expr build.Expr) {
-		start, end := expr.Span()
 		findings = append(findings,
-			makeFinding(f, start, end, "depset-union",
-				"Depsets should be joined using the depset constructor.", true, nil))
+			makeLinterFinding(expr, `Depsets should be joined using the "depset()" constructor.`))
 	}
 
 	types := detectTypes(f)
@@ -55,99 +53,71 @@ func depsetUnionWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
-func depsetIterationWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func depsetIterationWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
-	addWarning := func(expr build.Expr) {
-		start, end := expr.Span()
-		findings = append(findings,
-			makeFinding(f, start, end, "depset-iteration",
-				"Depset iteration is deprecated.", true, nil))
-	}
-
-	// fixNode returns a call for .to_list() on the input node (assuming that it's a depset)
-	fixNode := func(expr build.Expr) build.Expr {
-		_, end := expr.Span()
-		return &build.CallExpr{
+	addFinding := func(expr *build.Expr) {
+		_, end := (*expr).Span()
+		newNode := &build.CallExpr{
 			X: &build.DotExpr{
-				X:    expr,
+				X:    *expr,
 				Name: "to_list",
 			},
 			End: build.End{Pos: end},
 		}
+		findings = append(findings,
+			makeLinterFinding(*expr, `Depset iteration is deprecated, use the "to_list()" method instead.`, LinterReplacement{expr, newNode}))
 	}
 
 	types := detectTypes(f)
-	build.Edit(f, func(expr build.Expr, stack []build.Expr) build.Expr {
-		switch expr := expr.(type) {
+	build.WalkPointers(f, func(e *build.Expr, stack []build.Expr) {
+		switch expr := (*e).(type) {
 		case *build.ForStmt:
 			if types[expr.X] != Depset {
-				return nil
-			}
-			if !fix {
-				addWarning(expr.X)
-				return nil
+				return
 			}
-			expr.X = fixNode(expr.X)
+			addFinding(&expr.X)
 		case *build.ForClause:
 			if types[expr.X] != Depset {
-				return nil
-			}
-			if !fix {
-				addWarning(expr.X)
-				return nil
+				return
 			}
-			expr.X = fixNode(expr.X)
+			addFinding(&expr.X)
 		case *build.BinaryExpr:
 			if expr.Op != "in" && expr.Op != "not in" {
-				return nil
+				return
 			}
 			if types[expr.Y] != Depset {
-				return nil
-			}
-			if !fix {
-				addWarning(expr.Y)
-				return nil
+				return
 			}
-			expr.Y = fixNode(expr.Y)
+			addFinding(&expr.Y)
 		case *build.CallExpr:
 			ident, ok := expr.X.(*build.Ident)
 			if !ok {
-				return nil
+				return
 			}
 			switch ident.Name {
 			case "all", "any", "depset", "len", "sorted", "max", "min", "list", "tuple":
 				if len(expr.List) != 1 {
-					return nil
+					return
 				}
 				if types[expr.List[0]] != Depset {
-					return nil
+					return
 				}
-				if !fix {
-					addWarning(expr.List[0])
-					return nil
+				addFinding(&expr.List[0])
+				if ident.Name == "list" {
+					// `list(d.to_list())` can be simplified to just `d.to_list()`
+					findings[len(findings)-1].Replacement[0].Old = e
 				}
-				newNode := fixNode(expr.List[0])
-				if ident.Name != "list" {
-					expr.List[0] = newNode
-					return nil
-				}
-				// `list(d.to_list())` can be simplified to just `d.to_list()`
-				return newNode
 			case "zip":
 				for i, arg := range expr.List {
 					if types[arg] != Depset {
 						continue
 					}
-					if !fix {
-						addWarning(arg)
-						return nil
-					}
-					expr.List[i] = fixNode(arg)
+					addFinding(&expr.List[i])
 				}
 			}
 		}
-		return nil
+		return
 	})
 	return findings
 }
diff --git a/warn/warn_control_flow.go b/warn/warn_control_flow.go
index e5665c2..33e5ce6 100644
--- a/warn/warn_control_flow.go
+++ b/warn/warn_control_flow.go
@@ -155,7 +155,7 @@ func noEffectStatementsCheck(f *build.File, body []build.Expr, isTopLevel, isFun
 		}
 		switch s := (stmt).(type) {
 		case *build.DefStmt, *build.ForStmt, *build.IfStmt, *build.LoadStmt, *build.ReturnStmt,
-		*build.CallExpr, *build.CommentBlock, *build.BranchStmt, *build.AssignExpr:
+			*build.CallExpr, *build.CommentBlock, *build.BranchStmt, *build.AssignExpr:
 			continue
 		case *build.Comprehension:
 			if !isTopLevel || s.Curly {
@@ -335,6 +335,7 @@ If you want to re-export a symbol, use the following pattern:
 			continue
 		}
 
+		build.SortLoadArgs(&load)
 		var newStmt build.Expr = &load
 		if len(load.To) == 0 {
 			// If there are no loaded symbols left remove the entire load statement
diff --git a/warn/warn_cosmetic.go b/warn/warn_cosmetic.go
index da86b48..6c2176d 100644
--- a/warn/warn_cosmetic.go
+++ b/warn/warn_cosmetic.go
@@ -12,55 +12,81 @@ import (
 	"github.com/bazelbuild/buildtools/tables"
 )
 
-func sameOriginLoadWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
-	loaded := make(map[string]*build.LoadStmt)
+func sameOriginLoadWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
+
+	type loadInfo = struct {
+		load  *build.LoadStmt
+		index int
+	}
+
+	// Indices of first load statements for each module
+	first := make(map[string]loadInfo)
+
+	// Repeated load statements for each module together with their indices
+	repeated := make(map[string][]loadInfo)
+
 	for stmtIndex := 0; stmtIndex < len(f.Stmt); stmtIndex++ {
 		load, ok := f.Stmt[stmtIndex].(*build.LoadStmt)
 		if !ok {
 			continue
 		}
+		module := load.Module.Value
 
-		if fix {
-			start, end := load.Span()
-			if start.Line == end.Line {
-				load.ForceCompact = true
-			}
+		if _, ok := first[module]; ok {
+			repeated[module] = append(repeated[module], loadInfo{load, stmtIndex})
+		} else {
+			first[load.Module.Value] = loadInfo{load, stmtIndex}
 		}
+	}
 
-		previousLoad := loaded[load.Module.Value]
-		if previousLoad == nil {
-			loaded[load.Module.Value] = load
+	for module, info := range first {
+		reps, ok := repeated[module]
+		if !ok {
+			// Not duplicated
 			continue
 		}
 
-		if fix {
-			previousLoad.To = append(previousLoad.To, load.To...)
-			previousLoad.From = append(previousLoad.From, load.From...)
+		previousStart, _ := info.load.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.",
+			module,
+			previousStart.Line,
+		)
+
+		// Fix
+		newLoad := *info.load
+		newLoad.To = append([]*build.Ident{}, info.load.To...)
+		newLoad.From = append([]*build.Ident{}, info.load.From...)
+
+		start, end := newLoad.Span()
+		if start.Line == end.Line {
+			newLoad.ForceCompact = true
+		}
+
+		replacements := []LinterReplacement{{&f.Stmt[info.index], &newLoad}}
+
+		for _, rep := range reps {
+			newLoad.To = append(newLoad.To, rep.load.To...)
+			newLoad.From = append(newLoad.From, rep.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
+			start, end := rep.load.Span()
+			if start.Line != end.Line {
+				newLoad.ForceCompact = false
 			}
+			replacements = append(replacements, LinterReplacement{&f.Stmt[rep.index], nil})
+		}
 
-			f.Stmt = append(f.Stmt[:stmtIndex], f.Stmt[stmtIndex+1:]...)
-			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", message, true, nil))
+		build.SortLoadArgs(&newLoad)
+
+		for _, rep := range reps {
+			findings = append(findings, makeLinterFinding(rep.load.Module, message, replacements...))
 		}
 	}
 	return findings
 }
 
-func packageOnTopWarning(f *build.File, fix bool) []*Finding {
+func packageOnTopWarning(f *build.File) []*LinterFinding {
 	seenRule := false
 	for _, stmt := range f.Stmt {
 		_, isString := stmt.(*build.StringExpr) // typically a docstring
@@ -76,31 +102,30 @@ func packageOnTopWarning(f *build.File, fix bool) []*Finding {
 			if !seenRule { // OK: package is on top of the file
 				return nil
 			}
-			start, end := rule.Call.Span()
-			return []*Finding{makeFinding(f, start, end, "package-on-top",
+			return []*LinterFinding{makeLinterFinding(rule.Call,
 				"Package declaration should be at the top of the file, after the load() statements, "+
 					"but before any call to a rule or a macro. "+
-					"package_group() and licenses() may be called before package().", true, nil)}
+					"package_group() and licenses() may be called before package().")}
 		}
 		seenRule = true
 	}
 	return nil
 }
 
-func loadOnTopWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
-
+func loadOnTopWarning(f *build.File) []*LinterFinding {
 	if f.Type != build.TypeDefault && f.Type != build.TypeBzl {
 		// Only applicable to default and .bzl files
-		return findings
+		return nil
 	}
 
+	// Find the misplaced load statements
+	misplacedLoads := make(map[int]*build.LoadStmt)
 	firstStmtIndex := -1 // index of the first seen non-load statement
 	for i := 0; i < len(f.Stmt); i++ {
 		stmt := f.Stmt[i]
 		_, isString := stmt.(*build.StringExpr) // typically a docstring
 		_, isComment := stmt.(*build.CommentBlock)
-		if isString || isComment {
+		if isString || isComment || stmt == nil {
 			continue
 		}
 		load, ok := stmt.(*build.LoadStmt)
@@ -113,20 +138,40 @@ func loadOnTopWarning(f *build.File, fix bool) []*Finding {
 		if firstStmtIndex == -1 {
 			continue
 		}
-		if !fix {
-			start, end := load.Span()
-			findings = append(findings, makeFinding(f, start, end, "load-on-top",
-				"Load statements should be at the top of the file.", true, nil))
+		misplacedLoads[i] = load
+	}
+
+	// Calculate a fix
+	if firstStmtIndex == -1 {
+		firstStmtIndex = 0
+	}
+	offset := len(misplacedLoads)
+	var replacements []LinterReplacement
+	for i := range f.Stmt {
+		if i < firstStmtIndex {
+			// Docstring or a comment in the beginning, skip
 			continue
+		} else if _, ok := misplacedLoads[i]; ok {
+			// A misplaced load statement, should be moved up to the `firstStmtIndex` position
+			replacements = append(replacements, LinterReplacement{&f.Stmt[firstStmtIndex], f.Stmt[i]})
+			firstStmtIndex++
+			offset--
+			if offset == 0 {
+				// No more statements should be moved
+				break
+			}
+		} else {
+			// An actual statement (not a docstring or a comment in the beginning), should be moved
+			// `offset` positions down.
+			replacements = append(replacements, LinterReplacement{&f.Stmt[i+offset], f.Stmt[i]})
 		}
-		stmts := []build.Expr{}
-		stmts = append(stmts, f.Stmt[:firstStmtIndex]...)
-		stmts = append(stmts, load)
-		stmts = append(stmts, f.Stmt[firstStmtIndex:i]...)
-		stmts = append(stmts, f.Stmt[i+1:]...)
-		f.Stmt = stmts
-		firstStmtIndex++
 	}
+
+	var findings []*LinterFinding
+	for _, load := range misplacedLoads {
+		findings = append(findings, makeLinterFinding(load, "Load statements should be at the top of the file.", replacements...))
+	}
+
 	return findings
 }
 
@@ -175,11 +220,11 @@ func compareLoadLabels(load1Label, load2Label string) bool {
 // outOfOrderLoadWarning only sorts consequent chunks of load statements. If applied together with
 // loadOnTopWarning, should be applied after it. This is currently guaranteed by sorting the
 // warning categories names before applying them ("load-on-top" < "out-of-order-load")
-func outOfOrderLoadWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func outOfOrderLoadWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	// Consequent chunks of load statements (i.e. without statements of other types between them)
-	loadsChunks := [][]*build.LoadStmt{}
+	var loadsChunks [][]*build.LoadStmt
 	lastLoadIndex := -2
 
 	for i := 0; i < len(f.Stmt); i++ {
@@ -195,34 +240,28 @@ func outOfOrderLoadWarning(f *build.File, fix bool) []*Finding {
 		lastLoadIndex = i
 	}
 
-	if fix {
-		// Sort the chunks
-		for i, chunk := range loadsChunks {
-			sort.SliceStable(chunk, func(i, j int) bool {
-				load1Label := (chunk)[i].Module.Value
-				load2Label := (chunk)[j].Module.Value
-				return compareLoadLabels(load1Label, load2Label)
-			})
-			loadsChunks[i] = chunk
-		}
-
-		// Flatten the chunks
-		sortedLoads := []*build.LoadStmt{}
-		for _, chunk := range loadsChunks {
-			for _, load := range chunk {
-				sortedLoads = append(sortedLoads, load)
-			}
-		}
+	// Sort and flatten the chunks
+	var sortedLoads []*build.LoadStmt
+	for _, chunk := range loadsChunks {
+		sortedChunk := append([]*build.LoadStmt{}, chunk...)
+
+		sort.SliceStable(sortedChunk, func(i, j int) bool {
+			load1Label := (sortedChunk)[i].Module.Value
+			load2Label := (sortedChunk)[j].Module.Value
+			return compareLoadLabels(load1Label, load2Label)
+		})
+		sortedLoads = append(sortedLoads, sortedChunk...)
+	}
 
-		loadIndex := 0
-		for stmtIndex := range f.Stmt {
-			if _, ok := f.Stmt[stmtIndex].(*build.LoadStmt); !ok {
-				continue
-			}
-			f.Stmt[stmtIndex] = sortedLoads[loadIndex]
-			loadIndex++
+	// Calculate the replacements
+	var replacements []LinterReplacement
+	loadIndex := 0
+	for stmtIndex := range f.Stmt {
+		if _, ok := f.Stmt[stmtIndex].(*build.LoadStmt); !ok {
+			continue
 		}
-		return findings
+		replacements = append(replacements, LinterReplacement{&f.Stmt[stmtIndex], sortedLoads[loadIndex]})
+		loadIndex++
 	}
 
 	for _, chunk := range loadsChunks {
@@ -231,16 +270,14 @@ func outOfOrderLoadWarning(f *build.File, fix bool) []*Finding {
 				// Correct position
 				continue
 			}
-			start, end := load.Span()
-			findings = append(findings, makeFinding(f, start, end, "out-of-order-load",
-				"Load statement is out of its lexicographical order.", true, nil))
+			findings = append(findings, makeLinterFinding(load, "Load statement is out of its lexicographical order.", replacements...))
 		}
 	}
 	return findings
 }
 
-func unsortedDictItemsWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func unsortedDictItemsWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	compareItems := func(item1, item2 *build.KeyValueExpr) bool {
 		key1 := item1.Key.(*build.StringExpr).Value
@@ -260,8 +297,8 @@ func unsortedDictItemsWarning(f *build.File, fix bool) []*Finding {
 		return key1 < key2
 	}
 
-	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
-		dict, ok := expr.(*build.DictExpr)
+	build.WalkPointers(f, func(expr *build.Expr, stack []build.Expr) {
+		dict, ok := (*expr).(*build.DictExpr)
 
 		mustSkipCheck := func(expr build.Expr) bool {
 			return edit.ContainsComments(expr, "@unsorted-dict-items")
@@ -277,7 +314,7 @@ func unsortedDictItemsWarning(f *build.File, fix bool) []*Finding {
 				return
 			}
 		}
-		sortedItems := []*build.KeyValueExpr{}
+		var sortedItems []*build.KeyValueExpr
 		for _, stmt := range dict.List {
 			item, ok := stmt.(*build.KeyValueExpr)
 			if !ok {
@@ -289,31 +326,44 @@ func unsortedDictItemsWarning(f *build.File, fix bool) []*Finding {
 			}
 			sortedItems = append(sortedItems, item)
 		}
-		if fix {
-			sort.SliceStable(sortedItems, func(i, j int) bool {
-				return compareItems(sortedItems[i], sortedItems[j])
-			})
-			sortedItemIndex := 0
-			for originalItemIndex := 0; originalItemIndex < len(dict.List); originalItemIndex++ {
-				item, ok := dict.List[originalItemIndex].(*build.KeyValueExpr)
-				if !ok {
-					continue
-				}
-				if _, ok := item.Key.(*build.StringExpr); !ok {
-					continue
-				}
-				dict.List[originalItemIndex] = sortedItems[sortedItemIndex]
-				sortedItemIndex++
+
+		// Fix
+		comp := func(i, j int) bool {
+			return compareItems(sortedItems[i], sortedItems[j])
+		}
+
+		var misplacedItems []*build.KeyValueExpr
+		for i := 1; i < len(sortedItems); i++ {
+			if comp(i, i-1) {
+				misplacedItems = append(misplacedItems, sortedItems[i])
 			}
+		}
+
+		if len(misplacedItems) == 0 {
+			// Already sorted
 			return
 		}
+		newDict := *dict
+		newDict.List = append([]build.Expr{}, dict.List...)
 
-		for i := 1; i < len(sortedItems); i++ {
-			if compareItems(sortedItems[i], sortedItems[i-1]) {
-				start, end := sortedItems[i].Span()
-				findings = append(findings, makeFinding(f, start, end, "unsorted-dict-items",
-					"Dictionary items are out of their lexicographical order.", true, nil))
+		sort.SliceStable(sortedItems, comp)
+		sortedItemIndex := 0
+		for originalItemIndex := 0; originalItemIndex < len(dict.List); originalItemIndex++ {
+			item, ok := dict.List[originalItemIndex].(*build.KeyValueExpr)
+			if !ok {
+				continue
+			}
+			if _, ok := item.Key.(*build.StringExpr); !ok {
+				continue
 			}
+			newDict.List[originalItemIndex] = sortedItems[sortedItemIndex]
+			sortedItemIndex++
+		}
+
+		for _, item := range misplacedItems {
+			findings = append(findings, makeLinterFinding(item,
+				"Dictionary items are out of their lexicographical order.",
+				LinterReplacement{expr, &newDict}))
 		}
 		return
 	})
diff --git a/warn/warn_docstring.go b/warn/warn_docstring.go
index 84d6ef4..2358308 100644
--- a/warn/warn_docstring.go
+++ b/warn/warn_docstring.go
@@ -28,9 +28,9 @@ func getDocstring(stmts []build.Expr) (build.Expr, bool) {
 	return nil, false
 }
 
-func moduleDocstringWarning(f *build.File, fix bool) []*Finding {
+func moduleDocstringWarning(f *build.File) []*LinterFinding {
 	if f.Type != build.TypeDefault && f.Type != build.TypeBzl {
-		return []*Finding{}
+		return nil
 	}
 	if stmt, ok := getDocstring(f.Stmt); stmt != nil && !ok {
 		start, _ := stmt.Span()
@@ -39,11 +39,11 @@ func moduleDocstringWarning(f *build.File, fix bool) []*Finding {
 			LineRune: start.LineRune + 1,
 			Byte:     start.Byte + 1,
 		}
-		return []*Finding{
-			makeFinding(f, start, end, "module-docstring", "The file has no module docstring.", true, nil),
-		}
+		finding := makeLinterFinding(stmt, "The file has no module docstring.")
+		finding.End = end
+		return []*LinterFinding{finding}
 	}
-	return []*Finding{}
+	return nil
 }
 
 func stmtsCount(stmts []build.Expr) int {
@@ -198,8 +198,8 @@ func isDocstringRequired(def *build.DefStmt) bool {
 	return !strings.HasPrefix(def.Name, "_") && stmtsCount(def.Body) >= FunctionLengthDocstringThreshold
 }
 
-func functionDocstringWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func functionDocstringWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	for _, stmt := range f.Stmt {
 		def, ok := stmt.(*build.DefStmt)
@@ -215,15 +215,14 @@ func functionDocstringWarning(f *build.File, fix bool) []*Finding {
 			continue
 		}
 
-		start, end := def.HeaderSpan()
 		message := fmt.Sprintf("The function %q has no docstring.", def.Name)
-		findings = append(findings, makeFinding(f, start, end, "function-docstring", message, true, nil))
+		findings = append(findings, makeLinterFinding(def, message))
 	}
 	return findings
 }
 
-func functionDocstringHeaderWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func functionDocstringHeaderWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	for _, stmt := range f.Stmt {
 		def, ok := stmt.(*build.DefStmt)
@@ -239,16 +238,15 @@ func functionDocstringHeaderWarning(f *build.File, fix bool) []*Finding {
 		info := parseFunctionDocstring(doc.(*build.StringExpr))
 
 		if !info.hasHeader {
-			start, end := doc.Span()
 			message := fmt.Sprintf("The docstring for the function %q should start with a one-line summary.", def.Name)
-			findings = append(findings, makeFinding(f, start, end, "function-docstring-header", message, true, nil))
+			findings = append(findings, makeLinterFinding(doc, message))
 		}
 	}
 	return findings
 }
 
-func functionDocstringArgsWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func functionDocstringArgsWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	for _, stmt := range f.Stmt {
 		def, ok := stmt.(*build.DefStmt)
@@ -266,16 +264,18 @@ func functionDocstringArgsWarning(f *build.File, fix bool) []*Finding {
 		if info.argumentsPos.LineRune > 0 {
 			argumentsEnd := info.argumentsPos
 			argumentsEnd.LineRune += len("Arguments:")
-			message := "Prefer 'Args:' to 'Arguments:' when documenting function arguments."
-			findings = append(findings, makeFinding(f, info.argumentsPos, argumentsEnd, "function-docstring-args", message, true, nil))
+			argumentsEnd.Byte += len("Arguments:")
+			finding := makeLinterFinding(doc, `Prefer "Args:" to "Arguments:" when documenting function arguments.`)
+			finding.Start = info.argumentsPos
+			finding.End = argumentsEnd
+			findings = append(findings, finding)
 		}
 
 		if !isDocstringRequired(def) && len(info.args) == 0 {
 			continue
 		}
 
-		// If the docstring is required or there are any arguments described, check for their integrity.
-		start, end := doc.Span()
+		// If a docstring is required or there are any arguments described, check for their integrity.
 
 		// Check whether all arguments are documented.
 		notDocumentedArguments := []string{}
@@ -297,7 +297,7 @@ func functionDocstringArgsWarning(f *build.File, fix bool) []*Finding {
 					strings.Join(notDocumentedArguments, `", "`),
 				)
 			}
-			findings = append(findings, makeFinding(f, start, end, "function-docstring-args", message, true, nil))
+			findings = append(findings, makeLinterFinding(doc, message))
 		}
 
 		// Check whether all documented arguments actually exist in the function signature.
@@ -305,16 +305,18 @@ func functionDocstringArgsWarning(f *build.File, fix bool) []*Finding {
 			if !paramNames[name] {
 				posEnd := pos
 				posEnd.LineRune += len(name)
-				message := fmt.Sprintf("Argument %q is documented but doesn't exist in the function signature.", name)
-				findings = append(findings, makeFinding(f, pos, posEnd, "function-docstring-args", message, true, nil))
+				finding := makeLinterFinding(doc, fmt.Sprintf("Argument %q is documented but doesn't exist in the function signature.", name))
+				finding.Start = pos
+				finding.End = posEnd
+				findings = append(findings, finding)
 			}
 		}
 	}
 	return findings
 }
 
-func functionDocstringReturnWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func functionDocstringReturnWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	for _, stmt := range f.Stmt {
 		def, ok := stmt.(*build.DefStmt)
@@ -331,9 +333,8 @@ func functionDocstringReturnWarning(f *build.File, fix bool) []*Finding {
 
 		// Check whether the return value is documented
 		if isDocstringRequired(def) && hasReturnValues(def) && !info.returns {
-			start, end := doc.Span()
 			message := fmt.Sprintf("Return value of %q is not documented.", def.Name)
-			findings = append(findings, makeFinding(f, start, end, "function-docstring-return", message, true, nil))
+			findings = append(findings, makeLinterFinding(doc, message))
 		}
 	}
 	return findings
diff --git a/warn/warn_naming.go b/warn/warn_naming.go
index da40edf..6e7083e 100644
--- a/warn/warn_naming.go
+++ b/warn/warn_naming.go
@@ -14,7 +14,7 @@ var ambiguousNames = map[string]bool{
 }
 
 // ambiguousNameCheck checks for the names of idents and functions
-func ambiguousNameCheck(f *build.File, expr build.Expr, findings []*Finding) []*Finding {
+func ambiguousNameCheck(f *build.File, expr build.Expr, findings []*LinterFinding) []*LinterFinding {
 	var name string
 	switch expr := expr.(type) {
 	case *build.Ident:
@@ -26,17 +26,15 @@ func ambiguousNameCheck(f *build.File, expr build.Expr, findings []*Finding) []*
 	}
 
 	if ambiguousNames[name] {
-		start, end := expr.Span()
 		findings = append(findings,
-			makeFinding(f, start, end, "confusing-name",
-				`Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').`, true, nil))
+			makeLinterFinding(expr, `Never use 'l', 'I', or 'O' as names (they're too easily confused with 'I', 'l', or '0').`))
 	}
 	return findings
 }
 
-func confusingNameWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func confusingNameWarning(f *build.File) []*LinterFinding {
 
+	var findings []*LinterFinding
 	// check for global variable names
 	for _, ident := range collectLocalVariables(f.Stmt) {
 		findings = ambiguousNameCheck(f, ident, findings)
@@ -84,8 +82,8 @@ func isUpperSnakeCase(name string) bool {
 	return name == strings.ToUpper(name)
 }
 
-func nameConventionsWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func nameConventionsWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	build.WalkStatements(f, func(stmt build.Expr, stack []build.Expr) {
 		// looking for provider declaration statements: `xxx = provider()`
@@ -101,10 +99,9 @@ func nameConventionsWarning(f *build.File, fix bool) []*Finding {
 			if isUpperCamelCase(ident.Name) && strings.HasSuffix(ident.Name, "Info") {
 				continue
 			}
-			start, end := ident.Span()
 			findings = append(findings,
-				makeFinding(f, start, end, "name-conventions",
-					fmt.Sprintf(`Variable name "%s" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`, ident.Name), true, nil))
+				makeLinterFinding(ident,
+					fmt.Sprintf(`Variable name "%s" should be lower_snake_case (for variables), UPPER_SNAKE_CASE (for constants), or UpperCamelCase ending with 'Info' (for providers).`, ident.Name)))
 		}
 	})
 
diff --git a/warn/warn_operation.go b/warn/warn_operation.go
index 3d98b63..ea5b66b 100644
--- a/warn/warn_operation.go
+++ b/warn/warn_operation.go
@@ -3,18 +3,15 @@
 package warn
 
 import (
-	"fmt"
 	"github.com/bazelbuild/buildtools/build"
 )
 
-func dictionaryConcatenationWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func dictionaryConcatenationWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	var addWarning = func(expr build.Expr) {
-		start, end := expr.Span()
 		findings = append(findings,
-			makeFinding(f, start, end, "dict-concatenation",
-				"Dictionary concatenation is deprecated.", true, nil))
+			makeLinterFinding(expr, "Dictionary concatenation is deprecated."))
 	}
 
 	types := detectTypes(f)
@@ -39,14 +36,12 @@ func dictionaryConcatenationWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
-func stringIterationWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func stringIterationWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
 	addWarning := func(expr build.Expr) {
-		start, end := expr.Span()
 		findings = append(findings,
-			makeFinding(f, start, end, "string-iteration",
-				"String iteration is deprecated.", true, nil))
+			makeLinterFinding(expr, "String iteration is deprecated."))
 	}
 
 	types := detectTypes(f)
@@ -85,37 +80,30 @@ func stringIterationWarning(f *build.File, fix bool) []*Finding {
 	return findings
 }
 
-func integerDivisionWarning(f *build.File, fix bool) []*Finding {
-	findings := []*Finding{}
+func integerDivisionWarning(f *build.File) []*LinterFinding {
+	var findings []*LinterFinding
 
-	var addWarning = func(expr build.Expr, op string) {
-		start, end := expr.Span()
-		findings = append(findings,
-			makeFinding(f, start, end, "integer-division",
-				fmt.Sprintf(`The "%s" operator for integer division is deprecated in favor of "/%s".`, op, op), true, nil))
-	}
-
-	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
-		switch expr := expr.(type) {
+	build.WalkPointers(f, func(e *build.Expr, stack []build.Expr) {
+		switch expr := (*e).(type) {
 		case *build.BinaryExpr:
 			if expr.Op != "/" {
 				return
 			}
-			if fix {
-				expr.Op = "//"
-				return
-			}
-			addWarning(expr, expr.Op)
+			newBinary := *expr
+			newBinary.Op = "//"
+			findings = append(findings,
+				makeLinterFinding(expr, `The "/" operator for integer division is deprecated in favor of "//".`,
+					LinterReplacement{e, &newBinary}))
 
 		case *build.AssignExpr:
 			if expr.Op != "/=" {
 				return
 			}
-			if fix {
-				expr.Op = "//="
-				return
-			}
-			addWarning(expr, expr.Op)
+			newAssign := *expr
+			newAssign.Op = "//="
+			findings = append(findings,
+				makeLinterFinding(expr, `The "/=" operator for integer division is deprecated in favor of "//=".`,
+					LinterReplacement{e, &newAssign}))
 		}
 	})
 	return findings


Test patch:
diff --git a/warn/warn_bazel_operation_test.go b/warn/warn_bazel_operation_test.go
index 3a26a99..9cab90b 100644
--- a/warn/warn_bazel_operation_test.go
+++ b/warn/warn_bazel_operation_test.go
@@ -95,21 +95,21 @@ for x in l:
     pass
 `,
 		[]string{
-			":3: Depset iteration is deprecated.",
-			":4: Depset iteration is deprecated.",
-			":5: Depset iteration is deprecated.",
-			":6: Depset iteration is deprecated.",
-			":7: Depset iteration is deprecated.",
-			":9: Depset iteration is deprecated.",
-			":15: Depset iteration is deprecated.",
-			":17: Depset iteration is deprecated.",
-			":18: Depset iteration is deprecated.",
-			":19: Depset iteration is deprecated.",
-			":20: Depset iteration is deprecated.",
-			":21: Depset iteration is deprecated.",
-			":22: Depset iteration is deprecated.",
-			":24: Depset iteration is deprecated.",
-			":26: Depset iteration is deprecated.",
+			`:3: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:4: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:5: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:6: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:7: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:9: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:15: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:17: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:18: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:19: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:20: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:21: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:22: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:24: Depset iteration is deprecated, use the "to_list()" method instead.`,
+			`:26: Depset iteration is deprecated, use the "to_list()" method instead.`,
 		},
 		scopeEverywhere)
 }
@@ -140,22 +140,22 @@ ccc.union(ddd)
 eee + fff | ggg
 `,
 		[]string{
-			":3: Depsets should be joined using the depset constructor",
-			":4: Depsets should be joined using the depset constructor",
-			":5: Depsets should be joined using the depset constructor",
-			":5: Depsets should be joined using the depset constructor",
-			":6: Depsets should be joined using the depset constructor",
-			":8: Depsets should be joined using the depset constructor",
-			":9: Depsets should be joined using the depset constructor",
-			":10: Depsets should be joined using the depset constructor",
-			":10: Depsets should be joined using the depset constructor",
-			":11: Depsets should be joined using the depset constructor",
-			":13: Depsets should be joined using the depset constructor",
-			":14: Depsets should be joined using the depset constructor",
-			":15: Depsets should be joined using the depset constructor",
-			":16: Depsets should be joined using the depset constructor",
-			":18: Depsets should be joined using the depset constructor",
-			":19: Depsets should be joined using the depset constructor",
+			`:3: Depsets should be joined using the "depset()" constructor`,
+			`:4: Depsets should be joined using the "depset()" constructor`,
+			`:5: Depsets should be joined using the "depset()" constructor`,
+			`:5: Depsets should be joined using the "depset()" constructor`,
+			`:6: Depsets should be joined using the "depset()" constructor`,
+			`:8: Depsets should be joined using the "depset()" constructor`,
+			`:9: Depsets should be joined using the "depset()" constructor`,
+			`:10: Depsets should be joined using the "depset()" constructor`,
+			`:10: Depsets should be joined using the "depset()" constructor`,
+			`:11: Depsets should be joined using the "depset()" constructor`,
+			`:13: Depsets should be joined using the "depset()" constructor`,
+			`:14: Depsets should be joined using the "depset()" constructor`,
+			`:15: Depsets should be joined using the "depset()" constructor`,
+			`:16: Depsets should be joined using the "depset()" constructor`,
+			`:18: Depsets should be joined using the "depset()" constructor`,
+			`:19: Depsets should be joined using the "depset()" constructor`,
 		},
 		scopeEverywhere)
 }
diff --git a/warn/warn_control_flow_test.go b/warn/warn_control_flow_test.go
index ac124f9..5e2662b 100644
--- a/warn/warn_control_flow_test.go
+++ b/warn/warn_control_flow_test.go
@@ -368,7 +368,7 @@ foo(name = s1)`,
 		scopeEverywhere)
 
 	checkFindingsAndFix(t, "load", `
-load("foo", "a", "b", "c")
+load("foo", "b", "a", "c")
 load("foo", "a", "d", "e")
 
 z = a + b + d`, `
diff --git a/warn/warn_cosmetic_test.go b/warn/warn_cosmetic_test.go
index 63fd211..0d80554 100644
--- a/warn/warn_cosmetic_test.go
+++ b/warn/warn_cosmetic_test.go
@@ -8,12 +8,12 @@ func TestWarnSameOriginLoad(t *testing.T) {
 	checkFindingsAndFix(t, category, `
 	load(
 		":f.bzl",
-		"s1"
+		"s2"
 	)
 	load(":t.bzl", "s3")
 	load(
 		":f.bzl",
-		"s2"
+		"s1"
 	)`, `
 	load(
 		":f.bzl",
diff --git a/warn/warn_docstring_test.go b/warn/warn_docstring_test.go
index e3afe1f..d50d6c7 100644
--- a/warn/warn_docstring_test.go
+++ b/warn/warn_docstring_test.go
@@ -221,7 +221,7 @@ def f(x, y):
 `,
 		[]string{
 			`2: Argument "y" is not documented.`,
-			`4: Prefer 'Args:' to 'Arguments:' when documenting function arguments.`,
+			`4: Prefer "Args:" to "Arguments:" when documenting function arguments.`,
 		},
 		scopeEverywhere)
 
@@ -265,7 +265,7 @@ def f(x, y):
 `,
 		[]string{
 			`2: Argument "y" is not documented.`,
-			`4: Prefer 'Args:' to 'Arguments:' when documenting function arguments.`,
+			`4: Prefer "Args:" to "Arguments:" when documenting function arguments.`,
 			`7: Argument "z" is documented but doesn't exist in the function signature.`,
 		}, scopeEverywhere)