Fix clj meta java symbol #89
Conversation
A treaty could be written on the intricacies of clj-meta. Previously, java/resolve-symbol was at the end of the chain and was not consulted, cause conflicts in unqualified names, like max, would result in earlier returns (for instance of clojure.core/max). The problem is deeper and moving java/resolve-symbol up would not solve all the problems, as it is doing two things: returning static member information AND type information. Basically it broke records and macro refer resolution. This patch splits that function in two: java/resolve-symbol only checks if there exist Java members, and java/resolve-type only takes care of full classes and records. Interleaving the two with (m/resolve-var unqualified-sym) magically fulfills all the use cases. There be dragons.
|
@bbatsov I need a lil' review on this 😉 |
|
Yeah, certainly. I took a quick look at this PR a few days ago and then I forgot about it. I recall I was a bit confused about the nature of the fix, but it seems it's mostly a matter of doing some checks in a different order. I'll review this more carefully today or tomorrow. A changelog entry is needed, btw. |
| @@ -323,28 +323,43 @@ | |||
| (filter identity) | |||
| (distinct)))) | |||
|
|
|||
| (defn trim-one-dot | |||
There was a problem hiding this comment.
Probably this shouldn't be public API. I'm also not sure why exactly do you need this. Remove . from constructors?
There was a problem hiding this comment.
Yep, it was already there, I have just factored it out. I will make it private.
| (when-let [ms (seq (resolve-member ns sym*))] ; methodCall | ||
| (if (= 1 (count ms)) | ||
| (first ms) | ||
| {:candidates (zipmap (map :class ms) ms)}))))) | ||
|
|
||
| (defn resolve-type | ||
| "Return type info, for a Java class, interface or record." | ||
| [ns sym] |
There was a problem hiding this comment.
Probably sym is not the best param here.
There was a problem hiding this comment.
Uhm, ok Will think about a better name
| @@ -425,6 +429,19 @@ | |||
| (deftest info-java-test | |||
| (is (info/info-java 'clojure.lang.Atom 'swap))) | |||
|
|
|||
| (deftest info-java-member-precendence-test | |||
| (testing "Integer/max - issue #86" | |||
There was a problem hiding this comment.
I can't understand what this description means. :-)
|
Yes your analysis is correct, we need a different order and interleave. The commit message should contain this info but please let me know if not clear. |
Yeah, it does, but it's also nice to be able to infer all of this by reading the code, docstrings, etc. |
|
Ooops sorry, got busy, thanks for merging! |
|
@arichiardi Follow up PRs are always welcome as well. ;-) I just plan to cut a new release later this week, so some patch is better than no patch. :-) |
Before submitting a PR make sure the following things have been done:
Here is the fix that seems to work. Please read the commit message, this is becoming black magic at this point and probably I will need to refactor stuff at some point.
I left the previous function name mostly untouched but given they return
infocandidates I really think that thejava/resolve-prefix does not make too much sense. They probably belong toorchard.infobut maybe this deserves another PR.