From 8aca9b9e4bc8e3541b2c485094f836a5d2aa4c57 Mon Sep 17 00:00:00 2001
From: vemv <vemv@users.noreply.github.com>
Date: Fri, 24 Sep 2021 04:57:12 +0200
Subject: [PATCH] Try fixing a bug in `apropos`

See https://github.com/clojure-emacs/orchard/issues/128
---
 CHANGELOG.md                  |  3 ++-
 project.clj                   |  3 ++-
 src/orchard/apropos.clj       | 46 +++++++++++++++++++++++------------
 test/orchard/apropos_test.clj | 13 +++++++++-
 4 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index be9333f77..b28b33e77 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,7 +9,8 @@
 ### Bugs Fixed
 
 * [#123](https://github.com/clojure-emacs/orchard/pull/123): Fix info lookups from namespaces that don't yet exist
-* [#125](https://github.com/clojure-emacs/orchard/pull/125): Don't fail if the classpath references a non-existing .jar
+* [#125](https://github.com/clojure-emacs/orchard/issues/125): Don't fail if the classpath references a non-existing .jar
+* [#128](https://github.com/clojure-emacs/orchard/issues/128): Strengthen `apropos`
 
 ## 0.7.1 (2021-04-18)
 
diff --git a/project.clj b/project.clj
index 9b2e2cc36..49ec98c6c 100644
--- a/project.clj
+++ b/project.clj
@@ -106,7 +106,8 @@
                                      "not-a.jar"
                                      "does-not-exist.jar"]
                     ;; Initialize the cache verbosely, as usual, so that possible issues can be more easily diagnosed:
-                    :jvm-opts ["-Dorchard.initialize-cache.silent=false"]}
+                    :jvm-opts ["-Dorchard.initialize-cache.silent=false"
+                               "-Dorchard.internal.test-suite-running=true"]}
 
              :no-dynapath {:jvm-opts ["-Dorchard.use-dynapath=false"]
                            :resource-paths [~(unzipped-jdk-source)]
diff --git a/src/orchard/apropos.clj b/src/orchard/apropos.clj
index b5b58f8c3..3785c3858 100644
--- a/src/orchard/apropos.clj
+++ b/src/orchard/apropos.clj
@@ -17,28 +17,44 @@
 
 ;;; ## Symbol Search
 
-(defn- apropos-sort
+(defn- safe-comparator [x y]
+  (compare (pr-str x) (pr-str y)))
+
+(defn- default-comparator [ns clojure-ns?]
+  (fn [x y]
+    (cond
+      (= x y)  0
+      (nil? x) 1
+      (nil? y) -1
+      (= x ns) -1
+      (= y ns)  1
+      (and (clojure-ns? x) (not (clojure-ns? y))) -1
+      (and (clojure-ns? y) (not (clojure-ns? x)))  1
+      :else (safe-comparator x y))))
+
+(defn apropos-sort
   "Return a list of vars, ordered with `ns` first,
   followed by `clojure.*` namespaces, and then all others sorted
   alphabetically."
   [ns vars]
-  (let [clojure-ns? #(.startsWith (str (ns-name %)) "clojure.")]
-    (sort-by
-     (comp :ns meta)
-     (fn [x y]
-       (cond
-         (nil? x) 1
-         (nil? y) -1
-         (= x ns) -1
-         (= y ns)  1
-         (and (clojure-ns? x) (not (clojure-ns? y))) -1
-         (and (clojure-ns? y) (not (clojure-ns? x)))  1
-         :else (compare (str x) (str y))))
-     vars)))
+  (assert (every? (some-fn class? var? symbol?) vars)
+          (pr-str vars))
+  (let [clojure-ns? #(.startsWith (str (ns-name %)) "clojure.")
+        key-fn (comp :ns meta)]
+    ;; https://clojure.org/guides/comparators
+    (try
+      (sort-by key-fn (default-comparator ns clojure-ns?) vars)
+      ;; Handle https://github.com/clojure-emacs/orchard/issues/128
+      (catch IllegalArgumentException e
+        (when (System/getProperty "orchard.internal.test-suite-running")
+          ;; Don't accept this exception in our CI - we should fix this if it's reproducible.
+          (throw e))
+        ;; Fallback to a simpler comparator:
+        (sort-by key-fn safe-comparator vars)))))
 
 (defn find-symbols
   "Takes a map and returns a list of maps containing name, doc and type.
-  `:var-query` See `vars`.
+  `:var-query` See `#'query/vars`.
   `:full-doc?` Causes the full doc to be returned instead of the abbreviated form.
   `:ns` If provided, the results will be sorted to show this namespace first."
   [{:keys [ns full-doc? var-query]}]
diff --git a/test/orchard/apropos_test.clj b/test/orchard/apropos_test.clj
index 746d15487..3de96e1e2 100644
--- a/test/orchard/apropos_test.clj
+++ b/test/orchard/apropos_test.clj
@@ -10,6 +10,12 @@
 
 (defn some-random-function [])
 
+(deftest apropos-sort-test
+  (doseq [namespace (all-ns)]
+    (let [vars (->> namespace ns-interns vals)]
+      (is (sut/apropos-sort namespace vars)
+          "Doesn't throw errors"))))
+
 (deftest var-name-test
   (testing "Returns Var's namespace-qualified name"
     (is (= "clojure.core/conj" (var-name #'clojure.core/conj))))
@@ -64,7 +70,12 @@
     (is (empty? (find-symbols {:var-query {:search #"xxxxxxxx"}}))
         "Failing searches should return empty.")
     (is (= 1 (count (find-symbols {:var-query {:search #"some-random-function"}})))
-        "Search for specific fn should return it."))
+        "Search for specific fn should return it.")
+    (doseq [private? [true false]]
+      (is (< 1000
+             (count (find-symbols {:var-query {:search #".*"
+                                               :private? private?}})))
+          "Everything is searchable; it won't throw errors")))
 
   (testing "Types are correct"
     (is (= :special-form (:type (apropos-first "def"))))