From a0b65cce13e01871747308dcd37d4c136d0e06d0 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Fri, 9 Sep 2022 23:33:04 +0000 Subject: [PATCH 1/6] Support validation `defmethod` arglist counts (#59) --- .clj-kondo/config.edn | 3 +- src/methodical/macros.clj | 135 +++++++++++-- src/methodical/macros/validate_arities.clj | 112 +++++++++++ .../macros/validate_arities_test.clj | 127 +++++++++++++ test/methodical/macros_test.clj | 177 ++++++++++++++++-- 5 files changed, 514 insertions(+), 40 deletions(-) create mode 100644 src/methodical/macros/validate_arities.clj create mode 100644 test/methodical/macros/validate_arities_test.clj diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn index 3108f92..64d5fdf 100644 --- a/.clj-kondo/config.edn +++ b/.clj-kondo/config.edn @@ -21,7 +21,8 @@ :warn-on-reflection {:level :warning} :unresolved-symbol - {} + {:exclude + [(clojure.test/is [macroexpansion-spec-error?])]} :consistent-alias {:aliases diff --git a/src/methodical/macros.clj b/src/methodical/macros.clj index 1eadb2c..0173b86 100644 --- a/src/methodical/macros.clj +++ b/src/methodical/macros.clj @@ -1,11 +1,12 @@ (ns methodical.macros - "Methodical versions of vanilla Clojure `defmulti` and [[defmethod]] macros." + "Methodical versions of vanilla Clojure [[defmulti]] and [[defmethod]] macros." (:refer-clojure :exclude [defmulti defmethod]) (:require [clojure.spec.alpha :as s] [clojure.string :as str] [methodical.impl :as impl] [methodical.interface :as i] + [methodical.macros.validate-arities :as validate-arities] [methodical.util :as u]) (:import (methodical.impl.standard StandardMultiFn))) @@ -16,13 +17,29 @@ (s/alt :arity-1 :clojure.core.specs.alpha/params+body :arity-n (s/+ (s/spec :clojure.core.specs.alpha/params+body)))) +(s/def :methodical.macros.defmulti.attr-map/dispatch-value-spec + ;; not sure how to validate this. Is there a predicate for something that can be used as a + ;; spec? [[clojure.spec.alpha/spec]] will happily turn random things into specs for us + any?) + +(s/def :methodical.macros.defmulti.attr-map/defmethod-arities + (s/nilable ::validate-arities/arities-set)) + +(s/def :methodical.macros.defmulti/attr-map + (s/keys :opt-un [:methodical.macros.defmulti.attr-map/dispatch-value-spec + :methodical.macros.defmulti.attr-map/defmethod-arities])) + (s/def ::defmulti-args - (s/cat :name-symb (every-pred symbol? (complement namespace)) - :docstring (s/? string?) - :attr-map (s/? map?) - :dispatch-fn (s/? any?) - :options (s/* (s/cat :k keyword? - :v any?)))) + (s/& (s/cat :name-symb (every-pred symbol? (complement namespace)) + :docstring (s/? string?) + :attr-map (s/? map?) + :dispatch-fn (s/? any?) + :options (s/* (s/cat :k keyword? + :v any?))) + ;; do a "soft" cut here where any map argument is always interpreted as an attribute map, and only later subject + ;; it to stricter validation. See my questions in Slack here + ;; https://clojurians.slack.com/archives/C1B1BB2Q3/p1662755403589769 + (s/keys :opt-un [:methodical.macros.defmulti/attr-map]))) (defn- emit-defmulti [name-symb dispatch-fn {:keys [hierarchy dispatcher combo method-table cache default-value] @@ -49,7 +66,7 @@ (let [impl# (impl/standard-multifn-impl ~combo ~dispatcher ~method-table)] (vary-meta (impl/multifn impl# ~mta ~cache) merge (meta (var ~name-symb))))))) -(defn default-dispatch-value-spec +(defn- default-dispatch-value-spec "A dispatch value as parsed to [[defmethod]] (i.e., not-yet-evaluated) can be ANYTHING other than the following two things: @@ -78,10 +95,10 @@ ...))) ``` - but we are just UNFORTUNATELY going to have to throw up our hands and say we don't support it. The reason is in - the example above it's ambiguous whether this is a `:before` aux method with dispatch value `([toucan pigeon] i)`, - or a primary method with dispatch value `:before`. It's just impossible to tell what you meant. If you really want - to do something wacky like this, let-bind the dispatch value to a symbol or something. + but we are just UNFORTUNATELY going to have to throw up our hands and say we don't support it. The reason is in + the example above it's ambiguous whether this is a `:before` aux method with dispatch value `([toucan pigeon] i)`, + or a primary method with dispatch value `:before`. It's just impossible to tell what you meant. If you really want + to do something wacky like this, let-bind the dispatch value to a symbol or something. Note that if you specify a custom `:dispatch-value-spec` it overrides this spec. Hopefully your spec is stricter than this one is and it won't be a problem." @@ -103,7 +120,7 @@ (emit-defmulti name-symb dispatch-fn options))) (defmacro defmulti - "Creates a new Methodical multimethod named by a Var. Usage of this macro mimics usage of vanilla Clojure `defmulti`, + "Creates a new Methodical multimethod named by a Var. Usage of this macro mimics usage of [[clojure.core/defmulti]], and it can be used as a drop-in replacement; it does, however, support a larger set of options. Note the dispatch-fn is optional (if omitted, then identity will be used). In addition to the usual `:default` and `:hierarchy` options, you many specify: @@ -132,14 +149,92 @@ value -> methods. The default implementation is a pair of simple maps. The above options comprise the main constituent parts of a Methodical multimethod, and the majority of those parts - have several alternative implementations available in `methodical.impl`. Defining additional implementations is - straightforward as well: see `methodical.interface` for more details. + have several alternative implementations available in [[methodical.impl]]. Defining additional implementations is + straightforward as well: see [[methodical.interface]] for more details. - Other improvements over vanilla Clojure `defmulti`: + Other improvements over [[clojure.core/defmulti]]: * Evaluating the form a second time (e.g., when reloading a namespace) will *not* redefine the multimethod, unless you have modified its form -- unlike vanilla Clojure multimethods, which need to be unmapped from the namespace to - make such minor tweaks as changing the dispatch function." + make such minor tweaks as changing the dispatch function. + + Attribute map options: + + `defmulti` supports a few additional options in its attributes map that will be used to validate `defmethod` forms + during macroexpansion time. These are meant to help the users of your multimethods use them correctly by catching + mistakes right away rather than waiting for them to pull their hair out later wondering why a method they added isn't + getting called. + + * `:dispatch-value-spec` -- a spec for the `defmethod` dispatch value: + + ```clj + (m/defmulti mf + {:arglists '([x y]), :dispatch-value-spec (s/cat :x keyword?, :y int?)} + (fn [x y] [x y])) + + (m/defmethod mf [:x 1] + [x y] + {:x x, :y y}) + ;; => ok + + (m/defmethod mf [:x] + [x y] + {:x x, :y y}) + ;; failed: Insufficient input in: [0] at: [:args-for-method-type :primary :dispatch-value :y] [:x] + ``` + + Note that this spec is applied to the unevaluated arguments at macroexpansion time, not the actual evaluated values. + Note also that if you want to allow a `:default` method your spec will have to support it. + + * `:defmethod-arities` -- a set of allowed/required arities that `defmethod` forms are allowed to have. `defmethod` + forms must have arities that match *all* of the specified `:defmethod-arities`, and all of its arities must be + allowed by `:defmethod-arities`: + + ```clj + (m/defmulti ^:private mf + {:arglists '([x]), :defmethod-arities #{1}} + keyword) + + (m/defmethod mf :x [x] x) + ;; => ok + + (m/defmethod mf :x ([x] x) ([x y] x y)) + ;; => error: {:arities {:disallowed #{2}}} + + (m/defmethod mf :x [x y] x y) + ;; => error: {:arities {:required #{1}}} + + + (m/defmethod mf :x [x y] x) + ;; => error: {:arities {:required #{1 [:>= 3]}, :disallowed #{2}}} + ``` + + `:defmethod-arities` must be a set of either integers or `[:> n]` forms to represent arities with `&` rest + arguments, e.g. `[:>= 3]` to mean an arity of three *or-more* arguments: + + ```clj + ;; methods must both a 1-arity and a 3+-arity + (m/defmulti ^:private mf + {:arglists '([x] [x y z & more]), :defmethod-arities #{1 [:>= 3]}} + keyword) + + (m/defmethod mf :x ([x] x) ([x y z & more] x)) + ;; => ok + ``` + + When rest-argument arities are used, Methodical is smart enough to allow them when appropriate even if they do not + specifically match an arity specified in `:defmethod-arities`: + + ```clj + (m/defmulti ^:private mf + {:arglists '([x y z & more]), :defmethod-arities #{[:>= 3]}} + keyword) + + (m/defmethod mf :x + ([a b c] x) + ([a b c d] x) + ([a b c d & more] x)) + ;; => ok, because everything required by [:>= 3] is covered, and everything present is allowed by [:>= 3]" {:arglists '([name-symb docstring? attr-map? dispatch-fn? & {:keys [hierarchy default-value prefers combo method-table cache]}] [name-symb docstring? attr-map? & {:keys [dispatcher combo method-table cache]}]) @@ -230,7 +325,8 @@ primary-methods-allowed? (contains? allowed-qualifiers nil) allowed-aux-qualifiers (disj allowed-qualifiers nil) dispatch-value-spec (or (some-> (get (meta multifn) :dispatch-value-spec) s/spec) - (default-dispatch-value-spec allowed-aux-qualifiers))] + (default-dispatch-value-spec allowed-aux-qualifiers)) + arities-spec (validate-arities/allowed-arities-fn-tail-spec (:defmethod-arities (meta multifn)))] (s/cat :args-for-method-type (s/alt :primary (if primary-methods-allowed? (s/cat :dispatch-value dispatch-value-spec) (constantly false)) @@ -238,7 +334,8 @@ :dispatch-value dispatch-value-spec :unique-key (s/? (complement (some-fn string? sequential?))))) :docstring (s/? string?) - :fn-tail (s/& (s/+ any?) (s/nonconforming ::fn-tail))))) + :fn-tail (s/& (s/+ any?) + (s/nonconforming (s/& ::fn-tail arities-spec)))))) (defn- parse-defmethod-args [multifn args] (let [spec (defmethod-args-spec multifn) diff --git a/src/methodical/macros/validate_arities.clj b/src/methodical/macros/validate_arities.clj new file mode 100644 index 0000000..2e916e0 --- /dev/null +++ b/src/methodical/macros/validate_arities.clj @@ -0,0 +1,112 @@ +(ns ^:no-doc methodical.macros.validate-arities + "Implementation for `defmethod` arities validation (see [#59](https://github.com/camsaul/methodical/issues/59)). + + The arities sets passed around here are sets of numeric arity counts, or `[:> n]` for arity `n` that also accepts + varargs, i.e. + + ```clj + (fn + ([x]) + ([x y & more])) + ``` + + has the arities `#{1 [:>= 2]}`." + (:require + [clojure.data :as data] + [clojure.set :as set] + [clojure.spec.alpha :as s] + [methodical.macros.validate-arities :as validate-arities])) + +(defn- int-between-zero-and-twenty-inclusive? [n] + (and (integer? n) + (<= 0 n 20))) + +(s/def ::arities-set + (s/every + (s/or :int int-between-zero-and-twenty-inclusive? + :>=-form (s/spec (s/cat :>= (partial = :>=) + :int int-between-zero-and-twenty-inclusive?))) + :kind set? + :min-count 1)) + +(defn- arglist-arities + "Determine the arity sets for `arglists`. Deals with arglists already conformed using the + `:clojure.core.specs.alpha/param-list` spec. + + ```clj + (arglist-arities (s/conform (s/+ :clojure.core.specs.alpha/param-list) '([] [x]))) + => #{0 1} + + (arglist-arities (s/conform (s/+ :clojure.core.specs.alpha/param-list) '([] [x] [x y z & more]))) + => #{0 1 [:>= 3]} + ```" + [arglists] + (into #{} + (map (fn [{:keys [params var-params]}] + (if var-params + [:>= (count params)] + (count params)))) + arglists)) + +(defn- fn-tail-arities + "Determine arity sets for `fn-tails`. `fn-tails` should already be conformed using the + `:methodical.macros/fn-tail` spec or similar: + + ```clj + (fn-tail-arities (s/conform :methodical.macros/fn-tail '[([x] x) ([x y & more] x)])) + => + #{1 [:>= 2]} + ```" + [[arity-type x]] + (set (case arity-type + :arity-1 (arglist-arities [(:params x)]) + :arity-n (arglist-arities (map :params x))))) + +(defn- expand-arity + "Given an arity like `1` or `[:> 3]` expand the arity into a flat set of all arities that can be used to invoke a + function tail with that specific arity, e.g. `[:> 3]` can be invoked with `3`, `4`, `5,` or so on arguments. + + `clojure.lang.IFn/invoke` only supports distinct arities between 0 and 20, inclusive; any more than 20 arguments must + be invoked with `.applyTo`, so that's all we need to consider here; we'll use the keyword `:more` to represent > 20 + arguments. + + ```clj + (expand-arity 1) => #{1} + (expand-arity [:> 3]) => #{3 4 5 6 7 8 9 ... :more} + ```" + [arity] + (if (integer? arity) + (sorted-set arity) + (let [[_ arity] arity] + (into #{:more} (range arity 21))))) + +(defn- expand-arities [arities] + (into #{} (mapcat expand-arity) arities)) + +(defn- diff-arities + "Given a set of `required` arities (e.g., the `:defmethod-arities` in the `defmulti` metadata) and `actual` + arities (e.g. the function tail arities in a `defmethod` form, as generated by [[fn-tail-arities]]), return a map with + any `:required` arities that are missing and any `:disallowed` arities that are present. Returns `nil` if there are no + missing required arities or disallowed arities present." + [required actual] + (let [[missing disallowed] (data/diff (expand-arities required) (expand-arities actual))] + (not-empty + (into {} (for [[k orig expanded] [[:required required missing] + [:disallowed actual disallowed]] + :when (seq expanded)] + [k (set (for [arity orig + :when (not-empty (set/intersection (expand-arity arity) expanded))] + arity))]))))) + +(defn allowed-arities-fn-tail-spec + "Create a spec for a function tail to make sure it has all of the `required-arities`, and all of its arities are + allowed." + [required-arities] + (if (empty? required-arities) + identity + (s/and (s/conformer (fn [fn-tail] + (let [arities (fn-tail-arities fn-tail) + diff (diff-arities required-arities arities)] + (when (seq diff) + {:arities diff})))) + empty?))) diff --git a/test/methodical/macros/validate_arities_test.clj b/test/methodical/macros/validate_arities_test.clj new file mode 100644 index 0000000..a04a20f --- /dev/null +++ b/test/methodical/macros/validate_arities_test.clj @@ -0,0 +1,127 @@ +(ns methodical.macros.validate-arities-test + (:require + [clojure.spec.alpha :as s] + [clojure.test :as t] + [methodical.macros :as macros] + [methodical.macros.validate-arities :as validate-arities])) + +(t/deftest arglist-arities-test + (t/are [arglists expected] (= expected + (#'validate-arities/arglist-arities + (s/conform + (s/+ :clojure.core.specs.alpha/param-list) + (quote arglists)))) + + ([x] [x y z & more]) #{1 [:>= 3]} + ([x] [x y z & {:as options}]) #{1 [:>= 3]} + ([x] [x y z] [x y z & more]) #{1 3 [:>= 3]} + ([x] [x y z] [x y z k v] [x y z k v & more]) #{1 3 5 [:>= 5]} + ([x] [x y z] [x y z k] [x y z k v & more]) #{1 3 4 [:>= 5]} + ([x]) #{1} + ([x y z & {:as options}]) #{[:>= 3]} + ([x y z & {:as options}]) #{[:>= 3]} + ([x] [x y z]) #{1 3} + ([x] [x y] [x y z & more]) #{1 2 [:>= 3]} + ([] [x] [x y z & more]) #{0 1 [:>= 3]} + ([x & [y z & {:as options}]]) #{[:>= 1]} + ([x] [x y & more]) #{1 [:>= 2]} + ([a] [a b c] [a b c d]) #{1 3 4})) + +(t/deftest fn-tail-arities-test + (t/are [arglists expected] (= expected + (#'validate-arities/fn-tail-arities (s/conform ::macros/fn-tail (quote arglists)))) + + [([x] :ok) + ([x y z & more] :ok)] + #{1 [:>= 3]} + + [([x] :ok) + ([x y z & {:as options}] :ok)] + #{1 [:>= 3]} + + [([x] :ok) + ([x y z] :ok) + ([x y z & more] :ok)] + #{1 3 [:>= 3]} + + [([x] :ok) + ([x y z] :ok) + ([x y z k v] :ok) + ([x y z k v & more] :ok)] + #{1 3 5 [:>= 5]} + + [([x] :ok) + ([x y z] :ok) + ([x y z k] :ok) + ([x y z k v & more] :ok)] + #{1 3 4 [:>= 5]} + + [([x] :ok)] + #{1} + + [[x] :ok] + #{1} + + [([x y z & {:as options}] :ok)] + #{[:>= 3]} + + [[x y z & {:as options}] :ok] + #{[:>= 3]} + + [([x] :ok) ([x y z] :ok)] + #{1 3} + + [([x] :ok) + ([x y] :ok) + ([x y z & more] :ok)] + #{1 2 [:>= 3]} + + [([] :ok) + ([x] :ok) + ([x y z & more] :ok)] + #{0 1 [:>= 3]} + + [[x & [y z & {:as options}]] + :ok] + #{[:>= 1]} + + [([x] :ok) + ([x y & more] :ok)] + #{1 [:>= 2]} + + [([a] :ok) + ([a b c] :ok) + ([a b c d] :ok)] + #{1 3 4})) + +(t/deftest expand-arities-test + (t/are [arities expected] (= expected + (#'validate-arities/expand-arities arities)) + #{} #{} + #{1} #{1} + #{1 2} #{1 2} + #{1 [:>= 3]} #{1 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 :more} + #{1 3 [:>= 3]} #{1 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 :more})) + +(t/deftest diff-arities-test + (t/are [actual-arities expected] (= expected + (#'validate-arities/diff-arities #{1 [:>= 3]} actual-arities)) + ;; ok + #{1 [:>= 3]} nil + #{1 3 [:>= 3]} nil + #{1 3 4 [:>= 5]} nil + #{1 3 4 [:>= 4]} nil + #{1 3 [:>= 4]} nil + ;; missing some required arities + #{1} {:required #{[:>= 3]}} + #{[:>= 3]} {:required #{1}} + #{1 3} {:required #{[:>= 3]}} + #{1 3 4} {:required #{[:>= 3]}} + #{1 3 5 [:>= 5]} {:required #{[:>= 3]}} ; there's nothing that can handle 4 here. + ;; has disallowed arities + #{1 2 [:>= 3]} {:disallowed #{2}} + #{0 1 [:>= 3]} {:disallowed #{0}} + #{1 [:>= 2]} {:disallowed #{[:>= 2]}} ; disallowed because this handles 2 and that's not allowed. + #{[:>= 1]} {:disallowed #{[:>= 1]}} ; same, this one handles 2 + ;; disallowed AND missing + #{0 1} {:required #{[:>= 3]}, :disallowed #{0}})) diff --git a/test/methodical/macros_test.clj b/test/methodical/macros_test.clj index efd1948..b43372f 100644 --- a/test/methodical/macros_test.clj +++ b/test/methodical/macros_test.clj @@ -8,11 +8,27 @@ [methodical.util :as u] [potemkin.namespaces :as p.namespaces])) +(set! *warn-on-reflection* true) + ;;; so the tests that do macroexansion work correctly regardless of namespace (t/use-fixtures :each (fn [thunk] (binding [*ns* (the-ns 'methodical.macros-test)] (thunk)))) +(defn- re-quote [^String s] + (re-pattern (java.util.regex.Pattern/quote s))) + +(defmethod t/assert-expr 'macroexpansion-spec-error? [message [_ error form]] + (t/assert-expr + message + `(~'thrown-with-msg? + Exception + ~(re-quote error) + (try + (macroexpand ~form) + (catch clojure.lang.Compiler$CompilerException e# + (throw (or (ex-cause e#) e#))))))) + (t/deftest parse-defmulti-args-test (t/are [args parsed] (= (quote parsed) (s/conform :methodical.macros/defmulti-args (quote args))) @@ -24,12 +40,11 @@ :options [{:k :combo, :v (macros/clojure-method-combination)}]}) (t/testing "Throw error on invalid args (#36)" - (t/is (thrown? - clojure.lang.Compiler$CompilerException - (macroexpand - '(macros/defmulti multifn :default - [x y z] - :ok)))))) + (t/is (macroexpansion-spec-error? + "Call to methodical.macros/defmulti did not conform to spec." + '(macros/defmulti multifn :default + [x y z] + :ok))))) (t/deftest method-fn-symbol-test (letfn [(method-fn-symbol [dispatch-value] @@ -199,23 +214,25 @@ (assoc m :method :x)) (t/deftest validate-defmethod-args-test - (t/are [invalid-form] (thrown? - clojure.lang.Compiler$CompilerException - (macroexpand (quote invalid-form))) + (t/are [invalid-form msg] (macroexpansion-spec-error? msg (quote invalid-form)) ;; bad aux method (macros/defmethod mf1 :arounds :x [m] (assoc m :method :x)) + ":x - failed: vector? in: [0] at: [:fn-tail :arity-1 :params]" ;; missing function tail (macros/defmethod mf1 :around :x) + "failed: Insufficient input at: [:fn-tail]" ;; invalid function tail (macros/defmethod mf1 :around :x {} a b c) + "failed: Insufficient input" ;; string unique key - (macros/defmethod mf1 :around :x "unique-key" "docstr" [a] b c))) + (macros/defmethod mf1 :around :x "unique-key" "docstr" [a] b c) + "failed: Insufficient input")) (s/def ::arg-validation-spec (s/or :default (partial = :default) @@ -239,13 +256,18 @@ (macros/defmethod validate-args-spec-mf [:x :y] [x y]))) (t/testing "invalid" - (t/are [form] (thrown? - clojure.lang.Compiler$CompilerException - (macroexpand (quote form))) + (t/are [form msg] (macroexpansion-spec-error? msg (quote form)) (macros/defmethod validate-args-spec-mf :x [x y]) + ":x - failed: (partial = :default) in: [0]" + (macros/defmethod validate-args-spec-mf [:x 1] [x y]) + "failed: keyword? in: [0 1] at: [:args-for-method-type :primary :dispatch-value :x-y :y]" + (macros/defmethod validate-args-spec-mf [:x] [x y]) - (macros/defmethod validate-args-spec-mf [:x :y :z] [x y])))) + "failed: Insufficient input in: [0] at: [:args-for-method-type :primary :dispatch-value :x-y :y]" + + (macros/defmethod validate-args-spec-mf [:x :y :z] [x y]) + "failed: Extra input in: [0 2] at: [:args-for-method-type :primary :dispatch-value :x-y]"))) (t/deftest defmethod-primary-methods-test (t/is (= mf1 (let [impl (impl/standard-multifn-impl @@ -378,9 +400,124 @@ '(macros/defmethod mf-dispatch-value-spec-2 [:x 1] [x y] {:x x, :y y})))) - (t/is (thrown? - clojure.lang.Compiler$CompilerException - (macroexpand - '(macros/defmethod mf-dispatch-value-spec-2 [:x] - [x y] - {:x x, :y y})))))) + (t/is (macroexpansion-spec-error? + "failed: Insufficient input in: [0] at: [:args-for-method-type :primary :dispatch-value :y]" + '(macros/defmethod mf-dispatch-value-spec-2 [:x] + [x y] + {:x x, :y y}))))) + +(t/deftest defmulti-validate-defmethod-arities-metadata-test + (t/testing "ok" + (t/are [arities] (some? + (macroexpand + '(macros/defmulti my-mf {:defmethod-arities arities} identity))) + nil + #{0} + #{0 1 [:>= 3]} + #{20})) + + (t/testing "bad" + (t/are [arities] (macroexpansion-spec-error? + "Call to methodical.macros/defmulti did not conform to spec." + '(macros/defmulti my-mf {:defmethod-arities arities} identity)) + #{} + #{-1} + #{21} + [0] + {0 1} + 1 + [:>= 3] + #{[:> 3]} + #{:more}))) + +(macros/defmulti ^:private validate-arg-counts-mf + ;; actually allowed: + ;; + ;; * x (1 arg) + ;; * x y z (3 args) + ;; * x y z :k v ... (5 + args, odd) + ;; + ;; 2 ARGS ARE NOT ALLOWED. + {:arglists '([x] [x y z & {:as options}]), :defmethod-arities #{1 [:>= 3]}} + (fn [x & _] + x)) + +(t/deftest validate-defmethod-arities-test + (t/testing "We should be able to validate arg counts in defmethod forms (#59)" + (t/testing "valid" + (t/are [fn-tail] (some? + (macroexpand + (list* `macros/defmethod `validate-arg-counts-mf :x (quote fn-tail)))) + [([x] :ok) + ([x y z & more] :ok)] + [([x] :ok) + ([x y z & {:as options}] :ok)] + ;; this should be ok. + [([x] :ok) + ([x y z] :ok) + ([x y z & more] :ok)] + ;; so should this one. + [([x] :ok) + ([x y z] :ok) + ([x y z k] :ok) + ([x y z k v & more] :ok)] + ;; not sure about this one or not. TECHNICALLY this should be disallowed because it allows 4 args and we're + ;; doing key-value destructuring... but worrying about that might be too much for us right now. + [([x] :ok) + ([x y z] :ok) + ([x y z k] :ok) + ([x y z k v & more] :ok)])) + (t/testing "invalid" + (t/are [fn-tail msg] (macroexpansion-spec-error? + msg + (list* `macros/defmethod `validate-arg-counts-mf :x 'fn-tail)) + ;; missing 3+ + [([x] :ok)] + "{:arities {:required #{[:>= 3]}}}" + + [[x] :ok] + "{:arities {:required #{[:>= 3]}}}" + + ;; missing 1 + [([x y z & {:as options}] :ok)] + "{:arities {:required #{1}}}" + + [[x y z & {:as options}] :ok] + "{:arities {:required #{1}}}" + + ;; missing & + [([x] :ok) ([x y z] :ok)] + "{:arities {:required #{[:>= 3]}}}" + + ;; 2 is not allowed + [([x] :ok) + ([x y] :ok) + ([x y z & more] :ok)] + "{:arities {:disallowed #{2}}}" + + ;; 0 is not allowed + [([] :ok) + ([x] :ok) + ([x y z & more] :ok)] + "{:arities {:disallowed #{0}}}" + + ;; This one also would theoretically work for 1, 3, or 3+ args... Not sure about this one. Should this be + ;; allowed or not? I guess maybe not since you would be able to invoke it with 2 args which isn't allowed. + [[x & [y z & {:as options}]] + :ok] + "{:arities {:disallowed #{[:>= 1]}}}" + + ;; 2 or more is not ok. We only want 3 or more. + [([x] :ok) + ([x y & more] :ok)] + "{:arities {:disallowed #{[:>= 2]}}}" + + ;; not ok because while this handles 3 or 4 it doesn't handle 5+ + [([a] :ok) + ([a b c] :ok) + ([a b c d] :ok)] + "{:arities {:required #{[:>= 3]}}}" + + ;; missing arities and disallowed arities + ([x y] :ok) + "{:arities {:required #{1 [:>= 3]}, :disallowed #{2}}}")))) From a95444cdd1ebcecdf2811fda8b1bbc64f74d4aca Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Fri, 9 Sep 2022 23:37:31 +0000 Subject: [PATCH 2/6] Update README --- README.md | 52 +++++++++++++++++++++++++++++++++++++++ src/methodical/macros.clj | 3 ++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 62f4f80..760f7ed 100644 --- a/README.md +++ b/README.md @@ -517,6 +517,58 @@ dispatch value form of any `defmethod` forms at macroexpansion time: This is a great way to make sure people use your multimethods correctly and catch errors right away. +#### `:defmethod-arities` + +A set of allowed/required arities that `defmethod` forms are allowed to have. `defmethod` forms must have arities that +match *all* of the specified `:defmethod-arities`, and all of its arities must be allowed by `:defmethod-arities`: + +```clj +(m/defmulti ^:private mf + {:arglists '([x]), :defmethod-arities #{1}} + keyword) + +(m/defmethod mf :x [x] x) +;; => ok + +(m/defmethod mf :x ([x] x) ([x y] x y)) +;; => error: {:arities {:disallowed #{2}}} + +(m/defmethod mf :x [x y] x y) +;; => error: {:arities {:required #{1}}} + + +(m/defmethod mf :x [x y] x) +;; => error: {:arities {:required #{1 [:>= 3]}, :disallowed #{2}}} +``` + +`:defmethod-arities` must be a set of either integers or `[:> n]` forms to represent arities with `&` rest +arguments, e.g. `[:>= 3]` to mean an arity of three *or-more* arguments: + +```clj +;; methods must both a 1-arity and a 3+-arity +(m/defmulti ^:private mf + {:arglists '([x] [x y z & more]), :defmethod-arities #{1 [:>= 3]}} + keyword) + +(m/defmethod mf :x ([x] x) ([x y z & more] x)) +;; => ok +``` + +When rest-argument arities are used, Methodical is smart enough to allow them when appropriate even if they do not +specifically match an arity specified in `:defmethod-arities`: + +```clj +(m/defmulti ^:private mf + {:arglists '([x y z & more]), :defmethod-arities #{[:>= 3]}} + keyword) + +(m/defmethod mf :x + ([a b c] x) + ([a b c d] x) + ([a b c d & more] x)) +;; => ok, because everything required by [:>= 3] is covered, and everything present is allowed by [:>= 3] +``` + ### Debugging Methodical offers debugging facilities so you can see what's going on under the hood, such as the `trace` utility: diff --git a/src/methodical/macros.clj b/src/methodical/macros.clj index 0173b86..ffd8b43 100644 --- a/src/methodical/macros.clj +++ b/src/methodical/macros.clj @@ -234,7 +234,8 @@ ([a b c] x) ([a b c d] x) ([a b c d & more] x)) - ;; => ok, because everything required by [:>= 3] is covered, and everything present is allowed by [:>= 3]" + ;; => ok, because everything required by [:>= 3] is covered, and everything present is allowed by [:>= 3] + ```" {:arglists '([name-symb docstring? attr-map? dispatch-fn? & {:keys [hierarchy default-value prefers combo method-table cache]}] [name-symb docstring? attr-map? & {:keys [dispatcher combo method-table cache]}]) From e1e62ea45a9e74188aae0641d29762ed3de1184d Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Fri, 9 Sep 2022 23:42:51 +0000 Subject: [PATCH 3/6] Fix CodeSpell action --- .github/workflows/tests.yml | 4 +--- src/methodical/macros/validate_arities.clj | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index ba416c8..a134a23 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -119,6 +119,4 @@ jobs: files: ./target/coverage/codecov.json codespell: - runs-on: ubuntu-20.04 - steps: - - uses: codespell-project/actions-codespell@master + uses: codespell-project/actions-codespell@master diff --git a/src/methodical/macros/validate_arities.clj b/src/methodical/macros/validate_arities.clj index 2e916e0..48fe701 100644 --- a/src/methodical/macros/validate_arities.clj +++ b/src/methodical/macros/validate_arities.clj @@ -14,8 +14,7 @@ (:require [clojure.data :as data] [clojure.set :as set] - [clojure.spec.alpha :as s] - [methodical.macros.validate-arities :as validate-arities])) + [clojure.spec.alpha :as s])) (defn- int-between-zero-and-twenty-inclusive? [n] (and (integer? n) From cdf7d3c4b581548987b42511a94c1af5319fd294 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Fri, 9 Sep 2022 23:44:26 +0000 Subject: [PATCH 4/6] Fix CodeSpell --- .github/workflows/tests.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a134a23..0800c6e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -119,4 +119,5 @@ jobs: files: ./target/coverage/codecov.json codespell: - uses: codespell-project/actions-codespell@master + steps: + - uses: codespell-project/actions-codespell@master From 83880d3b857b4ad768cae57f3b8614439366aa91 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Fri, 9 Sep 2022 23:46:30 +0000 Subject: [PATCH 5/6] Fix CodeSpell --- .github/workflows/tests.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0800c6e..88ebf59 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -119,5 +119,7 @@ jobs: files: ./target/coverage/codecov.json codespell: + runs-on: ubuntu-20.04 steps: + - uses: actions/checkout@v3 - uses: codespell-project/actions-codespell@master From 59963d9b3a0429840a0098613407580916b45365 Mon Sep 17 00:00:00 2001 From: Cam Saul Date: Fri, 9 Sep 2022 23:47:22 +0000 Subject: [PATCH 6/6] Fix spelling in README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 760f7ed..050f813 100644 --- a/README.md +++ b/README.md @@ -580,7 +580,7 @@ and the `describe` utility, which outputs Markdown-formatted documentation, for ![Describe](assets/describe.png) -This extra information is automatically generated and appened to a multimethod's docstring whenever methods or +This extra information is automatically generated and appended to a multimethod's docstring whenever methods or preferences are added or removed. Methodical multimethods also implement `datafy`: