Repository: bazelbuild/buildtools
Pull request #407 (2018-10-31 12:53:40)

Provide warnings and fixes for removed native functions
Namely `git_repository`, `new_git_repository`, and `http_archive`.


Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index 4fd82a0..d34b713 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -297,3 +297,21 @@ just use a list of strings.
 
 The `output_group` field of a target is [deprecated](https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#disable-output-group-field-on-target)
 in favor of the [`OutputGroupInfo` provider](https://docs.bazel.build/versions/master/skylark/lib/OutputGroupInfo.html).
+
+--------------------------------------------------------------------------------
+
+## <a name="git-repository"></a>Function `git_repository` is not global anymore
+
+Native `git_repository` and `new_git_repository` functions are [being removed](https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#remove-native-git-repository).
+Please use the Starklark versions instead:
+
+    load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository", "new_git_repository")
+
+--------------------------------------------------------------------------------
+
+## <a name="http-archive"></a>Function `http_archive` is not global anymore
+
+Native `http_archive` function are [being removed](https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#remove-native-http-archive).
+Please use the Starklark versions instead:
+
+    load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
diff --git a/build/print.go b/build/print.go
index d460562..b4a3e03 100644
--- a/build/print.go
+++ b/build/print.go
@@ -279,7 +279,7 @@ func isFunctionDefinition(x Expr) bool {
 // isDifferentLines reports whether two positions belong to different lines.
 // If one of the positions is null (Line == 0), it's not a real position but probably an indicator
 // of manually inserted node. Return false in this case
-func isDifferentLines(p1, p2 Position) bool {
+func isDifferentLines(p1, p2 *Position) bool {
 	if p1.Line == 0 || p2.Line == 0 {
 		return false
 	}
@@ -430,7 +430,7 @@ func (p *printer) expr(v Expr, outerPrec int) {
 		addParen(precSuffix)
 		p.expr(v.X, precSuffix)
 		_, xEnd := v.X.Span()
-		isMultiline := isDifferentLines(v.NamePos, xEnd)
+		isMultiline := isDifferentLines(&v.NamePos, &xEnd)
 		if isMultiline {
 			p.margin += listIndentation
 			p.breakline()
@@ -717,16 +717,20 @@ func (p *printer) useCompactMode(start *Position, list *[]Expr, end *End, mode s
 	// should also keep the original style regardless of the mode.
 	if p.level != 0 || !p.buildMode || mode == modeDef {
 		// If every element (including the brackets) ends on the same line where the next element starts,
-		// use the compact mode, otherwise use multiline mode
-		previousEnd := start.Line
+		// use the compact mode, otherwise use multiline mode.
+		// If an node's line number is 0, it means it doesn't appear in the original file,
+		// its position shouldn't be taken into account.
+		previousEnd := start
 		for _, x := range *list {
 			start, end := x.Span()
-			if start.Line != previousEnd {
+			if isDifferentLines(&start, previousEnd) {
 				return false
 			}
-			previousEnd = end.Line
+			if end.Line != 0 {
+				previousEnd = &end
+			}
 		}
-		if end != nil && previousEnd != end.Pos.Line {
+		if end != nil && isDifferentLines(previousEnd, &end.Pos) {
 			return false
 		}
 		return true
diff --git a/edit/edit.go b/edit/edit.go
index b3c85f7..ca11ab2 100644
--- a/edit/edit.go
+++ b/edit/edit.go
@@ -830,10 +830,10 @@ func InsertLoad(stmts []build.Expr, location string, from, to []string) []build.
 
 	var all []build.Expr
 	added := false
-	for _, stmt := range stmts {
+	for i, stmt := range stmts {
 		_, isComment := stmt.(*build.CommentBlock)
-		c, isString := stmt.(*build.StringExpr)
-		isDocString := isString && c.Start.Line == 1 && c.Start.LineRune == 1
+		_, isString := stmt.(*build.StringExpr)
+		isDocString := isString && i == 0
 		if isComment || isDocString || added {
 			all = append(all, stmt)
 			continue
diff --git a/warn/warn.go b/warn/warn.go
index ae81a7e..38d5fba 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -446,6 +446,8 @@ var FileWarningMap = map[string]func(f *build.File, fix bool) []*Finding{
 	"ctx-actions":        ctxActionsWarning,
 	"duplicated-name":    duplicatedNameWarning,
 	"filetype":           fileTypeWarning,
+	"git-repository":     nativeGitRepositoryWarning,
+	"http-archive":       nativeHttpArchiveWarning,
 	"integer-division":   integerDivisionWarning,
 	"load":               unusedLoadWarning,
 	"load-on-top":        loadOnTopWarning,
diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go
index 63b1e61..ba82e0f 100644
--- a/warn/warn_bazel.go
+++ b/warn/warn_bazel.go
@@ -3,6 +3,7 @@ package warn
 import (
 	"github.com/bazelbuild/buildtools/build"
 	"github.com/bazelbuild/buildtools/bzlenv"
+	"github.com/bazelbuild/buildtools/edit"
 )
 
 // Bazel-specific warnings
@@ -112,6 +113,52 @@ func globalVariableUsageCheck(f *build.File, category, global, alternative strin
 	return findings
 }
 
+// notLoadedFunctionUsageCheck checks whether there's a usage of a given not imported  function in the file
+// and adds a load statement if necessary.
+func notLoadedFunctionUsageCheck(f *build.File, category string, globals []string, loadFrom string, fix bool) []*Finding {
+	findings := []*Finding{}
+	toLoad := []string{}
+
+	var walk func(e *build.Expr, env *bzlenv.Environment)
+	walk = func(e *build.Expr, env *bzlenv.Environment) {
+		defer bzlenv.WalkOnceWithEnvironment(*e, env, walk)
+
+		call, ok := (*e).(*build.CallExpr)
+		if !ok {
+			return
+		}
+
+		ident, ok := (call.X).(*build.Ident)
+		if !ok {
+			return
+		}
+
+		if binding := env.Get(ident.Name); binding != nil {
+			return
+		}
+		for _, global := range globals {
+			if ident.Name == global {
+				if fix {
+					toLoad = append(toLoad, global)
+					return
+				}
+				start, end := call.Span()
+				findings = append(findings,
+					makeFinding(f, start, end, category,
+						"Function \""+global+"\" is not global anymore and needs to be loaded from \""+loadFrom+"\".", true, nil))
+			}
+		}
+	}
+	var expr build.Expr = f
+	walk(&expr, bzlenv.NewEnvironment())
+
+	if fix && len(toLoad) > 0 {
+		f.Stmt = edit.InsertLoad(f.Stmt, loadFrom, toLoad, toLoad)
+	}
+
+	return findings
+}
+
 func attrConfigurationWarning(f *build.File, fix bool) []*Finding {
 	findings := []*Finding{}
 	build.Walk(f, func(expr build.Expr, stack []build.Expr) {
@@ -331,3 +378,11 @@ func outputGroupWarning(f *build.File, fix bool) []*Finding {
 	})
 	return findings
 }
+
+func nativeGitRepositoryWarning(f *build.File, fix bool) []*Finding {
+	return notLoadedFunctionUsageCheck(f, "git-repository", []string{"git_repository", "new_git_repository"}, "@bazel_tools//tools/build_defs/repo:git.bzl", fix)
+}
+
+func nativeHttpArchiveWarning(f *build.File, fix bool) []*Finding {
+	return notLoadedFunctionUsageCheck(f, "http-archive", []string{"http_archive"}, "@bazel_tools//tools/build_defs/repo:http.bzl", fix)
+}
\ No newline at end of file


Test patch:
diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index ca2d874..05d4de4 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -32,12 +32,10 @@ rule(
       "bar": attr.label_list(mandatory = True, non_empty = False),
       "baz": attr.label_list(mandatory = True, non_empty = foo.bar()),
       "qux": attr.label_list(mandatory = True, non_empty = not foo.bar()),
-      "aaa": attr.label_list(mandatory = True, non_empty = (
-					foo.bar())
-			),
+      "aaa": attr.label_list(mandatory = True, non_empty = (foo.bar())),
       "bbb": attr.label_list(mandatory = True, non_empty = (
-					not foo.bar())
-			),
+					not foo.bar()
+			)),
   }
 )`, `
 rule(
@@ -46,12 +44,10 @@ rule(
       "bar": attr.label_list(mandatory = True, allow_empty = True),
       "baz": attr.label_list(mandatory = True, allow_empty = not foo.bar()),
       "qux": attr.label_list(mandatory = True, allow_empty = foo.bar()),
-      "aaa": attr.label_list(mandatory = True, allow_empty = (
-					not foo.bar())
-			),
+      "aaa": attr.label_list(mandatory = True, allow_empty = (not foo.bar())),
       "bbb": attr.label_list(mandatory = True, allow_empty = (
-					foo.bar())
-			),
+					foo.bar()
+			)),
   }
 )`,
 		[]string{
@@ -60,7 +56,7 @@ rule(
 			":5: non_empty attributes for attr definitions are deprecated in favor of allow_empty.",
 			":6: non_empty attributes for attr definitions are deprecated in favor of allow_empty.",
 			":7: non_empty attributes for attr definitions are deprecated in favor of allow_empty.",
-			":10: non_empty attributes for attr definitions are deprecated in favor of allow_empty.",
+			":8: non_empty attributes for attr definitions are deprecated in favor of allow_empty.",
 		},
 		false)
 }
@@ -233,3 +229,86 @@ def _impl(ctx):
 		},
 		false)
 }
+
+func TestNativeGitRepositoryWarning(t *testing.T) {
+	checkFindingsAndFix(t, "git-repository", `
+"""My file"""
+
+def macro():
+    git_repository(foo, bar)
+`, `
+"""My file"""
+
+load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")
+
+def macro():
+    git_repository(foo, bar)
+`,
+		[]string{
+			":4: Function \"git_repository\" is not global anymore and needs to be loaded from \"@bazel_tools//tools/build_defs/repo:git.bzl\".",
+		},
+		false)
+
+	checkFindingsAndFix(t, "git-repository", `
+"""My file"""
+
+def macro():
+    git_repository(foo, bar)
+    new_git_repository(foo, bar)
+`, `
+"""My file"""
+
+load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository", "new_git_repository")
+
+def macro():
+    git_repository(foo, bar)
+    new_git_repository(foo, bar)
+`,
+		[]string{
+			":4: Function \"git_repository\" is not global anymore and needs to be loaded from \"@bazel_tools//tools/build_defs/repo:git.bzl\".",
+			":5: Function \"new_git_repository\" is not global anymore and needs to be loaded from \"@bazel_tools//tools/build_defs/repo:git.bzl\".",
+		},
+		false)
+
+	checkFindingsAndFix(t, "git-repository", `
+"""My file"""
+
+load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")
+
+def macro():
+    git_repository(foo, bar)
+    new_git_repository(foo, bar)
+`, `
+"""My file"""
+
+load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository", "new_git_repository")
+
+def macro():
+    git_repository(foo, bar)
+    new_git_repository(foo, bar)
+`,
+		[]string{
+			":7: Function \"new_git_repository\" is not global anymore and needs to be loaded from \"@bazel_tools//tools/build_defs/repo:git.bzl\".",
+		},
+		false)
+}
+
+func TestNativeHttpArchiveWarning(t *testing.T) {
+	checkFindingsAndFix(t, "http-archive", `
+"""My file"""
+
+def macro():
+    http_archive(foo, bar)
+`, `
+"""My file"""
+
+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
+
+def macro():
+    http_archive(foo, bar)
+`,
+		[]string{
+			":4: Function \"http_archive\" is not global anymore and needs to be loaded from \"@bazel_tools//tools/build_defs/repo:http.bzl\".",
+		},
+		false)
+}