Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arglists in info op are problematic #25

Closed
gtrak opened this issue Mar 17, 2014 · 8 comments
Closed

arglists in info op are problematic #25

gtrak opened this issue Mar 17, 2014 · 8 comments

Comments

@gtrak
Copy link
Contributor

gtrak commented Mar 17, 2014

cider.nrepl.middleware.util.misc> (transform-value '([]))
(nil)

This ends up cutting off data on the elisp side after an arglists entry, which can include valuable file/line information. Should decide on a canonical representation, whether strings or lists of strings or so on.

@gtrak
Copy link
Contributor Author

gtrak commented Mar 17, 2014

I'm seeing other cases where nils in a response map are breaking elisp's expectations, clearly missing something fundamental :-).

This serialized output:
("arglists"
nil
"doc"
nil
"ns"
"blah.component"
"name"
"Lifecycle"
"column"
1
"line"
6
"file"
"file:/home/gary/dev/path/src/main/clojure/blah/component.clj")

on the elisp side results in:
("arglists")

@gtrak
Copy link
Contributor Author

gtrak commented Mar 17, 2014

Seems like bencode or the parser is to blame, will explore the ramifications of converting nils to empty-list.

@bbatsov
Copy link
Member

bbatsov commented Mar 18, 2014

In Emacs Lisp an empty list and nil are the same thing. I'll have a look at the problem, but it's likely caused becausing we read Clojure forms using the Emacs Lisp reader.

@gtrak
Copy link
Contributor Author

gtrak commented Mar 28, 2014

Not sure where the problem is, but here's a clue:

Encoded message:
"d2:id2:357:session36:b6d78c86-182e-4602-8401-4ab270f72c175:valuel2:ns12:clojure.core4:name19:pop-thread-bindings8:arglistsllee6:columni1e5:added3:1.16:static4:true3:doc113:Pop one set of bindings pushed with push-binding before. It is an error to
pop bindings without pushing before.4:linei1737e4:file96:jar:file:/home/gary/.m2/repository/org/clojure/clojure/1.5.1/clojure-1.5.1.jar!/clojure/core.cljee"

Decoded message:
((dict ("id" . "35") ("session" . "b6d78c86-182e-4602-8401-4ab270f72c17") ("value" "ns" "clojure.core" "name" "pop-thread-bindings" "arglists")) "column" 1 "added" "1.1" "static" "true" "doc" "Pop one set of bindings pushed with push-binding before. It is an error to
pop bindings without pushing before." "line" 1737 "file" "jar:file:/home/gary/.m2/repository/org/clojure/clojure/1.5.1/clojure-1.5.1.jar!/clojure/core.clj" nil nil)

The data gets cut off after arglists.

@gtrak
Copy link
Contributor Author

gtrak commented Mar 29, 2014

I crafted a test case in clojure that roundtrips successfully, I think the blame is with the bdecode implementation in cider-client.el elisp:

       (while (setq item (nrepl-bdecode-buffer))
         (setq result (cons item result)))

Conflates nil/() for control-flow where it might be a valid encoded result.

Seems like bencode could indeed encode an empty list with 'le'. Not sure if this is valid.

@bbatsov
Copy link
Member

bbatsov commented Mar 29, 2014

The current implementation was mostly copy-pasted from the EmacsWiki (http://www.emacswiki.org/emacs-en/bencode.el) so it's quite possible there are bugs in it. I hope I'll be able to look into this soon myself, but I cannot make any promises.

@gtrak
Copy link
Contributor Author

gtrak commented Mar 31, 2014

I'll debug it again as soon as I get a chance, I confirmed the issue, what follows is the new information we should take into account for arbitrary nested data structures (which I think would make life a lot easier in the long run):

Bencode supports empty-list 'le', clojure impl can produce it from empty-vectors or lists, at the moment the elisp impl shorts on falsy, including empty-list.
Bencode does not seem to support nil, and the clojure implementation in tools.nrepl encodes nil as 'le'.

Here is my roundtrip function that I've been using to test this out:

(defn round-trip [data]
    (let [encoded (let [b (java.io.ByteArrayOutputStream.)]
                    (clojure.tools.nrepl.bencode/write-bencode b data)
                    (str b))
          _ (println encoded)
          decoded (let [s (java.io.PushbackInputStream. (java.io.StringBufferInputStream. encoded))]
                    (#'clojure.tools.nrepl.transport/<bytes (clojure.tools.nrepl.bencode/read-bencode s)))]
      decoded))

@gtrak
Copy link
Contributor Author

gtrak commented Mar 31, 2014

Just confirmed all this with someone in the ##bittorrent freenode channel. The clojure impl seems to be doing the right thing in all these edge-cases.

cider.nrepl.middleware.info> (round-trip [nil])
llee
[[]]
cider.nrepl.middleware.info> (round-trip ["" nil])
l0:lee
["" []]
cider.nrepl.middleware.info> (round-trip ["" nil {nil nil}])
NullPointerException   clojure.tools.nrepl.bencode/string>payload (bencode.clj:172)
cider.nrepl.middleware.info> (round-trip ["" nil {5 6}])
ClassCastException java.lang.Long cannot be cast to java.lang.String  clojure.tools.nrepl.bencode/string>payload (bencode.clj:172)
cider.nrepl.middleware.info> (round-trip ["" nil {"" 6}])
l0:led0:i6eee
["" [] {"" 6}]
cider.nrepl.middleware.info> (round-trip ["" nil {"" nil}])
l0:led0:leee
["" [] {"" []}]

gtrak added a commit to gtrak/cider that referenced this issue Apr 8, 2014
gtrak added a commit to gtrak/cider that referenced this issue Apr 8, 2014
@gtrak gtrak closed this as completed Apr 8, 2014
gtrak added a commit to gtrak/cider that referenced this issue Apr 8, 2014
gtrak added a commit to gtrak/cider that referenced this issue Apr 8, 2014
bbatsov added a commit to clojure-emacs/cider that referenced this issue Apr 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants