Strengthen/refine resolve-missing (#403)

vemv · web-flow · commit b4512c62f3e4 · 2023-07-24T21:39:13.000+02:00
Fixes #401 Fixes #402
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,15 @@
 
 ## Unreleased
 
+## 3.9.0
+
+* [#401](https://github.com/clojure-emacs/refactor-nrepl/issues/401) `resolve-missing` no longer returns the wrong type
+* [#401](https://github.com/clojure-emacs/refactor-nrepl/issues/401) `resolve-missing` no longer returns non- `import`able classes.
+* [#402](https://github.com/clojure-emacs/refactor-nrepl/issues/402) `resolve-missing` now flags with `:already-interned true` the candidates that are already interned into the calling namespace.
+  * This helps clients avoiding the suggestion/insertion of candidates which are redundant
+    * e.g. `+` is already interned, so is `Thread`
+    * same for `:refer`s and `:rename`s
+
 ## 3.8.0
 
 * [#396](https://github.com/clojure-emacs/refactor-nrepl/pull/396) Handle the analyzing and parsing of Clojure code from .cljc files.
diff --git a/Makefile b/Makefile
@@ -41,7 +41,7 @@ deploy: check-env .inline-deps
 jar: .inline-deps
 	lein with-profile -user,-dev,+$(VERSION),+plugin.mranderson/config jar
 
-# Usage: PROJECT_VERSION=3.8.0 make install
+# Usage: PROJECT_VERSION=3.9.0 make install
 # PROJECT_VERSION is needed because it's not computed dynamically
 install: check-install-env .inline-deps
 	 LEIN_JVM_OPTS="-Dmranderson.internal.no-parallelism=true" lein with-profile -user,-dev,+$(VERSION),+plugin.mranderson/config install
diff --git a/README.md b/README.md
@@ -26,7 +26,7 @@ Be aware that this isn't the case if you connect to an already running REPL proc
 Add the following, either in your project's `project.clj`,  or in the `:user` profile found at `~/.lein/profiles.clj`:
 
 ```clojure
-:plugins [[refactor-nrepl "3.8.0"]
+:plugins [[refactor-nrepl "3.9.0"]
           [cider/cider-nrepl "0.31.0"]]
 ```
 
@@ -202,11 +202,17 @@ classpath.  This symbol can be qualified, e.g. `walk/postwalk` or
 is a qualified reference to a clojure var and the second a reference
 to a static java method.
 
+The op also now expects (although does not require) `ns`, the current namespace expressed as a string.
+
 The return value `candidates` is a list of `({:name candidate.ns :type :ns} {:name candidate.package :type :type} ...)` where type is in `#{:type :class :ns
-:macro}` so we can branch on the various ways to make the symbol
-available.  `:type` means the symbol resolved to a var created by
-`defrecord` or `deftype`, `:class` is for Java classes but also includes interfaces.  `:macro`
-is only used if the op is called in a cljs context and means the var resolved to macro.
+:macro}` so we can branch on the various ways to make the symbol available.
+
+* `:type` means the symbol resolved to a var created by `defrecord` or `deftype`
+* `:class` is for Java classes but also includes interfaces.
+* `:macro` is only used if the op is called in a cljs context and means the var resolved to macro.
+
+The additional property `:already-interned` (boolean) indicates if the current namespace (as passed as `ns`) already had the given suggested
+interned (e.g.`Thread` and `Object` are interned by default in all JVM clojure namespaces). This helps avoiding the insertion of redundant requires/imports.
 
 ### hotload-dependency
 
@@ -365,7 +371,7 @@ When you want to release locally to the following:
 And here's how to deploy to Clojars:
 
 ```bash
-git tag -a v3.8.0 -m "3.8.0"
+git tag -a v3.9.0 -m "3.9.0"
 git push --tags
 ```
 
diff --git a/src/refactor_nrepl/ns/class_search.clj b/src/refactor_nrepl/ns/class_search.clj
@@ -3,15 +3,25 @@
 
   Formerly known as `refactor-nrepl.ns.slam.hound.search`."
   (:require
+   [clojure.java.io :as io]
    [clojure.string :as string]
-   [compliment.utils]))
+   [compliment.utils])
+  (:import
+   (java.io File)))
 
 (defn- get-available-classes []
-  (->> (dissoc (compliment.utils/classes-on-classpath)
-               "")
-       (vals)
-       (reduce into)
-       (mapv symbol)))
+  (let [classes (->> (dissoc (compliment.utils/classes-on-classpath)
+                             "")
+                     (vals)
+                     (reduce into))]
+    (into []
+          (comp (keep (fn [s]
+                        ;; https://github.com/alexander-yakushev/compliment/issues/105
+                        (when (io/resource (-> s (string/replace "." File/separator) (str ".class")))
+                          s)))
+                (distinct)
+                (map symbol))
+          classes)))
 
 (def available-classes
   (delay (get-available-classes)))
diff --git a/src/refactor_nrepl/ns/imports_and_refers_analysis.clj b/src/refactor_nrepl/ns/imports_and_refers_analysis.clj
@@ -21,14 +21,18 @@
 (defn- symbols->ns-syms
   ([]
    (symbols->ns-syms {} (all-ns)))
+
   ([init namespaces]
-   (reduce
-    (fn [m ns] (let [ns-sym (ns-name ns)]
-                 (reduce
-                  (fn [m k]
-                    (assoc m k (conj (or (m k) #{}) ns-sym)))
-                  m (keys (ns-publics ns)))))
-    init namespaces)))
+   (reduce (fn [m ns]
+             (let [ns-sym (ns-name ns)]
+               (reduce (fn [m k]
+                         (assoc m k (conj (or (m k) #{})
+                                          (with-meta ns-sym
+                                            {:refactor-nrepl/symbol k}))))
+                       m
+                       (keys (ns-publics ns)))))
+           init
+           namespaces)))
 
 (defn- ns-import-candidates
   "Search (all-ns) for imports that match missing-sym, returning a set of
@@ -38,14 +42,18 @@
   [missing-sym]
   (reduce (fn [s imports]
             (if-let [^Class cls (get imports missing-sym)]
-              (conj s (symbol (.getCanonicalName cls)))
+              (->> cls .getCanonicalName symbol (conj s))
               s))
           #{} (vals (all-ns-imports))))
 
 (defn candidates
   "Return a set of class or ns symbols that match the given constraints."
   [type missing _body _old-ns-map]
   (case type
-    :import (into (ns-import-candidates missing)
-                  (get @class-search/available-classes-by-last-segment missing))
-    :refer (get (symbols->ns-syms) missing)))
+    :import (into #{}
+                  (map (fn [s]
+                         (with-meta s
+                           {:refactor-nrepl/is-class true})))
+                  (reduce into #{} [(ns-import-candidates missing)
+                                    (get @class-search/available-classes-by-last-segment missing)]))
+    :refer  (get (symbols->ns-syms) missing)))
diff --git a/src/refactor_nrepl/ns/resolve_missing.clj b/src/refactor_nrepl/ns/resolve_missing.clj
@@ -2,41 +2,72 @@
   "Resolve a missing symbol on the classpath."
   (:require
    [cider.nrepl.middleware.util.cljs :as cljs]
-   [clojure.string :as str]
+   [clojure.string :as string]
    [orchard.cljs.analysis :as cljs-ana]
-   [orchard.info :refer [info]]
+   [orchard.info]
    [refactor-nrepl.core :refer [prefix suffix]]
    [refactor-nrepl.ns.imports-and-refers-analysis :as imports-and-refers-analysis]
    [refactor-nrepl.util :refer [self-referential?]]))
 
 (defn- candidates [sym]
   (reduce into
+          []
           [(when-let [p (prefix sym)]
              (imports-and-refers-analysis/candidates :import (symbol p) [] {}))
            (imports-and-refers-analysis/candidates :import (symbol (suffix sym)) [] {})
            (imports-and-refers-analysis/candidates :refer (symbol (suffix sym)) [] {})]))
 
-(defn- get-type [sym]
-  (let [info (info 'user sym)]
-    (if-let [_clazz (:class info)]
-      (cond
-        ((set (:interfaces info)) 'clojure.lang.IType) :type
-        ((set (:interfaces info)) 'clojure.lang.IRecord) :type
-        :else :class)                   ; interfaces are included here
-      :ns)))
+(defn- get-type [maybe-ns-sym sym]
+  (let [info (orchard.info/info (or maybe-ns-sym 'user)
+                                sym)
+        found? (:class info)
+        interfaces (if found?
+                     (some-> info :interfaces set)
+                     #{})]
+    (cond
+      (interfaces 'clojure.lang.IType)       :type
+      (interfaces 'clojure.lang.IRecord)     :type
+      found?                                 :class ;; interfaces are included here
+      (-> sym meta :refactor-nrepl/is-class) :class
+      :else                                  :ns)))
 
-(defn- collate-type-info
-  [candidates]
-  (map (fn [candidate]
-         (try
-           {:name candidate :type (get-type candidate)}
+(defn- collate-type-info [maybe-ns-sym candidates]
+  (mapv (fn [candidate]
+          (let [found-ns (some-> maybe-ns-sym find-ns)]
 
-           ;; This happends when class `candidate` depends on a class that is
-           ;; not available on the classpath.
-           (catch NoClassDefFoundError e
-             (refactor-nrepl.util/maybe-log-exception e)
-             {:name candidate :type :class})))
-       candidates))
+            (try
+              (cond-> {:name candidate
+                       :type (get-type maybe-ns-sym candidate)}
+                found-ns
+                (assoc :already-interned (if (-> candidate meta :refactor-nrepl/is-class)
+                                           (boolean (when-let [imports (some-> found-ns ns-imports)]
+                                                      (get imports (-> candidate str (string/split #"\.") last symbol))))
+                                           (boolean (when-let [refers (some-> found-ns ns-refers)]
+                                                      (let [var-namespace= (fn [var-ref]
+                                                                             (-> var-ref
+                                                                                 meta
+                                                                                 :ns
+                                                                                 ns-name
+                                                                                 (= candidate)))
+                                                            candidate-symbol (-> candidate meta :refactor-nrepl/symbol)]
+                                                        (or (some-> refers ;; refer
+                                                                    (get candidate-symbol)
+                                                                    (var-namespace=))
+                                                            (some->> refers ;; refer + rename
+                                                                     (some (fn [[k v]]
+                                                                             (when (and (var-namespace= v)
+                                                                                        (-> v
+                                                                                            meta
+                                                                                            :name
+                                                                                            (= candidate-symbol)))
+                                                                               k)))))))))))
+
+              ;; This happends when class `candidate` depends on a class that is
+              ;; not available on the classpath.
+              (catch NoClassDefFoundError e
+                (refactor-nrepl.util/maybe-log-exception e)
+                {:name candidate :type :class}))))
+        candidates))
 
 (defn- ns-publics-cljs [env ns-name]
   (->> ns-name (cljs-ana/public-vars env) keys))
@@ -60,19 +91,24 @@
                           (apply merge-with into))]
     (merge-with into ns-by-vars ns-by-macros)))
 
-(defn resolve-missing [{sym :symbol :as msg}]
-  (when (or (not (string? sym)) (str/blank? sym))
+(defn resolve-missing [{sym :symbol
+                        ns-str :ns
+                        jvm? :refactor-nrepl.internal/force-jvm?
+                        :as msg}]
+  (when (or (not (string? sym)) (string/blank? sym))
     (throw (IllegalArgumentException.
-            (str "Invalid input to resolve missing: '" sym "'"))))
-  (if-let [env (cljs/grab-cljs-env msg)]
-    (some->> sym
-             suffix
-             symbol
-             (get (cljs-vars-to-namespaces env))
-             pr-str)
-    (some->> sym
-             symbol
-             candidates
-             (remove self-referential?)
-             collate-type-info
-             pr-str)))
+            (str "Invalid input to resolve-missing: '" sym "'"))))
+  (let [ns-sym (some-> ns-str not-empty symbol)]
+    (if-let [env (and (not jvm?)
+                      (cljs/grab-cljs-env msg))]
+      (some->> sym
+               suffix
+               symbol
+               (get (cljs-vars-to-namespaces env))
+               pr-str)
+      (some->> sym
+               symbol
+               candidates
+               (remove self-referential?)
+               (collate-type-info ns-sym)
+               pr-str))))
diff --git a/test/refactor_nrepl/ns/imports_and_refers_analysis_test.clj b/test/refactor_nrepl/ns/imports_and_refers_analysis_test.clj
@@ -4,9 +4,18 @@
    [refactor-nrepl.ns.imports-and-refers-analysis :as sut]))
 
 (deftest works
-  (is (contains? '#{#{java.lang.Thread sun.jvm.hotspot.runtime.Thread}
-                    #{java.lang.Thread}}
-                 (sut/candidates :import 'Thread [] {})))
+
+  (is (= '#{java.lang.Thread}
+         (sut/candidates :import 'Thread [] {})))
+
+  (is (-> (sut/candidates :import 'Thread [] {}) first meta :refactor-nrepl/is-class))
+
+  (is (= '#{java.io.File}
+         (sut/candidates :import 'File [] {})))
+
+  (is (contains? #{'#{com.sun.tools.javac.util.List java.awt.List java.util.List}
+                   '#{com.sun.xml.internal.bind.v2.schemagen.xmlschema.List java.awt.List java.util.List}}
+                 (sut/candidates :import 'List [] {})))
 
   (is (contains? '#{#{clojure.core}
                     #{clojure.core cljs.core}}
diff --git a/test/refactor_nrepl/ns/resolve_missing_test.clj b/test/refactor_nrepl/ns/resolve_missing_test.clj
diff --git a/test/refactor_nrepl/ns/resolve_missing_unit_test.clj b/test/refactor_nrepl/ns/resolve_missing_unit_test.clj
