Introduce with-suppressed-errors (#317)

Fixes #291
clojure-emacs · Jul 8, 2021 · b78e354 · b78e354
1 parent 8457b03
commit b78e354
Show file tree

Hide file tree

Showing 23 changed files with 217 additions and 95 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,17 @@
 
 ## Unreleased
 
+#### Changes
+* [#291](https://github.com/clojure-emacs/refactor-nrepl/issues/291): The `:ignore-errors` option will be honored in more places, making refactor-nrepl more robust in face of files not particularly meant to be part of the AST corpus.
+  * Examples: WIP files, Moustache template files, scripts.
+* Upgrade Orchard
+  * Worth emphasizing: now the following options are available https://github.com/clojure-emacs/orchard/tree/v0.7.0#configuration-options
+  * They can make the refactor-nrepl experience simpler / more robust.
+* Reliability improvement: try using `require` prior to `find-ns`
+  * This increases the chances that a namespace will be found, which in turns makes refactor-nrepl more complete/accurate.
+* Replace Cheshire with `clojure.data.json`
+* Build ASTs more robustly (by using locks and ruling out certain namespaces like refactor-nrepl itself)
+
 ### Bugs fixed
 * [#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.

diff --git a/README.md b/README.md
@@ -81,6 +81,15 @@ Configuration settings are passed along with each msg, currently the recognized
 
  :debug false
 
+  ;; if `true`:
+  ;;   * files that can't be `read`, `require`d or analyzed (with `tools.analyzer`) will be ignored,
+  ;;     instead of aborting the early phases of refactor-nrepl execution.
+  ;;   * ops like `find-symbol` will carry on even if there is broken namespace which we can not build AST for.
+  ;; Setting `false` will be more strict, yielding possibly more correct usage,
+  ;; but it also needs that `:ignore-paths` is correctly set, that all namespaces are valid,
+  ;; that tools.analyzer is able to analyze all of them, etc
+  :ignore-errors true
+
  ;; When true `clean-ns` will remove unused symbols, otherwise just
  ;; sort etc
  :prune-ns-form true

diff --git a/eastwood.clj b/eastwood.clj
@@ -0,0 +1,4 @@
+(disable-warning
+ {:linter                      :unused-ret-vals-in-try
+  :if-inside-macroexpansion-of #{'clojure.test/deftest
+                                 'clojure.core/assert}})
diff --git a/project.clj b/project.clj
@@ -66,12 +66,13 @@
                                           with-debug-bindings [[:inner 0]]
                                           merge-meta [[:inner 0]]
                                           try-if-let [[:block 1]]}}}]
-             :eastwood {:plugins         [[jonase/eastwood "0.7.0"]]
+             :eastwood {:plugins         [[jonase/eastwood "0.7.1"]]
                         :eastwood {;; vendored - shouldn't be tweaked for satisfying linters:
                                    :exclude-namespaces [refactor-nrepl.ns.slam.hound.regrow]
                                    ;; :implicit-dependencies would fail spuriously when the CI matrix runs for Clojure < 1.10,
                                    ;; because :implicit-dependencies can only work for a certain corner case starting from 1.10.
-                                   :exclude-linters [:implicit-dependencies]}}
+                                   :exclude-linters [:implicit-dependencies]
+                                   :config-files ["eastwood.clj"]}}
              :clj-kondo [:test
                          {:dependencies [[clj-kondo "2021.06.18"]]}]}
 

diff --git a/src/refactor_nrepl/analyzer.clj b/src/refactor_nrepl/analyzer.clj
@@ -132,7 +132,7 @@
                              "OK"))))))
 
 (defn warm-ast-cache []
-  (doseq [f (tracker/project-files-in-topo-order)]
+  (doseq [f (tracker/project-files-in-topo-order true)]
     (try
       (ns-ast (slurp f))
       (catch Throwable th

diff --git a/src/refactor_nrepl/config.clj b/src/refactor_nrepl/config.clj
@@ -8,6 +8,15 @@
 
    :debug false
 
+   ;; if `true`:
+   ;;   * files that can't be `read`, `require`d or analyzed (with `tools.analyzer`) will be ignored,
+   ;;     instead of aborting the early phases of refactor-nrepl execution.
+   ;;   * ops like `find-symbol` will carry on even if there is broken namespace which we can not build AST for.
+   ;; Setting `false` will be more strict, yielding possibly more correct usage,
+   ;; but it also needs that `:ignore-paths` is correctly set, that all namespaces are valid,
+   ;; that tools.analyzer is able to analyze all of them, etc
+   :ignore-errors true
+
    ;; When true `clean-ns` will remove unused symbols, otherwise just
    ;; sort etc
    :prune-ns-form true

diff --git a/src/refactor_nrepl/find/find_macros.clj b/src/refactor_nrepl/find/find_macros.clj
@@ -81,7 +81,9 @@
 (defn- find-macro-definitions-in-project
   "Finds all macros that are defined in the project."
   [ignore-errors?]
-  (->> (core/find-in-project (some-fn core/cljc-file? core/clj-file?))
+  (->> (core/find-in-project (util/with-suppressed-errors
+                               (some-fn core/cljc-file? core/clj-file?)
+                               ignore-errors?))
        (mapcat #(try
                   (get-macro-definitions-in-file-with-caching %)
                   (catch Exception e
@@ -215,7 +217,7 @@
   (when (fully-qualified-name? fully-qualified-name)
     (let [all-defs (find-macro-definitions-in-project ignore-errors?)
           macro-def (first (filter #(= (:name %) fully-qualified-name) all-defs))
-          tracker (tracker/build-tracker)
+          tracker (tracker/build-tracker (util/with-suppressed-errors tracker/default-file-filter-predicate ignore-errors?))
           origin-ns (symbol (core/prefix fully-qualified-name))
           dependents (tracker/get-dependents tracker origin-ns)]
       (some->> macro-def

diff --git a/src/refactor_nrepl/find/find_symbol.clj b/src/refactor_nrepl/find/find_symbol.clj
@@ -135,20 +135,22 @@
         fully-qualified-name (if (= namespace "clojure.core")
                                var-name
                                (str/join "/" [namespace var-name]))
-        referred-syms (libspecs/referred-syms-by-file&fullname)]
+        referred-syms (libspecs/referred-syms-by-file&fullname ignore-errors)]
     (->> (core/dirs-on-classpath)
-         (mapcat (partial core/find-in-dir (every-pred (some-fn core/clj-file? core/cljc-file?)
-                                                       (fn [f]
-                                                         (try
-                                                           (let [n (some-> f
-                                                                           core/read-ns-form
-                                                                           parse/name-from-ns-decl)]
-                                                             (if-not n
-                                                               false
-                                                               (not (self-referential? n))))
-                                                           (catch Exception e
-                                                             (util/maybe-log-exception e)
-                                                             false))))))
+         (mapcat (partial core/find-in-dir (util/with-suppressed-errors
+                                             (every-pred (some-fn core/clj-file? core/cljc-file?)
+                                                         (fn [f]
+                                                           (try
+                                                             (let [n (some-> f
+                                                                             core/read-ns-form
+                                                                             parse/name-from-ns-decl)]
+                                                               (if-not n
+                                                                 false
+                                                                 (not (self-referential? n))))
+                                                             (catch Exception e
+                                                               (util/maybe-log-exception e)
+                                                               false))))
+                                             ignore-errors)))
          (mapcat (partial find-symbol-in-file fully-qualified-name ignore-errors referred-syms)))))
 
 (defn- get&read-enclosing-sexps

diff --git a/src/refactor_nrepl/middleware.clj b/src/refactor_nrepl/middleware.clj
@@ -157,8 +157,8 @@
   (delay
    (require-and-resolve 'refactor-nrepl.rename-file-or-dir/rename-file-or-dir)))
 
-(defn- rename-file-or-dir-reply [{:keys [transport old-path new-path] :as msg}]
-  (reply transport msg :touched (@rename-file-or-dir old-path new-path)
+(defn- rename-file-or-dir-reply [{:keys [transport old-path new-path ignore-errors] :as msg}]
+  (reply transport msg :touched (@rename-file-or-dir old-path new-path (= ignore-errors "true"))
          :status :done))
 
 (defn- namespace-aliases-reply [{:keys [transport] :as msg}]

diff --git a/src/refactor_nrepl/ns/libspecs.clj b/src/refactor_nrepl/ns/libspecs.clj
@@ -1,6 +1,7 @@
 (ns refactor-nrepl.ns.libspecs
   (:require [refactor-nrepl.core :as core]
-            [refactor-nrepl.ns.ns-parser :as ns-parser])
+            [refactor-nrepl.ns.ns-parser :as ns-parser]
+            [refactor-nrepl.util :as util])
   (:import [java.io File]))
 
 ;; The structure here is {path {lang [timestamp value]}}
@@ -46,13 +47,19 @@
 
   {:clj {util com.acme.util str clojure.string
    :cljs {gstr goog.str}}}"
-  []
-  {:clj (->> (core/find-in-project (some-fn core/clj-file? core/cljc-file?))
-             (map (partial get-libspec-from-file-with-caching :clj))
-             aliases-by-frequencies)
-   :cljs (->> (core/find-in-project (some-fn core/cljs-file? core/cljc-file?))
-              (map (partial get-libspec-from-file-with-caching :cljs))
-              aliases-by-frequencies)})
+  ([]
+   (namespace-aliases false))
+  ([ignore-errors?]
+   {:clj  (->> (core/find-in-project (util/with-suppressed-errors
+                                       (some-fn core/clj-file? core/cljc-file?)
+                                       ignore-errors?))
+               (map (partial get-libspec-from-file-with-caching :clj))
+               aliases-by-frequencies)
+    :cljs (->> (core/find-in-project (util/with-suppressed-errors
+                                       (some-fn core/cljs-file? core/cljc-file?)
+                                       ignore-errors?))
+               (map (partial get-libspec-from-file-with-caching :cljs))
+               aliases-by-frequencies)}))
 
 (defn- unwrap-refer
   [file {:keys [ns refer]}]
@@ -75,10 +82,16 @@
    Example:
    {:clj  {\"/home/someuser/projects/some.clj\" [\"example.com/foobar\" foobar]}
     :cljs}"
-  []
-  {:clj (->> (core/find-in-project (some-fn core/clj-file? core/cljc-file?))
-             (map (juxt identity (partial get-libspec-from-file-with-caching :clj)))
-             sym-by-file&fullname)
-   :cljs (->> (core/find-in-project (some-fn core/cljs-file? core/cljc-file?))
-              (map (juxt identity (partial get-libspec-from-file-with-caching :cljs)))
-              sym-by-file&fullname)})
+  ([]
+   (referred-syms-by-file&fullname false))
+  ([ignore-errors?]
+   {:clj  (->> (core/find-in-project (util/with-suppressed-errors
+                                       (some-fn core/clj-file? core/cljc-file?)
+                                       ignore-errors?))
+               (map (juxt identity (partial get-libspec-from-file-with-caching :clj)))
+               sym-by-file&fullname)
+    :cljs (->> (core/find-in-project (util/with-suppressed-errors
+                                       (some-fn core/cljs-file? core/cljc-file?)
+                                       ignore-errors?))
+               (map (juxt identity (partial get-libspec-from-file-with-caching :cljs)))
+               sym-by-file&fullname)}))
diff --git a/src/refactor_nrepl/ns/tracker.clj b/src/refactor_nrepl/ns/tracker.clj
@@ -7,6 +7,7 @@
              [repl :refer [refresh-dirs]]
              [track :as tracker]]
             [refactor-nrepl.core :as core]
+            [refactor-nrepl.util :as util]
             [refactor-nrepl.ns.ns-parser :as ns-parser])
   (:import [java.io File]))
 
@@ -36,13 +37,15 @@
       ;; corner case - use the mranderson-ized refresh-dirs (for supporting this project's test suite):
       refresh-dirs))
 
+(def default-file-filter-predicate (every-pred core/source-file?
+                                               safe-for-clojure-tools-namespace?))
+
 (defn build-tracker
   "Build a tracker for the project.
 
   If file-predicate is provided, use that instead of `core/source-file?`"
   ([]
-   (build-tracker #(and (core/source-file? %)
-                        (safe-for-clojure-tools-namespace? %))))
+   (build-tracker default-file-filter-predicate))
   ([file-predicate]
    (file/add-files (tracker/tracker) (core/find-in-project file-predicate))))
 
@@ -74,11 +77,16 @@
       (boolean (some #(str/starts-with? % file-as-absolute-paths)
                      refresh-dirs-as-absolute-paths)))))
 
-(defn project-files-in-topo-order []
-  (let [tracker (build-tracker (every-pred (partial in-refresh-dirs? (user-refresh-dirs))
-                                           core/clj-file?))
-        nses (dep/topo-sort (:clojure.tools.namespace.track/deps tracker))
-        filemap (:clojure.tools.namespace.file/filemap tracker)
-        ns2file (zipmap (vals filemap) (keys filemap))]
-    (->> (map ns2file nses)
-         (remove nil?))))
+(defn project-files-in-topo-order
+  ([]
+   (project-files-in-topo-order false))
+  ([ignore-errors?]
+   (let [tracker (build-tracker (util/with-suppressed-errors
+                                  (every-pred (partial in-refresh-dirs? (user-refresh-dirs))
+                                              core/clj-file?)
+                                  ignore-errors?))
+         nses (dep/topo-sort (:clojure.tools.namespace.track/deps tracker))
+         filemap (:clojure.tools.namespace.file/filemap tracker)
+         ns2file (zipmap (vals filemap) (keys filemap))]
+     (->> (map ns2file nses)
+          (remove nil?)))))
diff --git a/src/refactor_nrepl/rename_file_or_dir.clj b/src/refactor_nrepl/rename_file_or_dir.clj
@@ -194,10 +194,10 @@
 
 (defn- rename-source-file
   "Move file from old to new, updating any dependents."
-  [old-path new-path]
+  [old-path new-path ignore-errors?]
   (let [old-ns (core/ns-from-string (slurp old-path))
         new-ns (path->ns new-path)
-        tracker (tracker/build-tracker)
+        tracker (tracker/build-tracker (util/with-suppressed-errors tracker/default-file-filter-predicate ignore-errors?))
         dependents (tracker/get-dependents tracker old-ns)
         new-dependents (atom {})]
     (doseq [^File f dependents]
@@ -214,15 +214,17 @@
   [path old-parent new-parent]
   (str/replace path old-parent new-parent))
 
-(defn- rename-dir [old-path new-path]
+(defn- rename-dir [old-path new-path ignore-errors?]
   (let [old-path (util/normalize-to-unix-path old-path)
         new-path (util/normalize-to-unix-path new-path)
         old-path (if (.endsWith old-path "/") old-path (str old-path "/"))
         new-path (if (.endsWith new-path "/") new-path (str new-path "/"))]
     (flatten (for [^File f (file-seq (File. old-path))
                    :when (not (fs/directory? f))
                    :let [path (util/normalize-to-unix-path (.getAbsolutePath f))]]
-               (-rename-file-or-dir path (merge-paths path old-path new-path))))))
+               (-rename-file-or-dir path
+                                    (merge-paths path old-path new-path)
+                                    ignore-errors?)))))
 
 (defn- file-or-symlink-exists? [^String path]
   (let [f (File. path)]
@@ -234,12 +236,12 @@
             (when (.. target toFile exists)
               path)))))))
 
-(defn- -rename-file-or-dir [^String old-path new-path]
+(defn- -rename-file-or-dir [^String old-path new-path ignore-errors?]
   (let [affected-files  (if (fs/directory? old-path)
-                          (rename-dir old-path new-path)
+                          (rename-dir old-path new-path ignore-errors?)
                           (if ((some-fn core/clj-file? core/cljs-file?)
                                (File. old-path))
-                            (rename-source-file old-path new-path)
+                            (rename-source-file old-path new-path ignore-errors?)
                             (rename-file! old-path new-path)))]
     (->> affected-files
          flatten
@@ -271,7 +273,11 @@
   old-path and new-path are expected to be aboslute paths.
 
   Returns a list of all files that were affected."
-  [old-path new-path]
-  (assert-friendly old-path new-path)
-  (binding [*print-length* nil]
-    (-rename-file-or-dir old-path new-path)))
+
+  ([old-path new-path]
+   (rename-file-or-dir old-path new-path false))
+
+  ([old-path new-path ignore-errors?]
+   (assert-friendly old-path new-path)
+   (binding [*print-length* nil]
+     (-rename-file-or-dir old-path new-path ignore-errors?))))
diff --git a/src/refactor_nrepl/util.clj b/src/refactor_nrepl/util.clj
@@ -72,3 +72,19 @@
 (defn maybe-log-exception [^Throwable e]
   (when (System/getProperty "refactor-nrepl.internal.log-exceptions")
     (.printStackTrace e)))
+
+(defn with-suppressed-errors
+  "Wraps a predicate `f` in a try-catch, depending on `ignore-errors?`.
+
+  Typically used for making ingestion of data (`read`/`require`/`analyze`) more robust
+  in face of unconfigured or otherwise problematic source paths."
+  [f ignore-errors?]
+  (if-not ignore-errors?
+    f
+    (fn [x]
+      (try
+        (f x)
+        (catch Exception e
+          (maybe-log-exception e)
+          ;; return false, because `with-suppressed-errors` is oriented for predicate usage
+          false)))))
diff --git a/test-resources/readable_file_incorrect_aliases.clj b/test-resources/readable_file_incorrect_aliases.clj
@@ -0,0 +1,2 @@
+;; An incorrect alias usage (because `a` is not declared in the `ns` form):
+::a/foo
diff --git a/test-resources/readable_file_incorrect_data_readers.clj b/test-resources/readable_file_incorrect_data_readers.clj
@@ -0,0 +1,2 @@
+;; An incorrect data readers usage (because there's no such data reader declared anywhere):
+#totally.made.up/foo []
diff --git a/test-resources/unreadable_file.clj b/test-resources/unreadable_file.clj
@@ -0,0 +1 @@
+(ns unreadable-file
diff --git a/test/refactor_nrepl/extract_definition_test.clj b/test/refactor_nrepl/extract_definition_test.clj
@@ -1,16 +1,18 @@
 (ns refactor-nrepl.extract-definition-test
   (:require [clojure.java.io :as io]
             [clojure.test :refer :all]
-            [refactor-nrepl.extract-definition :refer :all]))
+            [refactor-nrepl.extract-definition :as sut]
+            [refactor-nrepl.unreadable-files :refer [ignore-errors-str]]))
 
 (defn- -extract-definition
   [name line col]
-  (get-in (extract-definition
+  (get-in (sut/extract-definition
            {:file (.getAbsolutePath (io/file "test-resources/extract_definition.clj"))
             :ns "extract-definition"
             :line line
             :column col
             :name name
+            :ignore-errors ignore-errors-str
             :dir "test-resources"})
           [:definition :definition]))
 
@@ -72,11 +74,12 @@
          "(+ 11 17)")))
 
 (deftest returns-meta-data
-  (let [res (extract-definition
+  (let [res (sut/extract-definition
              {:file (.getAbsolutePath (io/file "test-resources/extract_definition.clj"))
               :ns "extract-definition"
               :line 44
               :column 14
+              :ignore-errors ignore-errors-str
               :name "if-let-bound"
               :dir "."})]
     (is (= (count (:occurrences res)) 1))