From e6c6037183d9ea6d8c89a52d42ff0162211f1621 Mon Sep 17 00:00:00 2001 From: "Claude Opus 4.6" Date: Mon, 11 May 2026 19:40:22 +0200 Subject: [PATCH] fix(gen_srcs): skip unnecessary load symbols MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gen_srcs` always emitted `load("@rules_clojure//:rules.bzl", "clojure_library", "clojure_test")` in every BUILD file, even when: 1. There were no `clojure_test` targets — the "clojure_test" symbol was loaded but never used. 2. The directory had no `.clj` files at all — the entire `load()` statement was unnecessary. Both conditions are detected from a single `reduce` over the rules that `ns-rules` emits — `has-binary?` and `has-tests?` come from the same pass (`= :clojure_binary :type`, `= :clojure_test :type`). No re-scanning of paths. Empty directories (`has-content?` false) get a BUILD with the docstring header + `package()` only — no load, no `__clj_lib` / `__clj_files` filegroup. Matches Gazelle's behaviour. Also: - `test-path?` regex anchored: `#"_test.clj"` was unanchored — matched `_test.cljs`, `_test.cljbar`, etc. Tightened to `#"_test\.cljc?$"`. - `.clj-kondo/config.edn` with `exclude-destructured-keys-in-fn-args` + `exclude-destructured-as` — this codebase uses map destructuring in fn args as documentation of expected keys, so unused-binding warnings are noise. - `.gitignore` adjusted so `.clj-kondo/.cache/` is ignored but `.clj-kondo/config.edn` is tracked. Test coverage (gen_build_test.clj): - `load-symbols` helper parses the load() symbol set out of generated BUILD content; `run-gen-dir!` sets up a temp dir, calls gen-dir, returns content, cleans up. - `gen-dir-load-symbols`: covers .clj test, .cljc test, .cljs-only (negative — `ns-rules` emits `clojure_test` only for clj/cljc), no tests. - `gen-dir-includes-clojure-binary-when-ns-has-bazel-clojure-binary-meta`: a ns with `:bazel/clojure_binary` metadata adds "clojure_binary" to the load. - `gen-dir-subdirs-only`: dir with no own files but a clj-only subdir still emits load + `__clj_lib` referencing the subdir. - `gen-dir-skips-load-for-empty-dirs`: `__clj_lib` / `__clj_files` absent, `package()` still emitted. --- .clj-kondo/config.edn | 2 + .gitignore | 2 +- src/rules_clojure/gen_build.clj | 34 +++++----- test/rules_clojure/gen_build_test.clj | 93 ++++++++++++++++++++++++++- 4 files changed, 113 insertions(+), 18 deletions(-) create mode 100644 .clj-kondo/config.edn diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn new file mode 100644 index 0000000..f4a760c --- /dev/null +++ b/.clj-kondo/config.edn @@ -0,0 +1,2 @@ +{:linters {:unused-binding {:exclude-destructured-keys-in-fn-args true + :exclude-destructured-as true}}} diff --git a/.gitignore b/.gitignore index e69e4fc..f525096 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ -.clj-kondo/ +.clj-kondo/.cache/ .cpcache/ .ijwb/ .lsp/ diff --git a/src/rules_clojure/gen_build.clj b/src/rules_clojure/gen_build.clj index 3925a45..66ffdb5 100644 --- a/src/rules_clojure/gen_build.clj +++ b/src/rules_clojure/gen_build.clj @@ -628,7 +628,7 @@ (re-find #"\.js$" (str path))) (defn test-path? [path] - (boolean (re-find #"_test.clj" (str path)))) + (boolean (re-find #"_test\.cljc?$" (str path)))) (defn src-path? [path] (not (test-path? path))) @@ -808,25 +808,29 @@ subdirs (filter (fn [p] (some clj*-path? (seq (fs/ls-r p)))))) - [has-binary? rules] (reduce (fn [[hb? acc] rule] - [(or hb? (= :clojure_binary (:type rule))) - (conj acc (emit-rule rule))]) - [false []] - (->> paths - (group-by fs/basename) - (mapcat (fn [[_base paths]] - (ns-rules args paths))))) - + [has-binary? has-tests? rules] + (reduce (fn [[hb? ht? acc] rule] + [(or hb? (= :clojure_binary (:type rule))) + (or ht? (= :clojure_test (:type rule))) + (conj acc (emit-rule rule))]) + [false false []] + (->> paths + (group-by fs/basename) + (mapcat (fn [[_base paths]] + (ns-rules args paths))))) + has-content? (or (seq paths) (seq clj-subdirs)) content (str (build-file-header "the `//:gen_srcs` target from `rules_clojure`") - (emit-bazel (apply list 'load "@rules_clojure//:rules.bzl" - (cond-> ["clojure_library" "clojure_test"] - has-binary? (conj "clojure_binary")))) - "\n\n" + (when has-content? + (str (emit-bazel (apply list 'load "@rules_clojure//:rules.bzl" + (cond-> ["clojure_library"] + has-tests? (conj "clojure_test") + has-binary? (conj "clojure_binary")))) + "\n\n")) (emit-bazel (list 'package (kwargs {:default_visibility ["//visibility:public"]}))) "\n" (when (seq rules) (str "\n" (str/join "\n\n" rules) "\n")) - (when (or (seq paths) (seq clj-subdirs)) + (when has-content? (str "\n" (emit-bazel (list 'clojure_library (kwargs {:name "__clj_lib" :deps (vec diff --git a/test/rules_clojure/gen_build_test.clj b/test/rules_clojure/gen_build_test.clj index 703ff6c..9407e48 100644 --- a/test/rules_clojure/gen_build_test.clj +++ b/test/rules_clojure/gen_build_test.clj @@ -20,7 +20,7 @@ (fs/->path (.getAbsolutePath f)))) (defn- minimal-args - "Build a minimal args map for ns-rules with the given dep-ns->label map." + "Build a minimal args map for ns-rules / gen-dir with the given dep-ns->label map." [dir dep-ns->label] (let [dir-path (fs/->path (.getAbsolutePath dir))] {:src-ns->label {} @@ -174,7 +174,6 @@ (finally (fs/rm-rf (.toPath dir))))))) - ;; ---- formatting tests (emit-bazel) ---- (deftest test-emit-bazel-vector-inline @@ -260,3 +259,93 @@ (is (zero? (:exit result)) (str "buildifier failed: " (:err result))) (is (= content (:out result)) (str "buildifier produced diff. Expected:\n" content "\nGot:\n" (:out result)))))))) + +;; ---- gen-dir load-symbol pruning ---- + +(defn- load-symbols + "Extract the symbol args of the rules_clojure load() from generated BUILD content. + Returns a set of symbol strings, or nil if no rules_clojure load() is present. + The first captured string is the bzl path; drop it and keep the symbol names." + [content] + (when-let [load-call (re-find #"(?s)load\(\"@rules_clojure//:rules.bzl\"[^)]*\)" content)] + (set (->> (re-seq #"\"([^\"]+)\"" load-call) + (map second) + rest)))) + +(defn- run-gen-dir! + "Set up a temp dir with the given relative-path → content files, call gen-dir, + and return the BUILD.bazel content (nil if no BUILD was written). + Cleans the temp dir via teardown. dep-ns->label defaults to empty per platform." + [files & {:keys [dep-ns->label] + :or {dep-ns->label {:clj {} :cljs {}}}}] + (let [dir (make-temp-dir) + src-dir (fs/->path (.getAbsolutePath dir) "src" "example")] + (try + (doseq [[rel-path content] files] + (let [target (io/file (.toFile src-dir) rel-path)] + (.mkdirs (.getParentFile target)) + (spit target content))) + (gb/gen-dir (minimal-args dir dep-ns->label) src-dir) + (let [build-file (io/file (.toFile src-dir) "BUILD.bazel")] + (when (.exists build-file) (slurp build-file))) + (finally + (fs/rm-rf (.toPath dir)))))) + +(deftest gen-dir-load-symbols + (testing "library only — load includes only clojure_library" + (let [content (run-gen-dir! {"core.clj" "(ns example.core)"})] + (is (= #{"clojure_library"} (load-symbols content))))) + + (testing "library + .clj test — load includes clojure_test" + (let [content (run-gen-dir! + {"core.clj" "(ns example.core)" + "core_test.clj" "(ns example.core-test (:require [clojure.test :refer [deftest]]))"} + :dep-ns->label {:clj {'clojure.test "org_clojure_clojure"} :cljs {}})] + (is (= #{"clojure_library" "clojure_test"} (load-symbols content)) + "should load clojure_test when .clj test files exist"))) + + (testing "library + .cljc test — load includes clojure_test" + (let [content (run-gen-dir! + {"core.cljc" "(ns example.core)" + "core_test.cljc" "(ns example.core-test (:require [clojure.test :refer [deftest]]))"} + :dep-ns->label {:clj {'clojure.test "org_clojure_clojure"} :cljs {}})] + (is (= #{"clojure_library" "clojure_test"} (load-symbols content)) + "should load clojure_test when .cljc test files exist"))) + + (testing ".cljs-only test files — load does NOT include clojure_test (ns-rules emits clojure_test only for clj/cljc)" + (let [content (run-gen-dir! + {"core.cljs" "(ns example.core)" + "core_test.cljs" "(ns example.core-test (:require [cljs.test :refer-macros [deftest]]))"} + :dep-ns->label {:clj {} :cljs {'cljs.test "org_clojure_clojurescript"}})] + (is (= #{"clojure_library"} (load-symbols content)) + "should NOT load clojure_test for .cljs-only tests")))) + +(deftest gen-dir-includes-clojure-binary-when-ns-has-bazel-clojure-binary-meta + (testing "ns with :bazel/clojure_binary metadata adds clojure_binary to the load" + (let [content (run-gen-dir! + {"main.clj" (str "(ns example.main\n" + " {:bazel/clojure_binary {}}\n" + " (:gen-class))")})] + (is (contains? (load-symbols content) "clojure_binary") + (str "expected clojure_binary in load, got: " (load-symbols content)))))) + +(deftest gen-dir-skips-load-for-empty-dirs + (testing "directory with no clj files: load(), __clj_lib, __clj_files all absent; package() still emitted" + (let [content (run-gen-dir! {})] + (is (not (re-find #"(?m)^load\(" content)) + "should not emit load() for empty directories") + (is (not (re-find #"name = \"__clj_lib\"" content)) + "should not emit __clj_lib rule for empty directories") + (is (not (re-find #"name = \"__clj_files\"" content)) + "should not emit __clj_files filegroup for empty directories") + (is (re-find #"(?m)^package\(" content) + "should still emit package() (matches Gazelle behaviour)")))) + +(deftest gen-dir-subdirs-only + (testing "dir with no own files but a clj-only subdir still emits load + __clj_lib referencing the subdir" + (let [content (run-gen-dir! {"child/core.clj" "(ns example.child.core)"})] + (is (= #{"clojure_library"} (load-symbols content))) + (is (re-find #"name = \"__clj_lib\"" content) + "should emit __clj_lib aggregating subdirs") + (is (re-find #"\"//src/example/child:__clj_lib\"" content) + "__clj_lib deps should reference the clj subdir"))))