Clojure

Need better error report when omitting parameter list from (fn) or (defn)

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.5
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Somehow I just forgot to define parameters for my zero argument function and it took me a long time to notice it, mostly because I had been working with a lot of macros but especially because I didn't get an error that said "You must provide a list of parameter names", but instead: "Don't know how to create ISeq from: Symbol".

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Assembla Importer added a comment -

Converted from http://www.assembla.com/spaces/clojure/tickets/157

Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/157
Hide
Permalink
Alex Miller added a comment -

I have hit this several times. Would be very nice (esp for newbs) to have a guard-rail here that detected this as a possible issue.

Show
Alex Miller added a comment - I have hit this several times. Would be very nice (esp for newbs) to have a guard-rail here that detected this as a possible issue.
Hide
Permalink
Kevin Sookocheff added a comment -

I've came across this as well and would welcome a more user friendly error message.

Show
Kevin Sookocheff added a comment - I've came across this as well and would welcome a more user friendly error message.
Hide
Permalink
Stuart Sierra added a comment -

This is an easy mistake to make - I do it myself quite often. Better error messages for this common case would be good for new users.

CLJ-157 could probably be subsumed into a more general patch improving error messages about the expected arguments to fn.

defn is a macro which expands to fn, so a patch to fn should fix both.

Show
Stuart Sierra added a comment - This is an easy mistake to make - I do it myself quite often. Better error messages for this common case would be good for new users. CLJ-157 could probably be subsumed into a more general patch improving error messages about the expected arguments to fn. defn is a macro which expands to fn, so a patch to fn should fix both.
Hide
Permalink
Ambrose Bonnaire-Sergeant added a comment -

I'm working a patch for this.

Show
Ambrose Bonnaire-Sergeant added a comment - I'm working a patch for this.
Hide
Permalink
Ambrose Bonnaire-Sergeant added a comment -

Is it ok to break the behavior of these two cases?

(fn a)
(fn)

So far I am throwing an error "Parameter list missing" instead of silently returning a function with AFAICT undefined behavior.

Show
Ambrose Bonnaire-Sergeant added a comment - Is it ok to break the behavior of these two cases? (fn a) (fn) So far I am throwing an error "Parameter list missing" instead of silently returning a function with AFAICT undefined behavior.
Hide
Permalink
Ambrose Bonnaire-Sergeant added a comment -

I've attached a patch. There are some breaking changes for undefined behavior, I have detailed them in the commit message.

Show
Ambrose Bonnaire-Sergeant added a comment - I've attached a patch. There are some breaking changes for undefined behavior, I have detailed them in the commit message.
Hide
Permalink
Stuart Halloway added a comment -

The trickiest thing with this kind of error reporting is guessing what the programmer was trying to do:

The greater the probability a random string is a valid program, the harder it is to report errors well.

– http://www.paulgraham.com/redund.html
An example:

(defn add (a b) (+ a b))
IllegalArgumentException Parameter declaration a should be a vector  clojure.core/assert-valid-fdecl

Here the error message is based on a guess that I was trying to use the multi-arity syntax, but I was probably using the single-arity syntax with the wrong brackets. You might split these cases out by counting the top-level forms first.

I think I am cool with the change to (fn) and (fn a), but I can imagine a code generator relying on this behavior as a sentinel to avoid explicit coding for a corner case. What do others think?

One small thing in the code. Why the redundant boolean work here?

(if (instance? clojure.lang.Symbol name) false true)
Show
Stuart Halloway added a comment - The trickiest thing with this kind of error reporting is guessing what the programmer was trying to do:
The greater the probability a random string is a valid program, the harder it is to report errors well.
– http://www.paulgraham.com/redund.html An example: 
(defn add (a b) (+ a b))
IllegalArgumentException Parameter declaration a should be a vector  clojure.core/assert-valid-fdecl
Here the error message is based on a guess that I was trying to use the multi-arity syntax, but I was probably using the single-arity syntax with the wrong brackets. You might split these cases out by counting the top-level forms first. I think I am cool with the change to (fn) and (fn a), but I can imagine a code generator relying on this behavior as a sentinel to avoid explicit coding for a corner case. What do others think? One small thing in the code. Why the redundant boolean work here? 
(if (instance? clojure.lang.Symbol name) false true)
Hide
Permalink
Ambrose Bonnaire-Sergeant added a comment - 
(if (instance? clojure.lang.Symbol name) false true)

It's so early in the bootstrap that both `not` and `symbol?` are not defined. That line is in the definition of `defn`. I added a comment documenting this.

Show
Ambrose Bonnaire-Sergeant added a comment - 
(if (instance? clojure.lang.Symbol name) false true)
It's so early in the bootstrap that both `not` and `symbol?` are not defined. That line is in the definition of `defn`. I added a comment documenting this.
Hide
Permalink
Colin Jones added a comment -

I run into this all the time too - thanks for doing this, Ambrose!

Stuart's example is an interesting one - that could have meant at least one other thing in addition to multi-arity syntax and single-arity syntax with accidentally-list params: single-arity syntax with missing param list, and two successive body forms (where a and b are free variables). I think the error message here would still be an improvement over "Don't know how to create ISeq from: Symbol", but maybe it tries to be too specific in talking about "a". I'd tend toward being more generic and just saying "Parameter declaration should be a vector", which I think would be helpful and correct in all the intended cases we've imagined for this particular example.

I think breaking (fn) / (fn a) makes sense - if there's a valid use for those I'd be interested in seeing it. Relying on ArityException in code generation seems fairly error-prone to me, but I could easily be missing something here.

To the boolean point, I tend to agree with Stuart and would prefer reversing the if/else clauses to avoid it:

(if (instance? clojure.lang.Symbol name)
  nil      
  (throw (IllegalArgumentException. "First argument to defn must be a symbol")))
Show
Colin Jones added a comment - I run into this all the time too - thanks for doing this, Ambrose! Stuart's example is an interesting one - that could have meant at least one other thing in addition to multi-arity syntax and single-arity syntax with accidentally-list params: single-arity syntax with missing param list, and two successive body forms (where a and b are free variables). I think the error message here would still be an improvement over "Don't know how to create ISeq from: Symbol", but maybe it tries to be too specific in talking about "a". I'd tend toward being more generic and just saying "Parameter declaration should be a vector", which I think would be helpful and correct in all the intended cases we've imagined for this particular example. I think breaking (fn) / (fn a) makes sense - if there's a valid use for those I'd be interested in seeing it. Relying on ArityException in code generation seems fairly error-prone to me, but I could easily be missing something here. To the boolean point, I tend to agree with Stuart and would prefer reversing the if/else clauses to avoid it: 
(if (instance? clojure.lang.Symbol name)
  nil      
  (throw (IllegalArgumentException. "First argument to defn must be a symbol")))
Hide
Permalink
Ambrose Bonnaire-Sergeant added a comment -

Doh, of course Colin's suggestion is preferable.

Show
Ambrose Bonnaire-Sergeant added a comment - Doh, of course Colin's suggestion is preferable.
Hide
Permalink
Ambrose Bonnaire-Sergeant added a comment -

Added a second patch to fix redundant conditional

Show
Ambrose Bonnaire-Sergeant added a comment - Added a second patch to fix redundant conditional
Hide
Permalink
Ambrose Bonnaire-Sergeant added a comment -

Another breaking change to undefined behavior I didn't realize. (defn a) now throws an exception, instead of silently returning a function.

Show
Ambrose Bonnaire-Sergeant added a comment - Another breaking change to undefined behavior I didn't realize. (defn a) now throws an exception, instead of silently returning a function.
Hide
Permalink
Carin Meier added a comment - - edited

I checked out the changes and tried it out successfully. I think it is a great improvement. My only comment is regarding the text of the error message for 

user=> (defn [x])
IllegalArgumentException First argument to defn must be a symbol  clojure.core/defn (core.clj:273)

It might be helpful to newcomers to mention that the parameter is the name. Something like, "Name argument to defn must be a symbol".

Show
Carin Meier added a comment - - edited I checked out the changes and tried it out successfully. I think it is a great improvement. My only comment is regarding the text of the error message for 
user=> (defn [x])
IllegalArgumentException First argument to defn must be a symbol  clojure.core/defn (core.clj:273)
It might be helpful to newcomers to mention that the parameter is the name. Something like, "Name argument to defn must be a symbol".
Hide
Permalink
Stuart Sierra added a comment -

Vetted. Moving to "Approved Backlog" because it's an enhancement we would like to have in the next release.

Show
Stuart Sierra added a comment - Vetted. Moving to "Approved Backlog" because it's an enhancement we would like to have in the next release.
Hide
Permalink
Stuart Sierra added a comment -

Changing priority to "Minor"

Show
Stuart Sierra added a comment - Changing priority to "Minor"
Hide
Permalink
Andy Fingerhut added a comment -

clj-157-better-err-msgs-for-defn-fn-syntax-errors-patch2.txt has no substantive changes from Ambrose's two patches. It does combine them into one commit, and unlike the older ones applies cleanly to latest master as of March 7, 2012.

Show
Andy Fingerhut added a comment - clj-157-better-err-msgs-for-defn-fn-syntax-errors-patch2.txt has no substantive changes from Ambrose's two patches. It does combine them into one commit, and unlike the older ones applies cleanly to latest master as of March 7, 2012.
Hide
Permalink
Stuart Sierra added a comment -

Screened.

Show
Stuart Sierra added a comment - Screened.
Hide
Permalink
Stuart Sierra added a comment -

Patch applied.

Show
Stuart Sierra added a comment - Patch applied.

People

Vote (7)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.