Codox executes any code from namespaces during documentation generation

[esuomi]

Intro

Codox uses tools.namespace to load project namespaces. These namespaces are then required directly by codox to load the namespace and further various Clojure functions are used to extract the documentation strings and such.

The problem

This loading of the actual namespace has the unfortunate side effect that all and any code within the namespace gets executed as is because that's how Clojure works. As minimal example, consider this namespace definition in project path (eg. src/hello.clj):

(ns hello)

#=(println "Hello! I am reader eval macro")
(println "Hello! I'm just a printline")

When running codox with eg. lein codox both of these lines are visible in the console output:

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Hello! I am reader eval macro
Hello! I'm just a printline
Generated HTML docs in docs

This is troublesome. One could argue that top-level in source files should only contain side effect free def, defn etc. function calls which don't do anything when loaded, but this is unfortunately not true at all. Sente recommends using let at the top level, clojure.test's use-fixtures evals itself immediately and several macros which look like defns end up executing code when just read. In general, namespace should not get eval'd just to extract documentation.

Potential solution

I am currently working on a tangential problem (compare Clojure function definitions programmatically) for an unrelated reason and as I spent a bit of time to figure out how to safely read foreign Clojure code without evaluating it using clojure.tools.reader/read I had a small epiphany and decided to test how codox does this. And now I'm here, so yeah...

Anyway, I'd suggest reworking the parsing of namespace information using tools.reader instead with *read-eval* disabled. If time allows, I will eventually share my code as well to allow reading namespace data securely that could potentially be reused by codox, but that won't happen for a while so I don't recommend waiting on that... :)

[weavejester]

One could argue that top-level in source files should only contain side effect free def, defn etc. function calls which don't do anything when loaded

One could, and I do :)

Namespaces shouldn't have side-effects beyond defining (and sometimes altering) vars. Requiring a namespace should always be safe.

Sente recommends using let at the top level

Does it? In its README it gives a simplified example that uses let at the top level, but I'd imagine that's only for the sake of brevity.

clojure.test's use-fixtures evals itself immediately

Doesn't use-fixtures just add some metadata to your test vars? The only side-effect it has is altering the namespace's vars.

and several macros which look like defns end up executing code when just read

As long as the macro is only defining or altering vars, that's fine.

Anyway, I'd suggest reworking the parsing of namespace information using tools.reader instead with *read-eval* disabled.

Static analysis of the source code is fine to have as an option. Codox already statically analyzes cljs code in order to produce the documentation. If you want to give people the option of using the Clojure analyzer to find documentation, I'd welcome that PR.

However, it shouldn't be the default, and it certainly shouldn't be the only option. Sometimes documentation is produced by macros or functions that alter vars; Codox should handle that by default, and if people are writing their namespaces correctly, there shouldn't be an issue except in certain edge cases.

[esuomi]

Requiring a namespace should always be safe.

mount for example registers states when namespace containing defstate is required. It thankfully has separate set of functions for actually starting/stopping application states, but there's no guarantee those wouldn't be somewhere in eg. core ns of application on load time.

Sente recommends using let at the top level

Does it? In its README it gives a simplified example that uses let at the top level, but I'd imagine that's only for the sake of brevity.

This is of course digressing, but even their actual example project in the repo does this - I do agree it's not the cleanest way to do it, but it works quite well as an example of how wild and unruly people sometimes are :) Of course the one project where I use sente I've done the sensible thing and wrapped it to not do that, but unfortunately software has a slight problem with ankle biters like this at times.

Doesn't use-fixtures just add some metadata to your test vars? The only side-effect it has is altering the namespace's vars.

Ah, scratch that then from my list of examples :) This popped into my mind slightly misguided as we've had sort of related issues with it; things getting run multiple times when they shouldn't etc...unfortunate mental association in this case.

All that said, this isn't as an issue something I'd personally consider world-ending or absolutely terrible. All I wanted as result was to have this discussion, really :) I might look into that analyzer contribution PR eventually if the code I'm working on currently turns out to be reasonable enough to be shared in such way.

[weavejester]

I'd consider any namespace that has side effects beyond changing vars as doing The Wrong Thing™. The only way to be completely certain as to what vars are in a namespace is to actually evaluate it, so if a namespace is designed to have side-effects on load, it can't be fully documented or analyzed.

We can work around it to a degree by using static analysis and looking for known forms like def and defn, but there's no guaranteed way of documenting the namespace without evaluating.

[esuomi]

Closing this as there seems to be no further interest on the topic and the current functionality is by design.