WIP: Add constructor info to arglists of Java classes

[yuhan0]

This is partly an issue report and partly showing the current state of my edits for discussion.
Not suitable for merging as-is.

This change allows for java class constructors to provide the :arglists key in the info op response. No other change in cider-nrepl or cider.el is needed, it can be previewed by overriding the below type-info function in the cider-nrepl inlined namepsace.

At first I added a separate :ctors key but this required many changes to client code, I think using the :arglists key is alright for this purpose.

Before submitting a PR make sure the following things have been done:

• The commits are consistent with our contribution guidelines
• You've added tests to cover your change(s)
• All tests are passing
• The new code is not generating reflection warnings

Keep in mind that new orchard builds are automatically deployed to Clojars
once a PR is merged, but only if the CI build is green.

Thanks!

[yuhan0]

Java argnames seem to be an odd situation, they're not always present and sometimes go missing after the cache is cleared (a JVM optimization maybe?)

Arguably the type of the args is more important as that is what distinguishes constructors from each other. But currently orchard.java/member-info displays only the argnames if they are present.

eg.

(Integer/sum  | )
;; => eldoc displays  "java.lang.Integer/.sum ([a b])" with no indication of the types.

With the constructors I tried to display both argname and argtype by making a symbol with a space separating the two, such that eldoc will highlight it as one item.
But this is quite hacky and maybe other solutions would be preferred. (elide type when name is present like member-info? Display as name:-type ? )

In any case the behaviour should be made consistent between constructor and member info.

[bbatsov]

But this is quite hacky and maybe other solutions would be preferred. (elide type when name is present like member-info? Display as name:-type ? )

I have some vague memory that we were actually displaying only types at some point. I don't recall when this changed, but there's clearly some room for improvement. @jeffvalk Any thoughts on this?

[bbatsov]

If we go this route this should be something optional (and ideally something that clients can modify).

[yuhan0]

Yeah, I agree - it should probably be a client-side processing step, eg. eldoc could do a search-and-replace via regex.

[bbatsov]

Java argnames seem to be an odd situation, they're not always present and sometimes go missing after the cache is cleared (a JVM optimization maybe?)

Btw, which cache are you referring to here?

[yuhan0]

Java argnames seem to be an odd situation, they're not always present and sometimes go missing after the cache is cleared (a JVM optimization maybe?)

Btw, which cache are you referring to here?

I was referring to #'orchard.java/cache which I sometimes had to reset (to {}) on the fly when interactively monkey-patching cider.nrepl's inlined version of orchard to test these changes.
Eg. if the current repl session had already analyzed java.lang.Integer, patching the type-info function wouldn't change the cached info output. After (reset! cider.nrepl.inlined***.orchard.java/cache {}) the changes would show up in eldoc/cider-doc, but sometimes argnames would be lost. (this is on Java 11, from what I understand argnames that can be accessed via reflection was only introduced in Java 9)

[bbatsov]

@yuhan0 I totally forgot about this PR. Are you still interested in driving it to completion?

[vemv]

@yuhan0 I totally forgot about this PR. Are you still interested in driving it to completion?

Let us know, else it sounds practical to close the PR to avoid accumulating distractions in our trackers

[yuhan0]

Yes, sorry for dropping this one somewhat, I had hacked on it during a short period while doing some icky Java interop. , and now it seems that I've largely forgotten about the issues that I was facing at the time.

It looks like I wanted feedback regarding the following points:

• Is it ok to overload the :arglists key for Java constructors vs adding a new key to the response? It has arguably different semantics to Clojure arglists due to the additional dimension of types, but similar usage in terms of tooling.

• Formatting of argument types and argnames.This also applies to Java methods in addition to constructors which I was targeting with this PR. One can have overloaded methods and constructors:

static void foo (int a, double b) {....}
static void foo (double c, int d) {...} 

Should we represent these as (:arglists (("int a" "double b") ...)) or (:arglists (("a:-int" "b:-double") ...)) or something more faithful like (:typed-arglists ((("int" . "a") ("double" . "b")) ... )) and leave it up to the clients to handle and format appropriately? I believe argnames can also be non-existent or elided by JVM optimizations, not sure if it can differ within overloads of the same method/constructor(?)

[bbatsov]

I think it's time to close this one. No point in just keeping it around.

[vemv]

Yes, with #211 (which I still plan to tackle), the end solution would look definitely different.