Review usage of assert

[lread]

Raised me in rewrite-cljc-playground, see lread/rewrite-cljc-playground#59

Much thanks to @sogaiu for bringing this up on Slack.

Rewrite-clj (and therefore rewrite-cljc) makes use of assert, mostly to validate node creation, and therefore also for general parsing. The most common assertion is that a node has the expected number of children.

@sogaiu highlighted the following interesting points (which I am paraphrasing, hopefully correctly):

1. the naive use of assert can lead to unintented results as described in "Use of Assertions" by John Regehr from which @sogaiu quoted:

   if we are writing library code, we must be even more careful to use assertions correctly than if we are writing a standalone application. The only time to assert something in library code is when the code truly cannot continue executing without dire consequences.

   @sogaiu also came across "Beware of Assertions" by Alexander Yakushev

2. @borkdude effectively disables these asserts for some of his projects, for example here's how he does so for clj-kondo, mostly for performance.

If we take point 1 to heart, we can argue that rewrite-cljc really shouldn't be making use of assert. It should not be deciding that the situation of encountering invalid Clojure is dire for the calling library/application.

Note that I did replace all explicit Exception throws with ex-info exceptions for rewrite-cljc, for cross-platform support between clj and cljs - so I do have a precedent for replacing other types of exceptions. But I am not sure what makes sense moving forward. Some initial ideas:

1. do nothing and document - this preserves backward compatibility
2. replace with explicit throws - this would allow for a catchable Exception but would break behavior compatibility for folks who want to disable checks for performance (or other) reasons.
3. replace with spec - but spec is not normally enabled in production, so backward compatibility behavior would be an opt-in, and depending on how far we go with spec, might more coarser grained that a caller would like?

[lread]

Issue comments from rewrite-cljc-playground:

@borkdude:
Please don't replace with explicit throws.

Also please don't replace with spec, unless the specs are in a different namespace and optional to load.

@borkdude:
Elaborating:

The checks in asserts are mostly useful for developers. They should be elidable at compile time for performance.
For the same reason spec checking should be able to turned off in production environments.

It's nice to get some extra checking during development, but you should be able to turn that off when compiling a final artifact. E.g. linters and formatters should get the ultimate performance possible, since they run on every keystroke.

So I would really do nothing at this point, since people are relying on being able to turn asserts off.

@lread:
Thanks @borkdude, much appreciated.

Question to self: if asserts are disabled is rewrite-clj able to parse invalid (or less valid) Clojure code? (Note that this is not necessarily a bad thing).

@borkdude:
@lread Note that clj-kondo has been running without asserts for its entire lifetime. So far I haven't had any issues. I think the asserts are mostly there for users creating their own nodes. E.g. for debugging clj-kondo hooks code this could be useful feedback, but that's on clj-kondo to fix, since I've decided to turn the asserts off. In the current API there aren't any asserts that people would miss out on, though.

Asserts aren't bad, they are designed exactly for this purpose.

@lread:
Very much related is the usage of :pre on some functions, for example, which are effectively assertions.
Not suggesting we do anything, just taking notes.

@sogaiu:
Ah, I didn't realize that about :pre -- thanks for sharing!

[vemv]

This one is biting me as I'm trying to cut a new refactor-nrepl release. See e.g. clojure-emacs/refactor-nrepl#332

It'd be pretty bad to leave users with errors that can't quite be debugged.

*assert* (the dyn var) is OK-ish, assert (the macro) not so much because it doesn't tell us the value at fault.

It's fairly trivial to replace (assert) and :pre with a helper such that if the expectation fails, the value at fault is reported. There's https://github.com/ptaoussanis/truss for example.

Probably for refactor-nrepl I'd be OK with the performance hit of checking preconditions. I prefer correct code that can be debugged to opaque NPEs or such. Also for refactor-nrepl the bottleneck is tools.analyzer, typically it will trump any other cost.

You might want to define your own *assert* that consumers can customize at will, I guess that one size doesn't necessarily fit all.

Cheers - V

[lread]

Thanks for adding to the issue @vemv!

This is all a bit foggy so I shall recap and paraphrase:

• A while ago @borkdude helped me to understand that he disables assertions for production use for performance reasons.
  Here's what he does for clj-kondo for example.
• You are interested in leaving the existing assertions active but would appreciate better error reporting to help with diagnosis.

Since :pre is disabled by setting*assert* to false, and it will give you more context, I can carefully look into this.
Although truss looks interesting (thanks for sharing!), I remain extremely conservative about bringing in any new dependencies into rewrite-clj.

[vemv]

Hey there!

disables assertions for production use for performance reasons.

The nuance is that *assert* is a global var and software like refactor-nrepl (and even clj-kondo, depending on how you use it) shares the JVM with the end-user environment.

So it's easy to couple things. End users may tweak *assert* at will for unrelated reasons. Other tools also consuming rewrite-clj can have a different expectation/preference as to whether check asserts.

I think that a minimalistic solution would look like this:

(defn foo [x]
  (when *assert*
    (or (string? x)
        (throw (ex-info "..." {:faulty-value x})))))

...It's not merely a hand-rolled :pre: it moves the *assert* querying from compile-time to runtime. That way, rewrite-clj consumers can meaningfully bind *assert* and get custom behavior from rewrite-clj.

(If going this direction I'd recommend defining your own *assert* var for further decoupling)

I can contribute a POC PR with this setup upgrading a few :pres to demonstrate this pattern. WDYT?

[borkdude]

Note that moving checks from compile time to runtime isn't exactly better for performance, especially not when deref-ing dynamic vars, but perhaps in the grand scheme of things it doesn't matter that much.

Perhaps just moving the checks to a rewrite-clj.specs namespace that you can optionally load / instrument if you want spec checking for these functions is the way to go in this day and age.

[vemv]

Adding spec to the mix seems to go against I remain extremely conservative about bringing in any new dependencies into rewrite-clj. With spec1/spec2 uncertainty, spec seems bit of delicate one especially as end users aren't fully in control.

Also spec tempts one to bring in Orchestra or such, most devs out there appreciate checking the return value etc. Which can further complicate things. tldr Spec is not exactly agnostic/minimalistic for this use case.

Note that moving checks from compile time to runtime isn't exactly better for performance

I was thinking of a 'double-checked' pattern i.e. first you check against clojure.core/*assert* at compile-time and then against your own *assert* at runtime. That way one elides any cost.

[vemv]

I was thinking of a 'double-checked' pattern i.e. first you check against clojure.core/assert at compile-time and then against your own assert at runtime. That way one elides any cost.

Using more precise wording, if clojure.core/*assert* is false at compile-time, you macroexpand to 'nothing'.

[borkdude]

Adding spec to the mix seems to go against I remain extremely conservative about bringing in any new dependencies into rewrite-clj.

I don't really agree with this one: spec is an already available dependency and loading the specs can (and imo should) be made optional for users by providing them in a separate namespace. Instrumentation can be chosen at a fine-grained level (per function even).

[vemv]

Well I'll leave this one to @lread however this thing you wrote here sums up well a common sentiment:

You wrote fdefs. But how are you going to test them?

i.e. instrumentation is bit of a frustrating experience, that can be tracked in various places (for example the Expound issue tracker comes to mind; integrating it with Orchestra is not trivial).

Used expertly, sure it should work. I'd simply be pessimistic about edge cases i.e. rewrite-clj can be used in unexpected ways (e.g. inlined via mranderson) and composability (namely: how different rewrite-clj consumers instrument rewrite-clj concurrently)

Whereas a custom check macro can be so simply reasoned about.

[borkdude]

The respeced library aids in (unit) testing your fdefs. It was specifically written to verify if the fdefs written in speculative were correct: will they throw on certain function calls, and will they allow other function calls? This is one level of testing higher than what we're talking about here.

I'm not necessarily a fan of everything spec does and looking forward to spec2, but meanwhile providing a foo.specs is a pretty standard / built-in way to solve this problem in Clojure libraries in 2021.
E.g.: https://github.com/schmee/java-http-clj/blob/master/src/java_http_clj/specs.clj

Having said this, some custom code that doesn't incur any performance penalty when you won't want validation, could work.

[vemv]

I'm aware of the pattern however I'd be hesitant about instrumentation. Isn't it essentially a global side-effect? Whereas a binding would allow a per-consumer setting.

e.g. for the refactor-nrepl use case, we can bind e.g. [rewrite-clj/*assert* true] once at the middleware level (i.e. at the 'entry point' of execution, early and up) and rest assured that:

• all refactor-nrepl internal defns are being invoked under said binding
• we're not messing with libraries' or the user's preferences

If the exact same thing can be said of instrumentation, so much the better, LMK

[borkdude]

For me personally these extra checks are only useful during development and even then, mostly in development of rewrite-clj itself. Why is it important to turn this on in production code of refactor-nrepl? What action can the user take when something turns out to be wrong in the way that refactor-nrepl is using rewrite-clj? Moreover, I think these checks exist mostly as a development aid of rewrite-clj itself, since node construction isn't usually done "manually"?