Removes import only used in type hint

[seancorfield]

We import several classes only to use them in type hints. Slamhound removes the import for them.

Example:

(ns worldsingles.newrelic
  (:require [worldsingles.environment :as env]
            [worldsingles.logging :as log])
  (:import (com.newrelic.metrics.publish Agent AgentFactory Runner)
           (com.newrelic.metrics.publish.configuration ConfigurationException)
           (com.newrelic.metrics.publish.processors EpochCounter Processor)))

...
        (proxy [Agent] ;; extends Agent
            [agent-name agent-version] ;; super (constructor)

          (getComponentHumanLabel ^String [] name) ;; displays in New Relic

          (pollCycle [] ;; called every cycle by New Relic process
            (probe (fn [^String label ^String scale type value]
                     (.reportMetric ^Agent this label scale
                                    (if (= type :value)
                                      value
                                      (.process ^Processor (get-rate label scale) value)))))))))))

Agent is retained - used in the proxy call - but Processor is removed.

We ran into this with other New Relic code where we import com.newrelic.api.agent.Trace and use ^{Trace {}} as metadata to create Java annotations. Again the import is removed.

[guns]

I've identified the problem. Consider the following ns:

(ns slam.metadata-problem)

(def example-pattern
  #"foo")

(defn example []
  (.pattern ^Pattern example-pattern))

Running Slamhound on this buffer works as expected:

(ns slam.metadata-problem
  (:import (java.util.regex Pattern)))

(def example-pattern
  #"foo")

(defn example []
  (.pattern ^Pattern example-pattern))

However, if we change example-pattern to a function:

(ns slam.metadata-problem)

(defn example-pattern []
  #"foo")

(defn example []
  (.pattern ^Pattern (example-pattern)))

Slamhound does not detect the ^Pattern tag.

This clearly has something to do with metadata interaction with lists. While it's not yet clear whether we can really do anything about this on our end without hijacking the reader, there is an obvious solution that works in both this buffer and your example:

(ns slam.metadata-problem)

(defn ^Pattern example-pattern []
  #"foo")

(defn example []
  (.pattern (example-pattern)))

Tagging the var directly avoids reflection and also allows Slamhound to find the missing reference to Pattern.

I'd like to keep this open until I can ascertain whether or not there is a nice solution for this.

Thanks again for all the feedback! This is really great.

[seancorfield]

Interesting. So using a local let binding for the result of (get-rate label scale) should solve that? Let me try...

[seancorfield]

Yup, that works. Cool. This is also most likely the cause of Slamhound removing the com.newrelic.api.agent.Trace import since Trace is only used in metadata: ^{Trace {}}? I can use the fully-qualified type there instead of importing it I suspect, which should make our entire codebase Slamhound compatible.

[guns]

Well, it looks like literal metadata maps don't work either way. They do work on symbols on defn and defmacro however:

(defmacro ^{:require [Pattern]} make-pattern […])

This will require a deeper dive into the reader/compiler.

[seancorfield]

Whilst this issue still persists, we were able to make our code base "Slamhound-compatible" and run Slamhound across every source namespace. We haven't run it across our test namespaces yet, but probably will (although those are much simpler ns declarations). We really like the cleanups!

[guns]

That is very cool. Thank you for sticking through it and reporting issues.

I've always wanted a tool like Slamhound in every language I've worked in¹, so I am very eager to see this project become a standard tool among Clojure developers.

As for this metadata issue, I hope to have it fixed over the weekend.

¹ I am vaguely aware that "IDE"s may have had this feature for some time now

[guns]

I think I have discovered the issue. Commit 54cba5c introduced the use of clojure.walk/prewalk to munge some symbols that had been improperly qualified by the reader.

It turns out that clojure.walk/walk does not preserve metadata on compound forms, so the temporary namespace that gets evaluated during regrow never sees this metadata.

Adding a custom walk function that does preserve this metadata appears to fix the issue. Testing is much appreciated.

[seancorfield]

That doesn't seem to fix it, I'm afraid. With this import:

(:import (com.newrelic.api.agent Trace))

and this usage:

(deftype NR []
  INR
  (^{Trace {}} do_search
    [_ search-data order-by view-as page page-size skip-exact reverse-search search-rule]
    (do-search* search-data order-by view-as page page-size skip-exact reverse-search search-rule)))

Slamhound still removes the import.

[guns]

It does work now for tagging lists and other collections:

(.method ^AClass (make-object))

But yes, it doesn't work on the symbols of protocol implementations… thank you for testing. I'm looking into it now.

[guns]

It appears that symbols in the metadata of method implementations are only evaluated as Class names then they are attached to the :tag key:

(defprotocol Foo
  (foo [this]))

;; The following two expressions throw with
;; "Unable to resolve classname: ANonExistantClass"
(deftype AFoo []
  Foo
  (^{:tag ANonExistantClass} foo [this]))

(deftype BFoo []
  Foo
  (^ANonExistantClass foo [this]))

However, in any other position they remain as unevaluated symbols:

;; These compile with no errors!

(deftype AFoo []
  Foo
  (^{:anything ANonExistantClass} foo [this]))

(deftype BFoo []
  Foo
  (^{ANonExistantClass []} foo [this]))

I haven't finished following the macroexpansion, but this appears to be the issue. If your use of ^{Trace {}} really does have an effect on the method implementation, I suspect whatever is inspecting the metadata of the method may be looking for the Symbol 'Trace, and not the Class com.newrelic.api.agent.Trace.

Or maybe not; I'd love to have your feedback.

[seancorfield]

Sorry, that was just the easiest namespace to test against. I can confirm it now works as expected on the original namespace that I raised this issue for - Processor is retained now, without the let.

In the ^{Trace {}} case, that's a Java annotation in Clojure - @Trace in Java:

http://corfield.org/blog/post.cfm/instrumenting-clojure-for-new-relic-monitoring

So it's affecting something in the generated bytecode but I don't know enough about how Java annotations work to provide more details.

[guns]

Ah, this bit about annotations is new to me. I'll follow the directions in your blog post and take a closer look.

[guns]

Here are some notes so far.

(defn get-method-annotations
  "Get the annotations on a method.
   cf. https://gist.github.com/richhickey/377213"
  [^Class cls method-name]
  (seq (.getAnnotations (.getMethod cls method-name nil))))

(defprotocol IFoo
  (foo [this]))

(deftype Foo []
  IFoo
  (^{Retention {}} foo [this]))

(get-method-annotations Foo "foo") ; => nil

;;
;; Using the full class name works.
;;

(deftype Bar []
  IFoo
  (^{java.lang.annotation.Retention {}} foo [this]))

(get-method-annotations Bar "foo") ; => (#<$Proxy1 @java.lang.annotation.Retention()>)

;;
;; But so does a simple symbol if it resolves to a class.
;;

(import 'java.lang.annotation.Retention)

(deftype Baz []
  IFoo
  (^{Retention {}} foo [this]))

(get-method-annotations Baz "foo") ; => (#<$Proxy1 @java.lang.annotation.Retention()>)

So the issue is that ^{AClassSymbol {}} is interpreted as metadata of a symbol -> value if AClassSymbol does not resolve to class, and is interpreted as an annotation if it does resolve to class.

I think we can work around this.

[guns]

Okay, I've pushed a possible solution to the branch java-annotations, but I'm going to have to get the go-ahead before merging.

@technomancy

The rationale behind the implementation is in the commit message, but this is the way it works:

• While walking the body, look for definterface/deftype/defrecord forms
• Instead of trying to parse these forms, walk through them and extract all metadata keys on symbols that are:
  1. Symbols
  2. Capitalized
  3. Do not contain . or /
• Concat these symbols to the candidate body. e.g.

'((deftype AFoo [^{WebServiceRefs []} field]
    ^{SupportedOptions []}
    Foo
    (^{Retention RetentionPolicy/RUNTIME} foo [this])))

After munging it becomes:

'((deftype AFoo [^{WebServiceRefs []} field]
    ^{SupportedOptions []}
    Foo
    (^{Retention RetentionPolicy/RUNTIME} foo [this]))
  WebServiceRefs
  SupportedOptions
  Retention)
;; Note that RetentionPolicy is resolved once the compiler recognizes
;; the entry is an annotation

The appended symbols are effectively NOPs, but they do force the compiler to resolve them in the usual way. Besides being a bit sneaky, I currently do not see a downside to this approach.

I initially thought that this would be a good place to advise the user to use ^{:requires […]}, but it turns out that attaching the dummy metadata to the deftype symbol does nothing:

;; This does not work!
(deftype ^{:requires [WebServiceRefs SupportedOptions Retention]}
  AFoo []
  …)

Although this feature is a bit arcane, and the details of the implementation are a bit repulsive, I still think that this is a good inclusion if it precludes the existence of another future Twitter user proclaiming that Slamhound is "not ready for prime time".

[technomancy]

It's really unfortunate that the compiler is so sloppy here, but I guess we have to work with what we've got. I don't have any objection to merging this; thanks for putting this together. I don't use deftype, defrecord, or definterface myself.

[guns]

@seancorfield

Whenever you have a chance, I'd love to know how this branch works for you.

[seancorfield]

Just verified this works locally for our annotations. Thank you!!