diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ba4d68bac..3f6107b610 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ For a list of breaking changes, check [here](#breaking-changes). ## Unreleased +- [#2159](https://github.com/clj-kondo/clj-kondo/issues/2159): New linter, `:invalid-fn-name` - [#2207](https://github.com/clj-kondo/clj-kondo/issues/2207): New `:condition-always-true` linter, see [docs](doc/linters.md) - [#2013](https://github.com/clj-kondo/clj-kondo/issues/2013): Fix NPE and similar errors when linting an import with an illegal token diff --git a/doc/linters.md b/doc/linters.md index c4ef1e5965..24cac1a20f 100644 --- a/doc/linters.md +++ b/doc/linters.md @@ -47,6 +47,7 @@ configuration. For general configurations options, go [here](config.md). - [Format](#format) - [Def + fn instead of defn](#def--fn-instead-of-defn) - [Inline def](#inline-def) + - [Invalid fn name](#invalid-fn-name) - [Invalid arity](#invalid-arity) - [Conflicting arity](#conflicting-arity) - [Reduce without initial value](#reduce-without-initial-value) @@ -845,6 +846,21 @@ See [issue](https://github.com/clj-kondo/clj-kondo/issues/1920). *Example message:* `inline def`. +### Invalid fn name + +**Keyword:** `:invalid-fn-name`. + +*Description:* warn when a function's name is not valid. If present, it should +be an unquoted symbol. + +*Default level:* `:error`. + +*Example trigger:* `(fn :fn-name [x] (inc x))`. + +*Example message:* `First arg of fn should be a symbol, arg vector or body list`. + +*Config:* + ### Invalid arity **Keyword:** `:invalid-arity`. diff --git a/src/clj_kondo/impl/analyzer.clj b/src/clj_kondo/impl/analyzer.clj index a03a7c1848..219acd6692 100644 --- a/src/clj_kondo/impl/analyzer.clj +++ b/src/clj_kondo/impl/analyzer.clj @@ -934,6 +934,21 @@ (defn- let? [x] (one-of x [[clojure.core let] [cljs.core let]])) +(defn- invalid-fn-name? [?name-expr] + (let [valid? (some-fn utils/list-node? ; fn body (usually 1 of many) + utils/vector-node? ; fn args + utils/symbol-token?)] ; fn name + (not (valid? ?name-expr)))) + +(defn- reg-invalid-fn-name! [ctx expr filename] + (findings/reg-finding! + ctx + (node->line + filename + expr + :invalid-fn-name + "First arg of fn should be a symbol, arg vector or body list"))) + (defn- def-fn? [{:keys [callstack]}] (let [[_ parent extra-parent] callstack] (or (def? parent) @@ -985,6 +1000,10 @@ (when (and (not (linter-disabled? ctx :def-fn)) (def-fn? ctx)) (reg-def-fn! ctx expr filename)) + (when (and (not (linter-disabled? ctx :valid-fn-name)) + ?name-expr + (invalid-fn-name? ?name-expr)) + (reg-invalid-fn-name! ctx expr filename)) (with-meta parsed-bodies (when arities (cond-> {:arity {:fixed-arities fixed-arities diff --git a/src/clj_kondo/impl/config.clj b/src/clj_kondo/impl/config.clj index 91bb9d8e44..8b45fd9640 100644 --- a/src/clj_kondo/impl/config.clj +++ b/src/clj_kondo/impl/config.clj @@ -25,6 +25,7 @@ :private-call {:level :error} :inline-def {:level :warning} :def-fn {:level :off} + :invalid-fn-name {:level :error} :redundant-do {:level :warning} :redundant-let {:level :warning} :cond-else {:level :warning} diff --git a/src/clj_kondo/impl/macroexpand.clj b/src/clj_kondo/impl/macroexpand.clj index a38eb5e861..08f337ef45 100644 --- a/src/clj_kondo/impl/macroexpand.clj +++ b/src/clj_kondo/impl/macroexpand.clj @@ -12,6 +12,56 @@ x (if m* (assoc x :meta m*) x)] (with-meta x m))) +(defn find-children + "Recursively filters children by pred" + [pred children] + (mapcat #(if (pred %) + [(pred %)] + (when-let [cchildren (:children %)] + (find-children pred cchildren))) + children)) + +(defn fn-args [children] + (let [args (find-children + #(and (= :token (tag %)) + (:string-value %) + (when-let [[_ n] (re-matches #"%((\d?\d?)|&)" (:string-value %))] + (case n + "" 1 + "&" 0 + (Integer/parseInt n)))) + children) + args (sort args) + varargs? (when-let [fst (first args)] + (zero? fst)) + args (seq (if varargs? (rest args) args)) + max-n (last args) + args (when args (map (fn [i] + (symbol (str "%" i))) + (range 1 (inc max-n))))] + {:varargs? varargs? + :args args})) + +(defn expand-fn [{:keys [:children] :as expr}] + (let [{:keys [:row :col] :as m} (meta expr) + {:keys [:args :varargs?]} (fn-args children) + fn-body (with-meta (list-node children) + (assoc m + :row row + :col (inc col))) + arg-list (vector-node + (map #(with-meta (token-node %) + {:clj-kondo/mark-used true + :clj-kondo/skip-reg-binding true}) + (if varargs? + (concat args '[& %&]) + args))) + has-first-arg? (= '%1 (first args))] + (with-meta + (list-node [(token-node 'fn*) arg-list + fn-body]) + (assoc m :clj-kondo.impl/fn-has-first-arg has-first-arg?)))) + (defn expand-> [_ctx expr] (let [expr expr children (:children expr) @@ -19,12 +69,20 @@ ret (loop [x c, forms cforms] (if forms (let [form (first forms) - threaded (if (= :list (tag form)) - (with-meta-of - (list-node (list* (first (:children form)) - x - (next (:children form)))) - form) + threaded (case (tag form) + :list (with-meta-of + (list-node (list* (first (:children form)) + x + (next (:children form)))) + form) + ;; Short-form fns need expanding now, to thread the name in + :fn (with-meta-of + (let [{:keys [children] :as expanded} (expand-fn form)] + (assoc expanded :children + (list* (first children) + x + (next children)))) + form) (with-meta-of (list-node (list form x)) form))] (recur threaded (next forms))) @@ -130,56 +188,6 @@ (recur (cons node more) ) node)))) -(defn find-children - "Recursively filters children by pred" - [pred children] - (mapcat #(if (pred %) - [(pred %)] - (when-let [cchildren (:children %)] - (find-children pred cchildren))) - children)) - -(defn fn-args [children] - (let [args (find-children - #(and (= :token (tag %)) - (:string-value %) - (when-let [[_ n] (re-matches #"%((\d?\d?)|&)" (:string-value %))] - (case n - "" 1 - "&" 0 - (Integer/parseInt n)))) - children) - args (sort args) - varargs? (when-let [fst (first args)] - (zero? fst)) - args (seq (if varargs? (rest args) args)) - max-n (last args) - args (when args (map (fn [i] - (symbol (str "%" i))) - (range 1 (inc max-n))))] - {:varargs? varargs? - :args args})) - -(defn expand-fn [{:keys [:children] :as expr}] - (let [{:keys [:row :col] :as m} (meta expr) - {:keys [:args :varargs?]} (fn-args children) - fn-body (with-meta (list-node children) - (assoc m - :row row - :col (inc col))) - arg-list (vector-node - (map #(with-meta (token-node %) - {:clj-kondo/mark-used true - :clj-kondo/skip-reg-binding true}) - (if varargs? - (concat args '[& %&]) - args))) - has-first-arg? (= '%1 (first args))] - (with-meta - (list-node [(token-node 'fn*) arg-list - fn-body]) - (assoc m :clj-kondo.impl/fn-has-first-arg has-first-arg?)))) - (defn expand-do-template [_ctx node] (let [[_ argv expr & values] (:children node) c (count (:children argv)) diff --git a/src/clj_kondo/impl/utils.clj b/src/clj_kondo/impl/utils.clj index e320891297..5a9656831f 100644 --- a/src/clj_kondo/impl/utils.clj +++ b/src/clj_kondo/impl/utils.clj @@ -38,6 +38,10 @@ (and (instance? clj_kondo.impl.rewrite_clj.node.seq.SeqNode n) (identical? :set (tag n)))) +(defn vector-node? [n] + (and (instance? clj_kondo.impl.rewrite_clj.node.seq.SeqNode n) + (identical? :vector (tag n)))) + ;;; end export (defn print-err! [& strs] diff --git a/test/clj_kondo/main_test.clj b/test/clj_kondo/main_test.clj index 92183ad43d..8516773e88 100644 --- a/test/clj_kondo/main_test.clj +++ b/test/clj_kondo/main_test.clj @@ -9,7 +9,7 @@ [clojure.edn :as edn] [clojure.java.io :as io] [clojure.string :as str] - [clojure.test :as t :refer [deftest is testing]] + [clojure.test :as t :refer [deftest is are testing]] [missing.test.assertions])) (deftest self-lint-test @@ -59,6 +59,29 @@ (is (empty? (lint! "(def x (reify Object (toString [_] \"x\")))" "--lang" (name lang) "--config" (pr-str config)))) (is (empty? (lint! "(require '[some.ns :refer [my-reify]]) (def x (my-reify Object (toString [_] \"x\")))" "--lang" (name lang) "--config" (pr-str config))))))) +(deftest invalid-fn-name-test + (assert-submaps + '({:row 1, :col 1, :level :error, :message "First arg of fn should be a symbol, arg vector or body list"}) + (lint! "(fn \"fn-name\" [x] (inc x))")) + (assert-submaps + '({:row 1, :col 6, :level :error, :message "First arg of fn should be a symbol, arg vector or body list"}) + (lint! "(map (fn 'symbol ([x] (inc x))) coll)")) + (assert-submaps + '({:row 1, :col 7, :level :error, :message "First arg of fn should be a symbol, arg vector or body list"}) + (lint! "(-> 7 (fn [x] (inc x)))")) + (assert-submaps + '({:row 1, :col 7, :level :error, :message "First arg of fn should be a symbol, arg vector or body list"}) + (lint! "(-> 7 #(inc %))")) + (assert-submaps + '({:row 1, :col 1, :level :error, :message "First arg of fn should be a symbol, arg vector or body list"}) + (lint! "(fn* :fn-name [x] (inc x))")) + + (are [lint-form] (empty? lint-form) + (lint! "(fn fn-name [x] (inc x))") + (lint! "(fn* fn-name [x] (inc x))") + ;; A future linter could warn here, as it's almost never intentional, especially if fn-name is bound + (lint! "(-> fn-name #(inc %))"))) + (deftest redundant-let-test (let [linted (lint! (io/file "corpus" "redundant_let.clj")) row-col-files (map #(select-keys % [:row :col :file])