diff --git a/WARNINGS.md b/WARNINGS.md index 2514b8813..897f1cc64 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -554,15 +554,15 @@ Paths containing `/external/` (without a leading `//`) may indicate: Examples that trigger this warning: ```python -srcs = ["/external/some_repo/file.h"] -data = ["path/external/repo/data.txt"] +copts = ["-iquote $(BINDIR)/external/some_repo/include"] +args = ["./external/repo/path/data.txt"] ``` Instead, use proper Bazel labels: ```python -srcs = ["@some_repo//file.h"] -data = ["@repo//path:data.txt"] +copts = ["-iquote $(execpath @some_repo//include:foo.h)/.."] +args = ["$(execpath @repo//path:data.txt)"] ``` Note: This warning does not apply to main repository paths like `//external/...` diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index 8be362ea0..926dabeb0 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -337,13 +337,13 @@ warnings: { "- Code that relies on Bazel's internal directory structure\n\n" "Examples that trigger this warning:\n\n" "```python\n" - "srcs = [\"/external/some_repo/file.h\"]\n" - "data = [\"path/external/repo/data.txt\"]\n" + "copts = [\"-iquote $(BINDIR)/external/some_repo/include\"]\n" + "args = [\"./external/repo/path/data.txt\"]\n" "```\n\n" "Instead, use proper Bazel labels:\n\n" "```python\n" - "srcs = [\"@some_repo//file.h\"]\n" - "data = [\"@repo//path:data.txt\"]\n" + "copts = [\"-iquote $(execpath @some_repo//include:foo.h)/..\"]\n" + "args = [\"$(execpath @repo//path:data.txt)\"]\n" "```\n\n" "Note: This warning does not apply to main repository paths like `//external/...`\n" "which are legitimate Bazel labels." diff --git a/warn/warn_bazel.go b/warn/warn_bazel.go index be6000cd1..6e54f3798 100644 --- a/warn/warn_bazel.go +++ b/warn/warn_bazel.go @@ -256,6 +256,24 @@ func printWarning(f *build.File) []*LinterFinding { return findings } +func isDocStringContext(stringExpr *build.StringExpr, stack []build.Expr) bool { + if len(stack) == 0 { + return false + } + + // Function docstring ("""...""") + if stringExpr.TripleQuote { + predecessor := stack[len(stack)-1] + if defStmt, ok := predecessor.(*build.DefStmt); ok { + if len(defStmt.Body) > 0 && defStmt.Body[0] == stringExpr { + return true + } + } + } + + return false +} + func externalPathWarning(f *build.File) []*LinterFinding { if f.Type == build.TypeDefault { // Only applicable to Bazel files @@ -269,6 +287,11 @@ func externalPathWarning(f *build.File) []*LinterFinding { if !ok { return } + + if isDocStringContext(stringExpr, stack) { + return + } + // Warn for "/external/" but not if "//" appears in the string (main repository paths) if strings.Contains(stringExpr.Value, "/external/") && !strings.Contains(stringExpr.Value, "//") { findings = append(findings, @@ -291,6 +314,11 @@ func canonicalRepositoryWarning(f *build.File) []*LinterFinding { if !ok { return } + + if isDocStringContext(stringExpr, stack) { + return + } + if strings.Contains(stringExpr.Value, "@@") { findings = append(findings, makeLinterFinding(stringExpr, `String contains "@@" which indicates a canonical repository name reference that should be avoided.`)) diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go index 9201cea7d..f5ba88eb1 100644 --- a/warn/warn_bazel_test.go +++ b/warn/warn_bazel_test.go @@ -233,14 +233,20 @@ filegroup( ) some_rule( - arg1 = "normal/path/file.txt", + arg1 = "normal/path/file.txt", arg2 = "/external/repo/file.py", arg3 = ["file1.txt", "/external/another/file.cc"], -)`, +) + +def foo(): + pass + """unused triple-quote string with /external/""" +`, []string{ `:9: String contains "/external/" which may indicate a dependency on external repositories that could be fragile.`, `:25: String contains "/external/" which may indicate a dependency on external repositories that could be fragile.`, `:26: String contains "/external/" which may indicate a dependency on external repositories that could be fragile.`, + `31: String contains "/external/" which may indicate a dependency on external repositories that could be fragile.`, }, scopeBazel) @@ -260,6 +266,15 @@ py_binary( )`, []string{}, scopeBazel) + + // Test cases that should NOT warn (doc strings) + checkFindings(t, "external-path", ` +def foo(): + """/external/""" + pass +`, + []string{}, + scopeBazel) } func TestCanonicalRepositoryWarning(t *testing.T) { @@ -279,13 +294,28 @@ py_binary( srcs = ["tool.py"], data = ["@repo//file.txt"], # Should NOT warn (single @) args = ["@@some_canonical_repo//path:target"], -)`, +) + +def foo(): + pass + """unused triple-quote string with @@""" +`, []string{ `:1: String contains "@@" which indicates a canonical repository name reference that should be avoided.`, `:3: String contains "@@" which indicates a canonical repository name reference that should be avoided.`, `:7: String contains "@@" which indicates a canonical repository name reference that should be avoided.`, `:8: String contains "@@" which indicates a canonical repository name reference that should be avoided.`, `:15: String contains "@@" which indicates a canonical repository name reference that should be avoided.`, + `20: String contains "@@" which indicates a canonical repository name reference that should be avoided.`, }, scopeBazel) + + // Test cases that should NOT warn (doc strings) + checkFindings(t, "canonical-repository", ` +def foo(): + """@@foo""" + pass +`, + []string{}, + scopeBazel) }