Repository: bazelbuild/buildtools
Pull request #674 (2019-07-19 13:08:34)

Add native-proto fix to buildifier
This change adds a fix for the native-to-starlark migration of Protobuf
rules. Also, the implementation is refactored to add support for warnings
when symbols should be loaded.

/cc @hlopko @lberki @iirina 

lberki: /cc @lberki 

I'm not fully qualified to review this, so I'll let the others shine, but I'm happy that this is happening!
Yannic: @vladmos Can we get this in before Bazel 0.29.0 is released? https://github.com/bazelbuild/bazel/issues/8922

Patch:
diff --git a/WARNINGS.md b/WARNINGS.md
index bebe95d..2e8a988 100644
--- a/WARNINGS.md
+++ b/WARNINGS.md
@@ -33,6 +33,7 @@ Warning categories supported by buildifier's linter:
   * [native-cc](#native-cc)
   * [native-java](#native-java)
   * [native-package](#native-package)
+  * [native-proto](#native-proto)
   * [no-effect](#no-effect)
   * [out-of-order-load](#out-of-order-load)
   * [output-group](#output-group)
@@ -327,9 +328,9 @@ A docstring is a string statement which should be the first statement of the fil
 following way:
 
     """One-line summary: must be followed and may be preceded by a blank line.
-    
+
     Optional additional description like this.
-    
+
     If it's a function docstring and the function has more than one argument, the docstring has
     to document these parameters as follows:
 
@@ -446,7 +447,7 @@ they can follow only comments and docstrings.
   * Automatic fix: no
 
 `.bzl` files should have docstrings on top of them. A docstring is a string statement
-which should be the first statement of the file (it may follow comment lines). 
+which should be the first statement of the file (it may follow comment lines).
 
 --------------------------------------------------------------------------------
 
@@ -512,6 +513,17 @@ modify the semantics of a BUILD file and makes it hard to maintain.
 
 --------------------------------------------------------------------------------
 
+## <a name="native-proto"></a>All Proto build rules and symbols should be loaded from Starlark
+
+  * Category name: `native-proto`
+  * Flag in Bazel: [`--incompatible_load_proto_rules_from_bzl`](https://github.com/bazelbuild/bazel/issues/8922)
+  * Automatic fix: yes
+
+The Proto build rules and symbols should be loaded from Starlark. The native
+rules [will be disabled](https://github.com/bazelbuild/bazel/issues/8922).
+
+--------------------------------------------------------------------------------
+
 ## <a name="no-effect"></a>Expression result is not used
 
   * Category name: `no-effect`
diff --git a/tables/tables.go b/tables/tables.go
index 2dfe75a..9382843 100644
--- a/tables/tables.go
+++ b/tables/tables.go
@@ -258,6 +258,21 @@ var JavaNativeRules = []string{
 // JavaLoadPath is the load path for the Starlark Java Rules.
 var JavaLoadPath = "@rules_java//java:defs.bzl"
 
+// ProtoNativeRules lists all Proto rules that are being migrated from Native to Starlark.
+var ProtoNativeRules = []string{
+	"proto_lang_toolchain",
+	"proto_library",
+}
+
+// ProtoNativeSymbols lists all Proto symbols that are being migrated from Native to Starlark.
+var ProtoNativeSymbols = []string{
+	"ProtoInfo",
+	"proto_common",
+}
+
+// ProtoLoadPath is the load path for the Starlark Proto Rules.
+var ProtoLoadPath = "@rules_proto//proto:defs.bzl"
+
 // OverrideTables allows a user of the build package to override the special-case rules. The user-provided tables replace the built-in tables.
 func OverrideTables(labelArg, blacklist, listArg, sortableListArg, sortBlacklist, sortWhitelist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool) {
 	IsLabelArg = labelArg
diff --git a/warn/warn.go b/warn/warn.go
index b4fd56d..d65c4e0 100644
--- a/warn/warn.go
+++ b/warn/warn.go
@@ -131,6 +131,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{
 	"native-cc":                 nativeCcRulesWarning,
 	"native-java":               nativeJavaRulesWarning,
 	"native-package":            nativePackageWarning,
+	"native-proto":              nativeProtoRulesWarning,
 	"no-effect":                 noEffectWarning,
 	"output-group":              outputGroupWarning,
 	"out-of-order-load":         outOfOrderLoadWarning,
diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go
index d5bd901..30ef934 100644
--- a/warn/warn_bazel_api.go
+++ b/warn/warn_bazel_api.go
@@ -164,9 +164,79 @@ func insertLoad(f *build.File, module string, symbols []string) *LinterReplaceme
 	return &LinterReplacement{&(f.Stmt[i]), edit.NewLoad(module, symbols, symbols)}
 }
 
-// notLoadedFunctionUsageCheck checks whether there's a usage of a given not imported function in the file
+func notLoadedFunctionUsageCheckInternal(expr *build.Expr, env *bzlenv.Environment, globals []string, loadFrom string) ([]string, []*LinterFinding) {
+	var loads []string
+	var findings []*LinterFinding
+
+	call, ok := (*expr).(*build.CallExpr)
+	if !ok {
+		return loads, findings
+	}
+
+	var name string
+	var replacements []LinterReplacement
+	switch node := call.X.(type) {
+	case *build.DotExpr:
+		// Maybe native.`global`?
+		ident, ok := node.X.(*build.Ident)
+		if !ok || ident.Name != "native" {
+			return loads, findings
+		}
+
+		name = node.Name
+		// Replace `native.foo()` with `foo()`
+		newCall := *call
+		newCall.X = &build.Ident{Name: node.Name}
+		replacements = append(replacements, LinterReplacement{expr, &newCall})
+	case *build.Ident:
+		// Maybe `global`()?
+		if binding := env.Get(node.Name); binding != nil {
+			return loads, findings
+		}
+		name = node.Name
+	default:
+		return loads, findings
+	}
+
+	for _, global := range globals {
+		if name == global {
+			loads = append(loads, name)
+			findings = append(findings,
+				makeLinterFinding(call, fmt.Sprintf(`Function %q is not global anymore and needs to be loaded from %q.`, global, loadFrom), replacements...))
+			break
+		}
+	}
+
+	return loads, findings
+}
+
+func notLoadedSymbolUsageCheckInternal(expr *build.Expr, env *bzlenv.Environment, globals []string, loadFrom string) ([]string, []*LinterFinding) {
+	var loads []string
+	var findings []*LinterFinding
+
+	ident, ok := (*expr).(*build.Ident)
+	if !ok {
+		return loads, findings
+	}
+	if binding := env.Get(ident.Name); binding != nil {
+		return loads, findings
+	}
+
+	for _, global := range globals {
+		if ident.Name == global {
+			loads = append(loads, ident.Name)
+			findings = append(findings,
+				makeLinterFinding(ident, fmt.Sprintf(`Symbol %q is not global anymore and needs to be loaded from %q.`, global, loadFrom)))
+			break
+		}
+	}
+
+	return loads, findings
+}
+
+// notLoadedUsageCheck checks whether there's a usage of a given not imported function or symbol in the file
 // and adds a load statement if necessary.
-func notLoadedFunctionUsageCheck(f *build.File, globals []string, loadFrom string) []*LinterFinding {
+func notLoadedUsageCheck(f *build.File, functions, symbols []string, loadFrom string) []*LinterFinding {
 	toLoad := make(map[string]bool)
 	var findings []*LinterFinding
 
@@ -174,43 +244,16 @@ func notLoadedFunctionUsageCheck(f *build.File, globals []string, loadFrom strin
 	walk = func(expr *build.Expr, env *bzlenv.Environment) {
 		defer bzlenv.WalkOnceWithEnvironment(*expr, env, walk)
 
-		call, ok := (*expr).(*build.CallExpr)
-		if !ok {
-			return
+		functionLoads, functionFindings := notLoadedFunctionUsageCheckInternal(expr, env, functions, loadFrom)
+		findings = append(findings, functionFindings...)
+		for _, load := range functionLoads {
+			toLoad[load] = true
 		}
 
-		var name string
-		var replacements []LinterReplacement
-		switch node := call.X.(type) {
-		case *build.DotExpr:
-			// Maybe native.`global`?
-			ident, ok := node.X.(*build.Ident)
-			if !ok || ident.Name != "native" {
-				return
-			}
-
-			name = node.Name
-			// Replace `native.foo()` with `foo()`
-			newCall := *call
-			newCall.X = &build.Ident{Name: node.Name}
-			replacements = append(replacements, LinterReplacement{expr, &newCall})
-		case *build.Ident:
-			// Maybe `global`()?
-			if binding := env.Get(node.Name); binding != nil {
-				return
-			}
-			name = node.Name
-		default:
-			return
-		}
-
-		for _, global := range globals {
-			if name == global {
-				toLoad[global] = true
-				findings = append(findings,
-					makeLinterFinding(call, fmt.Sprintf(`Function %q is not global anymore and needs to be loaded from %q.`, global, loadFrom), replacements...))
-				break
-			}
+		symbolLoads, symbolFindings := notLoadedSymbolUsageCheckInternal(expr, env, symbols, loadFrom)
+		findings = append(findings, symbolFindings...)
+		for _, load := range symbolLoads {
+			toLoad[load] = true
 		}
 	}
 	var expr build.Expr = f
@@ -237,6 +280,12 @@ func notLoadedFunctionUsageCheck(f *build.File, globals []string, loadFrom 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, globals []string, loadFrom string) []*LinterFinding {
+	return notLoadedUsageCheck(f, globals, []string{}, loadFrom)
+}
+
 // makePositional makes the function argument positional (removes the keyword if it exists)
 func makePositional(argument build.Expr) build.Expr {
 	if binary, ok := argument.(*build.AssignExpr); ok {
@@ -592,6 +641,13 @@ func nativeJavaRulesWarning(f *build.File) []*LinterFinding {
 	return notLoadedFunctionUsageCheck(f, tables.JavaNativeRules, tables.JavaLoadPath)
 }
 
+func nativeProtoRulesWarning(f *build.File) []*LinterFinding {
+	if f.Type != build.TypeBzl && f.Type != build.TypeBuild {
+		return nil
+	}
+	return notLoadedUsageCheck(f, tables.ProtoNativeRules, tables.ProtoNativeSymbols, tables.ProtoLoadPath)
+}
+
 func contextArgsAPIWarning(f *build.File) []*LinterFinding {
 	if f.Type != build.TypeBzl {
 		return nil


Test patch:
diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go
index 9bd1be3..eab1190 100644
--- a/warn/warn_bazel_api_test.go
+++ b/warn/warn_bazel_api_test.go
@@ -579,3 +579,40 @@ java_test()
 		},
 		scopeBzl|scopeBuild)
 }
+
+func TestNativeProtoWarning(t *testing.T) {
+	checkFindingsAndFix(t, "native-proto", `
+"""My file"""
+
+def macro():
+    proto_library()
+    proto_lang_toolchain()
+    native.proto_lang_toolchain()
+    native.proto_library()
+
+    ProtoInfo
+    proto_common
+`, fmt.Sprintf(`
+"""My file"""
+
+load(%q, "ProtoInfo", "proto_common", "proto_lang_toolchain", "proto_library")
+
+def macro():
+    proto_library()
+    proto_lang_toolchain()
+    proto_lang_toolchain()
+    proto_library()
+
+    ProtoInfo
+    proto_common
+`, tables.ProtoLoadPath),
+		[]string{
+			fmt.Sprintf(`:4: Function "proto_library" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
+			fmt.Sprintf(`:5: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
+			fmt.Sprintf(`:6: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
+			fmt.Sprintf(`:7: Function "proto_library" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
+			fmt.Sprintf(`:9: Symbol "ProtoInfo" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
+			fmt.Sprintf(`:10: Symbol "proto_common" is not global anymore and needs to be loaded from "%s".`, tables.ProtoLoadPath),
+		},
+		scopeBzl|scopeBuild)
+}