Repository: clojure-lsp/clojure-lsp
Pull request #233 (2021-01-02 23:40:11)

Improve clean ns
* Fix bug where it was messing up require form if it the first unused require has a `:refer`
* Improve a little bit the performance calling `find-references` only one time instead of twice
* Now we finally remove unused refers even if it contains one used:

Before apply clean-ns
```clojure
(:require [clojure-sample.c :refer [foo bar]])
(foo)
```
After apply clean-ns
```clojure
(:require [clojure-sample.c :refer [foo]])
(foo)
```



Patch:
diff --git a/src/clojure_lsp/crawler.clj b/src/clojure_lsp/crawler.clj
index e378dae5..997ed3f3 100644
--- a/src/clojure_lsp/crawler.clj
+++ b/src/clojure_lsp/crawler.clj
@@ -22,33 +22,34 @@
 (defn ^:private to-file [path child]
   (.toFile (.resolve path child)))
 
-(defn ^:private process-unused
-  [usages declarations]
-  (let [ensure-sym (fn [s] (when-not (string? s) s))]
+(defn find-unused-aliases [usages]
+  (let [declarations (f.diagnostic/usages->declarations usages)
+        excludes (-> (get-in @db/db [:settings :linters :unused-namespace :exclude] #{}) set)
+        declared-aliases (->> declarations
+                              (filter (comp #(contains? % :alias) :tags))
+                              (remove (comp excludes :ns)))
+        ensure-sym (fn [s] (when-not (string? s) s))]
     (->> usages
          (remove (comp #(or (contains? % :declare)
                             (contains? % :refer)) :tags))
          (map #(some-> % :sym ensure-sym namespace symbol))
          set
-         (set/difference (set (map :ns declarations))))))
+         (set/difference (set (map :ns declared-aliases))))))
 
-(defn find-unused-aliases [uri]
-  (let [usages (f.references/safe-find-references uri (slurp uri) false false)
-        declarations (f.diagnostic/usages->declarations usages)
-        excludes (-> (get-in @db/db [:settings :linters :unused-namespace :exclude] #{}) set)
-        declared-aliases (->> declarations
-                              (filter (comp #(contains? % :alias) :tags))
-                              (remove (comp excludes :ns)))]
-    (process-unused usages declared-aliases)))
-
-(defn find-unused-refers [uri]
-  (let [usages (f.references/safe-find-references uri (slurp uri) false false)
-        declarations (f.diagnostic/usages->declarations usages)
+(defn find-unused-refers [usages]
+  (let [declarations (f.diagnostic/usages->declarations usages)
         excludes (-> (get-in @db/db [:settings :linters :unused-namespace :exclude] #{}) set)
         declared-refers (->> declarations
                               (filter (comp #(contains? % :refer) :tags))
-                              (remove (comp excludes :ns)))]
-    (process-unused usages declared-refers)))
+                              (remove (comp excludes :ns)))
+        ensure-sym (fn [s] (when-not (string? s) s))]
+    (->> usages
+         (remove (comp #(or (contains? % :declare)
+                            (contains? % :refer)) :tags))
+         (filter #(some-> % :sym ensure-sym namespace))
+         (map #(some-> % :sym ensure-sym))
+         set
+         (set/difference (set (map :sym declared-refers))))))
 
 (defn crawl-jars [jars dependency-scheme]
   (let [jar-dependency-scheme? (= "jar" dependency-scheme)
diff --git a/src/clojure_lsp/refactor/transform.clj b/src/clojure_lsp/refactor/transform.clj
index ecd978d3..293b1b1c 100644
--- a/src/clojure_lsp/refactor/transform.clj
+++ b/src/clojure_lsp/refactor/transform.clj
@@ -2,6 +2,7 @@
   (:require
    [clojure-lsp.crawler :as crawler]
    [clojure-lsp.db :as db]
+   [clojure-lsp.feature.references :as f.references]
    [clojure-lsp.parser :as parser]
    [clojure-lsp.refactor.edit :as edit]
    [clojure.set :as set]
@@ -253,30 +254,67 @@
                        (z/insert-child 'let)
                        (edit/join-let))}]))))))
 
+(defn ^:private safe-remove-node
+  [node]
+  (let [removed-node (-> node z/remove)]
+    (if (z/list? removed-node)
+      (z/down removed-node)
+      (z/up removed-node))))
+
+(defn ^:private remove-unused-refers
+  [node unused-refers]
+  (let [node-refers (-> node z/down (z/find-next-value ':refer) z/right z/sexpr set)
+        unused-refers-symbol (->> unused-refers (map (comp symbol name)) set)
+        removed-refers (set/difference node-refers unused-refers-symbol)]
+    (if (empty? removed-refers)
+      (safe-remove-node node)
+      (-> node
+          z/down
+          z/rightmost
+          (z/replace (n/vector-node (vec removed-refers)))
+          z/up))))
+
 (defn ^:private remove-unused-require
-  [node unused-aliases]
-  (if (z/vector? node)
-    (let [alias-node (-> node z/down z/leftmost)]
-      (if (contains? unused-aliases (z/sexpr alias-node))
-        (let [removed-node (-> node z/remove)]
-          (if (z/list? removed-node)
-            (z/down removed-node)
-            (z/up removed-node)))
-        node))
+  [node unused-aliases unused-refers]
+  (cond
+    (not (z/vector? node))
+    node
+
+    (contains? unused-aliases (-> node z/down z/leftmost z/sexpr))
+    (safe-remove-node node)
+
+    (contains? (->> unused-refers (map (comp symbol namespace)) set)
+               (-> node z/down z/leftmost z/sexpr))
+    (remove-unused-refers node unused-refers)
+
+    :else
     node))
 
 (defn ^:private remove-unused-requires
-  [unused-ns nodes]
+  [unused-aliases unused-refers nodes]
   (let [single-require? (= 1 (count (z/child-sexprs nodes)))
         first-node      (z/next nodes)
+        first-node-ns   (when (and single-require?
+                                   (z/vector? first-node))
+                          (-> first-node z/down z/leftmost z/sexpr))
+        first-node-refers (when (and single-require?
+                                     (z/vector? first-node))
+                            (->> (-> first-node
+                                     z/down
+                                     (z/find-next-value ':refer)
+                                     z/right
+                                     z/sexpr)
+                                 (map #(symbol (str first-node-ns) (str %)))
+                                 set))
         single-unused?  (when (and single-require? (z/vector? first-node))
-                         (contains? unused-ns (-> first-node
-                                                       z/down
-                                                       z/leftmost
-                                                       z/sexpr)))]
+                          (or (contains? unused-aliases first-node-ns)
+                              (set/subset? first-node-refers unused-refers)))]
     (if single-unused?
       (z/remove first-node)
-      (z/map #(remove-unused-require % unused-ns) nodes))))
+      (let [removed (z/map #(remove-unused-require % unused-aliases unused-refers) nodes)]
+        (if (= :vector (z/tag removed))
+          (z/up removed)
+          removed)))))
 
 (defn clean-ns
   [zloc uri]
@@ -291,11 +329,12 @@
               4)
         sep (n/whitespace-node (apply str (repeat col " ")))
         single-space (n/whitespace-node " ")
-        unused-aliases (crawler/find-unused-aliases uri)
-        unused-refers (crawler/find-unused-refers uri)
+        usages (f.references/safe-find-references uri (slurp uri) false false)
+        unused-aliases (crawler/find-unused-aliases usages)
+        unused-refers (crawler/find-unused-refers usages)
         removed-nodes (->> require-loc
                            z/remove
-                           (remove-unused-requires (set/union unused-aliases unused-refers)))
+                           (remove-unused-requires unused-aliases unused-refers))
         requires (->> removed-nodes
                       z/node
                       n/children


Test patch:
diff --git a/test/clojure_lsp/crawler_test.clj b/test/clojure_lsp/crawler_test.clj
index 2b6a8b73..e698e8a3 100644
--- a/test/clojure_lsp/crawler_test.clj
+++ b/test/clojure_lsp/crawler_test.clj
@@ -1,6 +1,8 @@
 (ns clojure-lsp.crawler-test
-  (:require [clojure-lsp.crawler :as crawler]
-            [clojure.test :refer :all]))
+  (:require
+   [clojure-lsp.crawler :as crawler]
+   [clojure-lsp.feature.references :as f.references]
+   [clojure.test :refer :all]))
 
 (def unused-alias-code
   "(ns foo.bar
@@ -10,9 +12,9 @@
 
 (deftest find-unused-aliases-test
   (testing "When there is unused aliases"
-    (with-redefs [slurp (constantly unused-alias-code)]
+    (let [usages (f.references/safe-find-references "file://a.clj" unused-alias-code false false)]
       (is (= #{'foo.baz}
-             (crawler/find-unused-aliases "file://a.clj"))))))
+             (crawler/find-unused-aliases usages))))))
 
 (def unused-refer-code
   "(ns foo.bar
@@ -31,7 +33,7 @@
 
 (def unused-refer-with-used-refers-on-same-require-code
   "(ns foo.bar
-     (:require [foo.baz :refer [other another]]
+     (:require [foo.baz :refer [other another some-other]]
                [foo.bah :as bah]))
    (def zas
      (bah/some)
@@ -39,14 +41,14 @@
 
 (deftest find-unused-refers-test
   (testing "When there is a unused refer"
-    (with-redefs [slurp (constantly unused-refer-code)]
-      (is (= #{'foo.baz}
-             (crawler/find-unused-refers "file://a.clj")))))
+    (let [usages (f.references/safe-find-references  "file://a.clj" unused-refer-code false false)]
+      (is (= #{'foo.baz/other}
+             (crawler/find-unused-refers usages)))))
   (testing "When there is a unused refer but used refers on other require"
-    (with-redefs [slurp (constantly unused-refer-with-used-refers-on-other-require-code)]
-      (is (= #{'foo.baz}
-             (crawler/find-unused-refers "file://a.clj")))))
+    (let [usages (f.references/safe-find-references  "file://a.clj" unused-refer-with-used-refers-on-other-require-code false false)]
+      (is (= #{'foo.baz/other}
+             (crawler/find-unused-refers usages)))))
   (testing "When there is a unused refer but used refers on same require"
-    (with-redefs [slurp (constantly unused-refer-with-used-refers-on-same-require-code)]
-      (is (= #{}
-             (crawler/find-unused-refers "file://a.clj"))))))
+    (let [usages (f.references/safe-find-references  "file://a.clj" unused-refer-with-used-refers-on-same-require-code false false)]
+      (is (= #{'foo.baz/other 'foo.baz/some-other}
+             (crawler/find-unused-refers usages))))))
diff --git a/test/clojure_lsp/refactor/transform_test.clj b/test/clojure_lsp/refactor/transform_test.clj
index d661aef2..6add9502 100644
--- a/test/clojure_lsp/refactor/transform_test.clj
+++ b/test/clojure_lsp/refactor/transform_test.clj
@@ -10,6 +10,8 @@
     [clojure.test :refer :all]
     [rewrite-clj.zip :as z]))
 
+(defn code [& strings] (string/join "\n" strings))
+
 (deftest paredit-test
   (let [zloc (edit/raise (z/find-value (z/of-string "(a (b))") z/next 'b))]
     (is (= 'b (z/sexpr zloc)))
@@ -149,121 +151,158 @@
       (is (= 'let (z/sexpr (z/down loc))))
       (is (= (str "(def foo (let [a 1]\n          (fn [bar] a 2)))") (z/root-string loc))))))
 
-(def ns-to-clean
-  (str "(ns foo.bar\n"
-       " (:require\n"
-       "   [foo  :as f] [bar :as b] baz [z] ))\n"
-       "(s/defn func []\n"
-       "  (f/some))"))
-
-(def first-require-ns-to-clean
-  (str "(ns foo.bar\n"
-       " (:require\n"
-       "   [foo  :as f] [bar :as b] baz [z] ))\n"
-       "(defn func []\n"
-       "  (b/some))"))
-
-(def single-require-ns-to-clean
-  (str "(ns foo.bar\n"
-       " (:require\n"
-       "   [foo  :as f] ))\n"
-       "(defn func []\n"
-       "  (b/some))"))
-
-(def require-with-refer-ns-to-clean
-  (str "(ns foo.bar\n"
-       " (:require\n"
-       "   [foo  :as f] [bar :refer [some]] baz [z] ))\n"
-       "(defn func []\n"
-       "  (f/some))"))
-
-(def ns-to-clean-with-spaces
-  (str "(ns foo.bar\n"
-       " (:require\n"
-       "   [foo  :as f] [bar :refer [some]] baz [z] ))\n"
-       "\n"
-       "(defn func []\n"
-       "  (f/some))"))
+(defn test-clean-ns
+  ([db input-code expected-code]
+   (test-clean-ns db input-code expected-code true))
+  ([db input-code expected-code in-form]
+   (with-redefs [slurp (constantly input-code)]
+     (reset! db/db db)
+     (let [zloc (when in-form
+                  (-> (z/of-string input-code) z/down z/right z/right))
+           [{:keys [loc range]}] (transform/clean-ns zloc "file://a.clj")]
+       (is (some? range))
+       (is (= expected-code
+              (z/root-string loc)))))))
 
 (deftest clean-ns-test
-  (with-redefs [slurp (constantly ns-to-clean)]
-    (testing "without keep-require-at-start?"
-      (reset! db/db {:settings {:keep-require-at-start? false}})
-      (let [zloc (-> (z/of-string ns-to-clean) z/down z/right z/right)
-            [{:keys [loc range]}] (transform/clean-ns zloc "file://a.clj")]
-        (is (some? range))
-        (is (= (str "(ns foo.bar\n"
-                    " (:require\n"
-                    "   [foo  :as f]\n"
-                    "   [z]\n"
-                    "   baz))\n"
-                    "(s/defn func []\n"
-                    "  (f/some))")
-               (z/root-string loc)))))
-    (testing "with keep-require-at-start?"
-      (reset! db/db {:settings {:keep-require-at-start? true}})
-      (let [zloc (-> (z/of-string ns-to-clean) z/down z/right z/right)
-            [{:keys [loc range]}] (transform/clean-ns zloc "file://a.clj")]
-        (is (some? range))
-        (is (= (str "(ns foo.bar\n"
-                    " (:require [foo  :as f]\n"
-                    "           [z]\n"
-                    "           baz))\n"
-                    "(s/defn func []\n"
-                    "  (f/some))")
-               (z/root-string loc))))))
-  (with-redefs [slurp (constantly first-require-ns-to-clean)]
-    (testing "with first require as unused"
-      (reset! db/db {})
-      (let [zloc (-> (z/of-string first-require-ns-to-clean) z/down z/right z/right)
-            [{:keys [loc range]}] (transform/clean-ns zloc "file://a.clj")]
-        (is (some? range))
-        (is (= (str "(ns foo.bar\n"
-                    " (:require\n"
-                    "   [bar :as b]\n"
-                    "   [z]\n"
-                    "   baz))\n"
-                    "(defn func []\n"
-                    "  (b/some))")
-               (z/root-string loc))))))
-  (with-redefs [slurp (constantly single-require-ns-to-clean)]
-    (testing "with single unused require on ns"
-      (reset! db/db {})
-      (let [zloc (-> (z/of-string single-require-ns-to-clean) z/down z/right z/right)
-            [{:keys [loc range]}] (transform/clean-ns zloc "file://a.clj")]
-        (is (some? range))
-        (is (= (str "(ns foo.bar)\n"
-                    "(defn func []\n"
-                    "  (b/some))")
-               (z/root-string loc))))))
-  (with-redefs [slurp (constantly require-with-refer-ns-to-clean)]
-    (testing "with refer at require"
-      (reset! db/db {})
-      (let [zloc (-> (z/of-string require-with-refer-ns-to-clean) z/down z/right z/right)
-            [{:keys [loc range]}] (transform/clean-ns zloc "file://a.clj")]
-        (is (some? range))
-        (is (= (str "(ns foo.bar\n"
-                    " (:require\n"
-                    "   [foo  :as f]\n"
-                    "   [z]\n"
-                    "   baz))\n"
-                    "(defn func []\n"
-                    "  (f/some))")
-               (z/root-string loc))))))
-  (with-redefs [slurp (constantly ns-to-clean-with-spaces)]
-    (testing "clearing in any form"
-      (reset! db/db {:documents {"file://a.clj" {:text ns-to-clean-with-spaces}}})
-      (let [[{:keys [loc range]}] (transform/clean-ns nil "file://a.clj")]
-        (is (some? range))
-        (is (= (str "(ns foo.bar\n"
-                    " (:require\n"
-                    "   [foo  :as f]\n"
-                    "   [z]\n"
-                    "   baz))\n"
-                    "\n"
-                    "(defn func []\n"
-                    "  (f/some))")
-               (z/root-string loc)))))))
+  (testing "without keep-require-at-start?"
+    (test-clean-ns {:settings {:keep-require-at-start? false}}
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [foo  :as f] [bar :as b] baz [z] ))"
+                         "(s/defn func []"
+                         "  (f/some))")
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [foo  :as f]"
+                         "   [z]"
+                         "   baz))"
+                         "(s/defn func []"
+                         "  (f/some))")))
+  (testing "with keep-require-at-start?"
+    (test-clean-ns {:settings {:keep-require-at-start? true}}
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [foo  :as f] [bar :as b] baz [z] ))"
+                         "(s/defn func []"
+                         "  (f/some))")
+                   (code "(ns foo.bar"
+                         " (:require [foo  :as f]"
+                         "           [z]"
+                         "           baz))"
+                         "(s/defn func []"
+                         "  (f/some))")))
+  (testing "with first require as unused"
+    (test-clean-ns {}
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [foo  :as f] [bar :as b] baz [z] ))"
+                         "(defn func []"
+                         "  (b/some))")
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [bar :as b]"
+                         "   [z]"
+                         "   baz))"
+                         "(defn func []"
+                         "  (b/some))")))
+  (testing "with single unused require on ns"
+    (test-clean-ns {}
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [foo  :as f] ))"
+                         "(defn func []"
+                         "  (b/some))")
+                   (code "(ns foo.bar)"
+                         "(defn func []"
+                         "  (b/some))")))
+  (testing "with refer at require"
+    (test-clean-ns {}
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [foo  :as f] [bar :refer [some]] baz [z] ))"
+                         "(defn func []"
+                         "  (f/some))")
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [foo  :as f]"
+                         "   [z]"
+                         "   baz))"
+                         "(defn func []"
+                         "  (f/some))")))
+  (testing "with refer as single require"
+    (test-clean-ns {}
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [bar :refer [some]]))")
+                   (code "(ns foo.bar)")))
+  (testing "in any form"
+    (let [to-clean (code "(ns foo.bar"
+                         " (:require"
+                         "   [foo  :as f] [bar :refer [some]] baz [z] ))"
+                         ""
+                         "(defn func []"
+                         "  (f/some))")]
+      (test-clean-ns {:documents {"file://a.clj" {:text to-clean}}}
+                     to-clean
+                     (code "(ns foo.bar"
+                           " (:require"
+                           "   [foo  :as f]"
+                           "   [z]"
+                           "   baz))"
+                           ""
+                           "(defn func []"
+                           "  (f/some))")
+                     false)))
+  (testing "with first require as a refer"
+    (test-clean-ns {}
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [bar :refer [some] ] [foo :as f]))"
+                         ""
+                         "(defn func []"
+                         "  (some))")
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [bar :refer [some] ]))"
+                         ""
+                         "(defn func []"
+                         "  (some))")))
+  (testing "with first require as a refer with alias"
+    (test-clean-ns {}
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [bar :as b :refer [some] ] [foo :as f]))"
+                         ""
+                         "(defn func []"
+                         "  b/some"
+                         "  (some))")
+                   (code "(ns foo.bar"
+                         " (:require"
+                         "   [bar :as b :refer [some] ]))"
+                         ""
+                         "(defn func []"
+                         "  b/some"
+                         "  (some))")))
+  (testing "unused refer from multiple refers"
+      (test-clean-ns {}
+                     (code "(ns foo.bar"
+                           " (:require"
+                           "   [bar :refer [some other] ]))"
+                           "(some)")
+                     (code "(ns foo.bar"
+                           " (:require"
+                           "   [bar :refer [some] ]))"
+                           "(some)")))
+  (testing "unused refer and alias"
+      (test-clean-ns {}
+                     (code "(ns foo.bar"
+                           " (:require"
+                           "   [bar :refer [some] ]"
+                           "   [baz :as b]))")
+                     (code "(ns foo.bar"
+                           " (:require"
+                           "   [baz :as b]))"))))
 
 (deftest add-missing-libspec
   (reset! db/db {:file-envs