Repository: bazelbuild/buildtools
Pull request #807 (2020-03-19 18:55:33)

Implement deprecated function warning
Deprecated funcitons are defined in other .bzl files, so buildifier is now also able to read from loaded .bzl files.

vladmos: Thanks for the thorough review :)

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 6e126da..c428fff 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -13,6 +13,7 @@ Warning categories supported by buildifier's linter:
   * [`constant-glob`](#constant-glob)
   * [`ctx-actions`](#ctx-actions)
   * [`ctx-args`](#ctx-args)
+  * [`deprecated-function`](#deprecated-function)
   * [`depset-iteration`](#depset-iteration)
   * [`depset-union`](#depset-union)
   * [`dict-concatenation`](#dict-concatenation)
@@ -241,6 +242,17 @@ depending on the desired behavior.
 
 --------------------------------------------------------------------------------
 
+## <a name="deprecated-function"></a>The function is deprecated
+
+  * Category name: `deprecated-function`
+  * Automatic fix: no
+
+The defined in another .bzl file has a docstring stating that it's deprecated, i. e. it
+contains a `Deprecated:` section. The convention for function docstrings is described by
+the [`function-docstring`](function-docstring) warning.
+
+--------------------------------------------------------------------------------
+
 ## <a name="depset-iteration"></a>Depset iteration is deprecated
 
   * Category name: `depset-iteration`
@@ -394,6 +406,9 @@ Returns:
   Description of the return value.
   Should be indented by at least one, preferably two spaces (as here)
   Can span multiple lines.
+
+Deprecated:
+  Optional, description of why the function is deprecated and what should be used instead. 
 """
 ```
 
diff --git a/buildifier/utils/utils.go b/buildifier/utils/utils.go
index 61fc98b..9c84f8a 100644
--- a/buildifier/utils/utils.go
+++ b/buildifier/utils/utils.go
@@ -3,6 +3,7 @@
 package utils
 
 import (
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
@@ -115,13 +116,33 @@ func SplitFilePath(filename string) (workspaceRoot, pkg string) {
 	return workspaceRoot, pkg
 }
 
+// getFileReader returns a *FileReader object that reads files from the local
+// filesystem if the workspace root is known.
+func getFileReader(workspaceRoot string) *warn.FileReader {
+	if workspaceRoot == "" {
+		return nil
+	}
+
+	readFile := func(filename string) ([]byte, error) {
+		// Use OS-specific path separators
+		filename = strings.ReplaceAll(filename, "/", string(os.PathSeparator))
+		path := filepath.Join(workspaceRoot, filename)
+
+		return ioutil.ReadFile(path)
+	}
+
+	return warn.NewFileReader(readFile)
+}
+
 // Lint calls the linter and returns a list of unresolved findings
 func Lint(f *build.File, lint string, warningsList *[]string, verbose bool) []*warn.Finding {
+	fileReader := getFileReader(f.WorkspaceRoot)
+
 	switch lint {
 	case "warn":
-		return warn.FileWarnings(f, *warningsList, nil, warn.ModeWarn)
+		return warn.FileWarnings(f, *warningsList, nil, warn.ModeWarn, fileReader)
 	case "fix":
-		warn.FixWarnings(f, *warningsList, verbose)
+		warn.FixWarnings(f, *warningsList, verbose, fileReader)
 	}
 	return nil
 }
diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel
index ef5d0ee..ed8d2ac 100644
--- a/warn/BUILD.bazel
+++ b/warn/BUILD.bazel
@@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 go_library(
     name = "go_default_library",
     srcs = [
+        "multifile.go",
         "types.go",
         "warn.go",
         "warn_bazel.go",
@@ -10,6 +11,7 @@ go_library(
         "warn_bazel_operation.go",
         "warn_control_flow.go",
         "warn_cosmetic.go",
+        "warn_deprecated.go",
         "warn_docstring.go",
         "warn_naming.go",
         "warn_operation.go",
@@ -36,6 +38,7 @@ go_test(
         "warn_bazel_test.go",
         "warn_control_flow_test.go",
         "warn_cosmetic_test.go",
+        "warn_deprecated_test.go",
         "warn_docstring_test.go",
         "warn_naming_test.go",
         "warn_operation_test.go",
diff --git a/warn/multifile.go b/warn/multifile.go
new file mode 100644
index 0000000..1fb9481
--- /dev/null
+++ b/warn/multifile.go
@@ -0,0 +1,51 @@
+package warn
+
+import (
+	"github.com/bazelbuild/buildtools/build"
+)
+
+// FileReader is a class that can read an arbitrary Starlark file
+// from the repository and cache the results.
+type FileReader struct {
+	cache    map[string]*build.File
+	readFile func(string) ([]byte, error)
+}
+
+// NewFileReader creates and initializes a FileReader instance with a
+// custom readFile function that can read an arbitrary file in the
+// repository using a path relative to the workspace root
+// (OS-independent, with forward slashes).
+func NewFileReader(readFile func(string) ([]byte, error)) *FileReader {
+	return &FileReader{
+		cache: make(map[string]*build.File),
+		readFile: readFile,
+	}
+}
+
+// retrieveFile reads a Starlark file using only the readFile method
+// (without using the cache).
+func (fr *FileReader) retrieveFile(filename string) *build.File {
+	contents, err := fr.readFile(filename)
+	if err != nil {
+		return nil
+	}
+
+	file, err := build.Parse(filename, contents)
+	if err != nil {
+		return nil
+	}
+
+	return file
+}
+
+// GetFile reads a Starlark file from the repository or the cache.
+// Returns nil if the file is not found or not valid.
+func (fr *FileReader) GetFile(filename string) *build.File {
+	// Try to retrieve from the cache
+	if file, ok := fr.cache[filename]; ok {
+		return file
+	}
+	file := fr.retrieveFile(filename)
+	fr.cache[filename] = file
+	return file
+}
diff --git a/warn/warn.go b/warn/warn.go
index e654a81..4af5e0c 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -155,6 +155,11 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"unused-variable":           unusedVariableWarning,
 }
 
+// MultiFileWarningMap lists the warnings that run on the whole file, but may use other files.
+var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader) []*LinterFinding{
+	"deprecated-function": deprecatedFunctionWarning,
+}
+
 // 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{
@@ -163,17 +168,26 @@ var nonDefaultWarnings = map[string]bool{
 }
 
 // fileWarningWrapper is a wrapper that converts a file warning function to a generic function.
-// A generic function takes a `pkg string` argument which is not used for file warnings, so it's just removed.
-func fileWarningWrapper(fct func(f *build.File) []*LinterFinding) func(f *build.File, pkg string) []*LinterFinding {
-	return func(f *build.File, pkg string) []*LinterFinding {
+// A generic function takes a `pkg string` and a `*ReadFile` arguments which are not used for file warnings,
+// so they are just removed.
+func fileWarningWrapper(fct func(f *build.File) []*LinterFinding) func(*build.File, string, *FileReader) []*LinterFinding {
+	return func(f *build.File, _ string, _ *FileReader) []*LinterFinding {
 		return fct(f)
 	}
 }
 
+// multiFileWarningWrapper is a wrapper that converts a multifile warning function to a generic function.
+// A generic function takes a `pkg string` argument which is not used for file warnings, so it's just removed.
+func multiFileWarningWrapper(fct func(f *build.File, fileReader *FileReader) []*LinterFinding) func(*build.File, string, *FileReader) []*LinterFinding {
+	return func(f *build.File, _ string, fileReader *FileReader) []*LinterFinding {
+		return fct(f, fileReader)
+	}
+}
+
 // ruleWarningWrapper is a wrapper that converts a per-rule function to a per-file function.
 // It also doesn't run on .bzl or default files, only on BUILD and WORKSPACE files.
-func ruleWarningWrapper(ruleWarning func(call *build.CallExpr, pkg string) *LinterFinding) func(f *build.File, pkg string) []*LinterFinding {
-	return func(f *build.File, pkg string) []*LinterFinding {
+func ruleWarningWrapper(ruleWarning func(call *build.CallExpr, pkg string) *LinterFinding) func(*build.File, string, *FileReader) []*LinterFinding {
+	return func(f *build.File, pkg string, _ *FileReader) []*LinterFinding {
 		if f.Type != build.TypeBuild {
 			return nil
 		}
@@ -200,9 +214,9 @@ func ruleWarningWrapper(ruleWarning func(call *build.CallExpr, pkg string) *Lint
 }
 
 // runWarningsFunction runs a linter/fixer function over a file and applies the fixes conditionally
-func runWarningsFunction(category string, f *build.File, fct func(f *build.File, pkg string) []*LinterFinding, formatted *[]byte, mode LintMode) []*Finding {
+func runWarningsFunction(category string, f *build.File, fct func(f *build.File, pkg string, fileReader *FileReader) []*LinterFinding, formatted *[]byte, mode LintMode, fileReader *FileReader) []*Finding {
 	findings := []*Finding{}
-	for _, w := range fct(f, f.Pkg) {
+	for _, w := range fct(f, f.Pkg, fileReader) {
 		if !DisabledWarning(f, w.Start.Line, category) {
 			finding := makeFinding(f, w.Start, w.End, category, w.URL, w.Message, true, nil)
 			if len(w.Replacement) > 0 {
@@ -265,7 +279,7 @@ func DisabledWarning(f *build.File, findingLine int, warning string) bool {
 }
 
 // FileWarnings returns a list of all warnings found in the file.
-func FileWarnings(f *build.File, enabledWarnings []string, formatted *[]byte, mode LintMode) []*Finding {
+func FileWarnings(f *build.File, enabledWarnings []string, formatted *[]byte, mode LintMode, fileReader *FileReader) []*Finding {
 	findings := []*Finding{}
 
 	// Sort the warnings to make sure they're applied in the same determined order
@@ -281,9 +295,11 @@ func FileWarnings(f *build.File, enabledWarnings []string, formatted *[]byte, mo
 
 	for _, warn := range warnings {
 		if fct, ok := FileWarningMap[warn]; ok {
-			findings = append(findings, runWarningsFunction(warn, f, fileWarningWrapper(fct), formatted, mode)...)
+			findings = append(findings, runWarningsFunction(warn, f, fileWarningWrapper(fct), formatted, mode, fileReader)...)
+		} else if fct, ok := MultiFileWarningMap[warn]; ok {
+			findings = append(findings, runWarningsFunction(warn, f, multiFileWarningWrapper(fct), formatted, mode, fileReader)...)
 		} else if fct, ok := RuleWarningMap[warn]; ok {
-			findings = append(findings, runWarningsFunction(warn, f, ruleWarningWrapper(fct), formatted, mode)...)
+			findings = append(findings, runWarningsFunction(warn, f, ruleWarningWrapper(fct), formatted, mode, fileReader)...)
 		} else {
 			log.Fatalf("unexpected warning %q", warn)
 		}
@@ -340,8 +356,8 @@ func calculateDifference(old, new *[]byte) (start, end int, replacement string)
 }
 
 // FixWarnings fixes all warnings that can be fixed automatically.
-func FixWarnings(f *build.File, enabledWarnings []string, verbose bool) {
-	warnings := FileWarnings(f, enabledWarnings, nil, ModeFix)
+func FixWarnings(f *build.File, enabledWarnings []string, verbose bool, fileReader *FileReader) {
+	warnings := FileWarnings(f, enabledWarnings, nil, ModeFix, fileReader)
 	if verbose {
 		fmt.Fprintf(os.Stderr, "%s: applied fixes, %d warnings left\n",
 			f.DisplayPath(),
@@ -355,6 +371,9 @@ func collectAllWarnings() []string {
 	for k := range FileWarningMap {
 		result = append(result, k)
 	}
+	for k := range MultiFileWarningMap {
+		result = append(result, k)
+	}
 	for k := range RuleWarningMap {
 		result = append(result, k)
 	}
diff --git a/warn/warn_deprecated.go b/warn/warn_deprecated.go
new file mode 100644
index 0000000..9738aea
--- /dev/null
+++ b/warn/warn_deprecated.go
@@ -0,0 +1,89 @@
+// Warnings for using deprecated functions
+
+package warn
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/bazelbuild/buildtools/build"
+)
+
+func getPathFromLabel(label, pkg string) string {
+	switch {
+	case strings.HasPrefix(label, "//"):
+		// Absolute label path
+		return strings.ReplaceAll(label[2:], ":", "/")
+	case strings.HasPrefix(label, ":"):
+		// Relative label path
+		return pkg + "/" + label[1:]
+	default:
+		// External repositories are not supported
+		return ""
+	}
+}
+
+func readBzlFile(path string, fileReader *FileReader) (*build.File, bool) {
+	file := fileReader.GetFile(path)
+	return file, file != nil
+}
+
+func checkDeprecatedFunction(stmt build.Expr, loadedSymbols *map[string]*build.Ident, path string) *LinterFinding {
+	def, ok := stmt.(*build.DefStmt)
+	if !ok {
+		return nil
+	}
+	node, ok := (*loadedSymbols)[def.Name]
+	if !ok {
+		return nil
+	}
+	docstring, ok := getDocstring(def.Body)
+	if !ok {
+		return nil
+	}
+	str, ok := docstring.(*build.StringExpr)
+	if !ok {
+		return nil
+	}
+	docstringInfo := parseFunctionDocstring(str)
+	if !docstringInfo.deprecated {
+		return nil
+	}
+
+	return makeLinterFinding(node, fmt.Sprintf("The function %q defined in %q is deprecated.", def.Name, path))
+}
+
+func deprecatedFunctionWarning(f *build.File, fileReader *FileReader) []*LinterFinding {
+	if fileReader == nil {
+		return nil
+	}
+
+	findings := []*LinterFinding{}
+	for _, stmt := range f.Stmt {
+		load, ok := stmt.(*build.LoadStmt)
+		if !ok {
+			continue
+		}
+		path := getPathFromLabel(load.Module.Value, f.Pkg)
+		if path == "" {
+			continue
+		}
+		loadedFile, ok := readBzlFile(path, fileReader)
+		if !ok {
+			continue
+		}
+
+		loadedSymbols := make(map[string]*build.Ident)
+		for _, from := range load.From {
+			loadedSymbols[from.Name] = from
+		}
+
+		for _, stmt := range loadedFile.Stmt {
+			if finding := checkDeprecatedFunction(stmt, &loadedSymbols, path); finding != nil {
+				findings = append(findings, finding)
+			}
+		}
+
+	}
+	return findings
+}
diff --git a/warn/warn_docstring.go b/warn/warn_docstring.go
index 232c1ff..d4392ce 100644
--- a/warn/warn_docstring.go
+++ b/warn/warn_docstring.go
@@ -67,6 +67,7 @@ type docstringInfo struct {
 	hasHeader    bool                      // whether the docstring has a one-line header
 	args         map[string]build.Position // map of documented arguments, the values are line numbers
 	returns      bool                      // whether the return value is documented
+	deprecated   bool                      // whether the function is marked as deprecated
 	argumentsPos build.Position            // line of the `Arguments:` block (not `Args:`), if it exists
 }
 
@@ -131,6 +132,10 @@ func parseFunctionDocstring(doc *build.StringExpr) docstringInfo {
 			isArgumentsDescription = false
 			info.returns = true
 			continue
+		case prefix + "Deprecated:":
+			isArgumentsDescription = false
+			info.deprecated = true
+			continue
 		}
 
 		if isArgumentsDescription {


Test patch:
diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh
index 5d10bcb..99f9dbc 100755
--- a/buildifier/integration_test.sh
+++ b/buildifier/integration_test.sh
@@ -360,3 +360,44 @@ diff json_report ../../golden/json_report_stdin_golden || die "$1: wrong console
 
 $buildifier --mode=check --format=json --lint=warn --warnings=-module-docstring -v to_fix_4.bzl foo.bar > json_report
 diff json_report ../../golden/json_report_invalid_file_golden || die "$1: wrong console output for --mode=check --format=json --lint=warn with an invalid file"
+
+cd ../..
+
+# Test the multifile functionality
+
+mkdir multifile
+cd multifile
+
+cat > lib.bzl <<EOF
+def foo():
+  """
+  This is a function foo.
+
+  Please use it in favor of the
+  deprecated function bar
+  """
+  pass
+
+def bar():
+  """
+  This is a function bar.
+
+  Deprecated:
+    please use foo instead.
+  """
+  pass
+EOF
+
+touch WORKSPACE
+
+cat > BUILD <<EOF
+load(":lib.bzl", "foo", "bar")
+load(":nonexistent.bzl", "foo2", "bar2")
+EOF
+
+cat > report_golden <<EOF
+BUILD:1: deprecated-function: The function "bar" defined in "/lib.bzl" is deprecated. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#deprecated-function)
+EOF
+
+$buildifier --lint=warn --warnings=deprecated-function BUILD 2> report || ret=$?
+diff report_golden report || die "$1: wrong console output for multifile warnings (WORKSPACE exists)"
diff --git a/warn/warn_deprecated_test.go b/warn/warn_deprecated_test.go
new file mode 100644
index 0000000..de762f8
--- /dev/null
+++ b/warn/warn_deprecated_test.go
@@ -0,0 +1,53 @@
+package warn
+
+import "testing"
+
+func TestDeprecatedFunction(t *testing.T) {
+	defer setUpFileReader(map[string]string{
+		"test/package/foo.bzl": `
+def foo():
+  """
+  This is a function foo.
+
+  Please use it in favor of the
+  deprecated function bar
+  """
+  pass
+
+def bar():
+  """
+  This is a function bar.
+
+  Deprecated:
+    please use foo instead.
+  """
+  pass
+`,
+		"test/package/invalid.bzl": `
+This is not a valid Starlark file
+`,
+	})()
+
+	checkFindings(t, "deprecated-function", `
+load(":foo.bzl", "foo", "bar", "baz")
+load("//test/package:foo.bzl", "foo", "bar", "baz")
+load(":invalid.bzl", "foo", "bar", "baz")
+load(":nonexistent.bzl", "foo", "bar", "baz")
+`,
+		[]string{
+			`1: The function "bar" defined in "test/package/foo.bzl" is deprecated.`,
+			`2: The function "bar" defined in "test/package/foo.bzl" is deprecated.`,
+		},
+		scopeEverywhere)
+}
+
+func TestDeprecatedFunctionNoReader(t *testing.T) {
+	checkFindings(t, "deprecated-function", `
+load(":foo.bzl", "foo", "bar", "baz")
+load("//test/package:foo.bzl", "foo", "bar", "baz")
+load(":invalid.bzl", "foo", "bar", "baz")
+load(":nonexistent.bzl", "foo", "bar", "baz")
+`,
+		[]string{},
+		scopeEverywhere)
+}
diff --git a/warn/warn_test.go b/warn/warn_test.go
index 54f8aa5..1521349 100644
--- a/warn/warn_test.go
+++ b/warn/warn_test.go
@@ -19,6 +19,25 @@ const (
 	scopeBazel      = scopeBuild | scopeBzl | scopeWorkspace
 )
 
+// A global FileReader object that can be used by tests. If a test redefines it it must
+// reset it when it finishes.
+var testFileReader *FileReader
+
+func setUpFileReader(data map[string]string) (cleanup func()) {
+	readFile := func(filename string) ([]byte, error) {
+		if contents, ok := data[filename]; ok {
+			return []byte(contents), nil
+		}
+		return nil, fmt.Errorf("File not found")
+	}
+	testFileReader = NewFileReader(readFile)
+
+	return func() {
+		// Tear down
+		testFileReader = nil
+	}
+}
+
 func getFilename(fileType build.FileType) string {
 	switch fileType {
 	case build.TypeBuild:
@@ -40,7 +59,7 @@ func getFindings(category, input string, fileType build.FileType) []*Finding {
 	}
 	buildFile.Pkg = "test/package"
 	buildFile.WorkspaceRoot = "/home/users/foo/bar"
-	return FileWarnings(buildFile, []string{category}, nil, ModeWarn)
+	return FileWarnings(buildFile, []string{category}, nil, ModeWarn, testFileReader)
 }
 
 func compareFindings(t *testing.T, category, input string, expected []string, scope, fileType build.FileType) {
@@ -88,7 +107,7 @@ func checkFix(t *testing.T, category, input, expected string, scope, fileType bu
 		panic(fmt.Sprintf("%v", err))
 	}
 
-	FixWarnings(buildFile, []string{category}, false)
+	FixWarnings(buildFile, []string{category}, false, testFileReader)
 	have := build.Format(buildFile)
 	want := build.Format(goldenFile)
 	if !bytes.Equal(have, want) {
@@ -108,7 +127,7 @@ func checkNoFix(t *testing.T, category, input string, fileType build.FileType) {
 	formatted := build.Format(buildFile)
 
 	// No fixes expected
-	FileWarnings(buildFile, []string{category}, nil, ModeWarn)
+	FileWarnings(buildFile, []string{category}, nil, ModeWarn, testFileReader)
 	fixed := build.Format(buildFile)
 
 	if !bytes.Equal(formatted, fixed) {
@@ -246,7 +265,7 @@ attr.baz("baz", cfg = "data")
 		t.Fatalf("Parse error: %v", err)
 	}
 
-	findings := FileWarnings(f, []string{"attr-cfg"}, nil, ModeSuggest)
+	findings := FileWarnings(f, []string{"attr-cfg"}, nil, ModeSuggest, testFileReader)
 	want := []struct {
 		start       int
 		end         int