Multiple NRepls.cs improvements

[setzer22]

Support for sending symbol metadata information via NRepl was incomplete because the :forms and :arglists typically present in symbol's metadata are expected to have different names: :forms-str and :arglists-str. This is related to the fact that these two keys usually contain edn that should not be parsed directly.

With this PR, the nrepl client in NRepl.cs now does the right thing and sends the -str version of the keys. This means that the argument list for user-defined fns and documentation for special forms (like let) should be now shown by most editor plugins. I've tested this behaviour to be correct both with Emacs (CIDER) and VSCode (Calva).

This PR also gets rid of a minor annoyance with Calva (and probably other editors that show documentation on mouse hover) where sometimes it would ask our nrepl server for an empty string, making it throw an exception that was logged to the Unity console. Now those empty string queries are properly ignored, making the Arcadia experience in Calva better.

[setzer22]

I've also implemented automatically setting the ns to the current file's ns when that can be safely determined by folder structure. This gets rid of an annoying issue where you would eval a region of code before evaluating that file's ns form and evaluate that code in another namespace instead.

I believe the old behaviour was error-prone, as it lead to situations where, for instance, you would define a function in multiple namespaces by mistake. However, let me know if the original behaviour was the intended, and I'll rever this last commit, or we can put it behind a config flag instead.

[setzer22]

I couldn't help myself and also implemented a simple version of Intellisense-like autocomplete :) Again, I've tested it to work with both CIDER and Calva.

Some highlights:

• Autocomplete fns in current namespace: The system tries to complete all the symbols available to the current context. That is, the public vars of the current namespace, as well as any use or :refer :all imported symbols.
• Autocomplete fns from other namespaces: You can also autocomplete things from other namespaces, both via alias str/split and fully qualified name clojure.string/split.
• Autocomplete namespaces: The name of namespaces can also be autocompleted.
• Autocomplete keywords: This tries to autocomplete browsing all the interned keywords in the system. NOTE: To implement this, I had to use reflection to access a private member field, similar to what the compliment library for JVM clojure does: https://github.com/alexander-yakushev/compliment/blob/master/src/compliment/sources/keywords.clj

As for the implementation details, I am not really satisfied with having clojure lambdas stored in strings inside the C# source. Suggestions are welcome!

Note this addition may be a bit resource hungry (especially since I do no caching at all). This works fine on my machine and there's no noticeable slowdown. However, I think it would be best if we allow disabling it behind a config flag in configuration.edn. With some instructions, I can take a look at it myself.

[timsgardner]

This looks great, thanks for the hard work! We're testing now

[setzer22]

Thanks! I'm glad to be able to help :D Please let me know if there's anything that should be addressed.

[nasser]

@setzer22 this is awesome! just confirmed working for me in VSCode/Calva on Linux. the autocomplete is a real game changer. its the feature ive missed the most when working in clojure compared to F#, C# or JavaScript -- this is going to make the Arcadia experience really great!

Feedback on the implementation:

As for the implementation details, I am not really satisfied with having clojure lambdas stored in strings inside the C# source. Suggestions are welcome!

What we've done is implement functionality that is more naturally expressed in Clojure in a .clj file, required that file from the C# source, looked up the var and then invoked the var. It's a bit more bureaucracy but it keeps things clean.

There are examples of this approach in the NRepl.cs file. For example, our editor callback system is primarily written in Clojure and lives in Source/arcadia/internal/editor_callbacks.clj. To invoke arcadia.internal.editor-callbacks/add-callback in NRepl.cs we:

1. Create a static Var field to hold the Var reference
2. Load the relevant namespace in the NRepl class's static constructor
3. Populate the static Var field using RT.var
4. Invoke the Var field whenever needed

You could move the Clojure functions that are currently inline into defns in a new file, maybe Source/arcadia/internal/nrepl_support.clj, and invoke them as above.

Note this addition may be a bit resource hungry (especially since I do no caching at all). This works fine on my machine and there's no noticeable slowdown. However, I think it would be best if we allow disabling it behind a config flag in configuration.edn.

Adding a flag to the configuration file makes sense. To do that:

1. Add an entry with the defualt value and documentation to configuration.edn
2. Load and invoke the arcadia.internal.config/config var from your C# source as above.
3. Cast the result to clojure.lang.APersistentMap
4. Invoke the valAt method passing in a Keyword instance that represents the keyword you are looking up.

Let me know if that makes sense. Thanks again for a great contribution!

[setzer22]

Many thanks @nasser ! Again, I'm very glad to be able to contribute :D

I've pushed a new commits implementing your two suggested refactors. Let me know if you think something else can be improved!

I also added a CIDER-specific workaround that I think requires a bit more discussion. Let me ellaborate:

There seems to be an effort in cider to gradually replace middlewares with their own sideloaded implementations (clojure-emacs/cider#2611) if those aren't present. That's actually good, but has an unfortunate side-effect: Since CIDER has no knowledge of CLR, it will assume ours is a Java nREPL and send some very JVM-specific code snippets to eval. If we do the right ting and throw an exception, CIDER acts strangely, and may even freeze.

One particular issue occurs when the "classpath" is not supported by the nrepl server (we have no classpath in CLR!). Cider will try to eval (seq (.split (System/getProperty "java.class.path") ":")) and fail, triggering the aforementioned issue. The workaround I've implemented for this will intercept this particular code string and return an empty list instead of an error. This mostly gets rid of the issue, but I think it's still a good idea to know when this is happening, so I added a LogWarning line that will print an explanatory message. At least affected users will then know what to ask.

Note that all these changes are pretty recent in cider (less than a year old). So it may very well be that some users are not experiencing this issue yet because their CIDER is not at the latest version. Could you please check if you can reproduce this behaviour? I suspect this issue affects Arcadia even prior to the changes introduced in this branch. Some minimal steps to reproduce on my machine are:

• Start Unity and let it start Arcadia and the nREPL server
• Go to emacs, open any namespace, run cider-connect.
• Once connected, run cider-connect again, and repeat
• The Unity console will print the following warning (on this branch): "Your clojure tooling (most likely CIDER) made a request to get the java.class.path which is not available on this platform". In older versions, cider would crash.

I've asked around in #cider at clojurians slack, and I may open an issue there to discuss this as well if they agree this is worth fixing. Hopefully we can get rid of this workaround. Another possibility would be to implement support for the "classpath" op, returning something that makes sense in CLR (a classpath equivalent, perhaps?). This may improve even more the nREPL experience in Arcadia so it's my preferred option.

Sorry for the wall of text! 😅

[nasser]

This looks great @setzer22, thanks for turning it around so fast! Two fine grained things in this review and we should be ready to merge and cut a new release with this functionality.

[setzer22]

@nasser Done!

I've talked to @bbatsov from #cider and with his help I managed to get rid of the classpath workaround. Now we do a cleaner thing by implementing the classpath nrepl op. CLR has no "classpath" per se, but what I'm doing is sending the full path to the Assets folder as well as the Assets/Arcadia/Source. These are the two source roots for clojure code, so cider now does the right thing.

That might become an issue if you're using this NRepl.cs outside of unity. Let me know if that happens so we can look for a better fix.

With this fix, more functionalities are now working: For instance, when you create a new file, cider will automatically detect the right namespace for that file, and add an ns declaration for you.

[bbatsov]

Feel free to ping me any time you need help with something nREPL related. :-)

Btw, is there any particular reason that you opted not to have the server as a separate project? I'm not sure if there's anything Arcadia specific in it, or it can be used by anyone interested in ClojureCLR.

I was also reminded of https://github.com/nrepl/bencode/pull/2/files today, as I wasn't sure if you plan to finish it or not.

[nasser]

One last thing -- using we can support Leiningen projects if you use the CLOJURE_LOAD_PATH environment variable that Arcadia computes on startup.

[nasser]

@setzer22 This is looking really good! Added a note about ClojureCLR's "load path" which is the closest thing to the class path we have. If you can land that change we can support Leiningen projects and the whole thing would be less Arcadia dependent -- should be OK to merge after that!

@bbatsov No reason not to make the server its own project. It is in the Arcadia repository out of convenience more than anything else. I think with @setzer22 latest changes it would make sense to factor it out into its own project given how powerful its become.

wrt nrepl/bencode#2, we ended up continuing to build on our C# implementation that uses the Bencode.NET library, so the Clojure implementation hasn't been a priority. Realistically speaking I won't have time to come back to it any time soon, so it might make sense just to close that PR for now.

[bbatsov]

wrt nrepl/bencode#2, we ended up continuing to build on our C# implementation that uses the Bencode.NET library, so the Clojure implementation hasn't been a priority. Realistically speaking I won't have time to come back to it any time soon, so it might make sense just to close that PR for now.

Makes sense to me.

@bbatsov No reason not to make the server its own project. It is in the Arcadia repository out of convenience more than anything else. I think with @setzer22 latest changes it would make sense to factor it out into its own project given how powerful its become.

I think that'd be pretty cool indeed. The lack of a working nREPL implementation has long been a problem for wider ClojureCLR adoption (at least IMO).

[setzer22]

@nasser I'll start working on the changes soon!

About making a standalone CLR nREPL, I'm all in for that! I haven't used Clojure CLR outside of Arcadia, but that's partly due to the difficulty in setting up a good development environment.

As for the autocompletion support, I've found out @sogaiu, another member of the community, has a fork of the clojure-complete library for CLR: https://github.com/sogaiu/clojure-complete/tree/clr-support

Since that is more tested, and has a few interesting features, I'd suggest merging that with my implementation in nrepl_support.clj. One of the nice additions would be completing the static methods of a class, which would be a great for Unity interop.

In fact, the only thing that's missing is support for keyword completions, but I could patch in my implementation easily and we'd have a very good autocompletion support!

What do you think?

[setzer22]

I went ahead and added the new autocompletion library I mentioned yesterday. I had to modify the original code slightly, and put it into a new namespace arcadia.internal.autocompletion. I've also added a short comment at the beginning listing the credits for the original contributors.

Let me know if you'd rather manage this as an external dependency. I am not aware we can do this yet in Arcadia, so I've just went the good ol' copy-paste approach 😄

The arcadia.internal.nrepl_support ns is now only used to bencode the results and call this other library. I've decided to split it this way so we can centralize all future nrepl features (e.g., better exception formatting) into this namespace.

The new autocompletion engine can now handle:

• Class names (e.g. "Vector3") that have been imported into the namespace
• Static members of classes (e.g. "Vector3/Dot")
• All the original functionality

[bbatsov]

Regarding the completion - you can also check this nrepl/nrepl#174

I'm planning to adapt the original clojure-complete, so it returns completions in the same format (a sequence of maps) as cider-nrepl's "complete" op, and inline this into nREPL.

[lccambiaghi]

Hi @setzer22 thank you for this amazing contribution! How can I as a CIDER user make use of auto-completion?

I have checked my Arcadia/configuration.edn and the designated option is true.

I simply executed cider-connect at port 3722 but I don't get any autocomplete.. I get this warning:

;; Connected to nREPL server - nrepl://localhost:3722
;; CIDER 1.0.0snapshot
WARNING: Can't determine Clojure version.  The refactor-nrepl middleware requires clojure 1.8.0 (or newer)WARNING: No Clojure project was detected. The
refactor-nrepl middleware was not enabled. (You can mute this
warning by customizing `cljr-suppress-no-project-warning'.)

Thank you in advance!

[setzer22]

Hi @lccambiaghi, I'm glad you like this :)

I don't have a lot of time to work on this right now. But I can tell you it used to work just fine in CIDER. Emacs is my main editor and I made sure every feature worked there. That being said, there might have been a breaking change in either cider, ClojureCLR/Arcadia or the NRepl implementation.

If I had to look somewhere, I'd start at the Unity console, see if you can find any errors there. Also, try having a look at the arcadia.internal.autocompletion namespace. You can open it in emacs and connect to the REPL, and you should be able to play around in it. Try invoking the autocompletion functions in there through the REPL and see whether they work that way.

I hope some of this helps you debug the issue :)

[lccambiaghi]

Thank you for your quick reply! It may very well be that I am doing something wrong. For example, should I have a project.clj? Do I need to do something particular other than cider-connect and set the port to 3722?

In the Unity console I read this error:

NullReferenceException: Object reference not set to an instance of an object
Arcadia.NRepl.HandleMessage (BencodeNET.Objects.BDictionary message, System.Net.Sockets.TcpClient client) (at Assets/Arcadia/Editor/NRepl.cs:471)
Arcadia.NRepl+<>c__DisplayClass36_0.<StartServer>b__1 () (at Assets/Arcadia/Editor/NRepl.cs:580)
UnityEngine.Debug:LogException(Exception)
Arcadia.<>c__

I understand you may not have the time! I wish I knew how to debug this ahah.. Thanks anyway!

[lccambiaghi]

Hi @setzer22 sorry to bother you again.. Would you be able to help me debug this issue? I tried to inspect the namespace indicated by you but I keep getting this NullReferenceExceptions..

What I could understand is that in the HandleMessage function either the client or the message is null.. When I connect with CIDER in the console I can see that the client is printed so I suspect the message? Do you have any recommendation on what I can do to enable the CIDER tooling? I feel like I am a few steps away from enabling the tools :D

[FilipCon]

Hello @lccambiaghi ! I also experience some issues with autocompletion using CIDER. I just found that by using CIDER v0.24.0, the autocompletion seems to work without issues. Unfortunately I'm not an expert so I can't pinpoint the cause of error, but I hope it helps.

[lccambiaghi]

Hi @FilipCon thank you so much!!! That fixed the NullReferenceException!

However, I still cannot see documentation and jump to definition for functions...

EDIT: ok, documentation is working with cider-doc!