fulcrologic / guardrails Goto Github PK

When I run kaocha against some code that depends on the 1.1.11 release of guardrails I'm seeing:

> clj -M:test --watch
WARNING: update-vals already refers to: #'clojure.core/update-vals in namespace: clojure.tools.analyzer.utils, being replaced by: #'clojure.tools.analyzer.utils/update-vals
WARNING: update-keys already refers to: #'clojure.core/update-keys in namespace: clojure.tools.analyzer.utils, being replaced by: #'clojure.tools.analyzer.utils/update-keys
WARNING: update-vals already refers to: #'clojure.core/update-vals in namespace: clojure.tools.analyzer, being replaced by: #'clojure.tools.analyzer.utils/update-vals
WARNING: update-keys already refers to: #'clojure.core/update-keys in namespace: clojure.tools.analyzer, being replaced by: #'clojure.tools.analyzer.utils/update-keys
WARNING: update-vals already refers to: #'clojure.core/update-vals in namespace: clojure.tools.analyzer.passes, being replaced by: #'clojure.tools.analyzer.utils/update-vals
WARNING: update-vals already refers to: #'clojure.core/update-vals in namespace: clojure.tools.analyzer.passes.uniquify, being replaced by: #'clojure.tools.analyzer.utils/update-vals

From the dependency tree I can see that guardrails is pulling in clojure.tools.analyzer via core.async:

$ clj -X:deps tree :aliases '[:test]'
org.clojure/clojure 1.11.1
  . org.clojure/spec.alpha 0.3.218
  . org.clojure/core.specs.alpha 0.2.62
[...]
com.fulcrologic/guardrails 1.1.11
  X expound/expound 0.8.7 :superseded
  . org.clojure/core.async 1.3.618
    . org.clojure/tools.analyzer.jvm 1.1.0
      . org.clojure/tools.analyzer 1.0.0 # <<< HERE
      . org.clojure/core.memoize 1.0.236
        . org.clojure/core.cache 1.0.207
          . org.clojure/data.priority-map 1.0.0
      . org.ow2.asm/asm 5.2
      X org.clojure/tools.reader 1.3.2 :older-version
[...]

Bumping core.async to its latest version in my project's deps.edn resolves this issue:

{:paths [...]

 :deps {...
        org.clojure/core.async {:mvn/version "1.6.673"} ;; <<< MITIGATION
        com.fulcrologic/guardrails {:mvn/version "1.1.11"}
        ...}

 :aliases {:dev
           {:extra-paths ["dev"]
            :jvm-opts ["-Dguardrails.enabled"]}

           :test
           {:extra-paths [...]
            :extra-deps {lambdaisland/kaocha {:mvn/version "1.83.1314"}}
            :main-opts ["-m" "kaocha.runner"]}}}

But since my project doesn't depend on core.async directly, and moreover this may be an annoyance to a number of downstream projects and not just my own, I think bumping the version in the deps.edn of guardrails is the best option. I've tried this and the make tests pass okay.

Raising a PR for such a minor change seems over the top, so I'm using this issue to request the change.

See also: https://clojureverse.org/t/tools-deps-test-runner-warnings/9548

This breaks:

(>defn add
  [[[k v] & wat]]
  [any? => number?]
  (+ 1 1))
;;=> Syntax error macroexpanding clojure.core/defn at (REPL:81:1).
;;=> ((k v) & wat :as arg_6480) - failed: Extra input at: [:fn-tail :arity-n :bodies :params] spec: :clojure.core.specs.alpha/param-list
;;=> ([(k v) & wat :as arg_6480]) - failed: Extra input at: [:fn-tail :arity-1 :params] spec: :clojure.core.specs.alpha/param-list

Unfortunately it takes a long time to load Malli namespace:

(time (require '[com.fulcrologic.guardrails.malli.core]))
"Elapsed time: 7574.215667 msecs"

Since this namespace require happens even if Guardrails is disabled (e.g. production), it would be nice to think about alternative ways to load the required namespaces to avoid that hit in production.

Another key insight is that about half of it is just the printer/reporter.

(time (require '[malli.dev.pretty :as mp]))
"Elapsed time: 3335.58975 msecs"

Malli's dev printer is very expensive to load, because it uses Fipp which takes a long time to load, and Fipp uses core.rrb-vector which is also heavy.

Slows specs can be a real burden when doing dev, because the slowdown can be significant.

ideas for speed up:

Tight loops: rapid repeated calls to check specs should stop checking after a small number, or should do random sampling. Another option would be to use a dropping async buffer and just shove lambdas to do the check/report into the queue, allowing production speeds with the chance that some checks won't run. A configurable queue would make this good enough in many many cases, and would allow the checks to use an alt thread.

Slow specs: Measure how long a particular function takes to check. If it is slow, check a smaller percentage of calls to it, perhaps always checking the 1st (since there may only be one) when there has been a reasonable time since it was last checked.

Devs usually do things in spurts, so the first soln might actually work quite well. A deep enough queue will check 100% of the things, but on whatever delay is required. An overall check "timeout" could be configured to cause it to stop discard the queue, and warn that the queue was too busy.

It would be great if we could choose to run Guardrails on a subset of spec namespaces. My exact use-case is that I'm using an external library (https://github.com/wilkerlucio/pathom3) that's heavily using Guardrails. I'm also using it internally as a development tool. Ideally I'd be able to disable the specs from running in external libraries so that there's not as much of a performance impact.

If this is in scope of the library, I can try to take an initial swing at it. Thanks!

Guardrails reports spec failure even though spec isn't defined in function definition.
Seems just to happen with a s/keys spec

Have a look at this REPL Session:

(require '[clojure.spec.alpha :as s])
;; => nil
(s/def ::foo int?)
;; => :user/foo
(s/def ::bar int?)
;; => :user/bar
(require '[com.fulcrologic.guardrails.core :refer [>defn =>]])
;; => nil
(>defn add-foo-bar [{::keys [foo bar]}]
  [any? => (s/keys :req [::foo])]
  {::foo 42
   ::bar (+ foo bar)})
;; => #'user/add-foo-bar
(add-foo-bar {::foo 1
              ::bar 3.0})
;; => #:user{:foo 42, :bar 4.0}

;; add-foo-bar's return type
;; -- Spec failed --------------------
;; 
;;   {:user/foo 42, :user/bar 4.0}
;;                            ^^^
;; 
;; should satisfy
;; 
;;   int?
;; 
;; -- Relevant specs -------
;; 
;; :user/bar:
;;   clojure.core/int?
;; 
;; -------------------------
;; Detected 1 error
;;
;; java.lang.AssertionError: 
;;    com.fulcrologic.guardrails.core$callsite_exception.invokeStatic (core.cljc:574)
;;    com.fulcrologic.guardrails.core$callsite_exception.invoke (core.cljc:574)
;;    user$add_foo_bar.invokeStatic (form-init18326497060998450108.clj:1)
;;    user$add_foo_bar.invoke (form-init18326497060998450108.clj:1)
;;    ...

Fails as well

(>defn add-foo-bar [{::keys [foo bar]}]
  [(s/keys :req [::foo]) => any?]
  {::foo 42
   ::bar (+ foo bar)})
;; => #'user/add-foo-bar
(add-foo-bar {::foo 1
              ::bar 3.0})
;; => #:user{:foo 42, :bar 4.0}
;; like above

Following works fine:

(>defn add-foo-bar [{::keys [foo bar]}]
  [any? => map?]
  {::foo 42
   ::bar (+ foo bar)})
;; => #'user/add-foo-bar
(add-foo-bar {::foo 1
              ::bar 3.0})
;; => #:user{:foo 42, :bar 4.0}

Hi,

Could there be another form of >defn that will behave like the current one but still trigger the same validation as the regular version in production builds? In a lot of cases you want to remove them all, for this guardrails works fine, but it would be handy if the same syntax could also be used to annotate critical functions that will remain in the final build (I want to avoid instrument and potential gen it triggers).

Or maybe I am missing something obvious and this already exists in another form?

Need to make sure macro code and analyzer do not affect cljs build size when disabled.

I noticed that guardrails has some clj-kondo hooks defined but it doesn't seem to be part of the released artifact so I'm unable to import them if I depend on a mvn version of guardrails.

Is this by design or an oversight?

#$ ~/.m2/repository/com/fulcrologic/guardrails/1.1.10
; jar tf guardrails-1.1.10.jar
META-INF/
META-INF/MANIFEST.MF
com/
com/fulcrologic/
com/fulcrologic/guardrails/
com/fulcrologic/guardrails/impl/
com/fulcrologic/guardrails/stubs/
com/fulcrologic/guardrails/impl/pro.cljc
com/fulcrologic/guardrails/impl/externs.cljc
com/fulcrologic/guardrails/impl/parser.clj
com/fulcrologic/guardrails/stubs/cljs_env.clj
com/fulcrologic/guardrails/stubs/ana_api.clj
com/fulcrologic/guardrails/core.cljc
com/fulcrologic/guardrails/utils.cljc
com/fulcrologic/guardrails/config.cljc
com/fulcrologic/guardrails/noop.cljc
com/fulcrologic/guardrails/registry.cljc
META-INF/maven/
META-INF/maven/com.fulcrologic/
META-INF/maven/com.fulcrologic/guardrails/
META-INF/maven/com.fulcrologic/guardrails/pom.xml
META-INF/maven/com.fulcrologic/guardrails/pom.properties

I would be happy to try and submit a PR however I haven't been able to find any scripts in the repo that produce the released jar. If someone has pointers wrt this, please do share.

Macros can be given a :style/indent setting that most editors respect.

Would be nice to have this on >defn.

(require '[com.fulcrologic.guardrails.malli.registry :as gr.reg])
=> nil
(malli.registry/set-default-registry! gr.reg/registry)
=>
#object[malli.registry$composite_registry$reify__3394
        0x626f9de6
        "malli.registry$composite_registry$reify__3394@626f9de6"]
(malli.core/schema [:map [:x :string]])
Execution error (StackOverflowError) at malli.registry$custom_default_registry$reify__3390/_schema (registry.cljc:48).
null

The reason for this is simple, guardrails registry refers to default registry and when it becomes default registry, that's a loop.

Why would one want to set guardrails as default registry? For consistency. Using >def or register! fills up the guardrails registry and if you want to use those schemas in non-guardrails code you'll want to use that registry. E.g. malli.util/optional-keys. Of course you can always pick registry on per-call/per-schema basis, so that fixes this, but it's kinda annoying.

Sometimes this library will fail/throw on spec failure for map keys not defined in spec tested (but defined elsewhere. Here's a minimal working example (assuming you're using fulcro).

This code will not throw an error:

(com.fulcrologic.guardrails.core/run-check
  false
  {:log-level nil
   :vararg? false
   :throw true
   :fn-name "whatever"}
  (s/keys :req [:com.fulcrologic.fulcro.algorithms.form-state/config])
  {:com.fulcrologic.fulcro.algorithms.form-state/config
   {:com.fulcrologic.fulcro.algorithms.form-state/id [:component/id :user-info],
    :com.fulcrologic.fulcro.algorithms.form-state/fields #{},
    :com.fulcrologic.fulcro.algorithms.form-state/subforms {}}})

Now I'm going to add definition of a new spec:

(s/def ::test number?)

Now I'm going to add this key to the value tested, but not to the spec tested:

(com.fulcrologic.guardrails.core/run-check
  false
  {:log-level nil
   :vararg? false
   :throw true
   :fn-name "whatever"}
  (s/keys :req [:com.fulcrologic.fulcro.algorithms.form-state/config])
  {::test nil
   :com.fulcrologic.fulcro.algorithms.form-state/config
   {:com.fulcrologic.fulcro.algorithms.form-state/id [:component/id :user-info],
    :com.fulcrologic.fulcro.algorithms.form-state/fields #{},
    :com.fulcrologic.fulcro.algorithms.form-state/subforms {}}})

This produces an error because ::test key doesn't fit its spec, but that's not expected behaviour for me because ::test is not part of the spec being tested.

It seems Malli has changed the arity of some internal function in Malli 0.14.0:

BUG: Internal error in Malli.

Wrong number of args (4) passed to: malli.dev.virhe/eval22220/fn--22221
at clojure.lang.AFn.throwArity (AFn.java:429)
    clojure.lang.AFn.invoke (AFn.java:44)
    clojure.lang.MultiFn.invoke (MultiFn.java:243)
    com.fulcrologic.guardrails.malli.core$_exception_doc.invokeStatic (core.cljc:99)
    com.fulcrologic.guardrails.malli.core$_exception_doc.invoke (core.cljc:97)
    com.fulcrologic.guardrails.malli.core$reporter$fn__23490.invoke (core.cljc:107)
    malli.dev.pretty$prettifier$fn__23448.doInvoke (pretty.cljc:173)
    clojure.lang.RestFn.invoke (RestFn.java:397)
    com.fulcrologic.guardrails.malli.core$humanize_schema$fn__23509.invoke (core.cljc:135)
    com.fulcrologic.guardrails.malli.core$humanize_schema.invokeStatic (core.cljc:134)
    com.fulcrologic.guardrails.malli.core$humanize_schema.invoke (core.cljc:124)
    com.fulcrologic.guardrails.core$run_check$fn__20550.invoke (core.cljc:132)
    com.fulcrologic.guardrails.core$run_check.invokeStatic (core.cljc:128)
    com.fulcrologic.guardrails.core$run_check.invoke (core.cljc:110)

I noticed when upgrading to guardrails 0.0.12 I am getting missing spec errors in the browser console for fulcro namespaces.

Version 0.0.11 does not have this issue.

Client only fulcro app demonstrating the bug:
https://github.com/dvingo/guardrails-0.0.12-bug

Dear @awkay,

Could you kindly help me understand the meaning of the below mentioned report in guardrails? Specifically it's hard for me to grok why the return type is considered to be LazySeq. I would expect (maybe wrongfully 🙈) guardrails to pinpoint the exact offending entry in returned vector like (io.blockether.datomic.core/join-migrations ...) => [entry-not-matching-schema].

Reproduction

Let's take into consideration the following schema in malli style:

(>def ::schematic
  [:map {}
   [:abc :string]])

(>def ::schematics
  [:vector ::schematic])

(>def ::schematics-list
  [:* ::schematic])

(>def ::migration
  [:map {}
   [:id :keyword]
   [:tx-data ::schematics]])

(>def ::migrations
  [:vector ::migration])

and let us define a simple function that takes migrations as a varargs and normalises them to vector of migrations

(>defn join-migrations
  [& migrations]
  [::schematics-list => [:maybe ::schematics]]
  (vec migrations))

(join-migrations 
  {:id :m1
   :tx-data [[:db/add "session" :session/id "someSessionId"]]}) ;; <-- see Guardrails Report

now I would expect guardrails to report the inferred type as vector and not a lazy-seq.

Guardrails Report

(please forgive me Calva comment marks)

; Guardrails:
;   (io.blockether.datomic.core/join-migrations {:id :m1, :tx-data [[:db/add "session" :session/id "someSessionId"]]})
;   Arg Errors: [{:abc ["missing required key"], :malli/error ["input remaining"]}]
;   Schema: [:catn
;            [:migrations
;             :io.blockether.datomic.core/schematics-list]]
;   GR functions on stack. (com.fulcrologic.guardrails.utils/last-failure 'io.blockether.datomic.core/join-migrations) for full stack:
;     (io.blockether.datomic.core/join-migrations ({:id :m1, :tx-data [[:db/add "session" :session/id "someSessionId"]]}))
;
;
;
;
;
; Guardrails: (ambiguous report)
;   (io.blockether.datomic.core/join-migrations ...) => clojure.lang.LazySeq@9e3791ee <-- LazySeq is ambiguous and misleading 
;   Return value {:abc ["missing required key"]}
;   Schema: [:maybe :io.blockether.datomic.core/schematics]
;   GR functions on stack. (com.fulcrologic.guardrails.utils/last-failure 'io.blockether.datomic.core/join-migrations) for full stack:
;     (io.blockether.datomic.core/join-migrations ({:id :m1, :tx-data [[:db/add "session" :session/id "someSessionId"]]}))

Hope this message finds you in good health. Thank you for all your hard work on guardrails. Cheers!
Karol

I've compiled my program that uses guardrails with 'warn-on-reflection' and 'warn-on-boxed-math' set to true. It appears that guardrails utilises a significant amount of reflection, resulting in these warnings appearing in my program. This is crucial, as I compile everything with GraalVM native-image, and minimising reflection is preferable.

The way I've done the function rewriting breaks varargs functions.

When a function input check fails with Malli integration, it is never explained which function failed. So you have to guess.

It always reports same line in lib itself as the location of the error:

Guardrails:
-- Validation Error  com.fulcrologic.guardrails.malli.core:117 --