From 7d75424cdf6bdff35a025728058e1e9b0772bfc5 Mon Sep 17 00:00:00 2001 From: Tom Dalziel Date: Mon, 9 Dec 2024 00:58:30 +0100 Subject: [PATCH] Add if-nil-return linter --- src/clj_kondo/impl/config.clj | 1 + src/clj_kondo/impl/linters.clj | 33 +++++++++++++++++++++++-- test/clj_kondo/main_test.clj | 42 ++++++++++++++++++++++++++++++++ test/clj_kondo/re_frame_test.clj | 3 ++- test/clj_kondo/recur_test.clj | 5 ++-- 5 files changed, 78 insertions(+), 6 deletions(-) diff --git a/src/clj_kondo/impl/config.clj b/src/clj_kondo/impl/config.clj index 017f101daa..39b1941318 100644 --- a/src/clj_kondo/impl/config.clj +++ b/src/clj_kondo/impl/config.clj @@ -95,6 +95,7 @@ :exclude #{}} :use {:level :warning} :missing-else-branch {:level :warning} + :if-nil-return {:level :off} :case-duplicate-test {:level :error} :case-quoted-test {:level :warning} :case-symbol-test {:level :off} diff --git a/src/clj_kondo/impl/linters.clj b/src/clj_kondo/impl/linters.clj index 025046cc7c..070611d9e8 100644 --- a/src/clj_kondo/impl/linters.clj +++ b/src/clj_kondo/impl/linters.clj @@ -113,6 +113,33 @@ (node->line (:filename ctx) expr :missing-else-branch (format "Missing else branch.")))))))) +(defn lint-if-nil-return + "Lint returning nil from if-like expressions. When-like expressions are + preferred." + [ctx expr] + (let [config (:config ctx) + level (-> config :linters :if-nil-return :level)] + (when-not (identical? :off level) + (let [children (:children expr) + [_condition then-branch else-branch] (rest children)] + (cond (= [:value nil] (find else-branch :value)) + (when-let [preferred (get '{if when + if-let when-let + if-some when-some + if-not when-not} + (:value (first children)))] + (findings/reg-finding! ctx + (node->line (:filename ctx) expr :if-nil-return + (format "For nil return, prefer %s." preferred)))) + (= [:value nil] (find then-branch :value)) + ;; We don't check if-let and if-some here as there'd be an unused binding warning anyway + (when-let [preferred (get '{if when-not + if-not when} + (:value (first children)))] + (findings/reg-finding! ctx + (node->line (:filename ctx) expr :if-nil-return + (format "For nil return, prefer %s." preferred))))))))) + (defn lint-single-key-in [ctx called-name call] (when-not (utils/linter-disabled? ctx :single-key-in) (let [[_ _ keyvec] (:children call)] @@ -133,7 +160,8 @@ (lint-cond ctx (:expr call)) ([clojure.core if-let] [clojure.core if-not] [clojure.core if-some] [cljs.core if-let] [cljs.core if-not] [cljs.core if-some]) - (lint-missing-else-branch ctx (:expr call)) + (do (lint-missing-else-branch ctx (:expr call)) + (lint-if-nil-return ctx (:expr call))) ([clojure.core get-in] [clojure.core assoc-in] [clojure.core update-in] [cljs.core get-in] [cljs.core assoc-in] [cljs.core update-in]) (lint-single-key-in ctx called-name (:expr call)) @@ -143,7 +171,8 @@ ;; special forms which are not fns (when (= 'if (:name call)) - (lint-missing-else-branch ctx (:expr call))) + (lint-missing-else-branch ctx (:expr call)) + (lint-if-nil-return ctx (:expr call))) (when (contains? var-info/unused-values (symbol (let [cns (str called-ns)] (if (= "cljs.core" cns) "clojure.core" cns)) diff --git a/test/clj_kondo/main_test.clj b/test/clj_kondo/main_test.clj index 78e653e4eb..f5a1083a2c 100644 --- a/test/clj_kondo/main_test.clj +++ b/test/clj_kondo/main_test.clj @@ -3162,6 +3162,48 @@ foo/baz {:linters {:missing-else-branch {:level :warning}} :config-in-ns '{foo {:linters {:missing-else-branch {:level :off}}}}})))) +(deftest if-nil-return-test + (assert-submaps + [{:file "", + :row 1, + :col 1, + :level :warning, + :message "For nil return, prefer when."} + {:file "", + :row 1, + :col 17, + :level :warning, + :message "For nil return, prefer when-not."} + {:file "", + :row 1, + :col 33, + :level :warning, + :message "For nil return, prefer when."} + {:file "", + :row 1, + :col 53, + :level :warning, + :message "For nil return, prefer when-let."} + {:file "", + :row 1, + :col 74, + :level :warning, + :message "For nil return, prefer when-some."}] + (lint! "(if true 1 nil) (if true nil 1) (if-not true nil 1) (if-let [x 1] x nil) (if-some [x 1] x nil)" + {:linters {:if-nil-return {:level :warning}}})) + (is (empty? (lint! "(if-let [x 7] nil :foo)" + {:linters {:if-nil-return {:level :warning}, :unused-binding {:level :off}}}))) + (is (empty? (lint! "(if true 1 nil) (if true nil 1) (if-not true nil 1) (if-let [x 1] x nil) (if-some [x 1] x nil)" + {:linters {:if-nil-return {:level :off}}}))) + (is (empty? (lint! "(ns foo {:clj-kondo/config '{:linters {:if-nil-return {:level :off}}}}) + (if true 1 nil) (if true nil 1) (if-not true nil 1) (if-let [x 1] x nil) (if-some [x 1] x nil)" + {:linters {:if-nil-return {:level :warning}}}))) + (is (empty? (lint! "#_:clj-kondo/ignore (if true 1 nil)" + {:linters {:if-nil-return {:level :warning}}}))) + (is (empty? (lint! "(ns foo) (if true 1 nil)" + {:linters {:if-nil-return {:level :warning}} + :config-in-ns '{foo {:linters {:if-nil-return {:level :off}}}}})))) + (deftest single-key-in-test (doseq [lang ["clj" "cljs"]] (assert-submaps diff --git a/test/clj_kondo/re_frame_test.clj b/test/clj_kondo/re_frame_test.clj index 6724497414..55bdb7d1f5 100644 --- a/test/clj_kondo/re_frame_test.clj +++ b/test/clj_kondo/re_frame_test.clj @@ -18,7 +18,8 @@ :unused-value {:level :off} :unsorted-required-namespaces {:level :warning} :uninitialized-var {:level :off} - :type-mismatch {:namespaces {cljs.core {throw {:arities {1 {:args [:any]}}}}}}} + :type-mismatch {:namespaces {cljs.core {throw {:arities {1 {:args [:any]}}}}}} + :if-nil-return {:level :off}} :lint-as {day8.re-frame.tracing/fn-traced clojure.core/fn day8.re-frame.tracing/defn-traced clojure.core/defn reagent.core/with-let clojure.core/let diff --git a/test/clj_kondo/recur_test.clj b/test/clj_kondo/recur_test.clj index ed6fca46bd..315c8c0246 100644 --- a/test/clj_kondo/recur_test.clj +++ b/test/clj_kondo/recur_test.clj @@ -31,12 +31,11 @@ (async/alt!! (async/chan) ([x] - (if x + (when x (async/alt!! (async/chan) ([_] - (recur))) - nil)))))" linter-config)))) + (recur))))))))" linter-config)))) (testing "if-some" (is (empty? (lint! "(loop [] (if true