From b5878749778d0dd6f26e8f91dfbc6a9f75701c54 Mon Sep 17 00:00:00 2001
From: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Date: Thu, 11 Dec 2014 18:06:12 -0800
Subject: [PATCH] Eliminate many incorrect unused-namespaces warnings

Two main categories of problems fixed here are (a) better parsing of
ns forms to correctly reconstruct namespace name when they are given
in 'nested' form, and (b) no longer ignore macro invocations when
looking for uses of vars in a namespace.

There is still at least one bug where uses of Vars in syntax-quoted
forms are not recognized as uses of a namespace, leading to some
remaining incorrect unused-namespace warnings, including one that is
checked in as part of the Eastwood test suite, marked with a "TBD" in
a comment as needing fixing.

On crucible, these changes cause raw number of unused-namespaces
warnings to go down from 618 to 390.
---
 cases/testcases/unusednss.clj       | 33 +++++++++++++
 cases/testcases/unusednss2.clj      |  3 ++
 src/eastwood/linters/unused.clj     | 72 ++++++++++++++++++-----------
 src/eastwood/util.clj               |  7 +++
 test/eastwood/test/linters_test.clj | 16 +++++++
 5 files changed, 105 insertions(+), 26 deletions(-)
 create mode 100644 cases/testcases/unusednss.clj
 create mode 100644 cases/testcases/unusednss2.clj

diff --git a/cases/testcases/unusednss.clj b/cases/testcases/unusednss.clj
new file mode 100644
index 00000000..e1ff84ad
--- /dev/null
+++ b/cases/testcases/unusednss.clj
@@ -0,0 +1,33 @@
+(ns testcases.unusednss
+  (:use testcases.unusednss2)
+  (:require [clojure [string :as s]]
+            [clojure [repl :as r]]
+            [clojure.data :as d]))
+
+;; clojure.repl should be considered used because of the macro
+;; invocation r/doc in foo1.
+
+(defn foo1 [x]
+  (r/doc x))
+
+
+;; TBD: clojure.data should be considered used because of the function
+;; call d/diff in macro foo2.  However, as of right now Eastwood does
+;; not search through syntax-quoted forms for uses of namespaced vars.
+
+(defmacro foo2 [x y]
+  `(d/diff ~x ~y))
+
+
+;; testcases.unusednss2 should be considered used because of the use
+;; of atom1 in foo3.
+
+(defn foo3 [n]
+  (swap! atom1 conj n))
+
+
+;; Should be no warning for this require, because it is outside of ns
+;; form.
+
+(defn foo4 [ns]
+  (require ns))
diff --git a/cases/testcases/unusednss2.clj b/cases/testcases/unusednss2.clj
new file mode 100644
index 00000000..02b1bd4e
--- /dev/null
+++ b/cases/testcases/unusednss2.clj
@@ -0,0 +1,3 @@
+(ns testcases.unusednss2)
+
+(def atom1 (atom []))
diff --git a/src/eastwood/linters/unused.clj b/src/eastwood/linters/unused.clj
index 9a8349ce..ca63466a 100644
--- a/src/eastwood/linters/unused.clj
+++ b/src/eastwood/linters/unused.clj
@@ -3,6 +3,7 @@
             [clojure.string :as str]
             [clojure.java.io :as io]
             [eastwood.copieddeps.dep10.clojure.tools.reader.edn :as edn]
+            [eastwood.copieddeps.dep9.clojure.tools.namespace.parse :as parse]
             [clojure.pprint :as pp]
             [eastwood.util :as util]
             [eastwood.passes :as pass]
@@ -31,6 +32,14 @@
        (map :var)
        set))
 
+(defn macros-invoked [asts]
+  (->> asts
+       (mapcat ast/nodes)
+       (mapcat :eastwood/partly-resolved-forms)
+       (map util/safe-first)
+       (remove nil?)
+       set))
+
 (defn unused-private-vars [{:keys [asts]} opt]
   (let [pdefs (private-non-const-defs asts)
         vars-used-set (vars-used asts)]
@@ -199,25 +208,20 @@ Example: (all-suffixes [1 2 3])
 
 ;; Unused namespaces
 
-;; If require is called outside of an ns form, on an argument that is
-;; not a constant, the #(-> % :expr :form) step below can introduce
-;; nil values into the sequence.  For now, just filter those out to
-;; avoid warnings about namespace 'null' not being used.  Think about
-;; if there is a better way to handle such expressions.  Examples are
-;; in core namespace clojure.main, and contrib library namespaces
-;; clojure.tools.namespace.reload and clojure.test.generative.runner.
-(defn required-namespaces [exprs]
-  (->> (mapcat ast/nodes exprs)
-       (filter #(and (= (:op %) :invoke)
-                     (let [v (-> % :fn :var)]
-                       (#{'clojure.core/require 'clojure.core/use}
-                        (util/var-to-fqsym v)))))
-       (mapcat :args)
-       (map #(-> % :expr :form))
-       (remove nil?)
-       (map #(if (coll? %) (first %) %))
-       (remove keyword?)
-       (into #{})))
+(defn ns-form-asts [asts]
+  (->> (mapcat ast/nodes asts)
+       (filter #(= 'clojure.core/ns
+                   (util/safe-first
+                    (first (:eastwood/partly-resolved-forms %)))))))
+
+(defn required-namespaces [ns-asts]
+  (->> ns-asts
+       (mapcat #(nnext (first (:eastwood/partly-resolved-forms %))))
+       (filter (fn [f] (#{:require :use} (first f))))
+       (mapcat rest)
+       (mapcat #(#'parse/deps-from-libspec nil %))
+       set))
+
 
 ;; TBD: (-> asts first ...) below is written with the assumption that
 ;; the ns form must be first in a file.  It usually is, but
@@ -232,13 +236,29 @@ Example: (all-suffixes [1 2 3])
 ;; beginning of the ns form it is in.
 
 (defn unused-namespaces [{:keys [asts]} opt]
-  (let [curr-ns (-> asts first :statements first :args first :expr :form)
-        required (required-namespaces asts)
-        used (set (map #(-> ^clojure.lang.Var % .ns .getName)
-                       (vars-used asts)))]
-    (for [ns (set/difference required used)]
-      {:linter :unused-namespaces
-       :msg (format "Namespace %s is never used in %s" ns curr-ns)})))
+  (let [;curr-ns (-> asts first :statements first :args first :expr :form)
+        ns-asts (ns-form-asts asts)
+        loc (pass/code-loc (first ns-asts))
+        curr-ns (-> ns-asts first :form second second second)
+        required (required-namespaces ns-asts)
+        used-vars (vars-used asts)
+        used-macros (macros-invoked asts)
+;;        _ (do
+;;            (println "dbg: required namespaces:")
+;;            (pp/pprint required)
+;;            (println "dbg: vars used:")
+;;            (pp/pprint (map (juxt #(.getName (.ns %)) #(.sym %)) used-vars))
+;;            (println "dbg: macros used:")
+;;            (pp/pprint used-macros))
+        used-namespaces (set
+                         (concat (map #(-> ^clojure.lang.Var % .ns .getName)
+                                      used-vars)
+                                 (keep #(if-let [n (namespace %)] (symbol n))
+                                       used-macros)))]
+    (for [ns (set/difference required used-namespaces)]
+      (util/add-loc-info loc
+       {:linter :unused-namespaces
+        :msg (format "Namespace %s is never used in %s" ns curr-ns)}))))
 
 
 ;; Unused return values
diff --git a/src/eastwood/util.clj b/src/eastwood/util.clj
index 4a565a27..379eb434 100644
--- a/src/eastwood/util.clj
+++ b/src/eastwood/util.clj
@@ -118,6 +118,13 @@ more interesting keys earlier."
   (every? #(contains? m %) key-seq))
 
 
+(defn safe-first [f]
+  (try
+    (first f)
+    (catch Exception e
+      nil)))
+
+
 (defn nil-safe-rseq [s]
   (if (nil? s)
     nil
diff --git a/test/eastwood/test/linters_test.clj b/test/eastwood/test/linters_test.clj
index ebd08ea3..67be33a0 100644
--- a/test/eastwood/test/linters_test.clj
+++ b/test/eastwood/test/linters_test.clj
@@ -1572,6 +1572,22 @@ the next."
      :line 164, :column 1}
     1,
     })
+  (lint-test
+   'testcases.unusednss
+   [:unused-namespaces]
+   default-opts
+   {
+    {:linter :unused-namespaces,
+     :msg "Namespace clojure.data is never used in testcases.unusednss",
+     :file (fname-from-parts "testcases" "unusednss.clj"),
+     :line 1, :column 1}
+    1,
+    {:linter :unused-namespaces,
+     :msg "Namespace clojure.string is never used in testcases.unusednss",
+     :file (fname-from-parts "testcases" "unusednss.clj"),
+     :line 1, :column 1}
+    1,
+    })
   ;; I would prefer if this threw an exception, but I think it does
   ;; not because Clojure reads, analyzes, and evaluates the namespace
   ;; before lint-test does, and thus the namespace is already there