Description

Note that the reported function name in the stack trace is "user$a", where it should be "user$a->b" (or some mangled variant thereof?)

Patch: clj-1083-better-throw-arity-messages-patch-v7.diff

Approach:

Demunge the name to print a better exception message. Note: demunging is not reversible if the original symbol contains a munged word (GT, LT, PLUS, etc). These cases are rare in actual code.

To avoid introducing a dependency on Clojure code from Java code, add new demunge() method to class Compiler, near the existing munge() method. Also replace the two existing Clojure implementations of demunge with a call to this new Java demunge(). Test contains both a couple example tests and a generative test for name munge/demunge roundtrip.

The throwArity now attempts to locate and call clojure.main/demunge. If it finds the function it invokes it and uses the returned string in the error. Otherwise it just throws the actual class name. This results in the following behaviour:

Timothy Baldridge
added a comment - 26/Nov/12 10:30 AM The throwArity now attempts to locate and call clojure.main/demunge. If it finds the function it invokes it and uses the returned string in the error. Otherwise it just throws the actual class name. This results in the following behaviour:
user=> (defn a->b [])
#'user/a->b
user=> (a->b 32)
ArityException Wrong number of args (1) passed to: user/a->b clojure.lang.AFn.throwArity (AFn.java:449)
user=>

The demunge function is created inside of a .clj file. So it is possible that we could hit this exception before demunge exists. The try simply says "if we can get a better error message, use it. Otherwise, fall back to the old (half-broken) method of getting method names"

Timothy Baldridge
added a comment - 30/May/13 2:02 PM The demunge function is created inside of a .clj file. So it is possible that we could hit this exception before demunge exists. The try simply says "if we can get a better error message, use it. Otherwise, fall back to the old (half-broken) method of getting method names"

Andy Fingerhut
added a comment - 07/Jun/13 2:01 PM Presumptuously changing approval from Incomplete back to its former value of Vetted after Timothy responded to what I believe is the reason it was marked Incomplete (Stu's question).

Is it ok to add a dependency from AFn (even if an optional one) to clojure.main/demunge? (The same code is repeated in clojure.repl/demunge btw.) Seems like it would be better for something more common to have a demunge function that all of these could call.

Alex Miller
added a comment - 02/Jul/13 6:47 PM There are some unnecessary whitespace changes in the patch.
Is it ok to add a dependency from AFn (even if an optional one) to clojure.main/demunge? (The same code is repeated in clojure.repl/demunge btw.) Seems like it would be better for something more common to have a demunge function that all of these could call.
Shouldn't we have a test in the patch?
Changing back to Incomplete.

Andy Fingerhut
added a comment - 23/Aug/13 12:44 PM Is Rich the only person that can authoritatively answer Alex's question "Is it ok to add a dependency from AFn (even if an optional one) to clojure.main/demunge?"
I am not sure, but it seems the only way to avoid a dependency from AFn to Clojure code in this case would be to write a version of demunge in Java.

Patch clj-1083-better-throw-arity-messages-patch-v2.txt dated Aug 23 2013 includes all of Timothy Baldridge's patch better-throw-arity-messages.diff dated Nov 26 2012 except it leaves out the unnecessary whitespace changes. It also adds a new test that fails without his patch, and passes with it.

It still has a dependency from AFn to clojure.main/demunge, so if that is not acceptable, something else will be needed.

Andy Fingerhut
added a comment - 23/Aug/13 1:19 PM Patch clj-1083-better-throw-arity-messages-patch-v2.txt dated Aug 23 2013 includes all of Timothy Baldridge's patch better-throw-arity-messages.diff dated Nov 26 2012 except it leaves out the unnecessary whitespace changes. It also adds a new test that fails without his patch, and passes with it.
It still has a dependency from AFn to clojure.main/demunge, so if that is not acceptable, something else will be needed.

Patch clj-1083-better-throw-arity-messages-patch-v3.txt dated Aug 23 2013 includes all of Timothy Baldridge's patch better-throw-arity-messages.diff dated Nov 26 2012 except it leaves out the unnecessary whitespace changes. It also adds a new test that fails without his patch, and passes with it, and fixes a bug with both copies of the demunge implementation that caused it to transform some munged names incorrectly.

It still has a dependency from AFn to clojure.main/demunge, so if that is not acceptable, something else will be needed.

Andy Fingerhut
added a comment - 23/Aug/13 7:52 PM Patch clj-1083-better-throw-arity-messages-patch-v3.txt dated Aug 23 2013 includes all of Timothy Baldridge's patch better-throw-arity-messages.diff dated Nov 26 2012 except it leaves out the unnecessary whitespace changes. It also adds a new test that fails without his patch, and passes with it, and fixes a bug with both copies of the demunge implementation that caused it to transform some munged names incorrectly.
It still has a dependency from AFn to clojure.main/demunge, so if that is not acceptable, something else will be needed.

Alex Miller
added a comment - 26/Aug/13 5:23 PM I still think AFn calling into clojure.main/demunge is weird.
Some other alternatives:
1) Move demunge to clojure.stacktrace (which is also not demunging). Still weird to be calling from the Java portions into the Clojure portions (this is not done elsewhere).
2) Another alternative would be to implement demunge() in another class (putting it in Compiler introduces another weird cycle), perhaps in Util?

Alex, sorry if I'm being a bit dense here, but how does adding a Java version of demunge() into class Compiler introduce a cycle of dependencies? I think all Java source files are compiled before any Clojure source files, so I am guessing you are referring to a dependency between different Java classes? To be clear, when I suggested writing a Java version of demunge(), I mean a "pure Java" implementation that does not refer to anything in the Clojure source files.

My main reason for suggesting adding it to class Compiler is simply to keep the related munge() and proposed new demunge() methods next to each other, and the fields they use.

Andy Fingerhut
added a comment - 26/Aug/13 7:44 PM Alex, sorry if I'm being a bit dense here, but how does adding a Java version of demunge() into class Compiler introduce a cycle of dependencies? I think all Java source files are compiled before any Clojure source files, so I am guessing you are referring to a dependency between different Java classes? To be clear, when I suggested writing a Java version of demunge(), I mean a "pure Java" implementation that does not refer to anything in the Clojure source files.
My main reason for suggesting adding it to class Compiler is simply to keep the related munge() and proposed new demunge() methods next to each other, and the fields they use.

Alex Miller
added a comment - 27/Aug/13 6:33 AM Sorry, I was thinking that it might be weird for AFn to depend on Compiler, but maybe that doesn't matter. Putting it next to munge() makes a lot of sense.

Patch clj-1083-better-throw-arity-messages-patch-v4.txt dated Aug 27 2013 adds a Java implementation of a demunge method to the Compiler class, and uses it in place of previously-existing Clojure implementations of demunge, as well as in the code for throwing arity exceptions.

It introduce a dependency on the Compiler class from class AFn, but not any dependencies on Clojure code from Java code.

Andy Fingerhut
added a comment - 27/Aug/13 8:33 AM Patch clj-1083-better-throw-arity-messages-patch-v4.txt dated Aug 27 2013 adds a Java implementation of a demunge method to the Compiler class, and uses it in place of previously-existing Clojure implementations of demunge, as well as in the code for throwing arity exceptions.
It introduce a dependency on the Compiler class from class AFn, but not any dependencies on Clojure code from Java code.

Patch clj-1083-better-throw-arity-messages-patch-v5.txt dated Aug 27 2013 is identical to previously attached -v4 described above, except it leaves out an unintentional change to the duration of Clojure's generative tests.

I reworked Andy's patch to clean up the Java code in Compiler. The logic seems correct. I added a generative test that checks for roundtripping a symbol name through munge/demunge. The only tricky part of that is avoiding symbol names which cannot properly roundtrip.

Alex Miller
added a comment - 02/Sep/13 10:43 PM I reworked Andy's patch to clean up the Java code in Compiler. The logic seems correct. I added a generative test that checks for roundtripping a symbol name through munge/demunge. The only tricky part of that is avoiding symbol names which cannot properly roundtrip.
Marking screened.

Thanks, Alex. I don't want to derail all of our work because of what is described in this comment, but wanted to record it somewhere after tracking it down.

The fact that demunge as it existed before this patch (and as it still behaves after this patch), can produce names that are not the original function name makes me finally realize why Chas Emerick made this comment in the email thread linked in the description:

"This is perhaps another case where pushing var metadata (or some subset thereof) down to the function being defined in defns would be beneficial; AFunction could then override throwArity to just use the :name of the function being called, thus avoiding any confusion introduced by munged or un-munging names."

It seems that if Chas's idea were implemented, it could be used to improve the output of throwArity to give the correct original Clojure function name in all cases. However, even then the demunge'ing of StackTraceElement class names could not be similarly improved (e.g. in the output of clojure.repl/pst), because all they contain are class names as strings, so functions b? and b_QMARK_ will always appear identical in a StackTraceElement.

Given all of that, I'd say take the best patch for this ticket, and simply advise people not to use substrings like "QMARK" or "COLON" in their function names. If they do, they should be prepared for confusion due to demunge'ing when examining stack traces with functions like clojure.repl/pst

Andy Fingerhut
added a comment - 03/Sep/13 6:24 AM Thanks, Alex. I don't want to derail all of our work because of what is described in this comment, but wanted to record it somewhere after tracking it down.
The fact that demunge as it existed before this patch (and as it still behaves after this patch), can produce names that are not the original function name makes me finally realize why Chas Emerick made this comment in the email thread linked in the description:
"This is perhaps another case where pushing var metadata (or some subset thereof) down to the function being defined in defns would be beneficial; AFunction could then override throwArity to just use the :name of the function being called, thus avoiding any confusion introduced by munged or un-munging names."
It seems that if Chas's idea were implemented, it could be used to improve the output of throwArity to give the correct original Clojure function name in all cases. However, even then the demunge'ing of StackTraceElement class names could not be similarly improved (e.g. in the output of clojure.repl/pst), because all they contain are class names as strings, so functions b? and b_QMARK_ will always appear identical in a StackTraceElement.
Given all of that, I'd say take the best patch for this ticket, and simply advise people not to use substrings like "QMARK" or "COLON" in their function names. If they do, they should be prepared for confusion due to demunge'ing when examining stack traces with functions like clojure.repl/pst

I think the current approach is fine. Munge never made any promises about being reversible.

Given typical naming styles in Clojure, this should rarely be an issue. The consequence is that now you'll get a wrongly demunged (instead of wrongly munged) version; in either case you're not getting the original function name. I enjoyed discovering this detail via the generative tests!!

I do agree that having some way to recover the original function name would be a better solution.

Alex Miller
added a comment - 03/Sep/13 7:47 AM I think the current approach is fine. Munge never made any promises about being reversible.
Given typical naming styles in Clojure, this should rarely be an issue. The consequence is that now you'll get a wrongly demunged (instead of wrongly munged) version; in either case you're not getting the original function name. I enjoyed discovering this detail via the generative tests!!
I do agree that having some way to recover the original function name would be a better solution.