Convention for necessarily required but not explicitly used namespaces

[borkdude]

Ideas for expressing: this namespace is necessarily required but not explicitly used (loading specs, loading a foreign lib, multimethods, etc).

Problem

The problem is that some tooling warns or even removes required namespaces that are not explicitly used in the code. However, sometimes these namespaces are still used for registering specs, multimethods, etc. So there should be a way to say: we need these namespaces regardless of explicit usage.

Example:

(ns foo
  (:require [foo.specs]))

Here specs related to foo are loaded, but the namespace foo.specs is not used explicitly.

Related issues with earlier discussions:

• clean-ns: Introduce ^:keep metadata on libspecs clojure-emacs/refactor-nrepl#189

Ideas:

1. single symbol libspec

Example:

(ns foo
  (:require [normal.ns]
             my-side-effecting.ns))

Pro: simple, no clutter
Con: with this approach: there might be others that are accidentally also unwrapped and not reported.
Con: doesn't work for unused java imports (but these don't cause side effects anyway).

2. Attach metadata ^:keep on a vector in which the ns name is wrapped

(ns foo
  (:require ^:keep [my-side-effecting.ns]
            [normal.ns :as n])
  (:import ^:keep [java.lang String]))

(n/foo)

Pro: works for requires + java imports
Pro: less accidental than 1
Con: breaks vertical alignment. However, it seems clojure-sort-ns already respects metadata:

  (:require
   ^:keep [bar.specs]
   ^:keep [foo.specs]
   [another.namespace.a]
   [another.namespace.b]))

so maybe this wouldn’t be a problem, since ‘keeped’ libspecs go on top anyways.
Also you can use a newline after the metadata to fix the vertical alignment.

Con: you always have to use a vector to place metadata on, since CLJS allows strings as namespace names in :require. This is not really a con, since this is already encouraged style in how to ns.

3. Empty :refer vector

(ns foo
  (:require [my-side-effecting.ns :refer []]
            [normal.ns :as n]))

(n/foo)

Pro: less clutter than 2
Pro: less accidental than 1

4. Use a comment or uneval (this has been my approach so far in earlier projects)

(ns foo
  (:require [my-side-effecting.ns] ;; keep
            [my-side-effecting.ns2] ;; #_keep
            [normal.ns :as n]))

(n/foo)

Pro: less accidental than 1
Pro: better alignment than metadata
Con: clutter
Con: comments might not be seen by all tooling at the moment the AST is processed (clj-kondo included)

5. Wrap in list instead of vector:

(ns foo (:require (foo.specs)))

Pro: no clutter
Con: might be accidental

[dadair-ca]

I like the metadata idea, because that's what it is—metadata on the require for other tools to use. It's explicit, and doesn't conflate existing patterns with implicit meaning.

[borkdude]

Conversation with @bbatsov on Slack:

bozhidar: I prefer option 2. I totally agree we should document this in the style guide. I’ve many times deleted a namespace like this, because joker told me it was unused. 🙂
borkdude: @bozhidar I like that idea, except that it breaks alignment
[^:keep foo] would also be possible, but some tools might not see the metadata because you can’t actually add metadata onto a symbol 😉
bozhidar:
Good points.
Well, I guess option (EDIT: 3?) is the most balanced then, but I assume some linters might consider an obsolete refer. 😄
Tricky situation…
borkdude:
clojure-mode could be adapted to align vectors vertically in namespace declarations
borkdude
about vertical alignment, it seems clojure-sort-ns already respects metadata, kind of:

  (:require
   ^:keep [bar.specs]
   ^:keep [foo.specs]
   [another.namespace.a]
   [another.namespace.b]))

so maybe this wouldn’t be a problem, since ‘kept’ libspecs go on top anyways

[didibus]

The #2 idea of using metadata seems the best to me definitely.

The only thing is, I think the meta should be namespaced. Hasn't the style namespace been used before?

^:style/keep

Or maybe a new linter namespace could be started:

^:linter/keep

Something like that.

I'd find that with the namespace, not only we prevent clashes. But also, it is even more explicit. Someone can quickly understand that the meta isn't part of the actual program but only for tooling as style or linting hints.

I'm really against #1 and #3 and #5. The ns macro is already so hard to comprehend and has so many rules. I don't want to add implicit tooling constraint on top.

For the comment idea, #4, I think it's just a bit weird. Feels more hacky then meta. Doesn't feel like it integrates with the language as naturally. Also what if I also want to put a comment? And what about comment before the line instead of after? Basically, the comment seems like it requires whitespace awareness. That said, the escape form could be a good option, but your example of it is weird, I was thinking more:

(ns foo.bar
  (require
    #_keep [bar.specs]
    [another.namespace]

This way, you use it like meta, but it's obvious that it's not program meta. It can apply to single forms, so whitespace doesn't matter, so you can do this:

(require #_keep [foo.specs] [foo.bar :as bar])

Maybe let's call inline escaped form option #6 ?

Another point to consider, what if we have a call to require out of the ns? Do tools remove those as well if they are unused? If so, should our solution work for those as well? For some reason it seems this doesn't work:

(meta ^:linter/keep '[foo])

And I'd like to add a #7 option as well. Add a standard styling/linter config file. Personally, I hate adding extra hints that aren't actually part of the logic just for tooling. So if there was a file say tooling-hints.edn and I could use that file to specify what namespace requires I want to keep, I would like that as well. What's good is, you can have option #7 on top of any of the other one. So maybe we go with #2 + #7 or #6 + #7. Both these combo would be my preferred ones.

In config file:

{:ns/keep [foo.specs bar.specs]}

[borkdude]

@didibus

Unevals (#_keep) are maybe not seen by some tooling which deal with the pure sexprs, so let's forget about that idea.

So far, the metadata option seems most popular. Namespacing it is a good idea, but until now I haven't seen any other tooling doing this, so clashes seem unlikely to me. If we were going to namespace it, I think word linter or style would both not cover the use case of tooling which is automatically going to remove it, so maybe ^:tooling/keep would be better?

A unified Clojure linter config file format is maybe also good idea, but I think too wide in scope for this issue. Both joker and clj-kondo both support this already in their own config files.

[didibus]

^:tooling/keep

I like it. Don't care too much what the namespace is, just like it to be. So we protect against future clashes or possible clashes with program code and it makes it more explicit it is used by tooling and not the actual program. The namespace tooling sounds like a good one for that.

Config file can be added later. Also, I don't mind as much having to configure multiple config files, as long as the tooling has that option so I can keep my code clean and free of tooling concerns if I want too.

So then my vote would be for ^:tooling/keep meta!

[didibus]

One more thing though, what are the existing meta used by popular tooling? Maybe we should take that into consideration a bit to try and make them all somewhat coherent?

[tonsky]

You totally can add metadata to symbols.

user=> (meta (with-meta 'abc {:a 1}))
{:a 1}

[borkdude]

You totally can add metadata to symbols.

Thanks. I must have confused this with keywords.
Since CLJS also supports strings as namespace names in libspecs, we still have the same problem though.

[didibus]

You totally can add metadata to symbols.

Ya, but somehow, not with the literal syntax:

(meta ^:a 'abc)
nil

[dadair-ca]

I’m tempted to suggest something more general like :ns/effectful to express intent rather than making it tied to the tooling aspect of it, but I can’t think of a non-tooling use case for it.

…

Sent from my iPhone

On Jun 7, 2019, at 11:43 AM, Michiel Borkent ***@***.***> wrote: You totally can add metadata to symbols. Thanks. I must have confused this with keywords. Since CLJS also supports strings as namespace names in libspecs, we still have the same problem though. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[tonsky]

Don’t put anything between meta and symbol:

user=> (meta '^:a abc)
{:a true}

It totally works, Clojure uses that to extract type annotations from function name/local params etc

[didibus]

So, I think Cider is the only thing I know that uses meta for tooling hint, and it does for formatting style using namespace key style

:style/indent

So I think using :tooling/keep would be consistent with that. Style hints can go under :style and other tooling related hints can go under :tooling.

An interesting this is that Cider have you specify the style at the function or macro level. That got me thinking, could we use meta on the namespace instead? That would solve all these issues we've been talking about:

(ns ^:tooling/keep [foo.specs bar.specs "bar.foo"]
  foo.bar
  (require
    [foo.specs]
    bar.specs
    "bar.foo"))

These would now all be supported

[borkdude]

@didibus Metadata on the ns would leads to duplication of namespace symbols and that would lead to another type of linter I'm afraid, while we were trying to make life easier on tooling :-). On the other hand, I guess you would discover it soon enough if you misspelled one of those, by getting a warning you didn't expect.
Fun fact: I have been considering this idea for namespace local clj-kondo config itself: #148

[didibus]

Would the elide-meta compiler flag be able to elide all this meta out of production builds? I'm not sure if it elides all meta on all things, or just on Vars? Anyone knows?

[kawas44]

Why not keep "side-effecting" requires outside of the ns macro ?
Linters could still clean the ns form and leave explicit requires by themselves.

Most people see the ns form as declarations of stuff to "compile" your code, so coming from others languages it feels natural to tidy those declarations.

But when you want "side-effecting" requires, it is usually to set some state for your running program. It seems to me, that it should live outside of the ns form.

Instead of adding new meta-keys convention on top of the language for tooling, maybe we just need to advise (in clojure style guide for example) the explicit usage of require out of the ns macro for runtime side-effects?

[borkdude]

Why not keep "side-effecting" requires outside of the ns macro ?

Because that isn't possible in CLJS.

With side-effect we mean loading specs, multimethods etc, so you are not using a function from those namespaces directly, but do need stuff to be registered.

[kawas44]

Why not keep "side-effecting" requires outside of the ns macro ?

Because that isn't possible in CLJS.

With side-effect we mean loading specs, multimethods etc, so you are not using a function from those namespaces directly, but do need stuff to be registered.

I was thinking about loading specs as well (setting the state of the spec registry).
But I did not know that CLJS cannot use require outside of ns.

In that case I'll go for meta annotations because some formatting tools may move comments.

[zane]

Another option, and the one I would probably prefer, would be to allow the passing of this kind of metadata to clj-kondo via a configuration file. I'm imagining something along the lines of how you can inform JSHint about global variables via a .jshintrc or package.json.

[candid82]

@zane both clj-kondo and joker already support this in their config files (:ignored-unused-namespaces in joker and :unused-namespace -> :exclude in clj-kondo). I do like this approach because it keeps the code uncluttered. I also think that it's usually a good idea to have a more elaborate comment like ;; required for XXX multimethods rather than just ;; keep, and if there is already a comment, additional signals like ^:tooling/keep are unnecessary from readability point of view.
^:tooling/keep might be a good option though (in addition to config files).

[zane]

@candid82 Ah, nice! I should have looked at the documentation more closely.

[borkdude]

@candid82 I agree that keeping the code uncluttered is preferable. Adding this metadata option has two reasons for existence:

1. More fine-grained control: tell the tooling to ignore this require only within certain namespaces
2. Tools like refactor-nrepl are not going to remove the requires automatically (see issue). It seems unlikely to me that refactor-nrepl is going to look into .joker or .clj-kondo/config.edn files.

[borkdude]

@cursive-ide responded on Twitter:

To summarize the current state of the discussion: it seems pretty much everyone is in favor of metadata (^:tooling/keep) although the exact keyword and where to put the metadata (on the vector or on the namespace as a global config) might still change.

[mynomoto]

I use 1 at the moment, but after thinking about the tradeoffs, I believe that 2 is the better option.

[vemv]

Note that by placing a newline after the metadata marker, one would preserve vertical alignment:

(:require
  ^:keep
  [bar.specs]
  [something.else]
  ^:keep
  [foo.specs]
  [clojure.string]

[fmnoise]

what do you think about ^:require/keep?

[vemv]

Maybe :orchard/keep, maximizing googleability and minimizing taking over something nobody really owns (especially :require/ sounds like belonging to the Clojure compiler; :linter/, :style/ a bit less but also nobody owns those concepts)

Unqualified ^:keep would sound good to me tbh, as it seems very unlikely that the Clojure compiler will ever have a meaning for that, especially when attaching metadata in usages (as opposed to definitions).

Plus there's a RC process so if that addition broke anything, we'd notice.

[RickMoynihan]

I'm not a fan of ensure-loaded because require ensures a namespace is loaded. An alternative to keep is retain... though as others have said I think it's better to state a fact about your code, rather than a behaviour for your tooling.

[borkdude]

Another option is:

When there is no :as foo or :refer [foo] the tooling MUST assume that this namespace is implicitly used?

I think this works for 99% of my "implicit requires"?

Example from a file that was in front of me right now:

The implicit requires are dre.yada.html and dre.yada.xml which are exactly the only ones that do not have an alias or a :refer.

[RickMoynihan]

^:implicit brings to mind other meanings, e.g. C#, Scala implicits etc...

When there is no :as foo or :refer [foo] the tooling MUST assume that this namespace is implicitly used?

That for me is definitely the best of all. If tools did this already I wouldn't need to use my require trick.

It would imply if you wanted both the effects and a var, but expected the var might be moved at some point you'd need to do something like:

(ns foo.bar
  (:require [foo.bar.baz]
            [foo.bar.baz :as baz]))

which seems fair enough to me.

[tatut]

When there is no :as foo or :refer [foo] the tooling MUST assume that this namespace is implicitly used?

⬆️ for this, less is more and there is no need to agree on new metadata keywords.

[arichiardi]

The only problem with this latest proposal is that you might still end up with wrong detection in case you forget an :as foo after a refactoring or a junior colleague does not know the convention... explicit is always more better IMHO...so I would vote for an explicit meta.

Maybe she two mechanisms for detecting this particular issue can cohexist?

A wording on the namespace of the keywords: I would not go for a ^:require/ because we are already in a require - the scope is clear.

What about ^lint/...?

[borkdude]

The only problem with this latest proposal is that you might still end up with wrong detection in case you forget an :as foo after a refactoring or a junior colleague does not know the convention

In that case you would end up with a false negative, which is probably better than all the false positives the tools are producing right now?

I'm fine with ^:lint/.., or ^:tooling/.. (which is more general, as not every tool is a linter, but a linter is always a tool): maybe we're bikeshedding.

[arichiardi]

Yeah...you are probably right on the former

On the latter my only concern is that we end up being incoherent with the other annotations we have for indentation (is it ^:style/?).

We should aim at a specific one if we want to keep being specific or give both a broader scope like ^:tooling

[arichiardi]

Sorry I realized my comment didn't add much to the conversations above.

I will propose my variant:

^:ns/effects

Coherent with the current narrow scope (like style), concise and agreeing with @RickMoynihan on the terminology (plural because I can register more than one protocol for instance).

[borkdude]

I've merged effects and side-effects into one option, so the votes can go here:

#241 (comment)

I assume people voting for ../side-effects would also be OK with ../effects.

The namespace ^:ns/..choice is already available here: #241 (comment)

[svdo]

Initially I liked @borkdude's suggestion about doing it implicitly, but after giving it a bit more thought I actually think I'd prefer something more explicit. That makes it a lot easier for people who are unfamiliar with it to find out (search) what is going on, what the annotation is for.

:lint/... or :tooling/... doesn't matter too much to me, I guess :lint is the more explicit, intention-revealing of the two. I mean, that's why this discussion was started in the first place I guess, right? Or do we already know of other types of tooling that would use this?

So I guess I still like my initial suggestion of implicitly_used (which is to me more clear than implicit, but I can live with either ;) ).

[borkdude]

This can potentially be used by more than only linters. See the issue mentioned in the top post (clojure-emacs/refactor-nrepl#189).

[svdo]

After having a bit of a side discussion on Slack, @borkdude and I came up with this suggestion:

^:used

Short, to the point, and clear, I like it :)

[danielcompton]

There are three broad classes of readers of this metadata:

1. Tooling
2. People who understand what the metadata means
3. People who don't understand what the metadata means

This is a tricky balancing act to be sure. I think that names like ^:used or ^:implicit don't have enough meaning on their own, and may confuse people who haven't seen it before. ^:ns/effects, ^:require/implicitly_used, and even ^:tooling/keep are more likely to be understandable, or at least give people an idea of what is going on.

[borkdude]

The newest clj-kondo already supports this feature.

[didibus]

For people suggesting that the name should be about declaring that the namespace is effectful, can we name a single example use case which would rely on this which isn't just so things that clean up the ns form don't accidentally clean it up?

Keep in mind this is a hint, so it should be assumed sometimes things might be labeled with it that aren't effectful as well. So the use case would need to be okay with that reality. Just like type hints are.

[didibus]

I just thought of another name:

^:tooling/do-not-remove

[cursive-ide]

For people suggesting that the name should be about declaring that the namespace is effectful, can we name a single example use case which would rely on this which isn't just so things that clean up the ns form don't accidentally clean it up?

Yes. When a user loads a file in Cursive, I transitively find all its dependencies and also load them if they're out of date. I could use this to warn the user if they're accidentally pulling in a side-effecting namespace, or allow them to specify that they never want side-effecting ns'es loaded.

[borkdude]

@cursive-ide That seems different semantics than originally envisioned in this topic. E.g. a namespace that only registers multimethods should be loaded, but isn't explicitly used. You want to mark that as foobar/side-effects or foobar/keep, but that doesn't seem to apply to your kind of use case where it's more like foobar/launch-missiles?

[njordhov]

The problem is that some tooling warns or even removes required namespaces that are not explicitly used in the code.

I like the current convention that required namespaces can be removed if not explicitly used in the code. Perhaps the tooling problem can be solved without changing the convention?

For example, by code explicitly referencing such namespaces, using metadata to declare that a function depends on the side effects of the namespace.

Take a re-frame app that requires a namespace declaring event handlers as side effects only, a recognized gotcha that can be resolved by explicitly referencing the namespace where it is used:

(ns app.core
  (:require
    [re-frame.core] 
    [app.events]))

(defn ^{:require app.events} start []
  (re-frame.core/dispatch-sync [:init]))

The metadata declares that the start function assumes re-frame event handlers (:init) defined as side effects in the app.events namespace. If the function is not called by other code, the compiler is free to prune out the app.events namespace (assuming it is not required elsewhere).

[didibus]

@cursive-ide Hum, that's interesting. Just some thoughts:

1. Isn't that already the point of defonce? Except it's already at a more granular level.
2. Wouldn't it be nicer for that to flag the namespace as side-effecting on load in the actual namespace which is? Instead of at every place where it is required?
3. Like @borkdude mentioned, it seems those two use cases don't fully overlap. I can think of cases where I want a namespace to be reloaded, yet I don't want it cleared from the ns form. So maybe having a seperate annotation is better, like ^:tooling/do-not-reload or ^:tooling/load-once
4. In my opinion, that's just bad style. We should encourage people not to have side effects of that sort on load. Your code shouldn't perform things that would break if it were to be AOT compiled for example (which does a full transitive load). You need to wrap that kind of logic in delays or functions. Doing that is both better code in my opinion, and solves the issue of Cursive transitive loads.

[borkdude]

June has ended so it's time to make up a summary of this issue before I close it.

^:tooling/keep and metadata on the libspec (instead of on the namespace) were the most popular options.
See:

• Convention for necessarily required but not explicitly used namespaces #241 (comment)
• Convention for necessarily required but not explicitly used namespaces #241 (comment)

But while voting, another option emerged, which is: if a libspec has no :as or :refer it is assumed to be used for side-effectful loading (of multimethods, specs, etc) so it should not be removed by the tooling.
See:

• Convention for necessarily required but not explicitly used namespaces #241 (comment)

The latter option has been implemented in clj-kondo for a while now and personally I find it a satisfying solution.

[marcomorain]

@borkdude thank you for all the hard work on this issue <3

[danielcompton]

@borkdude so to clarify, the outcome of this discussion was to not add any new metadata or keywords to the libspecs, and just rely on the convention that if there is no modifiers in the libspec then it is being required for side-effects?

[borkdude]

@danielcompton Correct. Example:

(ns foo
  (:require [foo.specs] ;; <-- leave as is
            [bar.utils :as bu] ;; <-- unused, free to remove
    ,,,))

Of course tools are still free to pick the most popular metadata option, if they prefer that solution, but for clj-kondo I think this is sane default behavior.