-
Notifications
You must be signed in to change notification settings - Fork 38
Removes import only used in type hint #72
Comments
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 (ns slam.metadata-problem)
(defn example-pattern []
#"foo")
(defn example []
(.pattern ^Pattern (example-pattern))) Slamhound does not detect the 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 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. |
Interesting. So using a local |
Yup, that works. Cool. This is also most likely the cause of Slamhound removing the |
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. |
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! |
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 |
In order to accurately test Slamhound's interaction with files, we must test the output of the entire process: - Open a reader on a file - Receive Clojure forms from reader (with reader macros expanded) - Reconstruct namespace - Write the new namespace back to the file, but copy everything else back verbatim Many tests within Slamhound use (StringReader. (str '(literal forms))) in order to avoid the drudgery of doing a manual pr-str. Unfortunately, (str '[^{:my-metadata "value"} x]) outputs: "[x]", so any metadata on the forms is irretrievably lost. Since the interaction of Slamhound and metadata on forms is non-obvious, I think writing our integration tests as comparisons of input and output _files_ will help us detect these issues sooner in the future.¹ I have placed with-transform-test and with-tempfile into a common testing namespace so that they can be used across all test files. RE: the name `with-transform-test`: - I read it as "With transform, test …" - Many editors commonly treat ^with-.* as a specially indented form - It's awkward, so if a better name is found, let's use it ¹ See issue #72, and commit 54cba5c for past issues with metadata
Commit 54cba5c introduced a call to clojure.walk/prewalk to dequalify vars that were qualified to the current *ns*. A side effect of this is that metadata on composite forms was lost because clojure.walk/walk does not preserve the metadata on these forms. To address this, we define our own version of prewalk that properly preserves metadata. Addresses #72
I think I have discovered the issue. Commit 54cba5c introduced the use of It turns out that Adding a custom walk function that does preserve this metadata appears to fix the issue. Testing is much appreciated. |
That doesn't seem to fix it, I'm afraid. With this import:
and this usage:
Slamhound still removes the import. |
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. |
It appears that symbols in the metadata of method implementations are only evaluated as Class names then they are attached to the (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 Or maybe not; I'd love to have your feedback. |
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 - In the 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. |
Ah, this bit about annotations is new to me. I'll follow the directions in your blog post and take a closer look. |
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 I think we can work around this. |
Rich Hickey introduced support for Java annotations on definterface, deftype, and defrecord types on 23 April, 2010¹. Unfortunately, since the Clojure compiler allows the use of unquoted symbols in metadata, annotations of the form ^{ClassSymbol value} are only interpreted as annotations when ClassSymbol resolves to an imported class. Since a compiler error is not generated in either case, we must walk any forms that may contain annotations and force the compiler to resolve them. We already walk the body once to dequalify erroneously qualified symbols, so this patch hooks into this traversal to collect a set of class symbols that may be interpreted as annotations. This set of symbols is appended to the candidate body so that they become visible to the compiler during check-for-failure. It is possible that metadata inside of a definterface/deftype/defrecord form of the form ^{CapitalizedSymbol value} may be erroneously interpreted as an annotation instead of just metadata of Symbol -> Any. This is impossible to determine without user intervention, but since most metadata keys are keywords, I think interpreting these entries as annotations is a good bet. Addresses #72 ¹ https://groups.google.com/forum/#!topic/clojure/0hKOFQXAwRc
Okay, I've pushed a possible solution to the branch The rationale behind the implementation is in the commit message, but this is the way it works:
'((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 ;; 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". |
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 |
Whenever you have a chance, I'd love to know how this branch works for you. |
Just verified this works locally for our annotations. Thank you!! |
We import several classes only to use them in type hints. Slamhound removes the import for them.
Example:
Agent
is retained - used in theproxy
call - butProcessor
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.The text was updated successfully, but these errors were encountered: