From 99889caf6ee1c2fe2cbe4cfecc8c85f132833519 Mon Sep 17 00:00:00 2001 From: vemv Date: Sat, 25 Sep 2021 06:28:09 +0200 Subject: [PATCH] Multiple fixes for `refactor-nrepl.ns.slam.hound.search` * Handle `delay` correctly * Handle separators correctly for Windows * Use a classpath value that will work * Filter out more Clojure auto-generated classes * Remove namespaces functionality, we don't use it (as we use `all-ns` instead) so it was confusing. Fixes https://github.com/clojure-emacs/refactor-nrepl/issues/335 --- CHANGELOG.md | 1 + src/refactor_nrepl/ns/slam/hound/search.clj | 106 +++++++++--------- .../ns/slam/hound/search_test.clj | 63 +++++++++++ 3 files changed, 115 insertions(+), 55 deletions(-) create mode 100644 test/refactor_nrepl/ns/slam/hound/search_test.clj diff --git a/CHANGELOG.md b/CHANGELOG.md index e778a51a..627879a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ ### Bugs fixed +* [#335](https://github.com/clojure-emacs/refactor-nrepl/issues/335): Strengthen `resolve-missing` against various edge cases. * [#289](https://github.com/clojure-emacs/refactor-nrepl/issues/289): Fix an edge-case with involving keywords that caused find-symbol to crash. * [#305](https://github.com/clojure-emacs/refactor-nrepl/issues/305): Don't put `:as` or `:refer` on their own lines in the ns form, when the libspec is so long it causes the line to wrap. * [clojure-emacs/clj-refactor.el#459](https://github.com/clojure-emacs/clj-refactor.el/issues/459): `clean-ns` should conform to the style guide: `(:require` in the ns form should be followed by a newline. diff --git a/src/refactor_nrepl/ns/slam/hound/search.clj b/src/refactor_nrepl/ns/slam/hound/search.clj index 01b50151..844b6802 100644 --- a/src/refactor_nrepl/ns/slam/hound/search.clj +++ b/src/refactor_nrepl/ns/slam/hound/search.clj @@ -3,20 +3,15 @@ ;;;; Distributed under the Eclipse Public License, the same as Clojure. (ns refactor-nrepl.ns.slam.hound.search "Search the classpath for vars and classes." - (:require [orchard.java.classpath :as cp] - [clojure.java.io :refer [file]] - [clojure.string :as string]) + (:require + [clojure.java.io :refer [file]] + [clojure.string :as string] + [refactor-nrepl.util :as util]) (:import - [java.io File FilenameFilter] - [java.util.jar JarFile JarEntry] - java.util.regex.Pattern - java.util.StringTokenizer)) - -;;; Mostly taken from leiningen.util.ns and swank.util.class-browse. - -;; TODO: replace with bultitude? but that doesn't do classes - -;;; Clojure namespaces + (java.io File FilenameFilter) + (java.util StringTokenizer) + (java.util.jar JarEntry JarFile) + (java.util.regex Pattern))) (defn jar? [^File f] (and (.isFile f) (.endsWith (.getName f) ".jar"))) @@ -24,16 +19,16 @@ (defn class-file? [^String path] (.endsWith path ".class")) -(defn clojure-fn-file? [f] - (re-find #"\$.*__\d+\.class" f)) +(defn clojure-fn-file? [^String file] + ;; originally this logic was: (re-find #"\$.*__\d+\.class" f) + ;; however that doesn't cover e.g. "clojure/spec/alpha$double_in.class" + ;; so we mimic the logic that e.g. Compliment has: + (or (.contains file "__") + (.contains file "$"))) (defn clojure-ns-file? [^String path] (.endsWith path "__init.class")) -;;; Java classes - -;; could probably be simplified - (def jar-filter (proxy [FilenameFilter] [] (accept [d n] (jar? (file n))))) @@ -47,24 +42,22 @@ (.. f getParentFile (list jar-filter)) [f]))) -(defn class-or-ns-name - "Returns the Java class or Clojure namespace name for a class relative path." +(def resource-separator + "Please do not use File/separator see e.g. https://git.io/Jzig3" + "/") + +(defn class-name [^String path] - (-> (if (clojure-ns-file? path) - (-> path (.replace "__init.class" "") (.replace "_" "-")) - (.replace path ".class" "")) - (.replace File/separator "."))) + (-> path + (.replace ".class" "") + (.replace resource-separator "."))) (defmulti path-class-files - "Returns a list of classes found on the specified path location - (jar or directory), each comprised of a map with the following keys: - :name Java class or Clojure namespace name - :loc Classpath entry (directory or jar) on which the class is located - :file Path of the class file, relative to :loc" - (fn [^File f _] - (cond (.isDirectory f) :dir - (jar? f) :jar - (class-file? (.getName f)) :class))) + (fn [^File f _loc] + (cond + (.isDirectory f) :dir + (jar? f) :jar + (class-file? (.getName f)) :class))) (defmethod path-class-files :default [& _] []) @@ -77,9 +70,13 @@ (comp (map #(.getName ^JarEntry %)) (filter class-file?) - (map class-or-ns-name)) + (remove clojure-fn-file?) + (map class-name)) (enumeration-seq (.entries (JarFile. f)))) - (catch Exception _e [])))) ; fail gracefully if jar is unreadable + (catch Exception e + (util/maybe-log-exception e) + ;; fail gracefully if jar is unreadable: + [])))) (defmethod path-class-files :dir ;; Dispatch directories and files (excluding jars) recursively. @@ -93,40 +90,39 @@ ;; Build class info using file path relative to parent classpath entry ;; location. Make sure it decends; a class can't be on classpath directly. [^File f ^File loc] - (let [fp (str f), lp (str loc) - loc-pattern (re-pattern (Pattern/quote (str "^" loc)))] - (if (re-find loc-pattern fp) ; must be descendent of loc + (let [fp (str f) + lp (str loc)] + (if (re-find (re-pattern (Pattern/quote (str "^" loc))) fp) ; must be descendent of loc (let [fpr (.substring fp (inc (count lp)))] - [(class-or-ns-name fpr)]) + [(class-name fpr)]) []))) (defn path-entries-seq "Split a string on the 'path separator', i.e. ':'. Used for splitting multiple classpath entries." [path-str] - (enumeration-seq - (StringTokenizer. path-str File/pathSeparator))) - -(defn all-classpath-entries [] - (mapcat cp/classpath-seq (cp/classpath))) + (-> path-str + (StringTokenizer. File/pathSeparator) + enumeration-seq)) (defn- get-available-classes [] (into () - (comp (mapcat path-entries-seq) - (mapcat expand-wildcard) - (mapcat #(path-class-files % %)) + (comp (mapcat expand-wildcard) + (mapcat (fn [file] + (path-class-files file file))) (remove clojure-fn-file?) (distinct) (map symbol)) - (all-classpath-entries))) + ;; We use `(System/getProperty "java.class.path")` (at least for the time being) because + ;; This code was originally written to handle that string, not a list + ;; (this code was broken for a while as `orchard.java.classpath` was being incompatibly used instead) + (path-entries-seq (System/getProperty "java.class.path")))) (def available-classes (delay (get-available-classes))) -(defn- get-available-classes-by-last-segment - [] - (delay - (group-by #(symbol (peek (string/split (str %) #"\."))) @available-classes))) +(defn- get-available-classes-by-last-segment [] + (group-by #(symbol (peek (string/split (str %) #"\."))) @available-classes)) (def available-classes-by-last-segment (delay (get-available-classes-by-last-segment))) @@ -134,5 +130,5 @@ (defn reset "Reset the cache of classes" [] - (alter-var-root #'available-classes (constantly (get-available-classes))) - (alter-var-root #'available-classes-by-last-segment (constantly (get-available-classes-by-last-segment)))) + (alter-var-root #'available-classes (constantly (delay (get-available-classes)))) + (alter-var-root #'available-classes-by-last-segment (constantly (delay (get-available-classes-by-last-segment))))) diff --git a/test/refactor_nrepl/ns/slam/hound/search_test.clj b/test/refactor_nrepl/ns/slam/hound/search_test.clj new file mode 100644 index 00000000..a95abe84 --- /dev/null +++ b/test/refactor_nrepl/ns/slam/hound/search_test.clj @@ -0,0 +1,63 @@ +(ns refactor-nrepl.ns.slam.hound.search-test + (:require + [clojure.test :refer [deftest is]] + [refactor-nrepl.ns.slam.hound.search :as sut])) + +(def acceptable-error-messages + #{"com/github/luben/zstd/ZstdInputStream" + "org/brotli/dec/BrotliInputStream" + "org/apache/tools/ant/Task" + "com/sun/jdi/request/EventRequest"}) + +(def non-initializable-classes + '#{org.mozilla.javascript.SecureCaller}) + +(defn resolve-class [sym] + (try + (Class/forName (str sym) + false + (-> (Thread/currentThread) .getContextClassLoader)) + (catch NoClassDefFoundError e + ;; there are only 4 in ~7922 classes that cause NoClassDefFoundError, + ;; see `#'acceptable-error-messages`. + ;; They don't have to do with classpath parsing so there's nothing to be fixed. + (is (contains? acceptable-error-messages (.getMessage e)) + (-> e (.getMessage))) + e) + (catch UnsupportedClassVersionError e + e))) + +(defn result-can-be-ignored? [v] + (or + (instance? NoClassDefFoundError v) + (instance? UnsupportedClassVersionError v) + (contains? non-initializable-classes v))) + +(defn ok [] + (is (< 3000 (count @sut/available-classes)) + "There are plenty of completions offered / these's a test corpus") + (is (< 3000 (count @sut/available-classes-by-last-segment))) + + (doseq [x @sut/available-classes + :let [v (resolve-class x)]] + (when-not (result-can-be-ignored? v) + (is (class? v) + (pr-str x)))) + + (doseq [[suffix classes] @sut/available-classes-by-last-segment] + (is (seq classes)) + (doseq [c classes + :let [v (resolve-class c)]] + (when-not (result-can-be-ignored? v) + (is (class? v) + (pr-str c))) + + (is (-> c str (.endsWith (str suffix)))))) + + (is (= '[clojure.lang.ExceptionInfo] + (get @sut/available-classes-by-last-segment 'ExceptionInfo)))) + +(deftest works + (ok) + (sut/reset) + (ok))