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

Don't step in to clojure.core functions #341

Merged
merged 1 commit into from
Apr 14, 2016

Conversation

grammati
Copy link
Contributor

The new step-in command makes it easy to step in (often accidentally) to
a function in clojure.core. But instrumenting clojure core functions is
dangerous, and can result in a stack overflow that crashes the nrepl
process. In fact, just instrumenting clojure.core/symbol? (without
even debugging it) causes a stack overflow. I'm not entirely sure yet
how this happens, but it seems safest to just prevent stepping in to all
core functions.

@grammati
Copy link
Contributor Author

Oops, I just realized I changed a debug test to step in to a clojure.core function just this morning! It's going to fail... stand by.

@bbatsov
Copy link
Member

bbatsov commented Apr 14, 2016

I'd suggest making this configurable - we can either use a function that would return true if some ns should be skipped or a list of namespaces. At any rate - a tiny bit of extra code will buy us a lot of flexibility. The advantage of using a function would be that you could check for namespaces starting with some common prefix (e.g. clojure.tools).

P.S. By this I mean this should configurable on the Emacs side and the config should be passed when the debugger is initialized.

@@ -464,6 +464,7 @@
;; We do not go so far as to actually try to read the code of the function
;; at this point, which is at macroexpansion time.
(and v
(not= 'clojure.core (ns-name (:ns m)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we hardcode this temporarily, it probably makes sense to check for all built-in namespaces (or at least all the commonly used ones).

@Malabarba
Copy link
Member

P.S. Buy this I mean this should configurable on the Emacs side and the config should be passed when the debugger is initialized.

I think the only risk of overflow here comes from the fact that the debugger is instrumenting functions used by the debugger. So one calls the other in an endless recursion.

If that's the case, we don't need to make this customizable, we only need to exclude all namespaces that debug.clj depends on (be it directly or indeirectly).

@grammati
Copy link
Contributor Author

OK, I already pushed a commit that makes it at least possible (if not easy) to configure this, before I saw your comment @Malabarba.

I think in the short term, we should just prevent people from accidentally stepping into a clojure.core function that's going to crash their process. Then we can refine the list of excluded namespaces as we find troublesome ones.

(def step-in-blacklist
"A set of names or regexes specifying namespaces that should not be stepped in
to. By default, this contains only \"clojure.core\"."
(atom #{"clojure.core"}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a regexp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it so either a string (exact namespace name) or a regexp works. But in light of your earlier comment, do you think I should just switch back to a simpler implementation that hard-codes a list of namespaces to skip (initially containing only "clojure.core")?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I see you have a string? clause below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in light of your earlier comment, do you think I should just switch back to a simpler implementation that hard-codes a list of namespaces to skip (initially containing only "clojure.core")?

I think so. And I think breakpoint-if-interesting should also avoid instrumenting if the current *ns* is in this list.

The new step-in command makes it easy to step in (often accidentally) to
a function in clojure.core. But instrumenting clojure core functions is
dangerous, and can result in a stack overflow that crashes the nrepl
process. In fact, just instrumenting `clojure.core/symbol?` (without
even debugging it) causes a stack overflow. I'm not entirely sure yet
how this happens, but it seems safest to just prevent stepping in to all
core functions by default.
@grammati
Copy link
Contributor Author

OK, I went back to a simpler, non-configurable implementation.

It still only prevents step-in, but I'll add code to prevent manually instrumenting functions in these namespaces if you think that's the right thing to do, @Malabarba .

@Malabarba
Copy link
Member

Let's keep it like this for now. I might block it for manual instrumentation soon, but there's no hury.

@Malabarba Malabarba merged commit bd8df90 into clojure-emacs:master Apr 14, 2016
@bbatsov
Copy link
Member

bbatsov commented Apr 14, 2016

If that's the case, we don't need to make this customizable, we only need to exclude all namespaces that debug.clj depends on (be it directly or indeirectly).

Sure. I originally thought the purpose of this PR was to just skip stepping into boring functions, which definitely seems useful to me.

Guess we should mention those peculiarities in the manual, as some people might wonder why they can't step into certain functions.

@grammati grammati deleted the dont-debug-clojure-core branch April 22, 2017 00:46
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

Successfully merging this pull request may close these issues.

None yet

3 participants