Don't prune require forms if they are needed for a given import t…

…o work (#367) Fixes #194
clojure-emacs · Feb 9, 2022 · 56ac8e2 · 56ac8e2
1 parent 0b3e68f
commit 56ac8e2
Show file tree

Hide file tree

Showing 8 changed files with 157 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## Unreleased
 
 * [#173](https://github.com/clojure-emacs/refactor-nrepl/issues/173): `rename-file-or-dir`: rename more kinds of constructs in dependent namespaces: namespace-qualified maps, fully-qualified functions, metadata.
+* [#194](https://github.com/clojure-emacs/refactor-nrepl/issues/194): Don't prune `require` forms if they are needed for a given `import` to work.
 
 ## 3.3.1
 

diff --git a/src/refactor_nrepl/ns/prune_dependencies.clj b/src/refactor_nrepl/ns/prune_dependencies.clj
@@ -1,6 +1,8 @@
 (ns refactor-nrepl.ns.prune-dependencies
   (:require
    [cider.nrepl.middleware.info :as info]
+   [clojure.set :as set]
+   [clojure.string :as string]
    [refactor-nrepl.core :as core]
    [refactor-nrepl.find.symbols-in-file :as symbols-in-file]
    [refactor-nrepl.ns.libspec-allowlist :as libspec-allowlist]
@@ -116,18 +118,77 @@
                      (-> pattern re-pattern (re-find ns-name)))
                    (libspec-allowlist/libspec-allowlist)))))
 
-(defn- prune-libspec [symbols-in-file current-ns libspec]
-  (if (libspec-should-never-be-pruned? libspec)
+(defn imports->namespaces
+  "Given a collection of `:import` clauses, returns the set of namespaces denoted by them, as symbols.
+
+  Some of those namespace symbols may not refer to actual namespaces.
+  e.g. a `java.io.File` import would return `java.io`, which isn't a Clojure namespace."
+  [imports]
+  (into #{}
+        (map (fn [import]
+               (-> (if (sequential? import)
+                     (first import)
+                     (->> (-> import str (string/split #"\."))
+                          (butlast)
+                          (string/join ".")))
+                   str
+                   (string/replace "_" "-")
+                   symbol)))
+        imports))
+
+(defn libspec->namespaces
+  "Given a libspec, returns the namespaces denoted by it (typically one, but possibly multiple,
+  if prefix notation was used), as symbols."
+  [libspec]
+  (cond
+    (symbol? libspec)
+    [libspec]
+
+    ;; Check if it doesn't denote prefix notation:
+    (and (sequential? libspec)
+         (or (-> libspec count #{1})
+             (some keyword? libspec)))
+    [(first libspec)]
+
+    :else
+    (let [suffixes (->> libspec
+                        rest
+                        (map (fn [suffix]
+                               (cond-> suffix
+                                 (sequential? suffix) first))))]
+      (map (fn [prefix suffix]
+             (symbol (str prefix "." suffix)))
+           (repeat (first libspec))
+           suffixes))))
+
+(defn imports-contain-libspec?
+  "Do `import-namespaces` contain at least one namespace that is denoted by `libspec`?
+
+  This is useful for keeping requires that emit classes (i.e. those defining deftypes/defrecords),
+  which are imported via `:import`."
+  [imports-namespaces libspec]
+  {:pre [(set? imports-namespaces)]}
+  (let [require-namespaces (set (libspec->namespaces libspec))]
+    (some? (seq (set/intersection imports-namespaces require-namespaces)))))
+
+(defn- prune-libspec [symbols-in-file current-ns imports-namespaces libspec]
+  (cond
+    (libspec-should-never-be-pruned? libspec)
+    libspec
+
+    (imports-contain-libspec? imports-namespaces (:ns libspec))
     libspec
+
+    :else
     (some->> libspec
              (remove-unused-renamed-symbols symbols-in-file)
              (remove-unused-requires symbols-in-file current-ns))))
 
 (defn- prune-libspecs
-  [libspecs symbols-in-file current-ns]
-  (->> libspecs
-       (map (partial prune-libspec symbols-in-file current-ns))
-       (filter (complement nil?))))
+  [libspecs symbols-in-file current-ns imports]
+  (let [imports-namespaces (imports->namespaces imports)]
+    (keep (partial prune-libspec symbols-in-file current-ns imports-namespaces)
+          libspecs)))
 
 (defn- prune-imports
   [imports symbols-in-file]
@@ -141,15 +202,17 @@
         symbols-in-file (->> (symbols-in-file/symbols-in-file path parsed-ns
                                                               dialect)
                              (map str)
-                             set)]
-    {dialect (merge {:require
-                     (prune-libspecs required-libspecs symbols-in-file current-ns)
-                     :import (prune-imports (some-> parsed-ns dialect :import)
-                                            symbols-in-file)}
+                             set)
+        ;; `imports` are calculated before `requires`, because
+        ;; the former's needs affect whether the latter can be pruned:
+        imports (prune-imports (some-> parsed-ns dialect :import)
+                               symbols-in-file)
+        requires (prune-libspecs required-libspecs symbols-in-file current-ns imports)]
+    {dialect (merge {:require requires
+                     :import imports}
                     (when (= dialect :cljs)
                       {:require-macros
-                       (prune-libspecs required-macro-libspecs symbols-in-file
-                                       current-ns)}))}))
+                       (prune-libspecs required-macro-libspecs symbols-in-file current-ns #{})}))}))
 
 (defn- prune-cljc-dependencies [parsed-ns path]
   (merge

diff --git a/test-resources/defines_deftype.clj b/test-resources/defines_deftype.clj
@@ -0,0 +1,3 @@
+(ns defines-deftype)
+
+(defrecord SomeDefType [])
diff --git a/test-resources/ns1.clj b/test-resources/ns1.clj
@@ -14,11 +14,13 @@
    (clojure data edn)
    [clojure.pprint :refer [get-pretty-writer formatter cl-format]]
    clojure.test.junit
-   [clojure.xml])
+   [clojure.xml]
+   [defines-deftype])
   (:use clojure.test
         clojure.test
         [clojure.string :rename {replace foo reverse bar} :reload-all true :reload true])
-  (:import java.util.Random
+  (:import [defines_deftype SomeDefType]
+           java.util.Random
            java.io.PushbackReader
            java.io.PushbackReader
            java.io.FilenameFilter
@@ -30,6 +32,8 @@
             SomeClass$InnerClass$InnerInnerClassThree]
            (java.util Date Calendar)))
 
+(SomeDefType.)
+
 (defmacro tt [writer]
   (Random.)
   `(get-pretty-writer ~writer))

diff --git a/test-resources/ns1_cleaned.clj b/test-resources/ns1_cleaned.clj
@@ -15,8 +15,10 @@
              [test :refer :all]
              [walk :refer [postwalk prewalk]]
              xml]
-            clojure.test.junit)
+            clojure.test.junit
+            [defines-deftype])
   (:import
+   [defines_deftype SomeDefType]
    [java.io Closeable FilenameFilter PushbackReader]
    [java.util Calendar Date Random]
    [refactor.nrepl

diff --git a/test-resources/ns1_cleaned_and_pprinted b/test-resources/ns1_cleaned_and_pprinted
@@ -17,8 +17,10 @@
    [clojure.test :refer :all]
    [clojure.test.junit]
    [clojure.walk :refer [postwalk prewalk]]
-   [clojure.xml])
+   [clojure.xml]
+   [defines-deftype])
   (:import
+   (defines_deftype SomeDefType)
    (java.io Closeable FilenameFilter PushbackReader)
    (java.util Calendar Date Random)
    (refactor.nrepl SomeClass$InnerClass$InnerInnerClassOne SomeClass$InnerClass$InnerInnerClassTwo)))
diff --git a/test-resources/ns1_cleaned_and_pprinted_prefix_notation b/test-resources/ns1_cleaned_and_pprinted_prefix_notation
@@ -15,8 +15,10 @@
     [string :refer :all :reload-all true]
     [test :refer :all]
     [walk :refer [postwalk prewalk]]]
-   [clojure.test.junit])
+   [clojure.test.junit]
+   [defines-deftype])
   (:import
+   (defines_deftype SomeDefType)
    (java.io Closeable FilenameFilter PushbackReader)
    (java.util Calendar Date Random)
    (refactor.nrepl SomeClass$InnerClass$InnerInnerClassOne SomeClass$InnerClass$InnerInnerClassTwo)))
diff --git a/test/refactor_nrepl/ns/prune_dependencies_test.clj b/test/refactor_nrepl/ns/prune_dependencies_test.clj
@@ -0,0 +1,62 @@
+(ns refactor-nrepl.ns.prune-dependencies-test
+  (:require
+   [clojure.test :refer [are deftest]]
+   [refactor-nrepl.ns.prune-dependencies :as sut]))
+
+(deftest imports->namespaces
+  (are [input expected] (= expected
+                           (sut/imports->namespaces input))
+    ['java.io.File]              #{'java.io}
+    ['my_ns.File]                #{'my-ns}
+    ['[java.io File]]            #{'java.io}
+    ['[java.io File FileReader]] #{'java.io}
+    ['(java.io File)]            #{'java.io}
+    ['java.io.File
+     'my_ns.File
+     '[java.io File]
+     '(java.io File)]            #{'java.io 'my-ns}))
+
+(deftest libspec->namespaces
+  (are [input expected] (= expected
+                           (sut/libspec->namespaces input))
+    'foo.bar
+    '[foo.bar],
+
+    '[foo.bar]
+    '[foo.bar],
+
+    '[foo.bar :as bar]
+    '[foo.bar],
+
+    '[clojure data edn
+      [instant :as inst :reload true]
+      [pprint :refer [cl-format formatter get-pretty-writer]]
+      [string :refer :all :reload-all true]
+      [test :refer :all]
+      [walk :refer [postwalk prewalk]]
+      xml]
+    '[clojure.data clojure.edn clojure.instant clojure.pprint clojure.string clojure.test clojure.walk clojure.xml]))
+
+(deftest imports-contain-libspec?
+  (are [imports libspec expected] (= expected
+                                     (sut/imports-contain-libspec? (sut/imports->namespaces imports)
+                                                                   libspec))
+    #_imports             #_libspec               #_expected
+    '[]                   'foo.bar                false
+    '[foo.bar.SomeType]   'foo.bar                true
+    '[foo_bar.SomeType]   'foo-bar                true
+    '[[foo.bar SomeType]] 'foo.bar                true
+    '[[foo_bar SomeType]] 'foo-bar                true
+    '[foo.bar.SomeType]   'foo.baz                false
+    '[]                   '[foo.bar]              false
+    '[foo.bar.SomeType]   '[foo.bar]              true
+    '[foo.bar.SomeType]   '[foo.bar :as f]        true
+    '[[foo.bar SomeType]] '[foo.bar]              true
+    '[[foo_bar SomeType]] '[foo-bar :as f]        true
+    '[foo_bar.SomeType]   '[foo-bar :as f]        true
+    '[[foo_bar SomeType]] '[foo-bar]              true
+    '[[foo_bar SomeType]] '[foo-bar :as f]        true
+    '[foo.bar.SomeType]   '[foo.baz]              false
+    '[clojure.data.Data]  '[clojure data]         true
+    '[clojure.data.Data]  '[clojure [data :as d]] true
+    '[clojure.data.Data]  '[clojure foo]          false))
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		(ns defines-deftype)

		(defrecord SomeDefType [])