Faulty logic in info op

[yuhan0]

Expected behavior

When calling orchard.info/info on a Java member or non-existent fully qualified symbol, the unqualified symbol should not be used to resolve a var in the current namespace.

(orchard.info/info 'user 'Integer/max)
(orchard.info/info 'user 'non-existing-ns/get)

should return the info for the Integer.max Java static member and nil respectively.

Actual behavior

Instead the unqualified symbols max and get are resolved to the clojure.core functions and produces an incorrect info response.

I traced it to this line in clj-meta:

orchard/src/orchard/info.clj

Line 76 in ccf90fe

(some-> ns (m/resolve-var unqualified-sym) (m/var-meta))

Even though sym is fully qualified, the processing step normalise-params added the key :unqualified-sym. I don't know enough to say if it's a predecence issue or if there should be an additional condition to not resolve unqualified-sym if the sym is fully qualified.

Steps to reproduce the problem

In Emacs, call cider-doc on the symbol Integer/max or observe the eldoc which displays the wrong arglists for clojure.core/max.

Environment & Version information

Clojure version

E.g. 1.10.2

Java version

E.g. 1.11

Operating system

macOS 10.15

[bbatsov]

I don't know enough to say if it's a predecence issue or if there should be an additional condition to not resolve unqualified-sym if the sym is fully qualified.

Sounds like we should have some additional check for qualified symbols. I don't remember the code details that much myself, but this should be easy to enough to fix. I guess there might also be an assumption that the caller should take care of the check, but it won't hurt to make the code in info more defensive.

Perhaps @jeffvalk will have something to add on this topic as well.

[arichiardi]

I can have a look at this, probably a simple addition to the conditional logic there, thank you for the detailed report!

[yuhan0]

The case with Integer/max was solved, thanks! However the issue still remains with the other case where the symbol can't be resolved, like non-existing-ns/get.

[bbatsov]

Probably we'll have to do a deeper clean-up of that codebase at some point, as there's a lot of mess there. I might get to doing this in a month or so, but I can make no promises, as my backlog is already quite full. Any help is welcome!

[arichiardi]

I would dare to disagree the code is a mess 😅

It's more Clojure's semantics non-linear - in any case of course any help is appreciated if I don't get to it soon.

I am quite confused though by the non-existent use case cause we test it and the test passed.

I will try to get there again

[arichiardi]

I think this one should be reopened.

This is the problem we have and it is one I am not sure how to solve.

The tension is between these two payloads (the first was part of #53):

{:ns orchard.test-ns, :sym replace, :dialect :clj, :unqualified-sym replace, :qualified-sym orchard.test-ns/replace}

{:ns orchard.test-ns, :sym Integer/max, :dialect :clj, :qualified-sym Integer/max, :unqualified-sym max, :computed-ns Integer}

The problem is that there exists both a clojure.string/replace and a java.lang.String/replace and java.lang is always available to the REPL. In order to resolve the clojure.string/replace referred in my orchard.test-ns I ns-resolve before resolving Java stuff.

This is all good, except that it means I resolve clojure.core/max before java.lang.Integer/max 😢

[arichiardi]

I think I will have to special-case for :refer clauses and make sure that:

1. I resolve the :refer-ed symbols local to a namespace
2. I resolve Java symbols
3. I call resolve for anything else

[arichiardi]

Nope, the problem is not with refer actually, it's about the symbol ambiguity. I will have to think about something else

[bbatsov]

@arichiardi We can do what we do with Java methods and return multiple candidates that the user can manually select, while defaulting the to the Clojure sym for eldoc.

[arichiardi]

@bbatsov Doesn't info always need to return one option? I think that if we change that it will be an even bigger reward. I have some idea - still haven't had time to implement it though.

[arichiardi]

@yuhan0 I reallly really tried but I don't think I can produce a nil for (orchard.info/info 'user 'non-existing-ns/get).

In order to make pragmatic choices, I wonder if you can show me the piece of code (in cider) that presents the defect...I want to see if it is acceptable to display something like clojure.core/get in that case.

Thanks!

[yuhan0]

The main entry point is cider-doc which displays a popup info buffer.
which then calls cider-doc -> cider-create-doc-buffer -> cider-var-info -> cider-sync-request:info
https://github.com/clojure-emacs/cider/blob/ed464423b5d3917384db460cff579adfd8d6d7fd/cider-client.el#L444

The other route is cider-eldoc-info which shows the var signature in the minibuffer.
This one forwards to cider-sync-request:eldoc which ends up calling the same orchard.info/info as parameter to the orchard.eldoc/eldoc function.
https://github.com/clojure-emacs/cider-nrepl/blob/94cc4be72ffc7cb868bd7acbf3ec0b44266f91f5/src/cider/nrepl/middleware/info.clj#L83

I wouldn't say it's unacceptable to display clojure.core/get in these cases, but it's definitely a "false positive" error that may cause confusion.

[yuhan0]

Here's a contrived example:

(ns foo)
(defn get [x] "not clojure.core!")

(ns bar 
  (:require [foo])

;; via cider-uneval or some REPL command:
(ns-unmap 'foo 'get)

(foo/get :something) ;; <- cursor here, displays eldoc for clojure.core/get 

[yuhan0]

Solved by #134 , thanks!

[vemv]

Indeed! That PR comes to implement the unqualified symbol should not be used to resolve a var in the current namespace.

[arichiardi]

Thank you!