Skip to content

Commit

Permalink
Fix clj-kondo#2096: analysis: :arglists meta applies to :arglist-strs (…
Browse files Browse the repository at this point in the history 
…clj-kondo#2110)

Highlights:
- `:arglists` metadata only overrides `:arglist-strs` if it looks valid
- `:fixed-arities` and `:varargs-min-arity` are never affected by
`:arglists` metadata, they always describe original arg lists params,
if present

Also includes:
- minor analysis doc update & corrections

Closes clj-kondo#2096
  • Loading branch information
@lread
lread authored Jun 16, 2023
1 parent e966b82 commit 74be7d2
Show file tree
Hide file tree
Showing 5 changed files with 227 additions and 47 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ For a list of breaking changes, check [here](#breaking-changes).
- [#2105](https://github.com/clj-kondo/clj-kondo/issues/2105): Consider .cljd files when linting.
- [#2101](https://github.com/clj-kondo/clj-kondo/issues/2101): false positive with `if-some` + `recur`
- [#2109](https://github.com/clj-kondo/clj-kondo/issues/2109): `java.util.List` type hint corresponds to `:list` or nil
- [#2096](https://github.com/clj-kondo/clj-kondo/issues/2096): apply `:arglists` metadata to `:arglist-strs` for analysis data ([@lread](https://github.com/lread))

## 2023.05.26

Expand Down
8 changes: 4 additions & 4 deletions analysis/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Further analysis can be returned by providing `:analysis` with a map of options:

- `:locals`: when truthy return `:locals` and `:local-usages` described below
- `:keywords`: when truthy return `:keywords` described below
- `:arglists`: when truthy return `:arglists` on `:var-definitions`
- `:arglists`: when truthy return `:arglist-strs` on `:var-definitions`
- `:protocol-impls`: when truthy return `:protocol-impls` described below
- `:symbols`: when truthy, return `:symbols` described below
- `:java-class-definitions`: when truthy, return `:java-class-definitions` as described below.
Expand Down Expand Up @@ -106,13 +106,13 @@ The analysis output consists of a map with:
- `:defined-by`: the namespaced symbol which defined this var

Optional:
- `:fixed-arities`: a set of fixed arities
- `:varargs-min-arity`: the minimal number of arguments of a varargs signature
- `:fixed-arities`: a set of fixed arities, unaffected by any `:arglists` metadata overrides
- `:varargs-min-arity`: the minimal number of arguments of a varargs signature, unaffected by any `:arglists` metadata overrides
- common metadata values: `:private`, `:macro`, `:deprecated`, `:doc`, `:added`
- `:meta` map of requested metadata for var
- `:lang`: if definition occurred in a `.cljc` file, the language in which the
definition was done: `:clj` or `:cljs`
- `:arglists-str`: a list of each set of args as written
- `:arglist-strs`: arg lists as written, or valid looking `:arglists` metadata override, ex. `["[x]" "[x y]"]`
- `:protocol-ns`, `:protocol-name`: the protocol namespace and name for a protocol method

- `:var-usages`, a list of maps with:
Expand Down
37 changes: 33 additions & 4 deletions src/clj_kondo/impl/analyzer.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,28 @@
:syntax
"Invalid function body."))))

(defn- meta-arglists-node->strs
"Return arglist-strs for `node` representing arglists metadata value.
Returns nil if node does not match expected quoted list of vectors shape, ex: '([x][x y] ...)."
[node]
(when (and node
(= :quote (tag node))
(= :list (some-> node :children first tag)))
(reduce (fn [acc arg-vec]
(if (= :vector (tag arg-vec))
(conj acc (str arg-vec))
(reduced nil)))
[]
(-> node :children first :children))))

(defn- meta-node->arglist-strs
"Returns arglists-strs for `:arglists` in `node` representing metadata map.
Returns nil if no `:arglists` found, or found `:arglists` looks invalid."
[node]
(some-> node
(utils/map-node-get-value-node :arglists)
meta-arglists-node->strs))

(defn analyze-pre-post-map [ctx expr]
(let [children (:children expr)]
(key-linter/lint-map-keys ctx expr)
Expand Down Expand Up @@ -611,8 +633,7 @@
;; poor naming, this is for type information
arities (extract-arity-info ctx parsed-bodies)
fixed-arities (into #{} (filter number?) (keys arities))
varargs-min-arity (get-in arities [:varargs :min-arity])
arglist-strs (mapv :arglist-str parsed-bodies)]
varargs-min-arity (get-in arities [:varargs :min-arity])]
(when fn-name
(namespace/reg-var!
ctx ns-name fn-name expr
Expand All @@ -626,7 +647,9 @@
:defined-by->lint-as defined-by->lint-as
:fixed-arities (not-empty fixed-arities)
:arglist-strs (when (:analyze-arglists? ctx)
arglist-strs)
(or (meta-node->arglist-strs meta-node2)
(meta-node->arglist-strs meta-node)
(mapv :arglist-str parsed-bodies)))
:arities arities
:varargs-min-arity varargs-min-arity
:doc docstring
Expand Down Expand Up @@ -1190,7 +1213,13 @@
:defined-by defined-by
:defined-by->lint-as defined-by->lint-as
:fixed-arities (:fixed-arities arity)
:arglist-strs (:arglist-strs arity)
:arglist-strs (when (:analyze-arglists? ctx)
;; match Clojure behaviour for var name meta nodes,
;; it will pick first :arglists, e.g.:
;; ^{:arglists '([x])} ^{:arglists '([y])} z
(or (some meta-node->arglist-strs var-name-node-meta-nodes)
(meta-node->arglist-strs extra-meta-node)
(:arglist-strs arity)))
:varargs-min-arity (:varargs-min-arity arity)
:arities (:arities init-meta)
:type type))))
Expand Down
14 changes: 13 additions & 1 deletion src/clj_kondo/impl/utils.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,16 @@
(defn map-node-vals [{:keys [:children]}]
(take-nth 2 (rest children)))

(defn map-node-get-value-node
"Return value node from map node matching given keyword `kw`"
[{:keys [:children]} kw]
(loop [kvs (partition 2 children)]
(let [kv (first kvs)]
(cond
(nil? kv) nil
(= kw (node->keyword (first kv))) (second kv)
:else (recur (rest kvs))))))

(defmacro one-of [x elements]
`(let [x# ~x]
(case x# (~@elements) x# nil)))
Expand Down Expand Up @@ -426,4 +436,6 @@
y\""))
(tag (parse-string "\"xy\""))
)
(map-node-get-value-node (p/parse-string "{:binky 2 :arglists #_ :ha '([a b c]) :boingo 4}")
:arglists)
)
214 changes: 176 additions & 38 deletions test/clj_kondo/analysis_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
[clj-kondo.test-utils :refer [assert-submap assert-submaps assert-submaps2]]
[clojure.edn :as edn]
[clojure.string :as string]
[clojure.test :as t :refer [deftest is testing]]))
[clojure.test :as t :refer [deftest is testing]]
[matcher-combinators.matchers :as m]))

(defn analyze
([input] (analyze input nil))
Expand Down Expand Up @@ -1132,43 +1133,180 @@
var-usages)))

(deftest analysis-arglists-test
(testing "arglist-strs are present on definitions"
(let [{:keys [:var-definitions]}
(analyze "(defn f1 [d] d)
(defn f2 ([e] e) ([f f'] f))
(defprotocol A (f3 [g] \"doc\") (f4 [h] [i i']))
(defrecord A [j k])
(defmacro f5 [l m])
(def f6 (fn [n] n))"
{:config {:analysis {:arglists true}}})]
(assert-submaps
'[{:name f1,
:defined-by clojure.core/defn
:arglist-strs ["[d]"]}
{:name f2,
:defined-by clojure.core/defn
:arglist-strs ["[e]" "[f f']"]}
{:name f3,
:defined-by clojure.core/defprotocol
:arglist-strs ["[g]"]}
{}
{:name f4,
:defined-by clojure.core/defprotocol
:arglist-strs ["[h]" "[i i']"]}
{}
{:name ->A
:defined-by clojure.core/defrecord
:arglist-strs ["[j k]"]}
{:name map->A
:defined-by clojure.core/defrecord
:arglist-strs ["[m]"]}
{:name f5
:defined-by clojure.core/defmacro
:arglist-strs ["[l m]"]}
{:name f6,
:defined-by clojure.core/def
:arglist-strs ["[n]"]}]
var-definitions))))
(doseq [analysis-opts [true {:arglists true}]]
(testing (format "analysis opts: %s" analysis-opts)
(let [{:keys [:var-definitions]}
(analyze "(defn f1 [d] d)
(defn f2 ([e] e) ([f f'] f))
(defprotocol AP (f3 [g] \"doc\") (f4 [h] [i i']))
(defrecord AR [j k])
(defmacro f5 [l m])
(def f6 (fn [n] n))"
{:config {:analysis analysis-opts}})
expected (cond->> '[{:name f1,
:defined-by clojure.core/defn
:arglist-strs ["[d]"]}
{:name f2,
:defined-by clojure.core/defn
:arglist-strs ["[e]" "[f f']"]}
{:name AP}
{:name f3,
:defined-by clojure.core/defprotocol
:arglist-strs ["[g]"]}
{:name f4,
:defined-by clojure.core/defprotocol
:arglist-strs ["[h]" "[i i']"]}
{:name AR}
{:name ->AR
:defined-by clojure.core/defrecord
:arglist-strs ["[j k]"]}
{:name map->AR
:defined-by clojure.core/defrecord
:arglist-strs ["[m]"]}
{:name f5
:defined-by clojure.core/defmacro
:arglist-strs ["[l m]"]}
{:name f6,
:defined-by clojure.core/def
:arglist-strs ["[n]"]}]
(not (:arglists analysis-opts))
(mapv #(assoc % :arglist-strs m/absent)))]
(assert-submaps2 expected var-definitions)))))

(deftest analysis-meta-arglists-test
(doseq [analysis-opts [true
{:arglists true}
{:arglists true :var-definitions {:meta true}}]]
(testing (format "analysis opts: %s" analysis-opts)
(let [{:keys [:var-definitions]}
(analyze "(defn fnone [x])
(defn fattr1 {:arglists '([y])} [x] x)
(defn fattr2 ([x] x) ([y y'] y) {:arglists '([y1] [y2 y3])})
(defn fattr-both
{:arglists '([no] [not me])}
([x] x) ([y y'] y)
{:arglists '([y] [e s])})
(defn fattr-both-last-invalid
{:arglists '([ok] [yes me])}
([x] x) ([y y'] y)
{:arglists '(oops not valid)})
(defn fattr-both-invalid
{:arglists ([not] [quoted])}
([x] x) ([y y'] y)
{:arglists '[(oops not valid)]})
(defn fedn-unfriendly-regex
{:arglists '([{:keys [x] :or {x #\"^foobar.*\"}}])}
[x])
(defn fedn-unfriendly-autoresolve
{:arglists '([{:keys [::foo/bar]}])}
[x])
(defn- pdefn {:arglists '([y])} [x] x)
(defmacro mattr2 ([x]) {:arglists '([y])} )
(defmulti ^{:arglists '([some args])} multi foo)
(def ^{:arglists '([d a])} d y)
(def ^{:arglists :fubar} d-invalid)
(def ^:private ^{:doc \"some docs\"} ^{:foo 3 :arglists '([y & more]) :bar 4} dmetas z)
(def ^{:arglists '([me please])} fn1 (fn [x]))"
{:config {:analysis analysis-opts}})
expected (cond->> [{:name 'fnone
:defined-by 'clojure.core/defn
:arglist-strs ["[x]"]
:meta nil}
{:name 'fattr1
:defined-by 'clojure.core/defn
:arglist-strs ["[y]"]
:meta {:arglists '(quote [[y]])}}
{:name 'fattr2,
:defined-by 'clojure.core/defn
:arglist-strs ["[y1]" "[y2 y3]"]
:meta {:arglists ''([y1] [y2 y3])}}
{:name 'fattr-both,
:defined-by 'clojure.core/defn
:arglist-strs ["[y]" "[e s]"]
:meta {:arglists ''([y] [e s])}}
{:name 'fattr-both-last-invalid,
:defined-by 'clojure.core/defn
:arglist-strs ["[ok]" "[yes me]"]
:meta {:arglists ''(oops not valid)}}
{:name 'fattr-both-invalid
:defined-by 'clojure.core/defn
:arglist-strs ["[x]" "[y y']"]
:meta {:arglists ''[(oops not valid)]}}
{:name 'fedn-unfriendly-regex
:defined-by 'clojure.core/defn
:arglist-strs ["[{:keys [x] :or {x #\"^foobar.*\"}}]"]
;; here we see some rewrite-clj node-isms
:meta {:arglists ''([{:keys [x] :or {x (re-pattern "^foobar.*")}}])}}
{:name 'fedn-unfriendly-autoresolve
:defined-by 'clojure.core/defn
:arglist-strs ["[{:keys [::foo/bar]}]"]
;; some rewrite-clj node-isms here
:meta {:arglists ''([{:keys [:foo/bar]}])}}
{:name 'pdefn
:defined-by 'clojure.core/defn-
:arglist-strs ["[y]"]
:meta {:arglists ''([y])}}
{:name 'mattr2
:defined-by 'clojure.core/defmacro
:arglist-strs ["[y]"]
:meta {:arglists ''([y])}}
{:name 'multi
:defined-by 'clojure.core/defmulti
:arglist-strs ["[some args]"]
:meta {:arglists ''([some args])}}
{:name 'd
:defined-by 'clojure.core/def
:arglist-strs ["[d a]"]
:meta {:arglists ''([d a])}}
{:name 'd-invalid
:defined-by 'clojure.core/def
:arglist-strs m/absent
:meta {:arglists :fubar}}
{:name 'dmetas
:defined-by 'clojure.core/def
:private true
:doc "some docs"
:arglist-strs ["[y & more]"]
:meta {:arglists ''([y & more])}}
{:name 'fn1
:defined-by 'clojure.core/def
:arglist-strs ["[me please]"]
:meta {:arglists ''([me please])}}]
(not (:arglists analysis-opts))
(mapv #(assoc % :arglist-strs m/absent))
(not (-> analysis-opts :var-definitions :meta))
(mapv #(assoc % :meta m/absent)))]
(assert-submaps2 expected var-definitions)))))

(deftest arglists-meta-does-not-affect-arities-test
;; arglist meta overrides are considered docs, they should not affect call fn call arities
(doseq [override ["{:arglists '([])}"
"{:arglists '([x & args])}"
"{:arglists '([x] [x y])}"]]
(testing (format "override: %s" override)
(assert-submaps2
[{:name 'one
:fixed-arities #{0}
:varargs-min-arity m/absent}
{:name 'two
:fixed-arities #{1 2}
:varargs-min-arity m/absent}
{:name 'vari0
:fixed-arities m/absent
:varargs-min-arity 0}
{:name 'vari1
:fixed-arities m/absent
:varargs-min-arity 1}
{:name 'fn1
:fixed-arities #{3}
:var-arg-min-arity m/absent}]
(-> (analyze (str (format "(defn one %s [])" override)
(format "(defn two %s ([a]) ([b c]))" override)
(format "(defn vari0 %s [& args])" override)
(format "(defn vari1 %s [a & args])" override)
(format "(def ^%s fn1 (fn [x y x]))" override))
{:config {:analysis {:arglists true}}})
:var-definitions)))))

(deftest analysis-is-valid-edn-test
(testing "solution for GH-476, CLJS with string require"
Expand Down

0 comments on commit 74be7d2

Please sign in to comment.