From 1f0b36d0af488e0f607ddb9057942f88c6c59085 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Wed, 27 Nov 2024 19:15:24 +0100 Subject: [PATCH 1/6] Require proto rules to be loaded by buildifier Because the rules are moved into the repository the loads become mandatory with Bazel@HEAD. Add new warning for each rule and symbol that needs to be loaded. Buildifier doesn't handle well loading from different bzl files in a single warning. Enable all proto rules warning by default. --- buildifier/config/config_test.go | 26 ++++++++++++++++++++- tables/tables.go | 19 ++------------- warn/docs/warnings.textproto | 7 ++---- warn/warn.go | 10 ++++++-- warn/warn_bazel_api.go | 29 +++++++++++++++++++++-- warn/warn_bazel_api_test.go | 40 ++++++++++++++++---------------- warn/warn_test.go | 26 ++++++++++----------- 7 files changed, 97 insertions(+), 60 deletions(-) diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index ff120d393..88f8318e0 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -79,9 +79,16 @@ func ExampleExample() { // "native-android", // "native-build", // "native-cc", + // "native-cc-proto", // "native-java", + // "native-java-lite-proto", + // "native-java-proto", // "native-package", // "native-proto", + // "native-proto-common", + // "native-proto-info", + // "native-proto-lang-toolchain", + // "native-proto-lang-toolchain-info", // "native-py", // "no-effect", // "output-group", @@ -260,9 +267,16 @@ func TestValidate(t *testing.T) { "native-android", "native-build", "native-cc", + "native-cc-proto", "native-java", + native-java-lite-proto", + "native-java-proto", "native-package", "native-proto", + "native-proto-common", + "native-proto-info", + "native-proto-lang-toolchain", + "native-proto-lang-toolchain-info", "native-py", "no-effect", "output-group", @@ -322,9 +336,16 @@ func TestValidate(t *testing.T) { // "native-android", "native-build", // "native-cc", + "native-cc-proto", // "native-java", + "native-java-lite-proto", + "native-java-proto", "native-package", - // "native-proto", + "native-proto", + "native-proto-common", + "native-proto-info", + "native-proto-lang-toolchain", + "native-proto-lang-toolchain-info", // "native-py", "no-effect", "output-group", @@ -383,7 +404,10 @@ func TestValidate(t *testing.T) { "name-conventions", // "native-android", "native-build", + "native-cc-proto", // "native-java", + "native-java-lite-proto", + "native-java-proto", "native-package", // "native-proto", // "native-py", diff --git a/tables/tables.go b/tables/tables.go index 720487edb..12cad176b 100644 --- a/tables/tables.go +++ b/tables/tables.go @@ -234,7 +234,6 @@ var CcNativeRules = []string{ "cc_test", "cc_library", "cc_import", - "cc_proto_library", "fdo_prefetch_hints", "fdo_profile", "cc_toolchain", @@ -251,8 +250,6 @@ var JavaNativeRules = []string{ "java_binary", "java_import", "java_library", - "java_lite_proto_library", - "java_proto_library", "java_test", "java_package_configuration", "java_plugin", @@ -274,20 +271,8 @@ var PyNativeRules = []string{ // PyLoadPath is the load path for the Starlark Python Rules. var PyLoadPath = "@rules_python//python: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" +// ProtoLoadPathPrefix is the load path prefix for the Starlark Proto Rules. +var ProtoLoadPathPrefix = "@com_google_protobuf//bazel" // IsModuleOverride contains the names of all Bzlmod module overrides available in MODULE.bazel. var IsModuleOverride = map[string]bool{ diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index fa42da71c..d8d4bfd58 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -518,11 +518,8 @@ warnings: { header: "All Proto build rules and symbols should be loaded from Starlark" description: "The Proto build rules should be loaded from Starlark.\n\n" - "Update: the plans for disabling native rules\n" - "[have been postponed](https://groups.google.com/g/bazel-discuss/c/XNvpWcge4AE/m/aJ-aQzszAwAJ),\n" - "at the moment it's not required to load Starlark rules." - bazel_flag: "--incompatible_load_proto_rules_from_bzl" - bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/8922" + bazel_flag: "--incompatible_autoload_externally" + bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true } diff --git a/warn/warn.go b/warn/warn.go index 6f7c24e51..818fa8387 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -155,7 +155,14 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ "native-cc": nativeCcRulesWarning, "native-java": nativeJavaRulesWarning, "native-package": nativePackageWarning, - "native-proto": nativeProtoRulesWarning, + "native-proto": NativeProtoRulesWarning("proto_library"), + "native-java-proto": NativeProtoRulesWarning("java_proto_library"), + "native-java-lite-proto": NativeProtoRulesWarning("java_lite_proto_library"), + "native-cc-proto": NativeProtoRulesWarning("cc_proto_library"), + "native-proto-lang-toolchain": nativeProtoLangToolchainWarning, + "native-proto-info": nativeProtoSymbolsWarning("ProtoInfo", "proto_info.bzl"), + "native-proto-common": nativeProtoSymbolsWarning("proto_common", "proto_common.bzl"), + "native-proto-lang-toolchain-info": nativeProtoSymbolsWarning("ProtoLangToolchainInfo", "proto_lang_toolchain_info.bzl"), "native-py": nativePyRulesWarning, "no-effect": noEffectWarning, "output-group": outputGroupWarning, @@ -190,7 +197,6 @@ var nonDefaultWarnings = map[string]bool{ "native-android": true, // disables native android rules "native-cc": true, // disables native cc rules "native-java": true, // disables native java rules - "native-proto": true, // disables native proto rules "native-py": true, // disables native python rules } diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go index ecce12fa4..5d73f258d 100644 --- a/warn/warn_bazel_api.go +++ b/warn/warn_bazel_api.go @@ -293,6 +293,12 @@ func NotLoadedFunctionUsageCheck(f *build.File, globals []string, loadFrom strin return notLoadedUsageCheck(f, globals, []string{}, loadFrom) } +// NotLoadedSymbolUsageCheck checks whether there's a usage of a given not imported function in the file +// and adds a load statement if necessary. +func NotLoadedSymbolUsageCheck(f *build.File, globals []string, loadFrom string) []*LinterFinding { + return notLoadedUsageCheck(f, []string{}, globals, 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 { @@ -709,11 +715,30 @@ func nativePyRulesWarning(f *build.File) []*LinterFinding { return NotLoadedFunctionUsageCheck(f, tables.PyNativeRules, tables.PyLoadPath) } -func nativeProtoRulesWarning(f *build.File) []*LinterFinding { +// NativeProtoRulesWarning produces a warning for missing loads of proto rules +func NativeProtoRulesWarning(rule string) func(f *build.File) []*LinterFinding { + return func(f *build.File) []*LinterFinding { + if f.Type != build.TypeBzl && f.Type != build.TypeBuild { + return nil + } + return NotLoadedFunctionUsageCheck(f, []string{rule}, tables.ProtoLoadPathPrefix + ":" + rule + ".bzl") + } +} + +func nativeProtoLangToolchainWarning(f *build.File) []*LinterFinding { if f.Type != build.TypeBzl && f.Type != build.TypeBuild { return nil } - return notLoadedUsageCheck(f, tables.ProtoNativeRules, tables.ProtoNativeSymbols, tables.ProtoLoadPath) + return NotLoadedFunctionUsageCheck(f, []string{"proto_lang_toolchain"}, tables.ProtoLoadPathPrefix + "/toolchains:proto_lang_toolchain.bzl") +} + +func nativeProtoSymbolsWarning(symbol string, bzlfile string) func(f *build.File) []*LinterFinding { + return func(f *build.File) []*LinterFinding { + if f.Type != build.TypeBzl && f.Type != build.TypeBuild { + return nil + } + return NotLoadedSymbolUsageCheck(f, []string{symbol}, tables.ProtoLoadPathPrefix + "/common:" + bzlfile) + } } func contextArgsAPIWarning(f *build.File) []*LinterFinding { diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go index b23f970ce..d887f3d77 100644 --- a/warn/warn_bazel_api_test.go +++ b/warn/warn_bazel_api_test.go @@ -552,7 +552,6 @@ def macro(): cc_library() native.cc_binary() cc_test() - cc_proto_library() native.fdo_prefetch_hints() native.objc_library() objc_import() @@ -564,13 +563,12 @@ cc_import() `, fmt.Sprintf(` """My file""" -load(%q, "cc_binary", "cc_import", "cc_library", "cc_proto_library", "cc_test", "cc_toolchain", "cc_toolchain_suite", "fdo_prefetch_hints", "fdo_profile", "objc_import", "objc_library") +load(%q, "cc_binary", "cc_import", "cc_library", "cc_test", "cc_toolchain", "cc_toolchain_suite", "fdo_prefetch_hints", "fdo_profile", "objc_import", "objc_library") def macro(): cc_library() cc_binary() cc_test() - cc_proto_library() fdo_prefetch_hints() objc_library() objc_import() @@ -584,14 +582,13 @@ cc_import() fmt.Sprintf(`:4: Function "cc_library" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), fmt.Sprintf(`:5: Function "cc_binary" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), fmt.Sprintf(`:6: Function "cc_test" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), - fmt.Sprintf(`:7: Function "cc_proto_library" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), - fmt.Sprintf(`:8: Function "fdo_prefetch_hints" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), - fmt.Sprintf(`:9: Function "objc_library" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), - fmt.Sprintf(`:10: Function "objc_import" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), - fmt.Sprintf(`:11: Function "cc_toolchain" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), - fmt.Sprintf(`:12: Function "cc_toolchain_suite" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), - fmt.Sprintf(`:14: Function "fdo_profile" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), - fmt.Sprintf(`:15: Function "cc_import" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), + fmt.Sprintf(`:7: Function "fdo_prefetch_hints" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), + fmt.Sprintf(`:8: Function "objc_library" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), + fmt.Sprintf(`:9: Function "objc_import" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), + fmt.Sprintf(`:10: Function "cc_toolchain" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), + fmt.Sprintf(`:11: Function "cc_toolchain_suite" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), + fmt.Sprintf(`:13: Function "fdo_profile" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), + fmt.Sprintf(`:14: Function "cc_import" is not global anymore and needs to be loaded from "%s".`, tables.CcLoadPath), }, scopeBzl|scopeBuild) } @@ -665,7 +662,7 @@ py_test() } func TestNativeProtoWarning(t *testing.T) { - checkFindingsAndFix(t, "native-proto", ` + checkFindingsAndFix(t, "native-proto,native-proto-lang-toolchain,native-proto-info,native-proto-common", ` """My file""" def macro(): @@ -679,7 +676,10 @@ def macro(): `, fmt.Sprintf(` """My file""" -load(%q, "ProtoInfo", "proto_common", "proto_lang_toolchain", "proto_library") +load("%[1]s:proto_library.bzl", "proto_library") +load("%[1]s/common:proto_common.bzl", "proto_common") +load("%[1]s/common:proto_info.bzl", "ProtoInfo") +load("%[1]s/toolchains:proto_lang_toolchain.bzl", "proto_lang_toolchain") def macro(): proto_library() @@ -689,14 +689,14 @@ def macro(): ProtoInfo proto_common -`, tables.ProtoLoadPath), +`, tables.ProtoLoadPathPrefix), []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), + fmt.Sprintf(`:4: Function "proto_library" is not global anymore and needs to be loaded from "%s:proto_library.bzl".`, tables.ProtoLoadPathPrefix), + fmt.Sprintf(`:5: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s/toolchains:proto_lang_toolchain.bzl".`, tables.ProtoLoadPathPrefix), + fmt.Sprintf(`:6: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s/toolchains:proto_lang_toolchain.bzl".`, tables.ProtoLoadPathPrefix), + fmt.Sprintf(`:7: Function "proto_library" is not global anymore and needs to be loaded from "%s:proto_library.bzl".`, tables.ProtoLoadPathPrefix), + fmt.Sprintf(`:9: Symbol "ProtoInfo" is not global anymore and needs to be loaded from "%s/common:proto_info.bzl".`, tables.ProtoLoadPathPrefix), + fmt.Sprintf(`:10: Symbol "proto_common" is not global anymore and needs to be loaded from "%s/common:proto_common.bzl".`, tables.ProtoLoadPathPrefix), }, scopeBzl|scopeBuild) } diff --git a/warn/warn_test.go b/warn/warn_test.go index 0ceccb600..38c2fadff 100644 --- a/warn/warn_test.go +++ b/warn/warn_test.go @@ -104,18 +104,18 @@ func getFileForTest(input string, fileType build.FileType) *build.File { return file } -func getFindings(category, input string, fileType build.FileType) []*Finding { +func getFindings(categories, input string, fileType build.FileType) []*Finding { file := getFileForTest(input, fileType) - return FileWarnings(file, []string{category}, nil, ModeWarn, testFileReader) + return FileWarnings(file, strings.Split(categories,","), nil, ModeWarn, testFileReader) } -func compareFindings(t *testing.T, category, input string, expected []string, scope, fileType build.FileType) { +func compareFindings(t *testing.T, categories, input string, expected []string, scope, fileType build.FileType) { // If scope doesn't match the file type, no warnings are expected if scope&fileType == 0 { expected = []string{} } - findings := getFindings(category, input, fileType) + findings := getFindings(categories, input, fileType) // We ensure that there is the expected number of warnings. // At the moment, we check only the line numbers. if len(expected) != len(findings) { @@ -139,7 +139,7 @@ func compareFindings(t *testing.T, category, input string, expected []string, sc } // checkFix makes sure that fixed file contents match the expected output -func checkFix(t *testing.T, category, input, expected string, scope, fileType build.FileType) { +func checkFix(t *testing.T, categories, input, expected string, scope, fileType build.FileType) { // If scope doesn't match the file type, no changes are expected if scope&fileType == 0 { expected = input @@ -148,7 +148,7 @@ func checkFix(t *testing.T, category, input, expected string, scope, fileType bu file := getFileForTest(input, fileType) goldenFile := getFileForTest(expected, fileType) - FixWarnings(file, []string{category}, false, testFileReader) + FixWarnings(file, strings.Split(categories, ","), false, testFileReader) have := build.Format(file) want := build.Format(goldenFile) if !bytes.Equal(have, want) { @@ -160,12 +160,12 @@ func checkFix(t *testing.T, category, input, expected string, scope, fileType bu // checkFix makes sure that the file contents don't change if a fix is not requested // (i.e. the warning functions have no side effects modifying the AST) -func checkNoFix(t *testing.T, category, input string, fileType build.FileType) { +func checkNoFix(t *testing.T, categories, input string, fileType build.FileType) { file := getFileForTest(input, fileType) formatted := build.Format(file) // No fixes expected - FileWarnings(file, []string{category}, nil, ModeWarn, testFileReader) + FileWarnings(file, strings.Split(categories, ","), nil, ModeWarn, testFileReader) fixed := build.FormatWithoutRewriting(file) if !bytes.Equal(formatted, fixed) { @@ -180,7 +180,7 @@ func checkFindings(t *testing.T, category, input string, expected []string, scop checkFindingsAndFix(t, category, input, input, expected, scope) } -func checkFindingsAndFix(t *testing.T, category, input, output string, expected []string, scope build.FileType) { +func checkFindingsAndFix(t *testing.T, categories, input, output string, expected []string, scope build.FileType) { fileTypes := []build.FileType{ build.TypeDefault, build.TypeBuild, @@ -190,10 +190,10 @@ func checkFindingsAndFix(t *testing.T, category, input, output string, expected } for _, fileType := range fileTypes { - compareFindings(t, category, input, expected, scope, fileType) - checkFix(t, category, input, output, scope, fileType) - checkFix(t, category, output, output, scope, fileType) - checkNoFix(t, category, input, fileType) + compareFindings(t, categories, input, expected, scope, fileType) + checkFix(t, categories, input, output, scope, fileType) + checkFix(t, categories, output, output, scope, fileType) + checkNoFix(t, categories, input, fileType) } } From 3c8b65dcfdda6533f1fe66511f8412d554955c9b Mon Sep 17 00:00:00 2001 From: Ivo List Date: Thu, 28 Nov 2024 13:09:14 +0100 Subject: [PATCH 2/6] Format with gofmt --- warn/warn.go | 126 +++++++++++++++++++++++----------------------- warn/warn_test.go | 2 +- 2 files changed, 64 insertions(+), 64 deletions(-) diff --git a/warn/warn.go b/warn/warn.go index 818fa8387..95f5d01f0 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -118,70 +118,70 @@ var RuleWarningMap = map[string]func(call *build.CallExpr, pkg string) *LinterFi // FileWarningMap lists the warnings that run on the whole file. var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ - "attr-applicable_licenses": attrApplicableLicensesWarning, - "attr-cfg": attrConfigurationWarning, - "attr-license": attrLicenseWarning, - "attr-licenses": attrLicensesWarning, - "attr-non-empty": attrNonEmptyWarning, - "attr-output-default": attrOutputDefaultWarning, - "attr-single-file": attrSingleFileWarning, - "build-args-kwargs": argsKwargsInBuildFilesWarning, - "bzl-visibility": bzlVisibilityWarning, - "confusing-name": confusingNameWarning, - "constant-glob": constantGlobWarning, - "ctx-actions": ctxActionsWarning, - "ctx-args": contextArgsAPIWarning, - "depset-items": depsetItemsWarning, - "depset-iteration": depsetIterationWarning, - "depset-union": depsetUnionWarning, - "dict-method-named-arg": dictMethodNamedArgWarning, - "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, - "keyword-positional-params": keywordPositionalParametersWarning, - "list-append": listAppendWarning, - "load": unusedLoadWarning, - "module-docstring": moduleDocstringWarning, - "name-conventions": nameConventionsWarning, - "native-android": nativeAndroidRulesWarning, - "native-build": nativeInBuildFilesWarning, - "native-cc": nativeCcRulesWarning, - "native-java": nativeJavaRulesWarning, - "native-package": nativePackageWarning, - "native-proto": NativeProtoRulesWarning("proto_library"), - "native-java-proto": NativeProtoRulesWarning("java_proto_library"), - "native-java-lite-proto": NativeProtoRulesWarning("java_lite_proto_library"), - "native-cc-proto": NativeProtoRulesWarning("cc_proto_library"), - "native-proto-lang-toolchain": nativeProtoLangToolchainWarning, - "native-proto-info": nativeProtoSymbolsWarning("ProtoInfo", "proto_info.bzl"), - "native-proto-common": nativeProtoSymbolsWarning("proto_common", "proto_common.bzl"), + "attr-applicable_licenses": attrApplicableLicensesWarning, + "attr-cfg": attrConfigurationWarning, + "attr-license": attrLicenseWarning, + "attr-licenses": attrLicensesWarning, + "attr-non-empty": attrNonEmptyWarning, + "attr-output-default": attrOutputDefaultWarning, + "attr-single-file": attrSingleFileWarning, + "build-args-kwargs": argsKwargsInBuildFilesWarning, + "bzl-visibility": bzlVisibilityWarning, + "confusing-name": confusingNameWarning, + "constant-glob": constantGlobWarning, + "ctx-actions": ctxActionsWarning, + "ctx-args": contextArgsAPIWarning, + "depset-items": depsetItemsWarning, + "depset-iteration": depsetIterationWarning, + "depset-union": depsetUnionWarning, + "dict-method-named-arg": dictMethodNamedArgWarning, + "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, + "keyword-positional-params": keywordPositionalParametersWarning, + "list-append": listAppendWarning, + "load": unusedLoadWarning, + "module-docstring": moduleDocstringWarning, + "name-conventions": nameConventionsWarning, + "native-android": nativeAndroidRulesWarning, + "native-build": nativeInBuildFilesWarning, + "native-cc": nativeCcRulesWarning, + "native-java": nativeJavaRulesWarning, + "native-package": nativePackageWarning, + "native-proto": NativeProtoRulesWarning("proto_library"), + "native-java-proto": NativeProtoRulesWarning("java_proto_library"), + "native-java-lite-proto": NativeProtoRulesWarning("java_lite_proto_library"), + "native-cc-proto": NativeProtoRulesWarning("cc_proto_library"), + "native-proto-lang-toolchain": nativeProtoLangToolchainWarning, + "native-proto-info": nativeProtoSymbolsWarning("ProtoInfo", "proto_info.bzl"), + "native-proto-common": nativeProtoSymbolsWarning("proto_common", "proto_common.bzl"), "native-proto-lang-toolchain-info": nativeProtoSymbolsWarning("ProtoLangToolchainInfo", "proto_lang_toolchain_info.bzl"), - "native-py": nativePyRulesWarning, - "no-effect": noEffectWarning, - "output-group": outputGroupWarning, - "overly-nested-depset": overlyNestedDepsetWarning, - "package-name": packageNameWarning, - "package-on-top": packageOnTopWarning, - "print": printWarning, - "provider-params": providerParamsWarning, - "redefined-variable": redefinedVariableWarning, - "repository-name": repositoryNameWarning, - "rule-impl-return": ruleImplReturnWarning, - "return-value": missingReturnValueWarning, - "skylark-comment": skylarkCommentWarning, - "skylark-docstring": skylarkDocstringWarning, - "string-iteration": stringIterationWarning, - "uninitialized": uninitializedVariableWarning, - "unreachable": unreachableStatementWarning, - "unsorted-dict-items": unsortedDictItemsWarning, - "unused-variable": unusedVariableWarning, + "native-py": nativePyRulesWarning, + "no-effect": noEffectWarning, + "output-group": outputGroupWarning, + "overly-nested-depset": overlyNestedDepsetWarning, + "package-name": packageNameWarning, + "package-on-top": packageOnTopWarning, + "print": printWarning, + "provider-params": providerParamsWarning, + "redefined-variable": redefinedVariableWarning, + "repository-name": repositoryNameWarning, + "rule-impl-return": ruleImplReturnWarning, + "return-value": missingReturnValueWarning, + "skylark-comment": skylarkCommentWarning, + "skylark-docstring": skylarkDocstringWarning, + "string-iteration": stringIterationWarning, + "uninitialized": uninitializedVariableWarning, + "unreachable": unreachableStatementWarning, + "unsorted-dict-items": unsortedDictItemsWarning, + "unused-variable": unusedVariableWarning, } // MultiFileWarningMap lists the warnings that run on the whole file, but may use other files. diff --git a/warn/warn_test.go b/warn/warn_test.go index 38c2fadff..405ffff42 100644 --- a/warn/warn_test.go +++ b/warn/warn_test.go @@ -106,7 +106,7 @@ func getFileForTest(input string, fileType build.FileType) *build.File { func getFindings(categories, input string, fileType build.FileType) []*Finding { file := getFileForTest(input, fileType) - return FileWarnings(file, strings.Split(categories,","), nil, ModeWarn, testFileReader) + return FileWarnings(file, strings.Split(categories, ","), nil, ModeWarn, testFileReader) } func compareFindings(t *testing.T, categories, input string, expected []string, scope, fileType build.FileType) { From 37212492d78ac4ccfc6a6b8a65f08795e2910708 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Thu, 28 Nov 2024 14:09:58 +0100 Subject: [PATCH 3/6] Add missing quote --- buildifier/config/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index 88f8318e0..b4d44a555 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -269,7 +269,7 @@ func TestValidate(t *testing.T) { "native-cc", "native-cc-proto", "native-java", - native-java-lite-proto", + "native-java-lite-proto", "native-java-proto", "native-package", "native-proto", From 135cfa11f903288c5670a838a8aadac23a472aa0 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Thu, 28 Nov 2024 14:26:30 +0100 Subject: [PATCH 4/6] Add and regenerated docs --- WARNINGS.md | 109 ++++++++++++++++++++++++++++++++--- warn/docs/warnings.textproto | 74 +++++++++++++++++++++++- 2 files changed, 174 insertions(+), 9 deletions(-) diff --git a/WARNINGS.md b/WARNINGS.md index 5ee0af111..104f9d09f 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -40,9 +40,16 @@ Warning categories supported by buildifier's linter: * [`native-android`](#native-android) * [`native-build`](#native-build) * [`native-cc`](#native-cc) + * [`native-cc-proto`](#native-cc-proto) * [`native-java`](#native-java) + * [`native-java-lite-proto`](#native-java-lite-proto) + * [`native-java-proto`](#native-java-proto) * [`native-package`](#native-package) * [`native-proto`](#native-proto) + * [`native-proto-common`](#native-proto-common) + * [`native-proto-info`](#native-proto-info) + * [`native-proto-lang-toolchain`](#native-proto-lang-toolchain) + * [`native-proto-lang-toolchain-info`](#native-proto-lang-toolchain-info) * [`native-py`](#native-py) * [`no-effect`](#no-effect) * [`out-of-order-load`](#out-of-order-load) @@ -693,6 +700,19 @@ Update: the plans for disabling native rules [have been postponed](https://groups.google.com/g/bazel-discuss/c/XNvpWcge4AE/m/aJ-aQzszAwAJ), at the moment it's not required to load Starlark rules. +-------------------------------------------------------------------------------- + +## cc_proto_library rule should be loaded from Starlark + + * Category name: `native-cc-proto` + * Flag in Bazel: [`--incompatible_autoload_externally`](https://github.com/bazelbuild/bazel/issues/23043) + * Automatic fix: yes + * [Suppress the warning](#suppress): `# buildifier: disable=native-cc-proto` + +The cc_proto_library rule should be loaded from Starlark. + + + -------------------------------------------------------------------------------- ## All Java build rules should be loaded from Starlark @@ -709,6 +729,32 @@ Update: the plans for disabling native rules [have been postponed](https://groups.google.com/g/bazel-discuss/c/XNvpWcge4AE/m/aJ-aQzszAwAJ), at the moment it's not required to load Starlark rules. +-------------------------------------------------------------------------------- + +## java_lite_proto_library rule should be loaded from Starlark + + * Category name: `native-java-lite-proto` + * Flag in Bazel: [`--incompatible_autoload_externally`](https://github.com/bazelbuild/bazel/issues/23043) + * Automatic fix: yes + * [Suppress the warning](#suppress): `# buildifier: disable=native-java-lite-proto` + +The java_lite_proto_library rule should be loaded from Starlark. + + + +-------------------------------------------------------------------------------- + +## java_proto_library rule should be loaded from Starlark + + * Category name: `native-java-proto` + * Flag in Bazel: [`--incompatible_autoload_externally`](https://github.com/bazelbuild/bazel/issues/23043) + * Automatic fix: yes + * [Suppress the warning](#suppress): `# buildifier: disable=native-java-proto` + +The java_proto_library rule should be loaded from Starlark. + + + -------------------------------------------------------------------------------- ## `native.package()` shouldn't be used in .bzl files @@ -722,19 +768,68 @@ It can silently modify the semantics of a BUILD file and makes it hard to mainta -------------------------------------------------------------------------------- -## All Proto build rules and symbols should be loaded from Starlark +## proto_library rule 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) + * Flag in Bazel: [`--incompatible_autoload_externally`](https://github.com/bazelbuild/bazel/issues/23043) * Automatic fix: yes - * [Disabled by default](buildifier/README.md#linter) * [Suppress the warning](#suppress): `# buildifier: disable=native-proto` -The Proto build rules should be loaded from Starlark. +The proto_library rule should be loaded from Starlark. + + + +-------------------------------------------------------------------------------- + +## proto_common module should be loaded from Starlark + + * Category name: `native-proto-common` + * Flag in Bazel: [`--incompatible_autoload_externally`](https://github.com/bazelbuild/bazel/issues/23043) + * Automatic fix: yes + * [Suppress the warning](#suppress): `# buildifier: disable=native-proto-common` + +The proto_common module should be loaded from Starlark. + + + +-------------------------------------------------------------------------------- + +## ProtoInfo provider should be loaded from Starlark + + * Category name: `native-proto-info` + * Flag in Bazel: [`--incompatible_autoload_externally`](https://github.com/bazelbuild/bazel/issues/23043) + * Automatic fix: yes + * [Suppress the warning](#suppress): `# buildifier: disable=native-proto-info` + +The ProtoInfo provider should be loaded from Starlark. + + + +-------------------------------------------------------------------------------- + +## proto_lang_toolchain rule should be loaded from Starlark + + * Category name: `native-proto-lang-toolchain` + * Flag in Bazel: [`--incompatible_autoload_externally`](https://github.com/bazelbuild/bazel/issues/23043) + * Automatic fix: yes + * [Suppress the warning](#suppress): `# buildifier: disable=native-proto-lang-toolchain` + +The proto_lang_toolchain rule should be loaded from Starlark. + + + +-------------------------------------------------------------------------------- + +## ProtoLangToolchainInfo provider should be loaded from Starlark + + * Category name: `native-proto-lang-toolchain-info` + * Flag in Bazel: [`--incompatible_autoload_externally`](https://github.com/bazelbuild/bazel/issues/23043) + * Automatic fix: yes + * [Suppress the warning](#suppress): `# buildifier: disable=native-proto-lang-toolchain-info` + +The ProtoLangToolchainInfo provider should be loaded from Starlark. + -Update: the plans for disabling native rules -[have been postponed](https://groups.google.com/g/bazel-discuss/c/XNvpWcge4AE/m/aJ-aQzszAwAJ), -at the moment it's not required to load Starlark rules. -------------------------------------------------------------------------------- diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index d8d4bfd58..752ebcf24 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -515,9 +515,79 @@ warnings: { warnings: { name: "native-proto" - header: "All Proto build rules and symbols should be loaded from Starlark" + header: "proto_library rule should be loaded from Starlark" description: - "The Proto build rules should be loaded from Starlark.\n\n" + "The proto_library rule should be loaded from Starlark.\n\n" + bazel_flag: "--incompatible_autoload_externally" + bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" + autofix: true +} + +warnings: { + name: "native-java-proto" + header: "java_proto_library rule should be loaded from Starlark" + description: + "The java_proto_library rule should be loaded from Starlark.\n\n" + bazel_flag: "--incompatible_autoload_externally" + bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" + autofix: true +} + +warnings: { + name: "native-java-lite-proto" + header: "java_lite_proto_library rule should be loaded from Starlark" + description: + "The java_lite_proto_library rule should be loaded from Starlark.\n\n" + bazel_flag: "--incompatible_autoload_externally" + bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" + autofix: true +} + +warnings: { + name: "native-cc-proto" + header: "cc_proto_library rule should be loaded from Starlark" + description: + "The cc_proto_library rule should be loaded from Starlark.\n\n" + bazel_flag: "--incompatible_autoload_externally" + bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" + autofix: true +} + +warnings: { + name: "native-proto-lang-toolchain" + header: "proto_lang_toolchain rule should be loaded from Starlark" + description: + "The proto_lang_toolchain rule should be loaded from Starlark.\n\n" + bazel_flag: "--incompatible_autoload_externally" + bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" + autofix: true +} + +warnings: { + name: "native-proto-info" + header: "ProtoInfo provider should be loaded from Starlark" + description: + "The ProtoInfo provider should be loaded from Starlark.\n\n" + bazel_flag: "--incompatible_autoload_externally" + bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" + autofix: true +} + +warnings: { + name: "native-proto-lang-toolchain-info" + header: "ProtoLangToolchainInfo provider should be loaded from Starlark" + description: + "The ProtoLangToolchainInfo provider should be loaded from Starlark.\n\n" + bazel_flag: "--incompatible_autoload_externally" + bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" + autofix: true +} + +warnings: { + name: "native-proto-common" + header: "proto_common module should be loaded from Starlark" + description: + "The proto_common module should be loaded from Starlark.\n\n" bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true From 918b8d1604c9fab83ad228884252094de03621db Mon Sep 17 00:00:00 2001 From: Ivo List Date: Thu, 28 Nov 2024 14:26:35 +0100 Subject: [PATCH 5/6] Fix tests --- buildifier/config/config_test.go | 6 +++++- buildifier/integration_test.sh | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index b4d44a555..1bba68e9e 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -409,7 +409,11 @@ func TestValidate(t *testing.T) { "native-java-lite-proto", "native-java-proto", "native-package", - // "native-proto", + "native-proto", + "native-proto-common", + "native-proto-info", + "native-proto-lang-toolchain", + "native-proto-lang-toolchain-info", // "native-py", "no-effect", "output-group", diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh index 3c1173b3b..4a9d94652 100755 --- a/buildifier/integration_test.sh +++ b/buildifier/integration_test.sh @@ -284,9 +284,16 @@ cat > golden/.buildifier.example.json < Date: Thu, 28 Nov 2024 21:02:50 +0100 Subject: [PATCH 6/6] Remove \n\n --- WARNINGS.md | 16 ---------------- warn/docs/warnings.textproto | 16 ++++++++-------- 2 files changed, 8 insertions(+), 24 deletions(-) diff --git a/WARNINGS.md b/WARNINGS.md index 104f9d09f..a44a6ab46 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -711,8 +711,6 @@ at the moment it's not required to load Starlark rules. The cc_proto_library rule should be loaded from Starlark. - - -------------------------------------------------------------------------------- ## All Java build rules should be loaded from Starlark @@ -740,8 +738,6 @@ at the moment it's not required to load Starlark rules. The java_lite_proto_library rule should be loaded from Starlark. - - -------------------------------------------------------------------------------- ## java_proto_library rule should be loaded from Starlark @@ -753,8 +749,6 @@ The java_lite_proto_library rule should be loaded from Starlark. The java_proto_library rule should be loaded from Starlark. - - -------------------------------------------------------------------------------- ## `native.package()` shouldn't be used in .bzl files @@ -777,8 +771,6 @@ It can silently modify the semantics of a BUILD file and makes it hard to mainta The proto_library rule should be loaded from Starlark. - - -------------------------------------------------------------------------------- ## proto_common module should be loaded from Starlark @@ -790,8 +782,6 @@ The proto_library rule should be loaded from Starlark. The proto_common module should be loaded from Starlark. - - -------------------------------------------------------------------------------- ## ProtoInfo provider should be loaded from Starlark @@ -803,8 +793,6 @@ The proto_common module should be loaded from Starlark. The ProtoInfo provider should be loaded from Starlark. - - -------------------------------------------------------------------------------- ## proto_lang_toolchain rule should be loaded from Starlark @@ -816,8 +804,6 @@ The ProtoInfo provider should be loaded from Starlark. The proto_lang_toolchain rule should be loaded from Starlark. - - -------------------------------------------------------------------------------- ## ProtoLangToolchainInfo provider should be loaded from Starlark @@ -829,8 +815,6 @@ The proto_lang_toolchain rule should be loaded from Starlark. The ProtoLangToolchainInfo provider should be loaded from Starlark. - - -------------------------------------------------------------------------------- ## All Python build rules should be loaded from Starlark diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index 752ebcf24..045256f28 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -517,7 +517,7 @@ warnings: { name: "native-proto" header: "proto_library rule should be loaded from Starlark" description: - "The proto_library rule should be loaded from Starlark.\n\n" + "The proto_library rule should be loaded from Starlark." bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true @@ -527,7 +527,7 @@ warnings: { name: "native-java-proto" header: "java_proto_library rule should be loaded from Starlark" description: - "The java_proto_library rule should be loaded from Starlark.\n\n" + "The java_proto_library rule should be loaded from Starlark." bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true @@ -537,7 +537,7 @@ warnings: { name: "native-java-lite-proto" header: "java_lite_proto_library rule should be loaded from Starlark" description: - "The java_lite_proto_library rule should be loaded from Starlark.\n\n" + "The java_lite_proto_library rule should be loaded from Starlark." bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true @@ -547,7 +547,7 @@ warnings: { name: "native-cc-proto" header: "cc_proto_library rule should be loaded from Starlark" description: - "The cc_proto_library rule should be loaded from Starlark.\n\n" + "The cc_proto_library rule should be loaded from Starlark." bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true @@ -557,7 +557,7 @@ warnings: { name: "native-proto-lang-toolchain" header: "proto_lang_toolchain rule should be loaded from Starlark" description: - "The proto_lang_toolchain rule should be loaded from Starlark.\n\n" + "The proto_lang_toolchain rule should be loaded from Starlark." bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true @@ -567,7 +567,7 @@ warnings: { name: "native-proto-info" header: "ProtoInfo provider should be loaded from Starlark" description: - "The ProtoInfo provider should be loaded from Starlark.\n\n" + "The ProtoInfo provider should be loaded from Starlark." bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true @@ -577,7 +577,7 @@ warnings: { name: "native-proto-lang-toolchain-info" header: "ProtoLangToolchainInfo provider should be loaded from Starlark" description: - "The ProtoLangToolchainInfo provider should be loaded from Starlark.\n\n" + "The ProtoLangToolchainInfo provider should be loaded from Starlark." bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true @@ -587,7 +587,7 @@ warnings: { name: "native-proto-common" header: "proto_common module should be loaded from Starlark" description: - "The proto_common module should be loaded from Starlark.\n\n" + "The proto_common module should be loaded from Starlark." bazel_flag: "--incompatible_autoload_externally" bazel_flag_link: "https://github.com/bazelbuild/bazel/issues/23043" autofix: true