Repository: clojure-lsp/clojure-lsp
Pull request #235 (2021-01-05 02:51:47)

Improve "add missing namespace" code action
This PR improves the add missing namespace code action:

- Add the namespace which will be required so `Add missing namespace` -> `Add 'clojure.test' missing namespace`
- It suggests known common alias namespaces like `set` -> `clojure.set`, `walk` -> `clojure.walk`, `string` -> `clojure.string` etc
- It suggests known common refers like `deftest`, `testing`, `is`, `are`, `fact`, `facts` etc.

![clojure-code-action-add-missing-ns](https://user-images.githubusercontent.com/7820865/103601103-b53cba80-4ee7-11eb-9f76-56e674b72fd8.gif)



Patch:
diff --git a/README.md b/README.md
index 91784554..fe746e33 100644
--- a/README.md
+++ b/README.md
@@ -26,32 +26,27 @@ You will get:
 
 ## Installation
 
-<details>
-<summary><b>Manually</b></summary>
+### Manually
 
 - You need `java` on your $PATH.
 - Grab the latest `clojure-lsp` from github [LATEST](https://github.com/snoe/clojure-lsp/releases/latest)
 - Place it in your $PATH with a chmod 755
 - Follow the documentation for your editor's language client. See [Clients](#clients) below.
 
-</details>
-
-<details>
-<summary><b>Nix</b></summary>
-
+### Nix
 `clojure-lsp` is available in the [nixpkgs](https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/misc/clojure-lsp/default.nix):
 
 ```bash
 nix-shell -p clojure-lsp
 ```
 
-</details>
+## Troubleshooting
 
-## [Troubleshooting](docs/troubleshooting.md)
+See [troubleshooting.md](docs/troubleshooting.md).
 
 ## Capabilities
 
-Below are all the currently supported LSP capabilities and their implementation status:
+Bellow are all the currently supported LSP capabilities and their implementation status:
 
 <details>
 <summary><b>Supported LSP capabilities</b></summary>
@@ -131,16 +126,13 @@ Besides LSP official capabilities, `clojure-lsp` has some extra features:
 
 It should be possible to introduce most of the refactorings [here](https://github.com/clojure-emacs/clj-refactor.el/tree/master/examples)
 
-<details>
-  <summary><b>More details</b></summary>
-
 Calling `executeCommand` with the following commands and additional args will notify the client with `applyEdit`.
 All commands expect the first three args to be `[document-uri, line, column]` (eg `["file:///home/snoe/file.clj", 13, 11]`)
 
 | done | command             | args                                          | notes                                |
 | ---- | ------------------- | ----                                          | -----                                |
 | √    | add-missing-libspec |                                               |                                      |
-| √    | clean-ns            |                                               | :require sort and remove unused      |
+| -    | clean-ns            |                                               | :require sort and remove unused only |
 | √    | cycle-coll          |                                               |                                      |
 | √    | cycle-privacy       |                                               |                                      |
 | √    | expand-let          |                                               |                                      |
@@ -157,18 +149,11 @@ All commands expect the first three args to be `[document-uri, line, column]` (e
 
 See Vim client section for an example.
 
-Emacs provides all those refactorings via [lsp-mode](https://emacs-lsp.github.io/lsp-mode/)  with the `lsp-clojure-` prefix.
-
 Other clients might provide a higher level interface to `workspace/executeCommand` you need to pass the path, line and column numbers.
 
-</details>
-
 ## InitializationOptions
 
-It is possible to pass some options to clojure-lsp through clients' `InitializationOptions`. Options are a map with keys.
-
-<details>
-  <summary><b>All supported settings</b></summary>
+It is possible to pass some options to clojure-lsp through clients' `InitializationOptions`. Options are a map with keys:
 
 `source-paths` value is a set of project-local directories to look for clj/cljc/cljs files. Default is `#{"src", "test"}`.
 
@@ -233,8 +218,6 @@ Each project-spec will add to the list of dependencies for lsp to crawl:
   - `:classpath-cmd` is the required vector of commands to get your project's classpath string (e.g. `["clj", "-Spath"]`)
   - `:env` optionally add environment variables to the classpath-cmd (e.g. `{"BOOT_FILE" "x.boot"}`)
 
-</details>
-
 ### macro-defs
 
 `macro-defs` value is a map of fully-qualified macro names to a vector of definitions of those macros' forms.
@@ -320,7 +303,7 @@ See https://github.com/snoe/clojure-lsp/blob/master/test/clojure_lsp/parser_test
 
 ### Project
 
-`clojure-lsp` will also look for project specific settings in a file called `.lsp/config.edn`. It will search from your root folder up the directory structure so you can have multiple projects share the same settings.
+LSP will also look for project specific settings in a file called '.lsp/config.edn'. It will search from your root folder up the directory structure so you can have multiple projects share the settings.
 
 ```clojure
 {:macro-defs {korma.core/defentity [:declaration :elements]}
@@ -419,8 +402,6 @@ Optionally you can add `lsp-ui` for UI feedback and `company-mode` for completio
 ```
 
 In `lsp-mode`, `lsp-clojure-server-command` variable is available to override the command to start the `clojure-lsp` server, might be necessary to do this on a Windows environment.
-
-For more `lsp-mode` clojure settings, check [here](https://emacs-lsp.github.io/lsp-mode/page/lsp-clojure/)
 </details>
 
 <details>
@@ -456,13 +437,6 @@ I tried making a client but my hello world attempt didn't seem to work. If someo
 - Full semantic tokens support
 - Outgoing only Call hierarchy support
 
-## Building manually
-
-For building manually, run `lein bin` to generate the binary inside `target` folder or `lein uberjar` for building the standalone jar.
-
-## Development
+## Building local
 
-For `clojure-lsp` development, there are 3 possible ways of finding a bug or implementing a new feature:
-- Create a test for your bug/feature, then implement the code following the test.
-- `clojure-lsp` starts a NREPL server, with that it's possible to change the code of a running instance and see the changes on your client in real time. To get the NREPL port, you can check the `/tmp/clojure-lsp.out` log, it will print the NREPL port on server startup or you can get it via `server-info` custom LSP command.
-- Build `clojure-lsp` with your changes and test it manually in your client, this is the slowest option, but it makes sense final tests.
+For building local, run `lein bin` to generate the binary inside `target` folder or `lein uberjar` for building the jar.
diff --git a/_config.yml b/_config.yml
deleted file mode 100644
index c4192631..00000000
--- a/_config.yml
+++ /dev/null
@@ -1,1 +0,0 @@
-theme: jekyll-theme-cayman
\ No newline at end of file
diff --git a/src/clojure_lsp/crawler.clj b/src/clojure_lsp/crawler.clj
index 997ed3f3..44cb5b88 100644
--- a/src/clojure_lsp/crawler.clj
+++ b/src/clojure_lsp/crawler.clj
@@ -82,6 +82,9 @@
 
 (defn crawl-source-dirs [dirs]
   (let [xf (comp
+             (map (fn [dir]
+                    (log/info "Crawling dir" (.getPath dir))
+                    dir))
             (mapcat file-seq)
             (filter #(.isFile %))
             (map file->uri)
diff --git a/src/clojure_lsp/feature/code_actions.clj b/src/clojure_lsp/feature/code_actions.clj
index 12b24523..369589cb 100644
--- a/src/clojure_lsp/feature/code_actions.clj
+++ b/src/clojure_lsp/feature/code_actions.clj
@@ -2,7 +2,8 @@
   (:require
    [clojure-lsp.feature.definition :as f.definition]
    [clojure-lsp.feature.refactor :as f.refactor]
-   [clojure-lsp.refactor.transform :as r.transform]))
+   [clojure-lsp.refactor.transform :as r.transform]
+   [rewrite-clj.zip :as z]))
 
 (defn all [zloc uri row col diagnostics client-capabilities]
   (let [workspace-edit-capability? (get-in client-capabilities [:workspace :workspace-edit])
@@ -12,21 +13,25 @@
         inline-symbol? (r.transform/inline-symbol? def-uri definition)
         line (dec row)
         character (dec col)
-        has-unknow-ns? (some #(= (compare "unresolved-namespace" (some-> % .getCode .get)) 0) diagnostics)
-        missing-ns (when has-unknow-ns?
+        has-unknown-ns? (some #(= (compare "unresolved-namespace" (some-> % .getCode .get)) 0) diagnostics)
+        unresolved-symbol (first (filter #(= (compare "unresolved-symbol" (some-> % .getCode .get)) 0) diagnostics))
+        known-refer? (when unresolved-symbol
+                       (get r.transform/common-refers->info (z/sexpr zloc)))
+        missing-ns (when (or has-unknown-ns? known-refer?)
                      (f.refactor/call-refactor {:loc zloc
                                                 :refactoring :add-missing-libspec
                                                 :uri uri
                                                 :version 0
                                                 :row row
-                                                :col col}))]
+                                                :col col
+                                                :args {:source :code-action}}))]
     (cond-> []
 
-      (and has-unknow-ns? missing-ns)
-      (conj {:title          "Add missing namespace"
+      (:result missing-ns)
+      (conj {:title          (str "Add missing '" (-> missing-ns :code-action-data :ns-name) "' namespace")
              :kind           :quick-fix
              :preferred?     true
-             :workspace-edit missing-ns})
+             :workspace-edit (f.refactor/refactor-client-seq-changes uri 0 (:result missing-ns))})
 
       inline-symbol?
       (conj {:title   "Inline symbol"
diff --git a/src/clojure_lsp/feature/refactor.clj b/src/clojure_lsp/feature/refactor.clj
index ca1b42d8..c40500d9 100644
--- a/src/clojure_lsp/feature/refactor.clj
+++ b/src/clojure_lsp/feature/refactor.clj
@@ -18,8 +18,8 @@
 
 (defmulti refactor (comp :refactoring))
 
-(defmethod refactor :add-missing-libspec [{:keys [loc]}]
-  (r.transform/add-missing-libspec loc))
+(defmethod refactor :add-missing-libspec [{:keys [loc args]}]
+  (r.transform/add-missing-libspec loc args))
 
 (defmethod refactor :clean-ns [{:keys [loc uri]}]
   (r.transform/clean-ns loc uri))
@@ -74,11 +74,20 @@
        (map name)
        vec))
 
+(defn refactor-client-seq-changes [uri version result]
+  (let [changes [{:text-document {:uri uri :version version}
+                  :edits (mapv #(update % :range shared/->range) (r.transform/result result))}]]
+    (client-changes changes)))
+
 (defn call-refactor [{:keys [loc uri refactoring row col version] :as data}]
   (let [result (refactor data)]
     (if (or loc
             (= :clean-ns refactoring))
       (cond
+        (and (map? result)
+             (contains? result :code-action-data))
+        result
+
         (map? result)
         (let [changes (vec (for [[doc-id sub-results] result]
                              {:text-document {:uri doc-id :version version}
@@ -86,9 +95,7 @@
           (client-changes changes))
 
         (seq result)
-        (let [changes [{:text-document {:uri uri :version version}
-                        :edits (mapv #(update % :range shared/->range) (r.transform/result result))}]]
-          (client-changes changes))
+        (refactor-client-seq-changes uri version result)
 
         (empty? result)
         (log/warn refactoring "made no changes" (z/string loc))
diff --git a/src/clojure_lsp/refactor/transform.clj b/src/clojure_lsp/refactor/transform.clj
index 293b1b1c..69550f97 100644
--- a/src/clojure_lsp/refactor/transform.clj
+++ b/src/clojure_lsp/refactor/transform.clj
@@ -11,7 +11,8 @@
    [rewrite-clj.custom-zipper.core :as cz]
    [rewrite-clj.node :as n]
    [rewrite-clj.zip :as z]
-   [rewrite-clj.zip.subedit :as zsub]))
+   [rewrite-clj.zip.subedit :as zsub]
+   [clojure.tools.logging :as log]))
 
 (defn result [zip-edits]
   (mapv (fn [zip-edit]
@@ -384,8 +385,14 @@
         [{:range (meta (z/node result-loc))
           :loc result-loc}]))))
 
-(defn add-missing-libspec
-  [zloc]
+(def common-alias->info
+  {:string {:alias-str "string" :label "clojure.string" :detail "clojure.string" :alias-ns 'clojure.string}
+   :set    {:alias-str "set" :label "clojure.set" :detail "clojure.set" :alias-ns 'clojure.set}
+   :walk   {:alias-str "walk" :label "clojure.walk" :detail "clojure.walk" :alias-ns 'clojure.walk}
+   :pprint {:alias-str "pprint" :label "clojure.pprint" :detail "clojure.pprint" :alias-ns 'clojure.pprint}
+   :async  {:alias-str "async" :label "clojure.core.async" :detail "clojure.core.async" :alias-ns 'clojure.core.async}})
+
+(defn ^:private add-missing-alias-ns [zloc source]
   (let [ns-str-to-add (some-> zloc z/sexpr namespace)
         ns-to-add (some-> ns-str-to-add symbol)
         alias->info (->> (:file-envs @db/db)
@@ -405,11 +412,80 @@
                                               sym)}))
                          (distinct)
                          (group-by :alias-str))
-        posibilities (get alias->info ns-str-to-add)
-        qualified-ns-to-add (cond
-                              (= 1 (count posibilities))
-                              (-> posibilities first :alias-ns))]
-    (add-known-libspec zloc ns-to-add qualified-ns-to-add)))
+        posibilities (or (get alias->info ns-str-to-add)
+                         [(get common-alias->info (keyword ns-str-to-add))])
+        qualified-ns-to-add (when (= 1 (count posibilities))
+                              (-> posibilities first :alias-ns))
+        result (add-known-libspec zloc ns-to-add qualified-ns-to-add)]
+    (if (= source :code-action)
+      {:result result
+       :code-action-data {:ns-name qualified-ns-to-add}}
+      result)))
+
+(def common-refers->info
+  {'deftest      'clojure.test
+   'testing      'clojure.test
+   'is           'clojure.test
+   'are          'clojure.test
+   'ANY          'compojure.core
+   'DELETE       'compojure.core
+   'GET          'compojure.core
+   'PATCH        'compojure.core
+   'POST         'compojure.core
+   'PUT          'compojure.core
+   'context      'compojure.core
+   'defroutes    'compojure.core
+   'defentity    'korma.core
+   'reg-event-db 're-frame.core
+   'reg-sub      're-frame.core
+   'reg-event-fx 're-frame.core
+   'fact         'midje.sweet
+   'facts        'midje.sweet})
+
+(defn ^:private add-missing-refer [zloc source]
+  (when-let [qualified-ns-to-add (get common-refers->info (z/sexpr zloc))]
+    (let [refer-to-add (-> zloc z/sexpr symbol)
+          ns-loc (edit/find-namespace zloc)
+          ns-zip (zsub/subzip ns-loc)
+          need-to-add? (not (z/find-value ns-zip z/next refer-to-add))]
+      (when need-to-add?
+        (let [existing-ns-require (z/find-value ns-zip z/next qualified-ns-to-add)
+              add-require? (and (not existing-ns-require)
+                                (not (z/find-value ns-zip z/next :require)))
+              require-loc (z/find-value (zsub/subzip ns-loc) z/next :require)
+              col (if require-loc
+                    (-> require-loc z/rightmost z/node meta :col)
+                    5)
+              result-loc (if existing-ns-require
+                           (z/subedit-> ns-loc
+                                        (z/find-value z/next qualified-ns-to-add)
+                                        (z/find-value z/next ':refer)
+                                        z/right
+                                        (cz/append-child (n/spaces 1))
+                                        (z/append-child (z/sexpr zloc)))
+                           (z/subedit-> ns-loc
+                                        (cond->
+                                            add-require? (z/append-child (n/newlines 1))
+                                            add-require? (z/append-child (n/spaces 2))
+                                            add-require? (z/append-child (list :require)))
+                                        (z/find-value z/next :require)
+                                        (z/up)
+                                        (cz/append-child (n/newlines 1))
+                                        (cz/append-child (n/spaces (dec col)))
+                                        (z/append-child [qualified-ns-to-add :refer [refer-to-add]])))
+              result [{:range (meta (z/node result-loc))
+                       :loc result-loc}]]
+          (if (= source :code-action)
+            {:result result
+             :code-action-data {:ns-name qualified-ns-to-add}}
+            result))))))
+
+(defn add-missing-libspec
+  [zloc {:keys [source]}]
+  (let [ns-str (some-> zloc z/sexpr namespace)]
+    (if ns-str
+      (add-missing-alias-ns zloc source)
+      (add-missing-refer zloc source))))
 
 (defn extract-function
   [zloc fn-name usages]


Test patch:
diff --git a/test/clojure_lsp/handlers_test.clj b/test/clojure_lsp/handlers_test.clj
index 4a1619b3..544a2bea 100644
--- a/test/clojure_lsp/handlers_test.clj
+++ b/test/clojure_lsp/handlers_test.clj
@@ -431,7 +431,8 @@
                       "  bar)")
         c-code   (str "(ns another-ns)\n"
                       "(def bar ons/bar)\n"
-                      "(def foo sns/foo)")
+                      "(def foo sns/foo)\n"
+                      "(deftest some-test)")
         db-state {:documents {"file://a.clj" {:text references-code}
                               "file://b.clj" {:text b-code}
                               "file://c.clj" {:text c-code}}
@@ -441,20 +442,30 @@
     (testing "Add missing namespace"
       (testing "when it has not unresolved-namespace diagnostic"
         (reset! db/db db-state)
-        (is (not-any? #(= (:title %) "Add missing namespace")
+        (is (not-any? #(string/starts-with? (:title %) "Add missing")
                       (handlers/code-actions "file://c.clj" [] 1 9))))
 
       (testing "when it has unresolved-namespace but cannot find namespace"
         (reset! db/db db-state)
         (let [unknown-ns-diagnostic (Diagnostic. (Range. (Position. 1 10) (Position. 1 16)) "Unknown namespace" DiagnosticSeverity/Error "some source" "unresolved-namespace")]
-          (is (not-any? #(= (:title %) "Add missing namespace")
+          (is (not-any? #(string/starts-with? (:title %) "Add missing")
                         (handlers/code-actions "file://c.clj" [unknown-ns-diagnostic] 1 10)))))
 
       (testing "when it has unresolved-namespace and can find namespace"
         (reset! db/db db-state)
         (let [unknown-ns-diagnostic (Diagnostic. (Range. (Position. 2 10) (Position. 2 16)) "Unknown namespace" DiagnosticSeverity/Error "some source" "unresolved-namespace")]
-          (is (some #(= (:title %) "Add missing namespace")
-                    (handlers/code-actions "file://c.clj" [unknown-ns-diagnostic] 2 10))))))
+          (is (some #(= (:title %) "Add missing 'some-ns' namespace")
+                    (handlers/code-actions "file://c.clj" [unknown-ns-diagnostic] 2 10)))))
+      (testing "when it has unresolved-symbol and it's a known refer"
+        (reset! db/db db-state)
+        (let [unknown-symbol-diagnostic (Diagnostic. (Range. (Position. 3 1) (Position. 3 8)) "Unresolved symbol deftest" DiagnosticSeverity/Error "some source" "unresolved-symbol")]
+          (is (some #(= (:title %) "Add missing 'clojure.test' namespace")
+                    (handlers/code-actions "file://c.clj" [unknown-symbol-diagnostic] 3 1)))))
+      (testing "when it has unresolved-symbol but it's not a known refer"
+        (reset! db/db db-state)
+        (let [unknown-symbol-diagnostic (Diagnostic. (Range. (Position. 3 10) (Position. 3 18)) "Unresolved symbol some-test" DiagnosticSeverity/Error "some source" "unresolved-symbol")]
+          (is (not-any? #(string/starts-with? (:title %) "Add missing")
+                        (handlers/code-actions "file://c.clj" [unknown-symbol-diagnostic] 3 10))))))
     (testing "Inline symbol"
       (testing "when in not a let/def symbol"
         (reset! db/db db-state)
diff --git a/test/clojure_lsp/refactor/transform_test.clj b/test/clojure_lsp/refactor/transform_test.clj
index 6add9502..0384e572 100644
--- a/test/clojure_lsp/refactor/transform_test.clj
+++ b/test/clojure_lsp/refactor/transform_test.clj
@@ -305,12 +305,67 @@
                            "   [baz :as b]))"))))
 
 (deftest add-missing-libspec
-  (reset! db/db {:file-envs
-                 {"file://a.clj" (parser/find-usages "(ns a (:require [foo.s :as s]))" :clj {})}})
-  (let [zloc (-> (z/of-string "(ns foo) s/thing") z/rightmost)
-        [{:keys [loc range]}] (transform/add-missing-libspec zloc)]
-    (is (some? range))
-    (is (= '(ns foo (:require [foo.s :as s])) (z/sexpr loc)))))
+  (testing "aliases"
+    (testing "known namespaces in project"
+      (reset! db/db {:file-envs
+                     {"file://a.clj" (parser/find-usages "(ns a (:require [foo.s :as s]))" :clj {})}})
+      (let [zloc (-> (z/of-string "(ns foo) s/thing") z/rightmost)
+            [{:keys [loc range]}] (transform/add-missing-libspec zloc nil)]
+        (is (some? range))
+        (is (= '(ns foo (:require [foo.s :as s])) (z/sexpr loc)))))
+    (testing "common ns aliases"
+      (reset! db/db {:file-envs {}})
+      (let [zloc (-> (z/of-string "(ns foo) set/subset?") z/rightmost)
+            [{:keys [loc range]}] (transform/add-missing-libspec zloc nil)]
+        (is (some? range))
+        (is (= '(ns foo (:require [clojure.set :as set])) (z/sexpr loc))))))
+  (testing "common refers"
+    (testing "when require doesn't exists"
+      (reset! db/db {:file-envs {}})
+      (let [zloc (-> (z/of-string "(ns foo) deftest") z/rightmost)
+            [{:keys [loc range]}] (transform/add-missing-libspec zloc nil)]
+        (is (some? range))
+        (is (= '(ns foo (:require [clojure.test :refer [deftest]])) (z/sexpr loc)))))
+    (testing "when already exists another require"
+      (reset! db/db {:file-envs {}})
+      (let [zloc (-> (z/of-string "(ns foo (:require [clojure.set :refer [subset?]])) deftest") z/rightmost)
+            [{:keys [loc range]}] (transform/add-missing-libspec zloc nil)]
+        (is (some? range))
+        (is (= '(ns foo (:require [clojure.set :refer [subset?]]
+                                  [clojure.test :refer [deftest]])) (z/sexpr loc)))))
+    (testing "when already exists that ns with another refer"
+      (reset! db/db {:file-envs {}})
+      (let [zloc (-> (z/of-string "(ns foo (:require [clojure.test :refer [deftest]])) testing") z/rightmost)
+            [{:keys [loc range]}] (transform/add-missing-libspec zloc nil)]
+        (is (some? range))
+        (is (= '(ns foo (:require [clojure.test :refer [deftest testing]])) (z/sexpr loc)))))
+    (testing "we don't add existing refers"
+      (reset! db/db {:file-envs {}})
+      (let [zloc (-> (z/of-string "(ns foo (:require [clojure.test :refer [testing]])) testing") z/rightmost)]
+        (is (= nil (transform/add-missing-libspec zloc nil)))))
+    (testing "we can add multiple refers"
+      (reset! db/db {:file-envs {}})
+      (let [zloc (-> (z/of-string "(ns foo (:require [clojure.test :refer [deftest testing]])) is") z/rightmost)
+            [{:keys [loc range]}] (transform/add-missing-libspec zloc nil)]
+        (is (some? range))
+        (is (= '(ns foo (:require [clojure.test :refer [deftest testing is]])) (z/sexpr loc))))))
+  (testing "from code-action source"
+    (testing "aliases"
+      (reset! db/db {:file-envs {}})
+      (let [zloc (-> (z/of-string "(ns foo) set/subset?") z/rightmost)
+            response (transform/add-missing-libspec zloc {:source :code-action})
+            [{:keys [loc range]}] (:result response)]
+        (is (= 'clojure.set (-> response :code-action-data :ns-name)))
+        (is (some? range))
+        (is (= '(ns foo (:require [clojure.set :as set])) (z/sexpr loc)))))
+    (testing "refers"
+      (reset! db/db {:file-envs {}})
+      (let [zloc (-> (z/of-string "(ns foo) deftest") z/rightmost)
+            response (transform/add-missing-libspec zloc {:source :code-action})
+            [{:keys [loc range]}] (:result response)]
+        (is (= 'clojure.test (-> response :code-action-data :ns-name)))
+        (is (some? range))
+        (is (= '(ns foo (:require [clojure.test :refer [deftest]])) (z/sexpr loc)))))))
 
 (deftest unwind-thread-test
   (let [zloc (z/of-string "(-> a b (c) d)")