[Fix #950] Check for existence of ns in cider-interactive-eval

[vspinu]

I am fixing this by porting the strategy from cider-interactive-source-tracking-eval, but with preliminary check if ns has been defined. This is necessary to avoid triggering namespace not found error in eval middleware.

[bbatsov]

This way we'll be doing an extra eval request every time, which would also obscure the log quite a bit. I've considered this originally, but I decided it's not worth it. Getting namespace not found is totally sensible if the ns is not evaluated; it's problematic only if you're trying to eval an ns form by itself.

[vspinu]

but I decided it's not worth it.

I don't think log output should decide if a feature is worth it or not. Additional eval takes mseconds and it's not noticibele even on remotes. We can add an enhancement to the logger to abbreviate some ouput if a request has :no-log flag for instance.

Getting namespace not found is totally sensible if the ns is not evaluated; it's problematic only if you're trying to eval an ns form by itself.

Well, the major point of using load-file was to avoid extra C-c C-n. This patch does exactly that - you never need to explicitly evaluate the ns form. Extra eval is a small price to pay for that.

Alternatively, a custom eval middleware should be designed. Maybe it should be done once and for all. This elisp hackery around eval started getting annoying. The eval/eval-with-source split was also a regression IMO. With an extra request you can check for piggiback (or whatever) instead of introducing different interactive eval paths. Now some interactive command use eval-with-source, others eval. This is confusing both for users and developers. Needless to say that most clojure users don't have source references.

[bbatsov]

I don't think log output should decide if a feature is worth it or not. Additional eval takes mseconds and it's not noticibele even on remotes. We can add an enhancement to the logger to abbreviate some ouput if a request has :no-log flag for instance.

There are other reasons I dislike this idea - it doesn't handle changes in the ns macro in the current buffer and we can't really re-eval the ns form all the time as this is super slow on ClojureScript. Not to mention that pretty few people will realise why exactly is going behind the scenes (and we might hide some actual issues this way).

Well, the major point of using load-file was to avoid extra C-c C-n. This patch does exactly that - you never need to explicitly evaluate the ns form. Extra eval is a small price to pay for that.

I'm confused, as I don't understand what you mean by this extra C-c C-n. The major point of using load-file was to get accurate locations for vars. As a corollary we also got for free ns forms being always in sync, which was great, but not nearly as important.

Alternatively, a custom eval middleware should be designed. Maybe it should be done once and for all. This elisp hackery around eval started getting annoying. The eval/eval-with-source split was also a regression IMO. With an extra request you can check for piggiback (or whatever) instead of introducing different interactive eval paths. Now some interactive command use eval-with-source, others eval. This is confusing both for users and developers. Needless to say that most clojure users don't have source references.

Ideally all of this should be handled in nREPL itself (especially the location tracking). Unfortunately I have no time to work on this and AFAIK neither does @cemerick. Still, even such changes will not handle ns synchronisation, but that's rarely a problem in practice.

[vspinu]

we can't really re-eval the ns form all the time as this is super slow on ClojureScript.

Aha!

Could we then explicitly check for clojure script and do eval as normal, otherwise do the old load-file aproach for clojure?

and we might hide some actual issues this way

What could go wrong? As @cemerick pointed out last time, ns evaluation is pretty harmless in clojure. If namespace is already loaded, nothing special happens.

[bbatsov]

Could we then explicitly check for clojure script and do eval as normal, otherwise do the old load-file aproach for clojure?

I'd like to keep the behaviour for Clojure and ClojureScript the same. Many people use both and will constantly be confused about the different behaviour (I know I would be).

What could go wrong? As @cemerick pointed out last time, ns evaluation is pretty harmless in clojure. If namespace is already loaded, nothing special happens.

We might create a ns which shouldn't actually exist (e.g. someone mistyped their ns declaration accidentally). Not a biggie, but still can cause some confusion... Anyways, if we're to create missing namespaces on-the-fly this should be done in nREPL eval itself - cleaner and most efficient. Alternatively we can simply handle namespace-not-found and the resubmit the eval request then. That way we won't be doing two requests all the time.

Once again - for me it will be enough to simply have ns be evaluated in the user namespace. Other things are good to have, but no as important right now.

[bbatsov]

Alternatively we can simply handle namespace-not-found and the resubmit the eval request then. That way we won't be doing two requests all the time.

This might a pretty good solution, btw - it will require no changes to the middleware, the logs will look fine and it won't be problematic on ClojureScript. And I've just figured out how we can handle in a smart way the ns form of a particular buffer - we can keep track of what the ns form was last time something was evaled in the buffer and re-eval it only if there's some difference. Actually, if we do this we won't have to handle namespace-not-found, as initially the buffer-local-variable will be nil, so we'll have to evaluate the initial ns once anyways.

[vspinu]

And I've just figured out how we can handle in a smart way the ns form of a particular buffer - we can keep track of what the ns form was last time something was evaled in the buffer and re-eval it only if there's some difference.

Sounds like a good idea!

[bbatsov]

@vspinu Would you like to take a stab at implementing this?

[vspinu]

@vspinu Would you like to take a stab at implementing this?

Yes. I will look into it right now.

[vspinu]

Ok. Please have a look. I am essentially reverting 1dba82f and thus restoring source refs.

The assumption is that the simple (ns foo.bar) is fast in clojurescript.

[bbatsov]

@vspinu Using load-file is out of the question for various reasons (beside the performance issue it doesn't return a result in ClojureScript). We should use it only for top-level form evaluation where the result is of little importance anyways.

[bbatsov]

Limit the scope of the change to simply tracking ns form.

[vspinu]

@vspinu Using load-file is out of the question for various reasons (beside the performance issue it doesn't return a result in ClojureScript).

Hm. I have now read the whole #830. The problem is pretty dare I must say.

Let's introduce a var (not a user option) cider-eval-backend that can take 'auto, 'eval or 'load-file. Many people don't use clojurescript and not having source refs is anoing.

[bbatsov]

Let's introduce a var (not a user option) cider-eval-backend that can take 'auto, 'eval or 'load-file. Many people don't use clojurescript and not having source refs is anoing.

Let's not overcomplicate things unless 100% necessary. We'll still get proper location info when using cider-eval-defun and this will work OK for cljs as well. And as Chas said - suggesting the use of load-file was a bad idea overall. This has to be fixed in nREPl's eval eventually. There's no point in adding something just to remove it again down the road.

[bbatsov]

P.S. We can also add a simple check in cider-interactive-eval that would check if they form you're trying to eval is a def something form and use cider-eval-defun then, although this is something most people can certainly live without.

[vspinu]

Fixed.

to eval is a def something form and use cider-eval-defun then, although this is something most people

I think people expect to see the result on C-c C-e and, as clojurescript is not returning the last value, it might be confusing. So let's not complicate things for now.

[bbatsov]

The current state of the change looks good to me. Add a changelog entry saying we're now keeping track of the ns form automatically, so it's no longer need to reevalute it manually.

I'd also suggest changing the commit message to something like "Auto-eval source buffer ns form when needed" as the current message is a bit vague.

[vspinu]

I'd also suggest changing the commit message to something like "Auto-eval source buffer ns form when needed" as the current message is a bit vague.

Done.