Skip to content

Fix parsing ns :require vectors with #_ comments #416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
weavejester merged 1 commit into from
May 24, 2026
Merged

Fix parsing ns :require vectors with #_ comments #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 25 additions & 12 deletions cljfmt/src/cljfmt/core.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,12 @@
(count)
(dec)))

(defn- meta? [zloc]
(#{:meta :meta*} (z/tag zloc)))

(defn- skip-meta [zloc]
(if (#{:meta :meta*} (z/tag zloc))
(-> zloc z/down z/right)
(if (meta? zloc)
(-> zloc z/down z/right recur)
zloc))

(defn- cursive-two-space-list-indent? [zloc]
Expand Down Expand Up @@ -829,13 +832,18 @@

#?(:clj
(defn- refer-zloc->refer-mapping [refer-zloc]
(let [refer-vec (some-> refer-zloc z/right z/sexpr)
Copy link
Copy Markdown
Contributor Author

@camsaul camsaul May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refer-vec was incorrect since x in :refer x can be either a list or a vector

current-ns (some-> refer-zloc leftmost-symbol z/sexpr)
grandparent-node (some-> refer-zloc z/up z/up)
parent-ns (ns-require-form-parent grandparent-node)]
(when (and (vector? refer-vec) (symbol? current-ns))
(let [refers (some-> refer-zloc
(z/find-next (comp skip-meta z/right)
(some-fn z/vector? z/list?))
z/sexpr)
current-ns (some-> refer-zloc leftmost-symbol z/sexpr)
grandparent-node (some-> refer-zloc
(z/find-next z/up (complement meta?))
(z/find-next z/up (complement meta?)))
parent-ns (ns-require-form-parent grandparent-node)]
(when (and (sequential? refers) (symbol? current-ns))
(let [ns-str (join-ns-str parent-ns current-ns)]
(->> refer-vec (map (fn [sym] [(str sym) ns-str])) (into {})))))))
(->> refers (map (fn [sym] [(str sym) ns-str])) (into {})))))))

#?(:clj (defn- refer-map-for-form [form]
(when-let [req-zloc (-> form z/of-node (z/find z/next ns-require-form?))]
Expand All @@ -845,10 +853,15 @@

#?(:clj
(defn- as-zloc->alias-mapping [as-zloc]
(let [alias (some-> as-zloc z/right z/sexpr)
current-ns (some-> as-zloc leftmost-symbol z/sexpr)
grandparent-node (some-> as-zloc z/up z/up)
parent-ns (ns-require-form-parent grandparent-node)]
(let [alias (some-> as-zloc
(z/find-next (comp skip-meta z/right)
symbol-node?)
z/sexpr)
current-ns (some-> as-zloc leftmost-symbol z/sexpr)
grandparent-node (some-> as-zloc
(z/find-next z/up (complement meta?))
(z/find-next z/up (complement meta?)))
parent-ns (ns-require-form-parent grandparent-node)]
(when (and (symbol? alias) (symbol? current-ns))
{(str alias) (join-ns-str parent-ns current-ns)}))))

Expand Down
31 changes: 22 additions & 9 deletions cljfmt/test/cljfmt/core_test.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
replace-newlines wrap-normalize-newlines
realign-form unalign-form]]
[cljfmt.test-util.common]
[clojure.string :as str]
[rewrite-clj.node :as n]
[rewrite-clj.parser :as p])
#?(:cljs (:require-macros [cljfmt.test-util.cljs])))
Expand Down Expand Up @@ -323,24 +324,36 @@
"#_:clj-kondo/ignore "
"^tag "
"#_old-thing "]
ns-vec-str [(str ignore-str "[thing.core :as t]")
(str ignore-str "[thing [core :as t]]")
(str ignore-str "(thing [core :as t])")
(str "[" ignore-str "thing.core :as t]")
(str ignore-str " [" ignore-str "thing.core :as t]")]
:let [ns-str (str "(ns example (:require " ns-vec-str "))")]]
(testing ns-str
require-str ["<>[<>thing.core :as <>t :refer <>[<>my-defn]]"
Copy link
Copy Markdown
Contributor Author

@camsaul camsaul May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would have been over 5000 variations getting tested here if we did just put the metadata or uneval #_ comment form in each individual location it could go separately so instead I'm just putting it in every possible slot at once. <> is a placeholder for places it can go here and then we str/replace below

"<>[<>thing <>[<>core :as <>t :refer <>[<>my-defn]]]"]
:let [ns-str (str "(ns my-namespace (:require " require-str "))")]
ns-str [ns-str
(-> ns-str
Copy link
Copy Markdown
Contributor Author

@camsaul camsaul May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try with both vectors for everything and lists for everything since either is allowed I guess, but change everything at once so we don't end up with too much of a combinatorial explosion

(str/replace #"\[" "(")
(str/replace #"\]" ")"))]
ns-str [(str/replace ns-str #"<>" ignore-str)
(str/replace ns-str #"<>" (str ignore-str ignore-str))]]
Comment on lines +334 to +335
Copy link
Copy Markdown
Contributor Author

@camsaul camsaul May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found another bug where multiple metadata forms e.g. ^:private ^:dynamic or whatever were not skipped over correctly in skip-meta, so I fixed that was well; updated this test to make sure we're checking strings where we use multiple uneval #_... comment forms or ^... metadata forms in a row

(testing (str \newline (pr-str ns-str))
(is (reformats-to?
[ns-str
""
"(t/defn foo [x]"
"(+ x 1))"
""
"(my-defn foo [x]"
Copy link
Copy Markdown
Contributor Author

@camsaul camsaul May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to also test :refer handling at the same time

"(+ x 1))"]
[ns-str
""
"(t/defn foo [x]"
" (+ x 1))"
""
"(my-defn foo [x]"
" (+ x 1))"]
{:indents {'ns [[:block 1]], 'thing.core/defn [[:inner 0]]}
#?@(:cljs [:alias-map {"t" "thing.core"}])})))))
{:indents {'ns [[:block 1]]
'thing.core/defn [[:inner 0]]
'thing.core/my-defn [[:inner 0]]}
#?@(:cljs [:alias-map {"t" "thing.core"}
:refer-map {"my-defn" "thing.core"}])})))))
(is (reformats-to?
["(comment)"
"(ns thing.core)"
Expand Down
Loading