[buildifier] false positive on canonical-repository linter

[bweston92]

canonical-repository is flagging a false positive, strings with string substitutions containing @@ as causing a flag.

Extract of code causing the flag:

...
    ctx.actions.expand_template(
        template = ctx.file._runner,
        output = out_file,
        substitutions = {
            "@@GOFMT_BIN@@": shell.quote(gofmt_path),
            "@@GOROOT@@": shell.quote(sdk.root_file.dirname),
        },
        is_executable = True,
    )
    transitive_depsets = [
        depset([sdk.go]),
    ]
...

Buildifier output on master:

$ buildifier -lint warn -r .
tools/linters/golang/defs.bzl:22: canonical-repository: String contains "@@" which indicates a canonical repository name reference that should be avoided. (https://github.com/bazelbuild/buildtools/blob/main/WARNINGS.md#canonical-repository)
tools/linters/golang/defs.bzl:23: canonical-repository: String contains "@@" which indicates a canonical repository name reference that should be avoided. (https://github.com/bazelbuild/buildtools/blob/main/WARNINGS.md#canonical-repository)

Buildifier output on v8.2.1

$ buildifier -lint warn -r .

[vladmos]

@nickschaap

[bweston92]

Patch containing a failing test case.

diff --git a/warn/warn_bazel_test.go b/warn/warn_bazel_test.go
index eacf90e..7d25de6 100644
--- a/warn/warn_bazel_test.go
+++ b/warn/warn_bazel_test.go
@@ -235,4 +235,13 @@ py_binary(
                        `:15: String contains "@@" which indicates a canonical repository name reference that should be avoided.`,
                },
                scopeBazel)
+
+       checkFindings(t, "canonical-repository", `
+substitutions = {
+    "@@FOO@@": shell.quote("foo"),
+    "@@BAR@@": shell.quote("bar"),
+}
+`,
+               []string{},
+               scopeBazel)
 }

Logs

$ bazel test --test_output=all //warn:warn_test
INFO: Analyzed target //warn:warn_test (0 packages loaded, 0 targets configured).
FAIL: //warn:warn_test (Exit 1) (see /home/bweston/.cache/bazel/_bazel_bweston/baad30b77e25348613b5116a74cb8fa8/execroot/_main/bazel-out/k8-fastbuild/testlogs/warn/warn_test/test.log)
INFO: From Testing //warn:warn_test:
==================== Test output for //warn:warn_test:
--- FAIL: TestCanonicalRepositoryWarning (0.01s)
    warn_test.go:124: Input: 
        substitutions = {
                "@@FOO@@": shell.quote("foo"),
                "@@BAR@@": shell.quote("bar"),
        }
    warn_test.go:125: number of matches: 2, want 0
    warn_test.go:130: got: 2: String contains "@@" which indicates a canonical repository name reference that should be avoided.
    warn_test.go:130: got: 3: String contains "@@" which indicates a canonical repository name reference that should be avoided.
    warn_test.go:124: Input: 
        substitutions = {
                "@@FOO@@": shell.quote("foo"),
                "@@BAR@@": shell.quote("bar"),
        }
    warn_test.go:125: number of matches: 2, want 0
    warn_test.go:130: got: 2: String contains "@@" which indicates a canonical repository name reference that should be avoided.
    warn_test.go:130: got: 3: String contains "@@" which indicates a canonical repository name reference that should be avoided.
    warn_test.go:124: Input: 
        substitutions = {
                "@@FOO@@": shell.quote("foo"),
                "@@BAR@@": shell.quote("bar"),
        }
    warn_test.go:125: number of matches: 2, want 0
    warn_test.go:130: got: 2: String contains "@@" which indicates a canonical repository name reference that should be avoided.
    warn_test.go:130: got: 3: String contains "@@" which indicates a canonical repository name reference that should be avoided.
    warn_test.go:124: Input: 
        substitutions = {
                "@@FOO@@": shell.quote("foo"),
                "@@BAR@@": shell.quote("bar"),
        }
    warn_test.go:125: number of matches: 2, want 0
    warn_test.go:130: got: 2: String contains "@@" which indicates a canonical repository name reference that should be avoided.
    warn_test.go:130: got: 3: String contains "@@" which indicates a canonical repository name reference that should be avoided.
FAIL
================================================================================
INFO: Found 1 test target...
Target //warn:warn_test up-to-date:
  bazel-bin/warn/warn_test_/warn_test
INFO: Elapsed time: 0.792s, Critical Path: 0.47s
INFO: 2 processes: 1 internal, 1 linux-sandbox.
INFO: Build completed, 1 test FAILED, 2 total actions
//warn:warn_test                                                         FAILED in 0.5s
  /home/bweston/.cache/bazel/_bazel_bweston/baad30b77e25348613b5116a74cb8fa8/execroot/_main/bazel-out/k8-fastbuild/testlogs/warn/warn_test/test.log

Executed 1 out of 1 test: 1 fails locally.

[nickschaap]

@vladmos Any ideas on how we can make this check more precise. Is it possible to determine if a given string expression is being used as a label? We could potentially check that the string expression contains both "@@" and "//" but then we miss out on cases where label shorthands are being used.

For example, we wouldn't catch:

@@my_external_repo

we would catch:

@@my_external_repo//:my_external_repo

[fmeum]

Checking for strings that start with @@ and contain // seems pretty decent. Canonical repo name shorthands are probably no longer relevant with Bzlmod since it's very unlikely that a canonical repo name would show up as a target name. They would likely lead to build errors anyway.