Wrap an error & some linting

.golangci.yml

@@ -34,6 +34,14 @@ linters:
         - shadow # Probably not useful enough to clean everything up
         - fieldalignment # TODO(peterebden): Should clean up these warnings and enable (although they are pretty finicky)
       enable-all: true
+      settings:
+        printf:
+          funcs:
+            # These panic with fmt.Errorf(msg, args...) under the hood, so treating them as
+            # printf wrappers lets vet check their format strings/args like any other Errorf call.
+            - (*github.com/thought-machine/please/src/parse/asp.scope).Error
+            - (*github.com/thought-machine/please/src/parse/asp.scope).Assert
+            - (*github.com/thought-machine/please/src/parse/asp.scope).NAssert
     perfsprint:
       errorf: false # Sometimes it's easier not to import another package
   exclusions:
@@ -105,7 +113,6 @@ linters:
     paths:
       - test_data
       - third_party$
-      - ^tree$
       - ^plz-out$
       - builtin$
       - examples$

src/audit/BUILD

@@ -3,8 +3,8 @@ go_library(
     srcs = [
         "audit.go",
     ],
-    visibility = ["PUBLIC"],
     pgo_file = "//:pgo",
+    visibility = ["PUBLIC"],
     deps = [
         "//src/cli/logging",
         "//src/fs",

src/parse/asp/interpreter.go

@@ -271,7 +271,7 @@ func (i *interpreter) Subinclude(pkgScope *scope, path string, label core.BuildL
 		}
 		return locals, nil
 	})
-	pkgScope.Assert(err == nil, "failed to subinclude %s: %s", label, err)
+	pkgScope.Assert(err == nil, "failed to subinclude %s: %w", label, err)
 	return globals
 }
 

src/plz/BUILD

@@ -1,20 +1,23 @@
 go_library(
     name = "plz",
-    srcs = glob(["*.go"], exclude=["*_test.go"]),
+    srcs = glob(
+        ["*.go"],
+        exclude = ["*_test.go"],
+    ),
     pgo_file = "//:pgo",
     visibility = ["PUBLIC"],
     deps = [
-        "///third_party/go/golang.org_x_sync//errgroup",
         "///third_party/go/github.com_peterebden_go-cli-init_v5//flags",
+        "///third_party/go/golang.org_x_sync//errgroup",
         "//src/build",
-        "//src/cmap",
         "//src/cli",
         "//src/cli/logging",
+        "//src/cmap",
         "//src/core",
         "//src/fs",
         "//src/metrics",
-        "//src/parse/asp",
         "//src/parse",
+        "//src/parse/asp",
         "//src/remote",
         "//src/test",
     ],

test/BUILD

@@ -66,16 +66,16 @@ gentest(
 plz_e2e_test(
     name = "test_outputs_move_test",
     cmd = "plz test //test:test_outputs_move_test_case -- some_test_arg",
-    expect_output_contains = "4 passed.",
     expect_file_exists = "plz-out/bin/test/foo.txt",
+    expect_output_contains = "4 passed.",
 )
 
 gentest(
     name = "test_outputs_move_test_case",
     data = ["test_output_test_1.txt"],
     labels = ["manual"],
-    test_outputs = ["foo.txt"],
     test_cmd = "cp $(location test_output_test_1.txt) test.results && echo 'hello' > foo.txt #",
+    test_outputs = ["foo.txt"],
 )
 
 # Test that on re-running a test it is cached.

test/audit/BUILD

@@ -2,8 +2,8 @@ subinclude("//test/build_defs")
 
 please_repo_e2e_test(
     name = "audit_test",
-    repo = "test_repo",
     plz_command = "./test_audit.sh",
+    repo = "test_repo",
     # Asserts that the plugin download is audited, which only happens if we actually download it.
     shared_cache = False,
 )

test/cyclic_dependency/BUILD

@@ -2,10 +2,10 @@ subinclude("//test/build_defs")
 
 please_repo_e2e_test(
     name = "cyclic_dependency_test",
-    expected_failure = True,
     expect_output_contains = {
         "output.txt": "Dependency cycle found",
     },
+    expected_failure = True,
     plz_command = "plz test //cycle:all > output.txt 2>&1",
     repo = "test_repo",
 )

test/get_labels/BUILD

@@ -4,10 +4,10 @@ def maxdepth_test(name:str, deps:list=None, maxdepth:int, expected:list):
     test_case = genrule(
         name = name,
         outs = [f"{name}.sh"],
+        binary = True,
         cmd = {
             "opt": "echo '#!/bin/sh' > $OUTS",
         },
-        binary = True,
         labels = [
             f"name:{name}",
             "manual",
@@ -41,63 +41,72 @@ def dep(name:str, deps:list=None):
 
 def echo_name_labels_up_to(maxdepth:int):
     def echo(name:str):
-        labels = get_labels(name, "name:", maxdepth=maxdepth)
+        labels = get_labels(name, "name:", maxdepth = maxdepth)
         set_command(name, "opt", " && ".join([get_command(name, "opt")] + [f"echo 'echo {l}' >> $OUTS" for l in labels]))
+
     return echo
 
-dep(name = "dep1", deps = [":dep3"])
+dep(
+    name = "dep1",
+    deps = [":dep3"],
+)
+
 dep(name = "dep2")
-dep(name = "dep3", deps = [":dep4", ":dep5"])
+
+dep(
+    name = "dep3",
+    deps = [
+        ":dep4",
+        ":dep5",
+    ],
+)
+
 dep(name = "dep4")
+
 dep(name = "dep5")
 
 maxdepth_test(
     name = "target_only",
+    expected = ["target_only"],
+    maxdepth = 0,
     deps = [
         ":dep1",
         ":dep2",
     ],
-    maxdepth = 0,
-    expected = ["target_only"],
 )
 
 maxdepth_test(
     name = "direct_deps",
-    deps = [
-        ":dep1",
-        ":dep2",
-    ],
-    maxdepth = 1,
     expected = [
         "dep1",
         "dep2",
         "direct_deps",
     ],
-)
-
-maxdepth_test(
-    name = "second_level_deps",
+    maxdepth = 1,
     deps = [
         ":dep1",
         ":dep2",
-        ":dep3",
     ],
-    maxdepth = 2,
+)
+
+maxdepth_test(
+    name = "second_level_deps",
     expected = [
         "dep1",
         "dep2",
         "dep3",
         "second_level_deps",
     ],
-)
-
-maxdepth_test(
-    name = "all_deps",
+    maxdepth = 2,
     deps = [
         ":dep1",
         ":dep2",
+        ":dep3",
     ],
-    maxdepth = -1,
+)
+
+maxdepth_test(
+    name = "all_deps",
     expected = [
         "all_deps",
         "dep1",
@@ -106,6 +115,11 @@ maxdepth_test(
         "dep4",
         "dep5",
     ],
+    maxdepth = -1,
+    deps = [
+        ":dep1",
+        ":dep2",
+    ],
 )
 
 # Regression test: a pre-build function calling get_labels() must see the target's whole transitive

test/parse/subinclude_race/BUILD

@@ -2,12 +2,6 @@ subinclude("//test/build_defs")
 
 please_repo_e2e_test(
     name = "subinclude_race_test",
-    expected_output = {
-        "plz-out/gen/a/x.txt": "hello",
-        "plz-out/gen/b/y.txt": "hello",
-    },
-    plz_command = "plz build //a:x //b:y",
-    repo = "test_repo",
     # Regression test for a deadlock between two packages that each subinclude their own output, where
     # that cascades into needing a target defined by the other package (a:self -> b:self -> b:dep ->
     # a:dep). Depending on which package's parse gets claimed first, the goroutine parsing one can end
@@ -17,4 +11,10 @@ please_repo_e2e_test(
     # able to catch it - only requesting both packages independently, in parallel, reproduces it.
     # Bounded so a regression fails the test rather than hanging until the CI job is killed.
     timeout = 60,
+    expected_output = {
+        "plz-out/gen/a/x.txt": "hello",
+        "plz-out/gen/b/y.txt": "hello",
+    },
+    plz_command = "plz build //a:x //b:y",
+    repo = "test_repo",
 )

test/runtime_deps/BUILD

@@ -13,14 +13,14 @@ please_repo_e2e_test(
 # print the target's direct run-time dependencies.
 please_repo_e2e_test(
     name = "query_print_test",
-    plz_command = " && ".join([
-        "plz query print -f runtime_deps //test:runtime_deps_test_case > runtime_deps_test_case",
-        "plz query print -f runtime_deps //test:target_with_no_runtime_deps > target_with_no_runtime_deps",
-    ]),
     expected_output = {
         "runtime_deps_test_case": "//test:target_with_runtime_deps",
         "target_with_no_runtime_deps": "",
     },
+    plz_command = " && ".join([
+        "plz query print -f runtime_deps //test:runtime_deps_test_case > runtime_deps_test_case",
+        "plz query print -f runtime_deps //test:target_with_no_runtime_deps > target_with_no_runtime_deps",
+    ]),
     repo = "repo",
 )
 
@@ -50,20 +50,20 @@ _expected_deps = """\
 
 please_repo_e2e_test(
     name = "query_deps_test",
-    plz_command = "plz query deps //test:runtime_deps_test_case > deps",
     expected_output = {
         "deps": _expected_deps,
     },
+    plz_command = "plz query deps //test:runtime_deps_test_case > deps",
     repo = "repo",
 )
 
 # Ensure that run-time dependencies are in fact considered dependencies by `plz query revdeps`.
 please_repo_e2e_test(
     name = "query_revdeps_test",
-    plz_command = "plz query revdeps //test:another_runtime_dep > revdeps",
     expected_output = {
         "revdeps": "//test:target_with_another_runtime_dep",
     },
+    plz_command = "plz query revdeps //test:another_runtime_dep > revdeps",
     repo = "repo",
 )
 

test/source_list_files/BUILD

@@ -2,34 +2,34 @@ subinclude("//test/build_defs")
 
 please_repo_e2e_test(
     name = "srcs_test",
-    plz_command = "plz build //:with_srcs",
     expected_output = {
-      "plz-out/gen/with_srcs.txt": "a.txt\nb.txt",
+        "plz-out/gen/with_srcs.txt": "a.txt\nb.txt",
     },
+    plz_command = "plz build //:with_srcs",
     repo = "test_repo",
 )
 
 please_repo_e2e_test(
     name = "src_test",
-    plz_command = "plz build //:with_one_src",
     expected_output = {
-      "plz-out/gen/with_one_src.txt": "a.txt",
+        "plz-out/gen/with_one_src.txt": "a.txt",
     },
+    plz_command = "plz build //:with_one_src",
     repo = "test_repo",
 )
 
 please_repo_e2e_test(
     name = "named_srcs_test",
-    plz_command = "plz build //:with_named_srcs",
     expected_output = {
-      "plz-out/gen/with_named_srcs.txt": "a.txt\nb.txt",
+        "plz-out/gen/with_named_srcs.txt": "a.txt\nb.txt",
     },
+    plz_command = "plz build //:with_named_srcs",
     repo = "test_repo",
 )
 
 please_repo_e2e_test(
     name = "flag_not_set_test",
+    expected_failure = True,
     plz_command = "plz build //:with_flag_not_set",
     repo = "test_repo",
-    expected_failure = True,
 )

test/subrepo/parse_deadlock/BUILD

@@ -2,11 +2,11 @@ subinclude("//test/build_defs")
 
 please_repo_e2e_test(
     name = "parse_deadlock_test",
+    # Bounded so a regression fails the test rather than hanging until the CI job is killed.
+    timeout = 120,
     expected_output = {
         "plz-out/gen/pkg/x.txt": "hello",
     },
     plz_command = "plz build //pkg:x",
     repo = "test_repo",
-    # Bounded so a regression fails the test rather than hanging until the CI job is killed.
-    timeout = 120,
 )