Remove unecessary code

[hsartoris-bard]

Fixes #205

[trptcolin]

I do think at least some of that code being deleted is needed, to enable the completion code to be loaded into any application that doesn't already have it as a dependency. Is there behavior you're seeing that's broken?

Also, looks like this fails a test, which makes sense since the app in that test doesn't have the incomplete dependency.

From https://app.circleci.com/pipelines/github/trptcolin/reply/28/workflows/71b8ccf1-e0b1-4a5a-bde0-95bf8bf50604/jobs/108:

 1) nrepl integration completion tab-completes clojure.core fns
     Expected: <"mapcat">
     to be in: <"REPL-y 0.5.1-SNAPSHOT, nREPL 0.8.3\nClojure 1.7.0\nOpenJDK 64-Bit Server VM 11.0.6+10\n        Exit: Control+D or (exit) or (quit)\n    Commands: (user/help)\n        Docs: (doc function-name-here)\n              (find-doc \"part-of-name-here\")\nFind by Name: (find-name \"part-of-name-here\")\n      Source: (source function-name-here)\n     Javadoc: (javadoc java-object-or-class-here)\n    Examples from clojuredocs.org: [clojuredocs or cdoc]\n              (user/clojuredocs name-here)\n              (user/clojuredocs \"ns-here\" \"name-here\")\nuser=> mapCompilerException java.lang.ClassNotFoundException: incomplete.core, compiling:(NO_SOURCE_PATH:1:131) \n\n#object[clojure.core$map 0x143fdd84 \"clojure.core$map@143fdd84\"]\nuser=> exit\nBye for now!\n"> (using .contains)
     /root/reply/spec/reply/integration_spec.clj:193

(and for easier readability of the actual error the PR code would hit in the remote-app scenario):

CompilerException java.lang.ClassNotFoundException: incomplete.core, compiling:(NO_SOURCE_PATH:1:131)

[trptcolin]

OK, I think I see the arity issue you're alluding to in #205 - incomplete.core defines resolve-class as arity 2 ([ns sym]), and reply.initialization has it as arity 1 ([sym]), mapping back to what clojure-complete did.

I can see how that difference could potentially cause issues - I haven't seen them myself yet (I guess no tests yet around whatever the scenario is, and I'm also not using Clojure much these days), but it seems like a diff that preserves the core (require 'incomplete.core) functionality and the surrounding try-catch machinery, but just deletes this resolve-class stuff could work?

[hsartoris-bard]

The issue I encountered involved autocompleting a protocol method in an aliased namespace, e.g.:

(require '[blah :as b])
(b/some-f

Tabbing resulted in an arity exception.

[hsartoris-bard]

I'm struggling to imagine a situation in which the try/catch is relevant, except in the repl-y tests nREPL integration tests, due to them being run with a different ClassLoader.

Maybe I'm misunderstanding something of the nature of classpath magic, but surely one can assume that 1. reply is on the classpath, since it's being run, and, as such, 2. incomplete is on the classpath, since it's a direct dependency. Are there situations (other than the tests) in which this won't actually be true?

[trptcolin]

These four lines do appear to be safe to delete, as well as the definition of resolve-class above.

[trptcolin]

Great - thanks, I've got the bug reproduced locally now:

user=> (require [clojure.string :as s])
nil
user=> (s/  ;;=> 💥 

Details on the classpath situation: in the most typical use case that I'm aware of (e.g. via lein repl inside of a project's context), you have:

• one process with your full application, which doesn't know about reply or incomplete in terms of its dependencies
• another process with reply/incomplete that connects to the application

lein repl details here. The reply tests use classloaders just because that was easier to spin up than subprocesses the way Leiningen does - but by isolating the classpaths entirely, they're actually catching the right issue here.

In the lein trampoline repl scenario you can put everything in the same JVM, but there are gotchas around dependency conflicts. At any rate, we definitely have to handle the case of totally separate processes for reply itself vs. the application that needs completion running.

I'm all for the majority of this change - just want to make sure we preserve being able to inject completion code across processes. I'm pretty sure this PR will work great if you just delete these 4 lines from the completion-code function, but preserve the rest of that function.

We should add a regression test for the above scenario while we're in there - the test suite is unfortunately fairly light, to my eternal shame. lein spec will run the tests locally - it looks like this bug only happens in some scenarios (e.g. running lein trampoline run on the reply project), and I wasn't able to get it to fail in the "nrepl integration" tests (in spec/reply/integration_spec.clj), but I got it to trigger in a test for "standalone" mode. Probably worth having both tests in there just to be sure we're covered - the test suite isn't that slow yet, even with all the classloader madness.

Here's what I think will work:

diff --git a/spec/reply/integration_spec.clj b/spec/reply/integration_spec.clj
index f0ec927..4870e46 100644
--- a/spec/reply/integration_spec.clj
+++ b/spec/reply/integration_spec.clj
@@ -107,6 +107,15 @@
     (should-contain "mapcat" (str @fake-out))
     (should-contain "map-indexed" (str @fake-out)))
 
+  (it "tab-completes fns in aliased namespaces"
+    (main/launch-standalone {:input-stream
+                             (java.io.ByteArrayInputStream.
+                               (.getBytes "(require '[clojure.string :as s])\ns/\tsplit\nexit\n"))
+                             :output-stream @fake-out})
+    (should= "" (str @fake-err))
+    (should-contain "s/split" (str @fake-out))
+    (should-contain "s/replace" (str @fake-out)))
+
   (it "tab-completes without putting results into *1"
     (main/launch-standalone {:input-stream
                              (java.io.ByteArrayInputStream.
@@ -182,7 +191,6 @@
                       (str @fake-out))))
 
   (describe "completion"
-
     (it "tab-completes clojure.core fns"
       (main/launch-nrepl {:attach (str *server-port*)
                           :input-stream
@@ -191,4 +199,14 @@
                           :output-stream @fake-out})
       (should= "" (str @fake-err))
       (should-contain "mapcat" (str @fake-out))
-      (should-contain "map-indexed" (str @fake-out)))))
+      (should-contain "map-indexed" (str @fake-out)))
+
+    (it "tab-completes fns in aliased namespaces"
+      (main/launch-nrepl {:attach (str *server-port*)
+                          :input-stream
+                          (java.io.ByteArrayInputStream.
+                            (.getBytes "(require '[clojure.string :as s])\ns/\tsplit\nexit\n"))
+                          :output-stream @fake-out})
+      (should= "" (str @fake-err))
+      (should-contain "s/split" (str @fake-out))
+      (should-contain "s/replace" (str @fake-out)))))
diff --git a/src/clj/reply/initialization.clj b/src/clj/reply/initialization.clj
index d114b54..7fe857b 100644
--- a/src/clj/reply/initialization.clj
+++ b/src/clj/reply/initialization.clj
@@ -32,15 +32,6 @@
 (defn export-definition [s]
   (read-string (clojure.repl/source-fn s)))
 
-(def resolve-class
-  (fn [sym]
-    (try (let [val (resolve sym)]
-      (when (class? val) val))
-        (catch Exception e
-          (when (not= ClassNotFoundException
-                      (class (clojure.main/repl-exception e)))
-            (throw e))))))
-
 (defn unresolve
   "Given a var, return a sequence of all symbols that resolve to the
   var from the current namespace *ns*."
@@ -149,11 +140,6 @@
 (defn completion-code []
   `(try
      (require '[incomplete.core])
-     ; hack for 1.2 support until we release the next clojure-complete version
-     ~(export-definition 'reply.initialization/resolve-class)
-     (~'reply.exports/intern-with-meta
-       '~'incomplete.core '~'resolve-class ~'#'resolve-class)
-
      (catch Exception e#
        (try
          (eval

[trptcolin]

Just let me know how you want to handle it, @hsartoris-bard. I'd be excited to take an updated PR that preserves the completion code being injected - but also happy to apply something like the above diff myself if you don't have bandwidth.

[bbatsov]

I didn't see this PR and I've opened anther one deleting just the code for which I'm fairly certain it's fine to remove. Pretty much what @trptcolin had suggested here as well. I wish I first saw this ticket, so I wouldn't have to do the research myself. :D

[bbatsov]

Okay, now I see that @trptcolin also added some tests in his proposed patch, so I guess that'd be the best course of action for now. And sorry for pitching in so late!

[trptcolin]

Yeah @bbatsov @hsartoris-bard if one of y'all have time to update one of your PRs to include a test for the thing that's broken (feel free to grab from that diff above), I'm all for it. 👍 👍 Sorry, a bit slammed today but I should be able to get a release out sometime tomorrow that includes this fix as well as the other two cleanup things @bbatsov.

[bbatsov]

@trptcolin I've just updated my PR.

[trptcolin]

Going to go ahead and close since we've got the fix in the other PR - thanks y'all!