From c07574b5ba238c3386482a2677299c153c5b761f Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 10 Jan 2025 10:04:11 +0100 Subject: [PATCH] Use apparent repo names when fixing loads Whether a load for e.g. proto rules should be from `@com_google_protobuf`, `@protobuf` or a fully custom name depends on the `repo_name` on the `bazel_dep` for the `protobuf` module in the root module's module file. This PR scans `MODULE.bazel` and included `*.MODULE.bazel` files for `bazel_dep`s and uses the derived mapping for the suggested loads. --- edit/bzlmod/bzlmod.go | 84 ++++++++++++++++++++++++ tables/tables.go | 8 ++- warn/BUILD.bazel | 1 + warn/warn.go | 112 ++++++++++++++++---------------- warn/warn_bazel_api.go | 123 +++++++++++++++++++++++------------- warn/warn_bazel_api_test.go | 81 ++++++++++++++++-------- warn/warn_test.go | 2 + 7 files changed, 283 insertions(+), 128 deletions(-) diff --git a/edit/bzlmod/bzlmod.go b/edit/bzlmod/bzlmod.go index 17edff1c1..4a3fc4219 100644 --- a/edit/bzlmod/bzlmod.go +++ b/edit/bzlmod/bzlmod.go @@ -18,6 +18,8 @@ limitations under the License. package bzlmod import ( + "path" + "github.com/bazelbuild/buildtools/build" "github.com/bazelbuild/buildtools/labels" ) @@ -348,3 +350,85 @@ func lastProxyUsage(f *build.File, proxies []string) (lastUsage int, lastProxy s return lastUsage, lastProxy } + +// ExtractModuleToApparentNameMapping collects the mapping of module names (e.g. "rules_go") to +// user-configured apparent names (e.g. "my_rules_go") from the repo's MODULE.bazel, if it exists. +// The given function is called with a repo-relative, slash-separated path and should return the +// content of the MODULE.bazel or *.MODULE.bazel file at that path, or nil if the file does not +// exist. +// See https://bazel.build/external/module#repository_names_and_strict_deps for more information on +// apparent names. +func ExtractModuleToApparentNameMapping(fileReader func(relPath string) *build.File) func(string) string { + moduleToApparentName := collectApparentNames(fileReader, "MODULE.bazel") + + return func(moduleName string) string { + return moduleToApparentName[moduleName] + } +} + +// Collects the mapping of module names (e.g. "rules_go") to user-configured apparent names (e.g. +// "my_rules_go"). See https://bazel.build/external/module#repository_names_and_strict_deps for more +// information on apparent names. +func collectApparentNames(fileReader func(relPath string) *build.File, relPath string) map[string]string { + apparentNames := make(map[string]string) + seenFiles := make(map[string]struct{}) + filesToProcess := []string{relPath} + + for len(filesToProcess) > 0 { + f := filesToProcess[0] + filesToProcess = filesToProcess[1:] + if _, seen := seenFiles[f]; seen { + continue + } + seenFiles[f] = struct{}{} + bf := fileReader(f) + if bf == nil { + return nil + } + names, includeLabels := collectApparentNamesAndIncludes(bf) + for name, apparentName := range names { + apparentNames[name] = apparentName + } + for _, includeLabel := range includeLabels { + l := labels.Parse(includeLabel) + p := path.Join(l.Package, l.Target) + filesToProcess = append(filesToProcess, p) + } + } + + return apparentNames +} + +func collectApparentNamesAndIncludes(f *build.File) (map[string]string, []string) { + apparentNames := make(map[string]string) + var includeLabels []string + + for _, dep := range f.Rules("") { + if dep.ExplicitName() == "" { + if ident, ok := dep.Call.X.(*build.Ident); !ok || ident.Name != "include" { + continue + } + if len(dep.Call.List) != 1 { + continue + } + if str, ok := dep.Call.List[0].(*build.StringExpr); ok { + includeLabels = append(includeLabels, str.Value) + } + continue + } + if dep.Kind() != "module" && dep.Kind() != "bazel_dep" { + continue + } + // We support module in addition to bazel_dep to handle language repos that use Gazelle to + // manage their own BUILD files. + if name := dep.AttrString("name"); name != "" { + if repoName := dep.AttrString("repo_name"); repoName != "" { + apparentNames[name] = repoName + } else { + apparentNames[name] = name + } + } + } + + return apparentNames, includeLabels +} diff --git a/tables/tables.go b/tables/tables.go index 32726144c..dbe0df808 100644 --- a/tables/tables.go +++ b/tables/tables.go @@ -260,7 +260,13 @@ var PyNativeRules = []string{ var PyLoadPath = "@rules_python//python:defs.bzl" // ProtoLoadPathPrefix is the load path prefix for the Starlark Proto Rules. -var ProtoLoadPathPrefix = "@com_google_protobuf//bazel" +var ProtoLoadPathPrefix = "@protobuf//bazel" + +// ModuleToLegacyRepoName contains the mapping from module name to WORKSPACE repository name +// for modules with load fixes if those names are different. +var ModuleToLegacyRepoName = map[string]string{ + "protobuf": "com_google_protobuf", +} // IsModuleOverride contains the names of all Bzlmod module overrides available in MODULE.bazel. var IsModuleOverride = map[string]bool{ diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel index 852c8ca9e..225b5fcfa 100644 --- a/warn/BUILD.bazel +++ b/warn/BUILD.bazel @@ -24,6 +24,7 @@ go_library( "//build", "//bzlenv", "//edit", + "//edit/bzlmod", "//labels", "//tables", ], diff --git a/warn/warn.go b/warn/warn.go index 48ba370c7..42f7ec187 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -118,40 +118,64 @@ 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, + "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, + "integer-division": integerDivisionWarning, + "keyword-positional-params": keywordPositionalParametersWarning, + "list-append": listAppendWarning, + "load": unusedLoadWarning, + "module-docstring": moduleDocstringWarning, + "name-conventions": nameConventionsWarning, + "native-build": nativeInBuildFilesWarning, + "native-package": nativePackageWarning, + "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. +var MultiFileWarningMap = map[string]func(f *build.File, fileReader *FileReader) []*LinterFinding{ + "deprecated-function": deprecatedFunctionWarning, "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-binary": NativeJavaRulesWarning("java_binary"), "native-java-import": NativeJavaRulesWarning("java_import"), @@ -164,7 +188,6 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ "native-java-common": NativeJavaSymbolsWarning("java_common", "java_common"), "native-java-info": NativeJavaSymbolsWarning("JavaInfo", "java_info"), "native-java-plugin-info": NativeJavaSymbolsWarning("JavaPluginInfo", "java_plugin_info"), - "native-package": nativePackageWarning, "native-proto": NativeProtoRulesWarning("proto_library"), "native-java-proto": NativeProtoRulesWarning("java_proto_library"), "native-java-lite-proto": NativeProtoRulesWarning("java_lite_proto_library"), @@ -174,30 +197,7 @@ var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ "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, -} - -// 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, - "unnamed-macro": unnamedMacroWarning, + "unnamed-macro": unnamedMacroWarning, } // nonDefaultWarnings contains warnings that are enabled by default because they're not applicable diff --git a/warn/warn_bazel_api.go b/warn/warn_bazel_api.go index f5dc670ac..8ca12f6a7 100644 --- a/warn/warn_bazel_api.go +++ b/warn/warn_bazel_api.go @@ -21,10 +21,13 @@ package warn import ( "fmt" "sort" + "strings" "github.com/bazelbuild/buildtools/build" "github.com/bazelbuild/buildtools/bzlenv" "github.com/bazelbuild/buildtools/edit" + "github.com/bazelbuild/buildtools/edit/bzlmod" + "github.com/bazelbuild/buildtools/labels" "github.com/bazelbuild/buildtools/tables" ) @@ -171,7 +174,36 @@ func insertLoad(f *build.File, module string, symbols []string) *LinterReplaceme return &LinterReplacement{&(f.Stmt[i]), edit.NewLoad(module, symbols, symbols)} } -func notLoadedFunctionUsageCheckInternal(expr *build.Expr, env *bzlenv.Environment, globals []string, loadFrom string) ([]string, []*LinterFinding) { +// Caches the result of bzlmod.ExtractModuleToApparentNameMapping. +var moduleToApparentRepoName func(string) string + +// useApparentRepoNameIfExternal replaces the module name in a load statement with the apparent repository +// name used by the root Bazel module (if any). +func useApparentRepoNameIfExternal(load string, fileReader *FileReader) string { + if !strings.HasPrefix(load, "@") { + // Not a load from an external repository. + return load + } + if moduleToApparentRepoName == nil { + moduleToApparentRepoName = bzlmod.ExtractModuleToApparentNameMapping(func(relPath string) *build.File { + return fileReader.GetFile("", relPath) + }) + } + l := labels.Parse(load) + apparentName := moduleToApparentRepoName(l.Repository) + if apparentName == "" { + // The module that hosts the load is not a bazel_dep of the root module. We assume that's + // because it is a WORKSPACE repo, which uses the legacy name. + apparentName = tables.ModuleToLegacyRepoName[l.Repository] + if apparentName == "" { + apparentName = l.Repository + } + } + l.Repository = apparentName + return l.Format() +} + +func notLoadedFunctionUsageCheckInternal(expr *build.Expr, env *bzlenv.Environment, globals []string, loadFrom string, fileReader *FileReader) ([]string, []*LinterFinding) { var loads []string var findings []*LinterFinding @@ -209,7 +241,7 @@ func notLoadedFunctionUsageCheckInternal(expr *build.Expr, env *bzlenv.Environme if name == global { loads = append(loads, name) findings = append(findings, - makeLinterFinding(call.X, fmt.Sprintf(`Function %q is not global anymore and needs to be loaded from %q.`, global, loadFrom), replacements...)) + makeLinterFinding(call.X, fmt.Sprintf(`Function %q is not global anymore and needs to be loaded from %q.`, global, useApparentRepoNameIfExternal(loadFrom, fileReader)), replacements...)) break } } @@ -217,7 +249,7 @@ func notLoadedFunctionUsageCheckInternal(expr *build.Expr, env *bzlenv.Environme return loads, findings } -func notLoadedSymbolUsageCheckInternal(expr *build.Expr, env *bzlenv.Environment, globals []string, loadFrom string) ([]string, []*LinterFinding) { +func notLoadedSymbolUsageCheckInternal(expr *build.Expr, env *bzlenv.Environment, globals []string, loadFrom string, fileReader *FileReader) ([]string, []*LinterFinding) { var loads []string var findings []*LinterFinding @@ -233,7 +265,7 @@ func notLoadedSymbolUsageCheckInternal(expr *build.Expr, env *bzlenv.Environment 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))) + makeLinterFinding(ident, fmt.Sprintf(`Symbol %q is not global anymore and needs to be loaded from %q.`, global, useApparentRepoNameIfExternal(loadFrom, fileReader)))) break } } @@ -243,7 +275,7 @@ func notLoadedSymbolUsageCheckInternal(expr *build.Expr, env *bzlenv.Environment // 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 notLoadedUsageCheck(f *build.File, functions, symbols []string, loadFrom string) []*LinterFinding { +func notLoadedUsageCheck(f *build.File, fileReader *FileReader, functions, symbols []string, loadFrom string) []*LinterFinding { toLoad := make(map[string]bool) var findings []*LinterFinding @@ -251,13 +283,13 @@ func notLoadedUsageCheck(f *build.File, functions, symbols []string, loadFrom st walk = func(expr *build.Expr, env *bzlenv.Environment) { defer bzlenv.WalkOnceWithEnvironment(*expr, env, walk) - functionLoads, functionFindings := notLoadedFunctionUsageCheckInternal(expr, env, functions, loadFrom) + functionLoads, functionFindings := notLoadedFunctionUsageCheckInternal(expr, env, functions, loadFrom, fileReader) findings = append(findings, functionFindings...) for _, load := range functionLoads { toLoad[load] = true } - symbolLoads, symbolFindings := notLoadedSymbolUsageCheckInternal(expr, env, symbols, loadFrom) + symbolLoads, symbolFindings := notLoadedSymbolUsageCheckInternal(expr, env, symbols, loadFrom, fileReader) findings = append(findings, symbolFindings...) for _, load := range symbolLoads { toLoad[load] = true @@ -270,6 +302,8 @@ func notLoadedUsageCheck(f *build.File, functions, symbols []string, loadFrom st return nil } + loadFrom = useApparentRepoNameIfExternal(loadFrom, fileReader) + loads := []string{} for l := range toLoad { loads = append(loads, l) @@ -289,14 +323,14 @@ func notLoadedUsageCheck(f *build.File, functions, symbols []string, loadFrom st // 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) +func NotLoadedFunctionUsageCheck(f *build.File, fileReader *FileReader, globals []string, loadFrom string) []*LinterFinding { + return notLoadedUsageCheck(f, fileReader, 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) +func NotLoadedSymbolUsageCheck(f *build.File, fileReader *FileReader, globals []string, loadFrom string) []*LinterFinding { + return notLoadedUsageCheck(f, fileReader, []string{}, globals, loadFrom) } // makePositional makes the function argument positional (removes the keyword if it exists) @@ -673,94 +707,94 @@ func outputGroupWarning(f *build.File) []*LinterFinding { return findings } -func nativeGitRepositoryWarning(f *build.File) []*LinterFinding { +func nativeGitRepositoryWarning(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl { return nil } - return NotLoadedFunctionUsageCheck(f, []string{"git_repository", "new_git_repository"}, "@bazel_tools//tools/build_defs/repo:git.bzl") + return NotLoadedFunctionUsageCheck(f, fileReader, []string{"git_repository", "new_git_repository"}, "@bazel_tools//tools/build_defs/repo:git.bzl") } -func nativeHTTPArchiveWarning(f *build.File) []*LinterFinding { +func nativeHTTPArchiveWarning(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl { return nil } - return NotLoadedFunctionUsageCheck(f, []string{"http_archive"}, "@bazel_tools//tools/build_defs/repo:http.bzl") + return NotLoadedFunctionUsageCheck(f, fileReader, []string{"http_archive"}, "@bazel_tools//tools/build_defs/repo:http.bzl") } -func nativeAndroidRulesWarning(f *build.File) []*LinterFinding { +func nativeAndroidRulesWarning(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl && f.Type != build.TypeBuild { return nil } - return NotLoadedFunctionUsageCheck(f, tables.AndroidNativeRules, tables.AndroidLoadPath) + return NotLoadedFunctionUsageCheck(f, fileReader, tables.AndroidNativeRules, tables.AndroidLoadPath) } -func nativeCcRulesWarning(f *build.File) []*LinterFinding { +func nativeCcRulesWarning(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl && f.Type != build.TypeBuild { return nil } - return NotLoadedFunctionUsageCheck(f, tables.CcNativeRules, tables.CcLoadPath) + return NotLoadedFunctionUsageCheck(f, fileReader, tables.CcNativeRules, tables.CcLoadPath) } // NativeJavaRulesWarning produces a warning for missing loads of java rules -func NativeJavaRulesWarning(rule string) func(f *build.File) []*LinterFinding { - return func(f *build.File) []*LinterFinding { +func NativeJavaRulesWarning(rule string) func(f *build.File, fileReader *FileReader) []*LinterFinding { + return func(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl && f.Type != build.TypeBuild { return nil } - return NotLoadedFunctionUsageCheck(f, []string{rule}, tables.JavaLoadPathPrefix+":"+rule+".bzl") + return NotLoadedFunctionUsageCheck(f, fileReader, []string{rule}, tables.JavaLoadPathPrefix+":"+rule+".bzl") } } // NativeJavaToolchainRulesWarning produces a warning for missing loads of java toolchain rules -func NativeJavaToolchainRulesWarning(rule string) func(f *build.File) []*LinterFinding { - return func(f *build.File) []*LinterFinding { +func NativeJavaToolchainRulesWarning(rule string) func(f *build.File, fileReader *FileReader) []*LinterFinding { + return func(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl && f.Type != build.TypeBuild { return nil } - return NotLoadedFunctionUsageCheck(f, []string{rule}, tables.JavaLoadPathPrefix+"/toolchains:"+rule+".bzl") + return NotLoadedFunctionUsageCheck(f, fileReader, []string{rule}, tables.JavaLoadPathPrefix+"/toolchains:"+rule+".bzl") } } // NativeJavaSymbolsWarning produces a warning for missing loads of java top-level symbols -func NativeJavaSymbolsWarning(symbol string, bzlfile string) func(f *build.File) []*LinterFinding { - return func(f *build.File) []*LinterFinding { +func NativeJavaSymbolsWarning(symbol string, bzlfile string) func(f *build.File, fileReader *FileReader) []*LinterFinding { + return func(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl && f.Type != build.TypeBuild { return nil } - return NotLoadedSymbolUsageCheck(f, []string{symbol}, tables.JavaLoadPathPrefix+"/common:"+bzlfile+".bzl") + return NotLoadedSymbolUsageCheck(f, fileReader, []string{symbol}, tables.JavaLoadPathPrefix+"/common:"+bzlfile+".bzl") } } -func nativePyRulesWarning(f *build.File) []*LinterFinding { +func nativePyRulesWarning(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl && f.Type != build.TypeBuild { return nil } - return NotLoadedFunctionUsageCheck(f, tables.PyNativeRules, tables.PyLoadPath) + return NotLoadedFunctionUsageCheck(f, fileReader, tables.PyNativeRules, tables.PyLoadPath) } // 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 NativeProtoRulesWarning(rule string) func(f *build.File, fileReader *FileReader) []*LinterFinding { + return func(f *build.File, fileReader *FileReader) []*LinterFinding { + if f.Type != build.TypeBzl && f.Type != build.TypeBuild { + return nil + } + return NotLoadedFunctionUsageCheck(f, fileReader, []string{rule}, tables.ProtoLoadPathPrefix+":"+rule+".bzl") } } -func nativeProtoLangToolchainWarning(f *build.File) []*LinterFinding { +func nativeProtoLangToolchainWarning(f *build.File, fileReader *FileReader) []*LinterFinding { if f.Type != build.TypeBzl && f.Type != build.TypeBuild { return nil } - return NotLoadedFunctionUsageCheck(f, []string{"proto_lang_toolchain"}, tables.ProtoLoadPathPrefix + "/toolchains:proto_lang_toolchain.bzl") + return NotLoadedFunctionUsageCheck(f, fileReader, []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 nativeProtoSymbolsWarning(symbol string, bzlfile string) func(f *build.File, fileReader *FileReader) []*LinterFinding { + return func(f *build.File, fileReader *FileReader) []*LinterFinding { + if f.Type != build.TypeBzl && f.Type != build.TypeBuild { + return nil + } + return NotLoadedSymbolUsageCheck(f, fileReader, []string{symbol}, tables.ProtoLoadPathPrefix+"/common:"+bzlfile) } } @@ -1133,7 +1167,6 @@ func providerParamsWarning(f *build.File) []*LinterFinding { return findings } - func attrNameWarning(f *build.File, names []string) []*LinterFinding { if f.Type != build.TypeBzl { return nil diff --git a/warn/warn_bazel_api_test.go b/warn/warn_bazel_api_test.go index 8157006d1..a3a734d41 100644 --- a/warn/warn_bazel_api_test.go +++ b/warn/warn_bazel_api_test.go @@ -305,6 +305,8 @@ def _impl(ctx): } func TestNativeGitRepositoryWarning(t *testing.T) { + defer setUpFileReader(nil)() + checkFindingsAndFix(t, "git-repository", ` """My file""" @@ -368,6 +370,8 @@ def macro(): } func TestNativeHttpArchiveWarning(t *testing.T) { + defer setUpFileReader(nil)() + checkFindingsAndFix(t, "http-archive", ` """My file""" @@ -511,6 +515,8 @@ rule(foo = bar) # no matching parameters } func TestNativeAndroidWarning(t *testing.T) { + defer setUpFileReader(nil)() + checkFindingsAndFix(t, "native-android", ` """My file""" @@ -545,6 +551,8 @@ android_binary() } func TestNativeCcWarning(t *testing.T) { + defer setUpFileReader(nil)() + checkFindingsAndFix(t, "native-cc", ` """My file""" @@ -594,6 +602,16 @@ cc_import() } func TestNativeJavaWarning(t *testing.T) { + defer setUpFileReader(map[string]string{ + "MODULE.bazel": ` +include("//my/pkg:java.MODULE.bazel") +`, + "my/pkg/java.MODULE.bazel": ` +bazel_dep(name = "rules_java", version = "1.2.3", repo_name = "my_rules_java") +`, + })() + + expectedLoadPrefix := "@my_rules_java//java" checkFindingsAndFix(t, "native-java-binary,native-java-import,native-java-library,native-java-plugin,native-java-test,native-java-package-config,native-java-runtime,native-java-toolchain,native-java-common,native-java-info,native-java-plugin-info", ` """My file""" @@ -642,25 +660,32 @@ def macro(): java_common java_test() -`, tables.JavaLoadPathPrefix), +`, expectedLoadPrefix), []string{ - fmt.Sprintf(`:4: Function "java_import" is not global anymore and needs to be loaded from "%s:java_import.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:5: Function "java_library" is not global anymore and needs to be loaded from "%s:java_library.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:6: Function "java_library" is not global anymore and needs to be loaded from "%s:java_library.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:7: Function "java_binary" is not global anymore and needs to be loaded from "%s:java_binary.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:8: Function "java_plugin" is not global anymore and needs to be loaded from "%s:java_plugin.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:9: Function "java_package_configuration" is not global anymore and needs to be loaded from "%s/toolchains:java_package_configuration.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:10: Function "java_runtime" is not global anymore and needs to be loaded from "%s/toolchains:java_runtime.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:11: Function "java_toolchain" is not global anymore and needs to be loaded from "%s/toolchains:java_toolchain.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:13: Symbol "JavaInfo" is not global anymore and needs to be loaded from "%s/common:java_info.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:14: Symbol "JavaPluginInfo" is not global anymore and needs to be loaded from "%s/common:java_plugin_info.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:15: Symbol "java_common" is not global anymore and needs to be loaded from "%s/common:java_common.bzl".`, tables.JavaLoadPathPrefix), - fmt.Sprintf(`:17: Function "java_test" is not global anymore and needs to be loaded from "%s:java_test.bzl".`, tables.JavaLoadPathPrefix), + fmt.Sprintf(`:4: Function "java_import" is not global anymore and needs to be loaded from "%s:java_import.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:5: Function "java_library" is not global anymore and needs to be loaded from "%s:java_library.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:6: Function "java_library" is not global anymore and needs to be loaded from "%s:java_library.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:7: Function "java_binary" is not global anymore and needs to be loaded from "%s:java_binary.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:8: Function "java_plugin" is not global anymore and needs to be loaded from "%s:java_plugin.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:9: Function "java_package_configuration" is not global anymore and needs to be loaded from "%s/toolchains:java_package_configuration.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:10: Function "java_runtime" is not global anymore and needs to be loaded from "%s/toolchains:java_runtime.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:11: Function "java_toolchain" is not global anymore and needs to be loaded from "%s/toolchains:java_toolchain.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:13: Symbol "JavaInfo" is not global anymore and needs to be loaded from "%s/common:java_info.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:14: Symbol "JavaPluginInfo" is not global anymore and needs to be loaded from "%s/common:java_plugin_info.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:15: Symbol "java_common" is not global anymore and needs to be loaded from "%s/common:java_common.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:17: Function "java_test" is not global anymore and needs to be loaded from "%s:java_test.bzl".`, expectedLoadPrefix), }, scopeBzl|scopeBuild) } func TestNativePyWarning(t *testing.T) { + defer setUpFileReader(map[string]string{ + "MODULE.bazel": ` +bazel_dep(name = "rules_python", repo_name = "my_rules_python") +`, + })() + + expectedLoadPath := "@my_rules_python//python:defs.bzl" checkFindingsAndFix(t, "native-py", ` """My file""" @@ -683,18 +708,22 @@ def macro(): py_runtime() py_test() -`, tables.PyLoadPath), +`, expectedLoadPath), []string{ - fmt.Sprintf(`:4: Function "py_library" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath), - fmt.Sprintf(`:5: Function "py_binary" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath), - fmt.Sprintf(`:6: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath), - fmt.Sprintf(`:7: Function "py_runtime" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath), - fmt.Sprintf(`:9: Function "py_test" is not global anymore and needs to be loaded from "%s".`, tables.PyLoadPath), + fmt.Sprintf(`:4: Function "py_library" is not global anymore and needs to be loaded from "%s".`, expectedLoadPath), + fmt.Sprintf(`:5: Function "py_binary" is not global anymore and needs to be loaded from "%s".`, expectedLoadPath), + fmt.Sprintf(`:6: Function "py_test" is not global anymore and needs to be loaded from "%s".`, expectedLoadPath), + fmt.Sprintf(`:7: Function "py_runtime" is not global anymore and needs to be loaded from "%s".`, expectedLoadPath), + fmt.Sprintf(`:9: Function "py_test" is not global anymore and needs to be loaded from "%s".`, expectedLoadPath), }, scopeBzl|scopeBuild) } func TestNativeProtoWarning(t *testing.T) { + // No MODULE.bazel file, so loads should use the legacy protobuf repo name. + defer setUpFileReader(nil)() + + expectedLoadPrefix := "@com_google_protobuf//bazel" checkFindingsAndFix(t, "native-proto,native-proto-lang-toolchain,native-proto-info,native-proto-common", ` """My file""" @@ -722,14 +751,14 @@ def macro(): ProtoInfo proto_common -`, tables.ProtoLoadPathPrefix), +`, expectedLoadPrefix), []string{ - 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), + fmt.Sprintf(`:4: Function "proto_library" is not global anymore and needs to be loaded from "%s:proto_library.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:5: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s/toolchains:proto_lang_toolchain.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:6: Function "proto_lang_toolchain" is not global anymore and needs to be loaded from "%s/toolchains:proto_lang_toolchain.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:7: Function "proto_library" is not global anymore and needs to be loaded from "%s:proto_library.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:9: Symbol "ProtoInfo" is not global anymore and needs to be loaded from "%s/common:proto_info.bzl".`, expectedLoadPrefix), + fmt.Sprintf(`:10: Symbol "proto_common" is not global anymore and needs to be loaded from "%s/common:proto_common.bzl".`, expectedLoadPrefix), }, scopeBzl|scopeBuild) } diff --git a/warn/warn_test.go b/warn/warn_test.go index 405ffff42..bf60c8793 100644 --- a/warn/warn_test.go +++ b/warn/warn_test.go @@ -58,6 +58,8 @@ func setUpFileReader(data map[string]string) (cleanup func()) { } testFileReader = NewFileReader(readFile) fileReaderRequests = nil + // The cached mapping depends on the file reader, so we need to reset it as well. + moduleToApparentRepoName = nil return func() { // Tear down