clean-ns: false positives for symbol usage

[vemv]

Hi!

Sometimes I may want to :require something just for using the alias later for symbol construction, with no var usage.

For example:

(ns om.g
  (:require
   [protocols.xyz :as xyz]))

(defn abc-impl [this] ...)

(defn new []
  ^{`xyz/abc abc-impl}
  {})

The above is a vanilla usage of metadata-based protocol extension, a new Clojure 1.10 feature.

If the ns didn't use xyz for any vars, clean-ns will wipe the require. And Clojure won't complain when compiling the ns again, since:

`xyz/abc

...is always a valid token.

Thanks - Victor

[benedekfazekas]

i suppose you can use cljr-libspec-whitelist config option to work around this.

[vemv]

Thanks!

That will help for the moment, had forgotten about the possibility.

Anyway I hope the root cause can get solved eventually.

I'd love to give it a shot myself, but can't at the moment. Busy life...

[expez]

We don't look for symbols in the metadata, which is why this isn't handled.

I'm not sure how much slower this would be, if we also looked at the metadata of everything. It might not be worth it to to handle this in general. In other words, to avoid not making clean-ns too slow, we might have to introduce a new setting to also consider metadata only when desired. At that point it might be just as easy to use the whitelist?

There's a half-finished PR in this repo about adding metadata to keep certain symbols from getting cleaned out. So that would be another solution to this.

At work I've used a third solution: I have one or two namespace forms that are immediately followed by a (require ...) form with a comment about this ns getting loaded for side-effects and it's in an explicit require to avoid tooling removing it when cleaning the ns form. If it was only me on the team I would've whitelisted it, but I'm not sure what the other tools would do so I left it like that.

Thanks for the report!

[vemv]

Gotchu!

Just let me make sure - wouldn't it be pretty fast to just slurp a given .clj file and do a plain naive search of ocurrences of a given alias?

e.g. for [clojure.string :as str] we grep that .clj file for the str/ string (or regex). This includes symbols (this issue), function calls in metadata (#240), and possibly other edge cases.

Admittedly this wouldn't be as powerful as a proper code analysis, but I think it'd work, with few possible false positives/negatives.

I'd say that it's better to err on the side of caution (don't strip requires because of a spurious match) than the opposite (strip requires because of a non-exhaustive search).

[expez]

Just let me make sure - wouldn't it be pretty fast to just slurp a given .clj file and do a plain naive search of ocurrences of a given alias?

Yes, that would be super fast. We did this in the first version of clean-ns where it was all implemented in elisp.

Anyway, the only reason this isn't working is because I didn't know anyone was referencing external symbols in their metadata when I implemented clean-ns :) Over the years there's been quite a bit of "I didn't know you could do that!' or "I didn't think anyone did that!" that's lead to changes in refactor-nrepl. Pretty terrific way of learning the language deeply, really.

[vemv]

:)

May I know if it is likely that this will get it fixed?

Perhaps it's a matter of git archeology, recovering the old approach and adding to the current behavior (perhaps optionally).

Can give it a shot if you don't foresee personal availability.

[expez]

May I know if it is likely that this will get it fixed?

I won't have time to implement this myself, but I'd be happy to help with code review or to answer any questions you might have if you give it a shot yourself.

Perhaps it's a matter of git archeology, recovering the old approach

You can certainly find this. It's in the clj-refactor project, though. We moved it over to the middleware quite a few years ago. 2015 maybe?

adding to the current behavior (perhaps optionally).

We moved away from the old approach for a reason :p It was buggy and the implementation was getting to be super convoluted, so I'd rather extend the current implementation with better support for metadata than revive the old one.

[vemv]

Thanks for the info! Appreciated.

I could implement a metadata-safe "used-namespaces" function, using it as a whitelist when interacting with refactor-nrepl.

It's working fine. In a couple weeks after some due battle-testing I'll be able to open-source it and open a PR.

[expez]

a whitelist when interacting with refactor-nrepl

We already have this in cljr-whitelisted-libspecs. It was implemented to support namespaces that are loaded only for side-effects, but you could put your metadata namespaces into that list if you want a manual solution.

[vemv]

Thanks for the pointer!

I was aware of the whitelist option, but it didn't occur to me that I could pass different whitelists on a per-file basis.

That simplified my implementation :)