gen_srcs: fix cljs goog deps and canonicalize output for gazelle#104
Open
miridius wants to merge 3 commits into
Open
gen_srcs: fix cljs goog deps and canonicalize output for gazelle#104miridius wants to merge 3 commits into
miridius wants to merge 3 commits into
Conversation
miridius
force-pushed
the
dave/gen-srcs-goog-and-dict-fixes
branch
from
506a25e to
78e0697
Compare
goog.* namespaces are Closure Library, shipped transitively inside the ClojureScript jar rather than as scannable cljs namespaces, so gen_srcs silently dropped them from a file's deps. Route them to cljs.core's label so the dependency is emitted. Co-authored-by: Claude <noreply@anthropic.com>
buildifier keeps a single-entry dict inline, but gazelle's renderer puts
every non-empty dict one entry per line. buildifier accepts both, so it
never converges the two tools; emit non-empty dicts multi-line so gen_srcs
matches gazelle. Only {} stays inline.
Tested with property-based specs, which add test.check and a clj-kondo
lint-as for defspec.
Co-authored-by: Claude <noreply@anthropic.com>
gen-dir's rule order came from group-by hash-map iteration — deterministic but arbitrary. A gazelle plugin reproducing gen_srcs output has to match it byte-for-byte, and a lexical sort is trivial to reproduce whereas Clojure's HAMT hash order is not. Aggregation rules stay emitted last. Co-authored-by: Claude <noreply@anthropic.com>
miridius
force-pushed
the
dave/gen-srcs-goog-and-dict-fixes
branch
from
78e0697 to
25fed23
Compare
english
approved these changes
Jun 2, 2026
Comment on lines
+231
to
+233
| (let [entries (->> x | ||
| (sort-by (comp emit-bazel* key)) | ||
| (mapv (fn [[k v]] (str (emit-bazel* k) ": " (emit-bazel* v)))))] |
Contributor
There was a problem hiding this comment.
these are always small so probably doesn't matter, but you could mapv first and just sort after, and I think you'll get the same coll
Comment on lines
+553
to
+558
| ns (cond | ||
| ;; goog.* (Closure Library) ships transitively with ClojureScript; | ||
| ;; resolve it to cljs.core's label. | ||
| (and (= :cljs platform) (goog-ns? ns)) 'cljs.core | ||
| (= :cljs platform) (or (cljs-auto-alias src-ns->label dep-ns->label ns) ns) | ||
| :else ns) |
Contributor
There was a problem hiding this comment.
it's a bit tricky to follow the branches on first glance. could extract a fn and thread it here with e.g.
(cond-> ns (= :cljs platform) resolve-cljs-ns
Comment on lines
842
to
847
| (->> paths | ||
| (group-by fs/basename) | ||
| ;; group-by orders by hash; sort lexically | ||
| (sort-by key) | ||
| (mapcat (fn [[_base paths]] | ||
| (ns-rules args paths))))) |
Contributor
There was a problem hiding this comment.
paths is already sorted by str above, so we could avoid the group-by and so something like:
(->> paths
(partition-by fs/basename)
(mapcat (fn [paths] (ns-rules args paths))))| (finally | ||
| (fs/rm-rf (.toPath dir))))))) | ||
|
|
||
| (deftest ns-rules-cljs-goog-requires-route-to-clojurescript |
Contributor
There was a problem hiding this comment.
these tests are quite verbose and have lots of duplication. claude suggested:
(defn- deps-for
"Write one source file into a fresh temp dir, run ns-rules, return its deps.
Cleans up the temp dir."
[filename content dep-ns->label]
(let [dir (make-temp-dir)]
(try
(-> (gb/ns-rules (minimal-args dir dep-ns->label)
[(write-file dir filename content)])
extract-deps)
(finally (fs/rm-rf (.toPath dir))))))(deftest ns-rules-cljs-goog-requires-route-to-clojurescript
(testing "goog.* requires in a .cljs file resolve to cljs.core's label"
(let [deps (deps-for "ui.cljs"
"(ns example.ui (:require [goog.string :as gstr] [goog.dom :as dom]))"
{:clj {} :cljs {'cljs.core "org_clojure_clojurescript"}})]
(is (some #(= "@deps//:org_clojure_clojurescript" %) deps)
"goog.* should resolve to ClojureScript's label, not be dropped"))))
; ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
goog.*requires now resolve to ClojureScript's label (@deps//:org_clojure_clojurescript). They were silently dropped, becausegoog.*ships inside the ClojureScript jar rather than as a scannable namespace.env = {…}) emit one entry per line, matching gazelle's renderer.group-byhash-map order.Why
Found while dogfooding the new gazelle plugin against gen_srcs. For gazelle to replace gen_srcs as a drop-in, its output has to match gen_srcs byte-for-byte — so gen_srcs's output must be correct (the missing goog deps were a real bug), canonical (dict line-breaking), and reproducible by a non-Clojure tool. gen_srcs is already deterministic, but a lexical sort is trivial for the Go plugin to reproduce whereas Clojure's
group-byhash order is not.