[Fix cider#1089] Don't instrument symbol in call position

[Malabarba]

Defines a new function instrument-function-call, which is like instrument-coll, except the first element is not instrumented.
This is only used if the first element is a symbol.

[Malabarba]

clojure-emacs/cider#1089

[bbatsov]

function call == Java method call?

[Malabarba]

Here, function call means a sexp that starts with a symbol which is not a macro nor a special form.
So it includes java methods, but also includes any regular function call.

I'll clarify that in the docstring. Would you prefer a different function name?

[bbatsov]

No need for a new name.

[Malabarba]

The tests are failing with

java.lang.Exception: Problem opening jar /home/travis/.m2/repository/org/clojure/clojure/1.7.0-beta2/clojure-1.7.0-beta2.jar

Is that an issue with travis?

[bbatsov]

The tests are failing with

java.lang.Exception: Problem opening jar /home/travis/.m2/repository/org/clojure/clojure/1.7.0-beta2/clojure-1.7.0-beta2.jar
Is that an issue with travis?

Maybe.

Btw, looking at the issue this is supposed to fix I'm kind of confused. How exactly is this change related to dealing with static Java methods?

[Malabarba]

Btw, looking at the issue this is supposed to fix I'm kind of confused. How exactly is this change related to dealing with static Java methods?

Static java methods (AFAIK) are either called as functions (as in (System/something) or (.stop)) or called with the . and .. forms. Both of the latter are already covered by the debugger's handling of macros and special forms, so only the former is a problem.

The symbol on a call position (AFAIK) is either:

1. A java method, which we can't breakpoint.
2. A clojure var bound to a regular function, which we can breakpoint but probably don't want to (it's signature is messy and doesn't really convey information).
3. A keyword, which we never breakpoint anyway because it's a constant.
4. A clojure var holding a set or a map.

So, with that in mind, I think only in the 4th case (which is probably the least common) would it be interesting to breakpoint a symbol at call position. So I figured I'd disable this completely, which, in turn, will fix item 1.

[cichli]

The tests are failing with

java.lang.Exception: Problem opening jar /home/travis/.m2/repository/org/clojure/clojure/1.7.0-beta2/clojure-1.7.0-beta2.jar

Is that an issue with travis?

I've cleared the relevant caches, will you try force pushing to restart the build? I can't seem to restart it from the web UI for some reason.

[bbatsov]

@Malabarba Sounds reasonable. So what happens when an expressions like this is encountered in instrumented code? Do we just step over it or something?

I'm asking because normally I instrument only definitions directly and I guess this code instruments the portions of the definition. Being able to step inside a function call in a definition is definitely useful.

P.S. I never got to reading the code for the debugger in greater detail, so forgive me if I'm asking something stupid.

[Malabarba]

Sounds reasonable. So what happens when an expressions like this is encountered in instrumented code? Do we just step over it or something?

This PR doesn't instrument the first symbol but does instrument the arguments. So, for instance,

(instrument '(some-function arg-1 arg-2))

Is essentially instrumented as (with some stuff omited)

(some-function (breakpoint arg-1)
               (breakpoint arg-2))

I'm asking because normally I instrument only definitions directly and I guess this code instruments the portions of the definition. Being able to step inside a function call in a definition is definitely useful.

Yes, this PR will still step inside the function call if it has arguments. It will keep doing what was done before, with the exception that it steps over the function name.

The current code wraps a breakpoint around the function name too (as long as it's not a special-symbol, a macro, or a name from the clojure.core namespace). This means that when you step inside a function call, the function name is one of the steps.
I figured that's rarely a useful step, so in this PR we just always go over the function name.

P.S. I never got to reading the code for the debugger in greater detail, so forgive me if I'm asking something stupid.

Not at all. The more people who understand what's going on there, the better.

[Malabarba]

Expanded the docstring and force-pushed.

[bbatsov]

Got it! Thanks!