Skip to content

Commit

Permalink
Always perform a require prior to analyzing a ns (#321)
Browse files Browse the repository at this point in the history
Part of #245
  • Loading branch information
vemv authored Jul 9, 2021
1 parent 67518a5 commit c05e692
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 2 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* 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)
* Build ASTs more robustly (by using locks, `require`, and ruling out certain namespaces like refactor-nrepl itself)
* Honor internal `future-cancel` calls, improving overall responsiveness and stability.

### Bugs fixed
Expand Down
1 change: 1 addition & 0 deletions project.clj
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
:test {:dependencies [[print-foo "1.0.2"]]}
:dev {:global-vars {*warn-on-reflection* true}
:dependencies [[org.clojure/clojurescript "1.10.520"]
[org.clojure/core.async "1.3.618" :exclusions [org.clojure/clojure org.clojure/tools.reader]]
[cider/piggieback "0.5.2"]
[commons-io/commons-io "2.8.0"]]
:repl-options {:nrepl-middleware [cider.piggieback/wrap-cljs-repl]}
Expand Down
8 changes: 7 additions & 1 deletion src/refactor_nrepl/analyzer.clj
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,13 @@
(not (util/self-referential? ns)))
;; Use `locking`, because AST analysis can perform arbitrary evaluation.
;; Parallel analysis is not safe, especially as it can perform `require` calls.
(locking core/require-lock
(locking core/require-lock ;; for both `require` and `aj/analyze-ns`

;; Performing this `require` makes it more likely that t.ana will succeed.
;; I believe it's because `require` will also require other namespaces recursively.
;; t.ana does so in theory as well, but it's slightly more rigid,
;; and/or does not always do the same exact thing the Clojure compiler would.
(require ns)
(let [opts {:passes-opts
{:validate/unresolvable-symbol-handler shadow-unresolvable-symbol-handler
:validate/throw-on-arity-mismatch false
Expand Down
4 changes: 4 additions & 0 deletions test-resources/core_async_usage.clj
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
(ns core-async-usage
"Analyzing this ns used to be problematic, see git.io/Jcd7W"
(:require
[clojure.core.async :refer [go-loop]]))
10 changes: 10 additions & 0 deletions test/refactor_nrepl/analyzer_test.clj
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
(ns refactor-nrepl.analyzer-test
(:require
[clojure.java.io :as io]
[refactor-nrepl.analyzer :as sut]
[clojure.test :refer [deftest is]]))

(deftest ns-ast-test
(doseq [f ["core_async_usage.clj"]
:let [c (-> f io/resource slurp)]]
(is (some? (sut/ns-ast c)))))

0 comments on commit c05e692

Please sign in to comment.