data reader depends on imports being available?

[borkdude]

Consider the following REPL sequence:

user=> (require '[flatland.ordered.map])
nil
user=> #ordered/map[[:a 1] [:b 2]]
Syntax error (IllegalArgumentException) compiling fn* at (/private/var/folders/2m/h3cvrr1x4296p315vbk7m32c0000gp/T/form-init53327008725414542.clj:1:8317).
Unable to resolve classname: IPersistentMap

user=> (import 'clojure.lang.IPersistentMap)
clojure.lang.IPersistentMap
user=> #ordered/map[[:a 1] [:b 2]]
Syntax error (IllegalArgumentException) compiling fn* at (/private/var/folders/2m/h3cvrr1x4296p315vbk7m32c0000gp/T/form-init53327008725414542.clj:1:8317).
Unable to resolve classname: IPersistentVector

user=> (import 'clojure.lang.IPersistentVector)
clojure.lang.IPersistentVector
user=> #ordered/map[[:a 1] [:b 2]]
#ordered/map ([:a 1] [:b 2])

It seems the data reader only works when certain imports are in place?

[jafingerhut]

I do not know why this happens, but off the top of my head as a guess, perhaps adding an import or three into the code, and/or replacing some occurrence of IPersistentVector with clojure.lang.IPersistentVector in the code of the ordered library, might avoid this behavior?

[borkdude]

@kolharsam Is this something you'd be interested in helping to fix?

[kolharsam]

@borkdude I'll try and get back to you soon. Upon investigating it further. If you've got any ideas, please share :)

[jafingerhut]

In an experiment with Clojure 1.10.1 and this ordered library, I made a local change to the deftype OrderedMap so that its type hints have fully qualified Java interface/class names, like so:

(deftype OrderedMap [^clojure.lang.IPersistentMap backing-map
                     ^clojure.lang.IPersistentVector order]

With only those changes, the REPL sequence in the original issue gives no exceptions or messages of any kind.

I do not know all the rules here, but I will point out that this Clojure source code written by Rich Hickey uses fully qualified interface/class names in similar situations: https://github.com/clojure/clojure/blob/master/src/clj/clojure/gvec.clj

[jafingerhut]

I would guess there should be no harm in replacing all occurrences of such Java class/interface names with their fully qualified counterparts, throughout this library, other than a few extra characters in the source code.

[kolharsam]

@jafingerhut I found one occurrence here:

ordered/src/flatland/ordered/map.clj

Line 29 in 714f32a

(deftype OrderedMap [^IPersistentMap backing-map

I guess the solution that you suggested is already present. Would it be wrong to think that something else might be the cause of the error?

[jafingerhut]

@kolharsam The lines of code you reference that are in the current latest version of this library:

(deftype OrderedMap [^IPersistentMap backing-map
                     ^IPersistentVector order]

lead to the exception being thrown in the REPL when attempting to evaluate the sequence of expressions in the first comment of this issue.

My experiment was to change those lines to something similar, but not the same as they currently are. Look carefully for the changes. I guarantee you my suggested replacement lines are different than the current ones:

(deftype OrderedMap [^clojure.lang.IPersistentMap backing-map
                     ^clojure.lang.IPersistentVector order]

[kolharsam]

@jafingerhut What you suggested seems to be working for the sequence above. And also my apologies for not looking at it carefully initially. Should I write some test(s) for this?

Also, might there be other classes where this problem might occur? You did already suggest replacing all of the classnames with their fully qualified names. Should I be doing that as well? Or would that better in a different fix/issue altogether?

[jafingerhut]

It might be best to only change the occurrences that cause the symptoms of this problem to go away, which seem to be the ones in my previous message for the ordered maps.

For ordered sets, I bet there is a similar issue, and a similar fix.

It would be good if there were a way to write a test that fails for the current code, but succeeds with the fix. I do not understand why this test is passing, for example, given the behavior at a REPL given in the original issue: https://github.com/clj-commons/ordered/blob/master/test/flatland/ordered/map_test.clj#L184-L187

[jafingerhut]

Given this interaction below, I do not know how to add a failing test for the current code, since read-string works without error, without any changes to the latest source code. I do not know why the difference between read-string versus entering the expression at a REPL, except that it might have something to do with Leiningen and/or nrepl.

$ lein version
Leiningen 2.9.3 on Java 1.8.0_192 Java HotSpot(TM) 64-Bit Server VM

$ lein repl
nREPL server started on port 63296 on host 127.0.0.1 - nrepl://127.0.0.1:63296
REPL-y 0.4.4, nREPL 0.6.0
Clojure 1.9.0
Java HotSpot(TM) 64-Bit Server VM 1.8.0_192-b12
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
 Javadoc: (javadoc java-object-or-class-here)
    Exit: Control+D or (exit) or (quit)
 Results: Stored in vars *1, *2, *3, an exception in *e

user=> *clojure-version*
{:major 1, :minor 9, :incremental 0, :qualifier nil}
user=> (require '[flatland.ordered.map :as m])
nil
user=> (read-string "#ordered/map[[:a 1] [:b 2]]")
#ordered/map ([:a 1] [:b 2])
user=> #ordered/map[[:a 1] [:b 2]]
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: IPersistentMap, compiling:(/private/var/folders/rv/2vbzx7zj64x4hss7rdl1xywc0000gn/T/form-init2106877030880329831.clj:1:1479) 

user=> (pst)
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: IPersistentMap, compiling:(/private/var/folders/rv/2vbzx7zj64x4hss7rdl1xywc0000gn/T/form-init2106877030880329831.clj:1:1479)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:7010)
	clojure.lang.Compiler.analyze (Compiler.java:6773)
	clojure.lang.Compiler.eval (Compiler.java:7059)
	clojure.lang.Compiler.eval (Compiler.java:7025)
	clojure.core/eval (core.clj:3206)
	clojure.core/eval (core.clj:3202)
	clojure.main/repl/read-eval-print--8572/fn--8575 (main.clj:243)
	clojure.main/repl/read-eval-print--8572 (main.clj:243)
	clojure.main/repl/fn--8581 (main.clj:261)
	clojure.main/repl (main.clj:261)
	clojure.main/repl (main.clj:177)
	nrepl.middleware.interruptible-eval/evaluate (interruptible_eval.clj:79)
Caused by:
IllegalArgumentException Unable to resolve classname: IPersistentMap
	clojure.lang.Compiler$HostExpr.tagToClass (Compiler.java:1129)
	clojure.lang.Compiler.tagClass (Compiler.java:8567)
	clojure.lang.Compiler$ObjExpr.emitValue (Compiler.java:4799)
	clojure.lang.Compiler$ObjExpr.emitConstants (Compiler.java:4927)
	clojure.lang.Compiler$ObjExpr.compile (Compiler.java:4605)
	clojure.lang.Compiler$FnExpr.parse (Compiler.java:4099)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:7001)
	clojure.lang.Compiler.analyze (Compiler.java:6773)
nil
user=> 

[jafingerhut]

It is not due to Leiningen or nrepl, as I guessed in my previous comment, because I get the same behavior with the Clojure CLI clj command, too.

It is due to attempting to eval the read result, as one can do in an automated test by trying to evaluate this expression: (eval (read-string "#ordered/map[[:a 1] [:b 2]]"))

[kolharsam]

@jafingerhut I have added these 2 commits, would it be sufficient to resolve this issue? (to submit a PR)
1.kolharsam@8c6c8cc
2.kolharsam@fa854a0

Also, I'm interested in how/when did you realize that eval was causing the problem? Once I read your last comment it was a real aha moment for me.

[borkdude]

Why the formatting change in kolharsam@fa854a0? And how do you test if the issue was resolved?

[jafingerhut]

@kolharsam asked: "I'm interested in how/when did you realize that eval was causing the problem?"

Two clues:

(1) The first is the stack trace in my previous comment, that came as output from evaluating (pst) after the exception occurred. It has several lines with clojure.core/eval in them.

(2) The call to read-string was not causing an exception, but entering the expression #ordered ... in the REPL was. REPL is Read, Eval, Print, Loop. Read was not the cause, so perhaps Eval was.

[jafingerhut]

Also, as borkdude mentioned, it would be good to add a test that fails without the fix, and passes with the fix.

The tests that you reformat in one of your commits are interesting. They include (eval (read-string "#ordered ...")), but they do not throw an exception already, without the fix? I wonder why they do not throw an exception now?

[jafingerhut]

I think I see now. The second commit you point at is a reformatting one only, after another separate commit just before that which added those tests. Those tests do not exist in the latest master of the original repository, so that is why the latest master is not failing those tests.

You should also add similar tests for ordered sets as the one you added for ordered maps.

[borkdude]

Ah right. Just to confirm, did the tests you added fail without the fix @kolharsam?

[kolharsam]

@borkdude affirmative!
The tests are failing. I got the same CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: IPersistentMap error on those tests

[kolharsam]

@jafingerhut upon adding similar tests for Ordered Sets, this should be good for a PR?

[jafingerhut]

@kolharsam I think so, yes.