Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions WARNINGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/...`
Expand Down
8 changes: 4 additions & 4 deletions warn/docs/warnings.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down
28 changes: 28 additions & 0 deletions warn/warn_bazel.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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.`))
Expand Down
36 changes: 33 additions & 3 deletions warn/warn_bazel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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) {
Expand All @@ -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)
}