Add lint/update-with-swap

Closes #30

Author: NoahTheDuke

CHANGELOG.md

@@ -4,6 +4,10 @@ This changelog is loose. Versions are not semantic, they are incremental. Splint
 
 ## Unreleased
 
+### New Rules
+
+- `lint/update-with-swap`: Prefer `(swap! (:counter state) + 5)` over `(update state :counter swap! + 5)`. (See [#30](https://github.com/NoahTheDuke/splint/issues/30).)
+
 ### Changed
 
 - Narrow `lint/existing-constant` to only `clj`, and expand it to cover clojure 1.10 (by using `java.lang.Math/PI` and `java.lang.Math/E`).

resources/config/default.edn

@@ -301,6 +301,13 @@
   :added "1.11"
   :updated "1.11"}
 
+ lint/update-with-swap
+ {:description "Don't use `swap!` in `update` or `update-in` calls."
+  :enabled true
+  :safe false
+  :added "<<next>>"
+  :updated "<<next>>"}
+
  lint/warn-on-reflection
  {:description "Always set *warn-on-reflection* to avoid reflection in interop."
   :enabled false

src/noahtheduke/splint.clj

@@ -60,6 +60,7 @@
    noahtheduke.splint.rules.lint.thread-macro-one-arg
    noahtheduke.splint.rules.lint.try-splicing
    noahtheduke.splint.rules.lint.underscore-in-namespace
+   noahtheduke.splint.rules.lint.update-with-swap
    noahtheduke.splint.rules.lint.warn-on-reflection
 
    noahtheduke.splint.rules.metrics.fn-length

src/noahtheduke/splint/rules/lint/update_with_swap.clj

@@ -0,0 +1,37 @@
+; This Source Code Form is subject to the terms of the Mozilla Public
+; License, v. 2.0. If a copy of the MPL was not distributed with this
+; file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+(ns ^:no-doc noahtheduke.splint.rules.lint.update-with-swap
+  (:require
+   [noahtheduke.splint.diagnostic :refer [->diagnostic]]
+   [noahtheduke.splint.rules :refer [defrule]]
+   [noahtheduke.splint.clojure-ext.core :refer [->list]]))
+
+(set! *warn-on-reflection* true)
+
+(defrule lint/update-with-swap
+  "If an atom is stored inside if a map, the atom can be changed using `swap!` within an `update` or `update-in` call. However, `swap!` returns the new value, not the atom itself, so the container map will hold the deref'ed value of the atom, not the original atom. If the result of the `update` call is stored/used, this can lead to bugs.
+
+  Additionally, if the return value of the `update` call is ignored, then the `update` form will work as expected (because the return value won't overwrite the existing map and the atom will be updated in place). This should be avoided as it breaks expectations about the value of values and normal behavior.
+
+  @safety
+  If the `update` call's return value isn't ignored (it's used in an assignment or passed to another call), switching to `swap!` will break the expected return value. Care must be exercised when switching.
+
+  @examples
+
+  ; avoid
+  (update state :counter swap! + 5)
+
+  ; prefer
+  (swap! (:counter state) + 5)
+  "
+  {:pattern '((?| update [update update-in]) ?map ?key swap! ?+args)
+   :autocorrect false
+   :message "swap! in update derefs the value in the map."
+   :on-match (fn [ctx rule form {:syms [?update ?map ?key ?args]}]
+               (let [getter (if (= 'update ?update)
+                              (list ?key ?map)
+                              (list 'get-in ?map ?key))
+                     new-form (->list (list* 'swap! getter ?args))]
+                 (->diagnostic ctx rule form {:replace-form new-form})))})

src/noahtheduke/splint/runner.clj

@@ -100,7 +100,8 @@
   diagnostic and store it in `ctx`."
   [ctx rule-names form]
   (when-let [diagnostics (check-form ctx rule-names form)]
-    (update ctx :diagnostics swap! into diagnostics)))
+    (swap! (:diagnostics ctx) into diagnostics)
+    nil))
 
 (defn update-rules [rules-map form]
   (if-let [disabled-rules (some-> form meta :splint/disable)]
@@ -213,11 +214,13 @@
                        (assoc :form-meta {:line (:line data)
                                           :column (:column data)}))
                 diagnostic (runner-error->diagnostic ex data)]
-            (update ctx :diagnostics swap! conj diagnostic))
+            (swap! (:diagnostics ctx) conj diagnostic)
+            nil)
           (let [diagnostic (runner-error->diagnostic
                              ex {:error-name 'splint/unknown-error
                                  :filename file})]
-            (update ctx :diagnostics swap! conj diagnostic)))))))
+            (swap! (:diagnostics ctx) conj diagnostic)
+            nil))))))
 
 (defn slurp-file [file-obj]
   (if (:contents file-obj)

src/noahtheduke/splint/runners/autocorrect.cljc

@@ -185,12 +185,12 @@
                                        zloc))))
                        zloc))))
              (catch Exception ex
-               (update ctx :diagnostics swap! conj
-                       (run/runner-error->diagnostic
-                        ex {:error-name 'splint/error
-                            :form zloc
-                            :rule-name (:full-name rule)
-                            :filename (:filename ctx)}))
+               (swap! (:diagnostics ctx) conj
+                 (run/runner-error->diagnostic
+                   ex {:error-name 'splint/error
+                       :form zloc
+                       :rule-name (:full-name rule)
+                       :filename (:filename ctx)}))
                zloc))
            zloc)))
      zloc
@@ -280,11 +280,13 @@
                            (assoc :form-meta {:line (:line data)
                                               :column (:column data)}))
                   diagnostic (run/runner-error->diagnostic ex data)]
-              (update ctx :diagnostics swap! conj diagnostic))
+              (swap! (:diagnostics ctx) conj diagnostic)
+              nil)
             (let [diagnostic (run/runner-error->diagnostic
                               ex {:error-name 'splint/unknown-error
                                   :filename file})]
-              (update ctx :diagnostics swap! conj diagnostic))))))))
+              (swap! (:diagnostics ctx) conj diagnostic)
+              nil)))))))
 
 (comment
   (let [config {:clojure-version *clojure-version*

test/noahtheduke/splint/rules/lint/update_with_swap_test.clj

@@ -0,0 +1,43 @@
+; This Source Code Form is subject to the terms of the Mozilla Public
+; License, v. 2.0. If a copy of the MPL was not distributed with this
+; file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+(ns noahtheduke.splint.rules.lint.update-with-swap-test
+  (:require
+   [lazytest.core :refer [defdescribe it]]
+   [noahtheduke.splint.test-helpers :refer [expect-match single-rule-config]]))
+
+(set! *warn-on-reflection* true)
+
+(def rule-name 'lint/update-with-swap)
+
+(defdescribe update-with-swap-test
+  (it "works with update"
+    (expect-match
+      [{:rule-name rule-name
+        :form '(update state :counter swap! + 5)
+        :message "swap! in update derefs the value in the map."
+        :alt '(swap! (:counter state) + 5)}]
+      "(update state :counter swap! + 5)"
+      (single-rule-config rule-name)))
+  (it "works with update-in"
+    (expect-match
+      [{:rule-name rule-name
+        :form '(update-in state [:users :counter] swap! + 5)
+        :message "swap! in update derefs the value in the map."
+        :alt '(swap! (get-in state [:users :counter]) + 5)}]
+      "(update-in state [:users :counter] swap! + 5)"
+      (single-rule-config rule-name)))
+  (it "expects at least 1 arg to swap!"
+    (expect-match
+      nil
+      "(update state :counter swap!)"
+      (single-rule-config rule-name)))
+  (it "doesn't care about the args to swap!"
+    (expect-match
+      [{:rule-name rule-name
+        :form '(update state :counter swap! (fn [foo] foo))
+        :message "swap! in update derefs the value in the map."
+        :alt '(swap! (:counter state) (fn [foo] foo))}]
+      "(update state :counter swap! (fn [foo] foo))"
+      (single-rule-config rule-name))))