Implement a new linter: :performance

Closes #416

README.md

@@ -124,6 +124,7 @@ enabled by default unless they have '(disabled)' after their name.
 | `:no-ns-form-found` | Warn about Clojure files where no `ns` form could be found. | [[more]](#no-ns-form-found) |
 | `:non-clojure-file` (disabled) | Warn about files that will not be linted because they are not Clojure source files, i.e. their name does not end with '.clj'. | [[more]](#non-clojure-file) |
 | `:non-dynamic-earmuffs` | Vars marked `^:dynamic` should follow the "earmuff" naming convention, and vice versa. | [[more]](#non-dynamic-earmuffs) |
+| `:performance` | Performance warnings | [[more]](#performance) |
 | `:redefd-vars` | Redefinitions of the same name in the same namespace. | [[more]](#redefd-vars) |
 | `:reflection` | Reflection warnings | [[more]](#reflection) |
 | `:suspicious-expression` | Suspicious expressions that appear incorrect, because they always return trivial values. | [[more]](#suspicious-expression) |
@@ -2030,6 +2031,25 @@ such that namespaces re-compiled by Eastwood's analysis will be visible and used
 
 </details>
 
+### `:performance`
+
+#### Performance warnings from the Clojure compiler
+
+<details>
+
+The Clojure compiler optionally emits performance warnings related to the use of `case` and `recur` and their relationship with primitive numerics.
+
+> Please refer to https://clojure.org/reference/java_interop for a guide on primive math
+(tldr: use `(long)`, or occasionally `^long` where it is permitted).
+
+Eastwood wraps these warnings, enhacing them when needed (the reported file name can be misleading),
+restricting them to _your project's_ source paths and allowing them to be omitted on a file/line basis.  
+
+This linter is disabled by default because it's not customary or necessarily justified to address these warnings.
+For some corner cases it might not be even possible (however the [`:ignored-faults`](https://github.com/jonase/eastwood#ignored-faults) would allow you to prevent that corner case from failing your build).
+
+</details>
+
 ### `:reflection`
 
 #### Reflection warnings from the Clojure compiler

cases/testcases/performance/green/case.clj

@@ -0,0 +1,5 @@
+(ns testcases.performance.green.case)
+
+(defn foo [x]
+  (case (long x)
+    1 :one))

cases/testcases/performance/green/hash.clj

@@ -0,0 +1,3 @@
+(ns testcases.performance.green.hash)
+
+(case 1 1 :long 922337203900225948N :big 2)

cases/testcases/performance/green/recur.clj

@@ -0,0 +1,41 @@
+(ns testcases.performance.green.recur
+  "Like `testcases.performance.green.recur` but adds a `long`` call
+  to address the recur warning."
+  (:require
+   [clojure.string :as string]))
+
+(def ^:const vlq-base-shift 5)
+(def ^:const vlq-base (bit-shift-left 1 vlq-base-shift))
+(def ^:const vlq-base-mask (dec vlq-base))
+(def ^:const vlq-continuation-bit vlq-base)
+
+(defn to-vlq-signed [v]
+  (if (neg? v)
+    (inc (bit-shift-left (- v) 1))
+    (+ (bit-shift-left v 1) 0)))
+
+(defn from-vlq-signed [v]
+  (let [neg? (= (bit-and v 1) 1)
+        shifted (bit-shift-right v 1)]
+    (if neg?
+      (- shifted)
+      shifted)))
+
+(defn decode [^String s]
+  (let [l (.length s)]
+    (loop [i 0 result 0 shift 0]
+      (when (>= i l)
+        (throw (Error. "Expected more digits in base 64 VLQ value.")))
+      (let [digit (rand-int 10)]
+        (let [i (inc i)
+              continuation? (pos? (bit-and digit vlq-continuation-bit))
+              digit (bit-and digit vlq-base-mask)
+              result (+ result (bit-shift-left digit shift))
+              shift (long (+ shift vlq-base-shift))]
+          (if continuation?
+            (recur i result shift)
+            (lazy-seq
+             (cons (from-vlq-signed result)
+                   (let [s (.substring s i)]
+                     (when-not (string/blank? s)
+                       (decode s)))))))))))

cases/testcases/performance/red/case.clj

@@ -0,0 +1,5 @@
+(ns testcases.performance.red.case)
+
+(defn foo [x]
+  (case x
+    1 :one))

cases/testcases/performance/red/hash.clj

@@ -0,0 +1,3 @@
+(ns testcases.performance.red.hash)
+
+(case 1 1 :long 9223372039002259457N :big 2)

cases/testcases/performance/red/recur.clj

@@ -0,0 +1,39 @@
+(ns testcases.performance.red.recur
+  (:require
+   [clojure.string :as string]))
+
+(def ^:const vlq-base-shift 5)
+(def ^:const vlq-base (bit-shift-left 1 vlq-base-shift))
+(def ^:const vlq-base-mask (dec vlq-base))
+(def ^:const vlq-continuation-bit vlq-base)
+
+(defn to-vlq-signed [v]
+  (if (neg? v)
+    (inc (bit-shift-left (- v) 1))
+    (+ (bit-shift-left v 1) 0)))
+
+(defn from-vlq-signed [v]
+  (let [neg? (= (bit-and v 1) 1)
+        shifted (bit-shift-right v 1)]
+    (if neg?
+      (- shifted)
+      shifted)))
+
+(defn decode [^String s]
+  (let [l (.length s)]
+    (loop [i 0 result 0 shift 0]
+      (when (>= i l)
+        (throw (Error. "Expected more digits in base 64 VLQ value.")))
+      (let [digit (rand-int 10)]
+        (let [i (inc i)
+              continuation? (pos? (bit-and digit vlq-continuation-bit))
+              digit (bit-and digit vlq-base-mask)
+              result (+ result (bit-shift-left digit shift))
+              shift (+ shift vlq-base-shift)]
+          (if continuation?
+            (recur i result shift)
+            (lazy-seq
+             (cons (from-vlq-signed result)
+                   (let [s (.substring s i)]
+                     (when-not (string/blank? s)
+                       (decode s)))))))))))

changes.md

@@ -1,3 +1,12 @@
+## Changes from 0.9.7 to 1.0.0
+
+#### New
+
+* Implement a new linter: `:performance`
+  * Note that it's disabled by default. There are no drawbacks to enabling it (other than the burden of having to fix or silence these).
+  * Closes https://github.com/jonase/eastwood/issues/416
+  * [Documentation](https://github.com/jonase/eastwood#performance)
+
 ## Changes from 0.9.6 to 0.9.7
 
 #### New

src/eastwood/analyze_ns.clj

@@ -244,9 +244,40 @@
 
 (defn- replace-path-in-compiler-error
   [msg cwd]
-  (let [[match pre _ ^String path
-         line-col post] (re-matches #"((Reflection|Boxed math) warning), (.*?)(:\d+:\d+)(.*)"
-                                    msg)
+  (let [[linter
+         kind
+         [match
+          pre
+          _
+          ^String path
+          line-col
+          post]] (or (when-let [[^String s p :as m]
+                                (re-matches #"((Reflection|Performance|Boxed math) warning), (.*?)(:\d+:\d+)(.*)"
+                                            msg)]
+                       (let [l (case p
+                                 "Boxed math warning" :boxed-math
+                                 "Performance warning" :performance
+                                 "Reflection warning" :reflection)]
+
+                         [l
+                          (when (= :performance l)
+                            (cond
+                              (-> s (.contains "case has int tests, but tested expression"))
+                              :case
+
+                              (-> s (.contains "hash collision of some case test constants"))
+                              :hash))
+                          m]))
+                     (when-let [[m f l c x y] (re-matches #"(.*?)(:\d+)(:\d+)?( recur arg for primitive local)(.*)"
+                                                          msg)]
+                       [:performance
+                        :recur
+                        [m
+                         "Performance warning"
+                         :_
+                         f
+                         (str l (or c ":1"))
+                         (str x y)]]))
         [line column] (some->> (some-> line-col (string/replace #"^:" "") (string/split #":"))
                                (mapv (fn [s]
                                        (Long/parseLong s))))
@@ -255,41 +286,48 @@
                    ;; The Clojure compiler can report files that originally are .cljc as .clj,
                    ;; which would hinder our `:reflection` linter:
                    (str path "c"))
+        ;; For recur warnings, the clojure compiler can print opaque filenames.
+        ;; For those, we fall back:
+        recur-path (when (= kind :recur)
+                     *file*)
         url (and match
                  (or (io/resource path)
-                     (some-> alt-path io/resource)))
+                     (some-> alt-path io/resource)
+                     (some-> recur-path io/resource)))
         uri (some-> url
                     (util/file-warn-info cwd)
                     :uri-or-file-name)]
     (if uri
-      {:type (if (re-find #"Boxed math warning" msg)
-               :boxed-math
-               :reflection)
+      {:type linter
+       :kind kind
        :pre pre
        :uri uri
        :line line
        :column column
        :post post}
-      (when-not (re-find #"not declared dynamic and thus is not dynamically rebindable, but its name suggests otherwise" msg)
+      (when-not (or (re-find #"not declared dynamic and thus is not dynamically rebindable, but its name suggests otherwise" msg)
+                    (re-find #"Auto-boxing loop arg" msg))
         msg))))
 
 (defn- do-eval-output-callbacks [out-msgs-str err-msgs-str cwd]
   (when-not (= out-msgs-str "")
     (doseq [s (string/split-lines out-msgs-str)]
       (println s)))
   (let [reflection-warnings (atom [])
-        boxed-math-warnings (atom [])]
+        boxed-math-warnings (atom [])
+        performance-warnings (atom [])]
     (when-not (= err-msgs-str "")
       (doseq [s (string/split-lines err-msgs-str)
               :let [v (replace-path-in-compiler-error s cwd)]]
         (if (map? v)
           (swap! (case (:type v)
                    :reflection reflection-warnings
-                   :boxed-math boxed-math-warnings)
+                   :boxed-math boxed-math-warnings
+                   :performance performance-warnings)
                  conj
                  v)
           (some-> v println))))
-    [@reflection-warnings @boxed-math-warnings]))
+    [@reflection-warnings @boxed-math-warnings @performance-warnings]))
 
 (defn cleanup [form]
   (let [should-change? (and (-> form list?)
@@ -369,7 +407,8 @@
   [source-path & {:keys [reader opt]}]
   (let [linting-boxed-math? (:eastwood/linting-boxed-math? opt)
         eof (reify)
-        reader-opts {:read-cond :allow :features #{:clj} :eof eof}]
+        reader-opts {:read-cond :allow :features #{:clj} :eof eof}
+        source-path-str (str source-path)]
     (before-analyze-file-debug source-path opt)
 
     ;; prevent t.ana from possibly altering *ns* or such:
@@ -378,7 +417,7 @@
               *compile-path* *compile-path*
               *data-readers* *data-readers*
               *default-data-reader-fn* *default-data-reader-fn*
-              *file* (str source-path)
+              *file* source-path-str
               *math-context* *math-context*
               *ns* *ns*
               *print-length* *print-length*
@@ -392,17 +431,19 @@
         (loop [forms []
                asts []
                reflection-warnings-plural []
-               boxed-math-warnings-plural []]
+               boxed-math-warnings-plural []
+               performance-warnings-plural []]
           (let [form (cleanup (reader/read reader-opts reader))]
             (if (identical? form eof)
               {:forms forms
                :asts asts
                :exception nil
                :reflection-warnings (distinct reflection-warnings-plural)
-               :boxed-math-warnings (distinct boxed-math-warnings-plural)}
+               :boxed-math-warnings (distinct boxed-math-warnings-plural)
+               :performance-warnings (distinct performance-warnings-plural)}
               (let [cur-env (env/deref-env)
                     _ (pre-analyze-debug asts form cur-env *ns* opt)
-                    [exc ast reflection-warnings boxed-math-warnings]
+                    [exc ast reflection-warnings boxed-math-warnings performance-warnings]
                     (try
                       (let [skip? (skip-form? form)
                             {:keys [val out err]}
@@ -437,10 +478,12 @@
                                                                                   last)))
                                                                         (list))))))
                                     v))))
-                            [reflection-warnings boxed-math-warnings] (if skip?
-                                                                        [[] []]
-                                                                        (do-eval-output-callbacks out err (:cwd opt)))]
-                        [nil val reflection-warnings boxed-math-warnings])
+                            [reflection-warnings boxed-math-warnings performance-warnings]
+                            (if skip?
+                              [[] [] []]
+                              (binding [*file* source-path-str]
+                                (do-eval-output-callbacks out err (:cwd opt))))]
+                        [nil val reflection-warnings boxed-math-warnings performance-warnings])
                       (catch Exception e
                         (let [had-go-call? (atom false)]
                           (when-not (:abort-on-core-async-exceptions? opt)
@@ -495,12 +538,13 @@
                              (cond-> reflection-warnings-plural
                                conj? (into reflection-warnings))
                              (cond-> boxed-math-warnings-plural
-                               conj? (into boxed-math-warnings))))))))))))))
+                               conj? (into boxed-math-warnings))
+                             (cond-> performance-warnings-plural
+                               conj? (into performance-warnings))))))))))))))
 
 (defn analyze-ns
-  "Takes an IndexingReader and a namespace symbol.
-  Returns a map of results of analyzing the namespace. The map
-  contains these keys:
+  "Returns a map of results of analyzing the namespace.
+  The map contains these keys:
 
   :analyze-results - The value associated with this key is itself a
       map with the following keys:
@@ -514,16 +558,15 @@
   Options:
   - :reader  a pushback reader to use to read the namespace forms
   - :opt     a map of analyzer options
-    - same as analyze-file. See there.
-
-  eg. (analyze-ns 'my-ns :opt {} :reader (pb-reader-for-ns 'my.ns))"
+    - same as analyze-file."
   [source-nsym & {:keys [reader opt]
                   :or {reader (pb-reader-for-ns source-nsym)}}]
   (with-form-writers opt
     (fn []
       (let [source-path (#'move/ns-file-name source-nsym)
             {:keys [reflection-warnings
-                    boxed-math-warnings]
+                    boxed-math-warnings
+                    performance-warnings]
              :as m}     (analyze-file source-path :reader reader :opt opt)
             source      (-> source-nsym uri-for-ns slurp)]
         (-> m
@@ -534,6 +577,7 @@
                                      :forms         (:forms m)
                                      :reflection-warnings reflection-warnings
                                      :boxed-math-warnings boxed-math-warnings
+                                     :performance-warnings performance-warnings
                                      :asts          (->> m
                                                          :asts
                                                          (mapv (fn [m]

src/eastwood/lint.clj

@@ -19,6 +19,7 @@
    [eastwood.linters.deprecated :as deprecated]
    [eastwood.linters.implicit-dependencies :as implicit-dependencies]
    [eastwood.linters.misc :as misc]
+   [eastwood.linters.performance :as performance]
    [eastwood.linters.reflection :as reflection]
    [eastwood.linters.typetags :as typetags]
    [eastwood.linters.typos :as typos]
@@ -201,6 +202,13 @@
     ;; since reflection detection doesn't work at tools.analyzer level, so one doesn't get to inspect macroexpasions for this purpose.
     :fn reflection/linter}
 
+   {:name :performance
+    :enabled-by-default false
+    :url "https://github.com/jonase/eastwood#performance",
+    ;; NOTE :ignore-faults-from-foreign-macroexpansions? is useless for this specific linter,
+    ;; since reflection detection doesn't work at tools.analyzer level, so one doesn't get to inspect macroexpasions for this purpose.
+    :fn performance/linter}
+
    {:name :boxed-math
     ;; Disabled by default as it's not customary or excessively justified to always fix these:
     :enabled-by-default false,

src/eastwood/linters/performance.clj

@@ -0,0 +1,16 @@
+(ns eastwood.linters.performance
+  (:require
+   [clojure.string :as string]))
+
+(defn linter [{:keys [performance-warnings]} _]
+  (->> performance-warnings
+       (map (fn [{:keys [post uri line column kind]}]
+              {:linter           :performance
+               :kind             kind
+               :uri-or-file-name uri
+               :loc              {:line   line
+                                  :column column
+                                  :file   uri}
+               :msg              (-> post
+                                     (string/replace #"^ - " "")
+                                     (string/replace #"^  " " "))}))))