Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Namespace not found" considered harmful #1980

Closed
phillord opened this issue Apr 26, 2017 · 15 comments
Closed

"Namespace not found" considered harmful #1980

phillord opened this issue Apr 26, 2017 · 15 comments

Comments

@phillord
Copy link
Contributor

phillord commented Apr 26, 2017

I have some code (https://github.com/phillord/tawny-owl/blob/master/emacs/tawny-mode.el) which uses cider. In the latest version of cider it doesn't seem to work any more. This is, of course, my problem rather than CIDERs (and in fact I have fixed it!)

However, it gives the "Namespace not found" error. I feel that this is not a helpful error message. It's actually easy to find out where this comes from directly (nrepl-make-response-handler), but then the trail goes cold. Is something happening in the cider-nrepl middleware which is signalling this?

There are quite a few other "Namespace not found" issues around which suggests that I am not the only one finding this problematic. So I wonder, can we make the error handling better, or the documentation, for dealing with this error.

@bbatsov
Copy link
Member

bbatsov commented Apr 29, 2017

This error occurs when you evaluate some code in a namespace that doesn't exist yet. As such evaluations can't really work, I don't see a way around it. What would you suggest as an alternative approach?

@phillord
Copy link
Contributor Author

Reading some of the issues, it seems also to occur when there is a bug in cider. Do you have any more information at the point of failure that could be fed back? Do we know what the name of the non-existant namespace is?

@bbatsov
Copy link
Member

bbatsov commented Apr 29, 2017

Reading some of the issues, it seems also to occur when there is a bug in cider.

Perhaps. Don't remember anything specific, though.

Do we know what the name of the non-existant namespace is?

Every response has a ns I think, so we should be able to add it to the error message.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2017

Guess I was wrong:

(<-- 
  id  "13"
  session  "31f55dd0-f8d6-4c14-9b0e-c093dec26015"
  status  ("namespace-not-found" "done" "error")
)

I think it'd be nice to patch nREPL to return the ns in such cases, as it's hard to present an informative message otherwise. //cc @cemerick

This function does some ns magic to prevent users from having to think about evaluating the ns declaration all the time - this is why normally you'd never see this error when doing interactive evaluation.

(defun cider--prep-interactive-eval (form)
  "Prepare the environment for an interactive eval of FORM.

Ensure the current ns declaration has been
evaluated (so that the ns containing FORM exists).
If FORM is a ns declaration it's not processed and cached.

Clears any compilation highlights and kills the error window."
  (cider--clear-compilation-highlights)
  (cider--quit-error-window)
  (let ((cur-ns-form (cider-ns-form)))
    (when (and cur-ns-form
               (not (string= cur-ns-form (cider--cached-ns-form)))
               (not (cider-ns-form-p form)))
      ;; TODO: check for evaluation errors
      (cider-eval-ns-form 'sync)
      (cider--cache-ns-form))))

I still wonder if hiding this was a good or a bad idea overall - it's more convenient, but it might also be a bit confusing.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2017

Is something happening in the cider-nrepl middleware which is signalling this?

I guess I failed to answer this - it's from nREPL's built-in eval middleware. As mentioned before it doesn't return the ns in question, which is the reason why the error message is so vague.

@phillord
Copy link
Contributor Author

it's from nREPL's built-in eval middleware.

Ah, now I understand why I couldn't grep it. I was looking in cider-nrepl.

If it's this bit of code:

(if (and ns (not explicit-ns-binding))
  (t/send transport (response-for msg {:status #{:error :namespace-not-found :done}}))
 ....

Then it certainly seems to have access to the namespace.

Still, even here, I think that the message could be made better. What normal user circumstances can it occur under? Given that the intentionality of the code above is to make it happen magically, it this not indicative of a bug now?

@bbatsov
Copy link
Member

bbatsov commented May 1, 2017

Then it certainly seems to have access to the namespace.

Definitely. That's why I suggested we should patch in nREPL.

Still, even here, I think that the message could be made better. What normal user circumstances can it occur under? Given that the intentionality of the code above is to make it happen magically, it this not indicative of a bug now?

This magical ns loading will not always work - you can always write something wrong using the evaluation API for instance, so guarding against a missing ns makes sense to me. Can you give me an example of some code where this was harmful?

@phillord
Copy link
Contributor Author

phillord commented May 4, 2017

Oh, no I think that the magic loading is a good thing. Just suggesting that if it doesn't work, then we should have an error message which is more "we've found a bug somewhere in CIDER" rather than a more "normal" error message. CIDER already makes this distinction in other places.

@bbatsov
Copy link
Member

bbatsov commented May 5, 2017

Oh, no I think that the magic loading is a good thing. Just suggesting that if it doesn't work, then we should have an error message which is more "we've found a bug somewhere in CIDER" rather than a more "normal" error message. CIDER already makes this distinction in other places.

The problem is that this magic loading relies on you doing interactive evaluation in a source buffer - if you're not doing this the check is absolutely mandatory. So you can't really tell wether there was a bug in CIDER or not (at least now without significantly reworking the evaluation logic). Just imagine the case you're leveraging the API directly and pass a non-existing namespace there - then the message makes perfect sense. Not to mention that you can disable the magic ns tracking if you don't like it (it adds some noise to evaluation commands and does things you might not really want to do after all).

Something that was discussed many times but never done, mostly due to lack of time. I've been wary of picking up major tasks lately simply because I don't really have time to finish any of them... The whole evaluation callback code is a total mess and my desire was to rewrite it completely, but whether this is ever going to happen I cannot promise.

@phillord
Copy link
Contributor Author

phillord commented May 5, 2017

Okay, so I think the best we can do in the short term is, as above, include the namespace in the error message. I am sure that this will help clarify the problem somewhat.

@bbatsov
Copy link
Member

bbatsov commented May 5, 2017

Fine by me. Obviously we need to submit a trivial patch to nREPL for this. You you mind doing this? Or we can just ask @cemerick if he's willing to directly make this small change to save a bit of time. (although he's pretty busy and I doubt he'll see the mention here).

@dpsutton
Copy link
Contributor

dpsutton commented May 5, 2017

The whole evaluation callback code is a total mess and my desire was to rewrite it completely, but whether this is ever going to happen I cannot promise.

@bbatsov think you could write an article describing your idea and asking for contributors?

@bbatsov
Copy link
Member

bbatsov commented May 5, 2017

See #1099

@cemerick
Copy link
Contributor

cemerick commented May 5, 2017

The namespace-not-found error only occurs if an evaluation namespace (ns) is specified in the request, but doesn't exist:

https://github.com/clojure/tools.nrepl/blob/c14eaa89dbb750e230530afd267cd53fdd486c51/src/main/clojure/clojure/tools/nrepl/middleware/interruptible_eval.clj#L83-L84

So the only "fix" on the nREPL side would be to echo back the non-existent namespace. This is fine IMO, but shouldn't block fixing/debugging whatever the root problem is here (I didn't read all of the issue's history): just look at the request preceding the error response, and you should see what ns is being sent.

@bbatsov
Copy link
Member

bbatsov commented May 6, 2017

So the only "fix" on the nREPL side would be to echo back the non-existent namespace. This is fine IMO, but shouldn't block fixing/debugging whatever the root problem is here (I didn't read all of the issue's history): just look at the request preceding the error response, and you should see what ns is being sent.

Yeah, that's exactly the fix I had in mind. There's no actual problem apart from the lacking ns in the error response. I thought about pulling it out from the request, but that'd be a bit complex as we'll also have to implement some way to find the request the triggered certain response.

bbatsov added a commit to nrepl/nrepl that referenced this issue Jul 24, 2018
Previously you'd only get an error that a ns is missing,
but no mention of the name of that namespace.

Refer to clojure-emacs/cider#1980 for more
details regarding this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants