technomancy/slamhound

Slamhound is confused if a namespace aliases to user

Closed this issue · 11 comments

We have a namespace worldsingles.user which we commonly alias to user in our ns require. This confuses Slamhound - it fails to resolve any names from the user/ alias. Example: https://www.refheap.com/61516

Since we have this alias all over our 18kloc it makes it hard to use Slamhound :)

guns commented

Ah, this is because user is a fully qualified ns on its own, so Slamhound does not recognize it as a possible alias.

I'll have to think about this one for a little bit… thanks for the report!

guns commented

@technomancy

This is pretty gross, but what do you think about the following workaround?

  • Munge symbols in the candidate body matching user/(.*) to a-gensym/$1
  • regrow ns
  • Update the final ns-reference map by swapping any alias values
    matching the gensym back to user (there should only be zero or one)

This could be implemented easily as a wrapper around regrow so there would be no need to pollute the logic for this hack.

I do think this is worth working around since user is a fine name for an alias, and it is the only (?) official single segment ns.

Is anything from the (default) user namespace ever available to other namespaces without an explicit require bringing it in? Is there real world code out there that really expects to bring in the (default) user namespace rather than something the developer created? Do people really write code in src/user.clj and work with that as a single segment ns?

Just curious, since the fact that the (default) user namespace was even a candidate for Slamhound surprised me...

guns commented

It's more that user is active and available during development, so from Slamhound's perspective, user/foo isn't semantically different from clojure.string/join.

Do people really write code in src/user.clj and work with that as a single segment ns?

I define several debugging functions/macros in my ~/.lein/user.clj, then insert them temporarily into my namespaces when I run into trouble. AFAIK, this is a sanctioned use of the user ns.

Thanx @guns - I wasn't sure that ~/.lein/user.clj was still a supported thing for Leiningen.

guns commented

Actually, on closer inspection, it appears I am abusing ~/.lein/user.clj by loading it in my :repl-options -> :init!

Regardless, the user ns is typically created/loaded by the REPL and thus is globally available during development. Slamhound currently uses the compiler to identify missing ns references, and the compiler will report user/var as a missing var in an existing ns, not as a totally unknown reference.

… which means, actually, that this is a general problem with single segment namespaces and not just a special case for user.

I think I see a better solution now. I'll try to post a solution tonight.

Oh, it gets better!

(ns xy.z
  (:require [clojure.set :as clojure.string]))

clojure.string/union ; -> #<set$union clojure.set$union@717e57a>

Technically there is no way to tell the difference between an alias and a fully-qualified namespace just by looking at them. Fun times! However, realistically I am totally fine with special-casing user; people who run into collisions with other namespaces are just looking for trouble.

guns commented

Ugh, aliases can have dots in them? I was just writing a test for a simple change that assumes that isn't possible:

diff --git a/src/slam/hound/regrow.clj b/src/slam/hound/regrow.clj
index fa0c2ab..d5389f6 100644
--- a/src/slam/hound/regrow.clj
+++ b/src/slam/hound/regrow.clj
@@ -59,7 +59,8 @@
                   "Can't resolve: "
                   "No such namespace: "
                   #"No such var: \S+/"]]
-    (mapv #(Pattern/compile (str % sym-pat)) prefixes)))
+    (into [#"No such var: ([^\s\.]+)/.*"]
+          (mapv #(Pattern/compile (str % sym-pat)) prefixes))))

 (defn- missing-sym-name [msg]
   (second (some #(re-find % msg) missing-sym-patterns)))

This patch would return the ns qualifying portion of the missing reference instead of the var part when the missing ns part has no dots.

I like this change better, but it doesn't work if clojure.set is a valid alias.

(sorry for the close/reopen, hit the wrong button)

guns commented

I asked clojure-dev about the status of dots in aliases:

https://groups.google.com/d/msg/clojure-dev/8LNeAcip9fM/jiUU2Bi80XcJ

If this is a feature, then I will add the user workaround. If this is undefined, then I'd like to add the change above.

guns commented

It's been two days since the post to clojure-dev without an official response, but it turns out that dots in aliases is not the problem at all.

There is only one condition that triggers the error message "No such var":

  • We are trying to resolve a var ns-sym/var-sym,
  • ns-sym is an existing namespace,
  • but there is no var named var-sym in that ns.

https://github.com/clojure/clojure/blob/f758350f9b66750483cb712f7403652261e7f0e1/src/jvm/clojure/lang/Compiler.java#L6914

Therefore Slamhound is returning the wrong part of the missing reference in this situation (should be returning the ns-sym, not the var-sym).

This bug is a holdout from a time when Slamhound was not distinguishing between missing aliases and missing refers at this point in the regrow process.

In the end, Slamhound will support silly aliases like (:require [clojure.set :as clojure.string]) since it's not explicitly disallowed. There's even a test case for it!

@seancorfield

Thanks for the report; I look forward to seeing how Slamhound handles the World Singles code base! (please mind the gotchas regarding macros).

It certainly solved that problem. After installing a local snapshot from Git, I can run Slamhound (via LightTable, FYI) on the namespace that caused problems before. It eliminated an unused :as alias too. Yay!

I ran it on a few other namespaces and ran into problems in a couple of other places - I'll open tickets for those.