clj-commons/fs changes standard clojure behaviour in clojure.java.io/file

[ikappaki]

Expected behavior

The REPL shouldn't change standard Clojure behaviour, otherwise it might lead to unexpected behaviour in release builds.

Brining up a REPL with clj-refactor installed allows clojure.java.io/file to support java.nio.file.Path as an argument, while it does not in the standard core, .e.g. if we bring up a vanilla REPL (wo clj-refactor) and try to open a Path with io/file it fails as expected:

user> (-> "a-path" (java.nio.file.Paths/get (into-array String [])) clojure.java.io/file)
Execution error (IllegalArgumentException) at user/eval5725 (REPL:59).
No implementation of method: :as-file of protocol: #'clojure.java.io/Coercions found for class: sun.nio.fs.WindowsPath

Analysis follows at the end.

Actual behavior

clj-refactor changes standard Clojure behaviour with regards to opening files with clojure.java.io/file.

Steps to reproduce the problem

1. M-x cider-jack-in to a minimal deps.edn file.
2. At the REPL, try to open a file from a java.nio.Path

user> (-> "a-path" (java.nio.file.Paths/get (into-array String [])) clojure.java.io/file)
#object[java.io.File 0x62eac79 "a-path"]

i.e. it returns the file from the PATH, while this is unsupported in standard Clojure behaviour.

Environment & Version information

clj-refactor.el version information

clj-refactor 3.2.2 (package: 20211117.1008), refactor-nrepl 3.1.0

CIDER version information

Include here the version string displayed when
CIDER's REPL is launched. Here's an example:

;; CIDER 1.2.0snapshot (package: 20211126.1158), nREPL 0.9.0-beta5
;; Clojure 1.10.3, Java 17

Leiningen or Boot version

Emacs version

GNU Emacs 27.2

Operating system

MS-Windows 10

Analysis

The standard supported io/file arguments types are defined by the io/Coercions protocol's as-file method and are String, File, URL and URI.

The support for Path's in io/file is brought in by the clj-common/fs dependency adding Path to io/Coercions. clj-common/fs is part of refactor-nrepl middleware injected by clj-refactor:

user> (-> "a-path" (java.nio.file.Paths/get (into-array String [])) clojure.java.io/file)
#object[java.io.File 0x17537a12 "a-path"]
user> (:impls clojure.java.io/Coercions)
{nil
 {:as-file #function[clojure.java.io/fn--11402],
  :as-url #function[clojure.java.io/fn--11404]},
 java.lang.String
 {:as-file #function[clojure.java.io/fn--11406],
  :as-url #function[clojure.java.io/fn--11408]},
 java.io.File
 {:as-file #function[clojure.java.io/fn--11410],
  :as-url #function[clojure.java.io/fn--11412]},
 java.net.URL
 {:as-url #function[clojure.java.io/fn--11414],
  :as-file #function[clojure.java.io/fn--11416]},
 java.net.URI
 {:as-url #function[clojure.java.io/fn--11418],
  :as-file #function[clojure.java.io/fn--11420]},
 java.nio.file.Path
 {:as-file
  #function[refactor-nrepl.inlined-deps.fs.v1v6v307.me.raynes.fs/eval2296/fn--2297],
  :as-url
  #function[refactor-nrepl.inlined-deps.fs.v1v6v307.me.raynes.fs/eval2296/fn--2299]}}

I was caught off guard with this while providing a PR about filtering filenames given a path and would like to bring this to your attention. Thinking my input path was a String I was passing it io/file with the aim to retrieve its filename with .getName, but it turned out it was a Path. It should have failed while I was testing on the REPL before reaching the release build, but it got missed because of this. I even naively thought afterwards that Pathd were supported in the standard API, but it does not https://clojure.atlassian.net/browse/CLJ-2333.

IMHO REPL middleware shouldn't modify standard behaviour since this might lead to confusion with what we are getting during dev vs release. Not exactly sure how to fix this particular issue since this is not part of the middleware core but rather an external dependency, but wanted to open the discussion anyway.

nextjournal/clerk#28

Thanks!

[vemv]

Thanks! Will check if refactor-nrepl actually does use this extension in any way. If not, I might fork the dep and remove the extension.

[vemv]

otherwise it might lead to unexpected behaviour in release builds.

Just stating the obvious, by using profiles/aliases, refactor-nrepl should never be included in a release build.

Similarly, most behavior should be captured by unit tests, which also should not include refactor-nrepl in a CI environment.

(Some people still don't realise the value of a strict separation between :dev and test profiles/aliases, this issue is a good example of what might go wrong)

[ikappaki]

Thanks! Will check if refactor-nrepl actually does use this extension in any way. If not, I might fork the dep and remove the extension.

Thanks! Another alternative hack would be to rebind the extensions immediately after clj-commons/fs is loaded, mimicking the original exception:

;; undo clj-common/fs extension
(extend-protocol clojure.java.io/Coercions
    java.nio.file.Path
    (as-file [this] (throw (IllegalArgumentException. (str "No implementation of method: " 
                                                           :as-file  
                                                           " of protocol: " (var clojure.java.io/Coercions) 
                                                           " found for class: " 'java.nio.file.Path))))
    (as-url [this] (throw (IllegalArgumentException. (str "No implementation of method: " 
                                                          :as-url
                                                          " of protocol: " (var clojure.java.io/Coercions) 
                                                          " found for class: " 'java.nio.file.Path))))
    )

user> (-> "a-path" (java.nio.file.Paths/get (into-array String [])) clojure.java.io/file)
Execution error (IllegalArgumentException) at eval19905$fn (REPL:25).
No implementation of method: :as-file of protocol: #'clojure.java.io/Coercions found for class: java.nio.file.Path

user> (-> "a-path" (java.nio.file.Paths/get (into-array String [])) clojure.java.io/as-url)
Execution error (IllegalArgumentException) at eval19905$fn (REPL:29).
No implementation of method: :as-url of protocol: #'clojure.java.io/Coercions found for class: java.nio.file.Path

;; with original exception for unsupported types being 
user> (clojure.java.io/as-file [])
Execution error (IllegalArgumentException) at user/eval19919 (REPL:133).
No implementation of method: :as-file of protocol: #'clojure.java.io/Coercions found for class: clojure.lang.PersistentVector

But I am not sure how this would play out with Mr. Anderson inlining the namespace.

Just stating the obvious, by using profiles/aliases, refactor-nrepl should never be included in a release build.

Similarly, most behavior should be captured by unit tests, which also should not include refactor-nrepl in a CI environment.

I agree on both counts. It so happens that particular library is in early stages of development and does not have a test suite published yet. Thus I am at the mercy of testing the behaviour via the REPL.

Btw, I've noticed that cljr injects/autoloads its middlewares to the CIDER REPL regardless of the minor mode being enabled or not, which in IMHO is both unexpected and suboptimal:

;;;###autoload
(eval-after-load 'cider
  '(cljr--inject-jack-in-dependencies))

would you consider a PR to inject the middlewares only when the minor mode is on?

Thanks!

[vemv]

Thanks! Another alternative hack would be to rebind the extensions immediately after clj-commons/fs is loaded, mimicking the original exception:

Nice! Will consider.

Btw, I've noticed that cljr injects/autoloads its middlewares to the CIDER REPL regardless of the minor mode being enabled or not, which in IMHO is both unexpected and suboptimal:

Each refactor-nrepl functionality is lazily loaded; it doesn't slow down startup time unless/until a specific feature is used.

You can learn about it here:

refactor-nrepl/src/refactor_nrepl/middleware.clj

Lines 128 to 147 in da81035

	(def ^:private clean-ns
	(delay
	(require-and-resolve 'refactor-nrepl.ns.clean-ns/clean-ns)))
	(def ^:private pprint-ns
	(delay
	(require-and-resolve 'refactor-nrepl.ns.pprint/pprint-ns)))
	(defn- clean-ns-reply [{:keys [transport] :as msg}]
	(reply transport msg :ns (some-> msg (@clean-ns) (@pprint-ns)) :status :done))
	(def ^:private find-used-locals
	(delay
	(require-and-resolve 'refactor-nrepl.find.find-locals/find-used-locals)))
	(defn- find-used-locals-reply [{:keys [transport] :as msg}]
	(reply transport msg :used-locals (@find-used-locals msg) :status :done))
	(defn- version-reply [{:keys [transport] :as msg}]
	(reply transport msg :status :done :version (core/version)))

Given this laziness, further optimizing things has diminishing returns - it's simpler to focus simply on the JVM side instead of having to coordinate our 'backend' and 'frontend'.

Cheers - V

[ikappaki]

Each refactor-nrepl functionality is lazily loaded; it doesn't slow down startup time unless/until a specific feature is used.

It does indeed :)

I was more concerned about the case where the user installed the clojure-refactor package, and for whatever reason hasn't enabled the minor mode in their session, e.g. as per set up instructions. IMHO, the user's expectation is that absolutely nothing should happen, though CIDER will inject the cljr middlewares regardless. For example, during my investigation above, I disabled/not configured the minor mode for clj files, but he middleware was still injected and loaded adding the extra extension to the REPL environment, which was a bit buflling. I had to completely uninstall and restart Emacs for the problem to go away. Is there value injecting a middleware if is not going to be used that could lead to unexpected (but hopefully very rare) issues? I suppose you are saying code gets complicated if we don't do so:

Given this laziness, further optimizing things has diminishing returns - it's simpler to focus simply on the JVM side instead of having to coordinate our 'backend' and 'frontend'.

thanks again!

[vemv]

IDK, we're tallking about a pretty extreme corner case here. The expectation is that if you have clj-refactor installed / set up, you will use it. One typically doesn't enable modes 'by hand'.

One might even use it without realising. cljr-slash and cljr-add-missing are such essential features for me that I cannot imagine myself doing Clojure without them :)

If there's a specific problem, feel free to open it as an issue (if it's this one i.e. the fs matter, please rest assured that it will be fixed)

[vemv]

Created clj-commons/fs#11

Transfered the issue to refactor-nrepl as well

[ikappaki]

If there's a specific problem, feel free to open it as an issue (if it's this one i.e. the fs matter, please rest assured that it will be fixed)

I think we've exhausted the subject, no need for raising another issue. Thanks for looking at the fs 👍🏻

[vemv]

To be released soon, maybe I'll bundle something else while we're there

[vemv]

Released as 3.2.0