Problem with basic functionality

[ivarref]

Hi, and thanks for a library that looks very promising!

I do have some problem to get started though.

To reproduce this problem:

git clone https://github.com/ivarref/pyro-demo.git
cd pyro-demo
lein repl
(do (load-file "src/demo/core.clj") (demo.core/foo "a"))

And the stacktrace will be like this:

user=> (do (load-file "src/demo/core.clj") (demo.core/foo "a"))

java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Number
 at demo.core/foo (core.clj:7)
java.lang.NullPointerException: null
 at pyro.source/file-source (source.clj:36)
user=>     34      colorschemes/terminal-default
    35      (parse/parse
--> 36       (string/join "\n" (line-seq (filepath->buffered-reader filepath))))))
    37
    38   (def memoized-file-source

    pyro.source/file-source (source.clj:31)
    29       (BufferedReader. (InputStreamReader. strm))))
    30
--> 31   (defn file-source
    32     [filepath]
    33     (terminal/ansi-colorize

    pyro.source/source-fn (source.clj:49)
    47     {:added "0.1.0"}
    48     [filepath line number]
--> 49     (let [rdr (memoized-file-source filepath)]
    50       (let [content (drop (- line (inc number))
    51                           (string/split rdr #"\n"))

    pyro.source/source-fn (source.clj:41)
    39     (lu file-source :lu/threshold 64))
    40
--> 41   (defn source-fn
    42     "A function for pulling in source code.
    43

    pyro.printer/print-trace-element (printer.clj:21)
    19     (when (and show-source (:clojure element))
    20       (when-let [file (source/get-var-filename element)]
--> 21         (println (source/source-fn
    22                   file
    23                   line 2)

    pyro.printer/print-trace-element (printer.clj:13)
    11     (str "(" file ":" line ")"))
    12
--> 13   (defn print-trace-element
    14     "Prints a Clojure-oriented view of one element in a stack trace."
    15     {:added "0.1.0"}

    pyro.printer/pprint-exception (printer.clj:50)
    48        (print " at ")
    49        (if-let [e (first st)]
--> 50          (print-trace-element e opts)
    51          (print "[empty stack trace]"))
    52        (doseq [e (rest st)]

    pyro.printer/pprint-exception (printer.clj:26)
    24                  "\n"))))
    25
--> 26   (defn pprint-exception
    27     "Pretty-print the exception.
    28

    clojure.tools.nrepl.middleware.interruptible-eval/evaluate[fn] (interruptible_eval.clj:125)
    123                                                                 :ex (-> e class str)
    124                                                                 :root-ex (-> root-ex class str)}))
--> 125                            (clojure.main/repl-caught e)))))
    126            (finally
    127              (.flush ^Writer out)

I'm guessing this is not the expected output.
Any ideas on what might be wrong?

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

I'm using Mac OS X High Sierra.
Did I miss something obvious somewhere?

Kind regards

[ivarref]

I'm no expert on classpath loading etc, but I propose the following change to source.clj:

(defn filepath->buffered-reader
  [filepath]
  (when-let [strm (io/input-stream (io/file filepath))]
    (BufferedReader. (InputStreamReader. strm StandardCharsets/UTF_8))))

which fixes the above issue for me (using Oracle JDK). This also seems more standard than using RT/baseloader. Will this break anything?

Thanks and regards.

[venantius]

Interesting problem.

I don't think your solution is the right one, but it's a good starting point for consideration. I don't think we want to use io/file because we want to be able to load Clojure source that might be packaged in a jar or otherwise on our resource path but isn't on the direct filepath.

I also think using load-file is a little non-idiomatic - typically you'd require a namespace. load-file is one of those vestigial bits from Clojure 1.0 that isn't really in practical use as far as I know, but I might be wrong about that.

Regardless, Pyro should support your use case without throwing an NPE. I'll need to do some more work here - there are a few different options, from just silently failing to retrieve the source file to first trying to load it using baseLoader and then trying to load it using a file loader.

[ivarref]

Thanks for the feedback. You are of course correct about loading files inside JARs.

I was just using load-file to reproduce the issue. The same problem occurs in Cursive IDE using REPL -> Load file in REPL, so while I'm not sure exactly what Cursive does under the hood, it seems that the namespace path loading is the same as load-file and thus this will affect Cursive users.

(meta (resolve (symbol "demo.core" "foo"))) ; demo/core.clj loaded using Cursive
=>
{:arglists ([x]),
 :line 6,
 :column 1,
 :file "/Users/ivref/clojure/demo/src/demo/core.clj",
 :name foo,
 :ns #object[clojure.lang.Namespace 0x734a2101 "demo.core"]}

The following code handles both absolute paths and classpath resources:

(defn filepath->buffered-reader
  [filepath]
  (when-let [strm (or (.getResourceAsStream (RT/baseLoader) filepath)
                      (io/input-stream (io/file filepath)))]
    (BufferedReader. (InputStreamReader. strm StandardCharsets/UTF_8))))

[venantius]

That meta printout is actually quite helpful as it highlights the problem - the :file key is being set differently by load-file than by require.

If we require the namespace instead, we get the following:

user=> (meta #'demo.core/foo)
{:arglists ([x]),
 :line 6,
 :column 1,
 :file "demo/core.clj",
 :name foo,
 :ns #object[clojure.lang.Namespace 0x5fdb9c46 "demo.core"]}

So there's a bit of work to be done here but basically the path forward seems clear. I think the default behavior should still be to try to load it as a resource, but if that returns nil then we should check for the existence of the file in the file system, and if it exists we can try to load that using the mechanism you've described.

And in both mechanisms we should actually handle the nil return value instead of throwing an NPE down the line.

[venantius]

Hopefully resolved by #15.