Clojure

The quote special form should throw an exception if passed more than one form to quote

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.5
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Triaged

Description

Quote currently ignores all but the first argument. In the case of being called accidentally with multiple values, it should throw an exception specifying the error.

user> (quote 1 2 3)
1

------- Original: --------

Every once in a while, you can just go down the rabbit hole.

I had an errant expression in my code:

(-> message get-message-values 'DESTINATION_MERCHANT_ID)

One would think this would work; it certainly would if the key was a keyword and not a symbol.

One would expect this to expand to:

('DESTINATION_MERCHANT_ID (get-message-values message))

however, the reader is involved, so it is as if the source were:

(-> message get-message-values (quote DESTINATION_MERCHANT_ID))

which expands to:

(quote (-> message get-message-values) DESTINATION_MERCHANT_ID))

... hilarity ensues! Because quote currently ignores extra parameters, my code gets the quoted value '(clojure.core/-> message get-message-values) rather than the expected string from the map; this shifts us from the "there's a bug in my code" to "the nature of reality is broken".

The correct expression is:

(-> message get-message-values (get 'DESTINATION_MERCHANT_ID))

This took quite a while to track down; if the

special form checked that it was passed exactly one form to quote and threw an exception otherwise, I think I would have caught this much earlier. It could even identify the expression it is quoting, which would provide a lot better understanding of where I went wrong.

  1. CLJ-1282-p1.patch
    09/Dec/13 9:32 AM
    2 kB
    Gary Fredericks
  2. CLJ-1282-p2.patch
    01/Feb/14 9:58 AM
    2 kB
    Gary Fredericks

Activity

Hide
Nicola Mometto added a comment -

I'm sorry I did not notice your previois comment.
I'm asking because I need to know whether I should throw on (quote) for tools.analyzer, currently it is allowed but I too think that (quote) should be an error.

Show
Nicola Mometto added a comment - I'm sorry I did not notice your previois comment. I'm asking because I need to know whether I should throw on (quote) for tools.analyzer, currently it is allowed but I too think that (quote) should be an error.
Hide
Gary Fredericks added a comment -

I consciously chose to make (quote) an error – I made a comment about that earlier and didn't get any feedback, so I unilaterally decided to make it an error due to the fact that I couldn't think of any possible use for (quote).

It's an easy switch if somebody thinks differently.

Show
Gary Fredericks added a comment - I consciously chose to make (quote) an error – I made a comment about that earlier and didn't get any feedback, so I unilaterally decided to make it an error due to the fact that I couldn't think of any possible use for (quote). It's an easy switch if somebody thinks differently.
Hide
Nicola Mometto added a comment -

Currently (quote) returns nil, is it intended that this patch makes that an error or was this by accident?

Show
Nicola Mometto added a comment - Currently (quote) returns nil, is it intended that this patch makes that an error or was this by accident?
Hide
Alex Miller added a comment -

Moving back to Triaged for more looks.

Show
Alex Miller added a comment - Moving back to Triaged for more looks.
Hide
Gary Fredericks added a comment -

Attached CLJ-1282-p2.patch which is off of the current master and addresses Stu's points.

Show
Gary Fredericks added a comment - Attached CLJ-1282-p2.patch which is off of the current master and addresses Stu's points.
Hide
Gary Fredericks added a comment -

Throwing an ex-info is easy enough. I don't know how to avoid at least incidentally checking for the exception type, since the ExceptionInfo is wrapped in a CompilerException. I'll make a patch that keeps the class name in the test but doesn't do any checks on the cause aside from the ex-data. Let me know if I should do anything different.

Show
Gary Fredericks added a comment - Throwing an ex-info is easy enough. I don't know how to avoid at least incidentally checking for the exception type, since the ExceptionInfo is wrapped in a CompilerException. I'll make a patch that keeps the class name in the test but doesn't do any checks on the cause aside from the ex-data. Let me know if I should do anything different.
Hide
Andy Fingerhut added a comment -

Patch CLJ-1282-p1.patch no longer applies cleanly after commits made to Clojure master on Jan 31 2014, probably due to the patch committed for CLJ-1318, and probably only because some lines of context changed in the test file. That would be trivial to update, but Stu's comments above suggest that more significant changes need to be made.

Show
Andy Fingerhut added a comment - Patch CLJ-1282-p1.patch no longer applies cleanly after commits made to Clojure master on Jan 31 2014, probably due to the patch committed for CLJ-1318, and probably only because some lines of context changed in the test file. That would be trivial to update, but Stu's comments above suggest that more significant changes need to be made.
Hide
Stuart Halloway added a comment -

I recommend the following changes:

  • throw an ex-info that includes the offending form in its map {:form ...}
  • check only for the map data, not exception type or message, in the tests
Show
Stuart Halloway added a comment - I recommend the following changes:
  • throw an ex-info that includes the offending form in its map {:form ...}
  • check only for the map data, not exception type or message, in the tests
Hide
Gary Fredericks added a comment - - edited

Attached p1, which throws an IllegalArgumentException (wrapped in a CompilerException of course) for anything but 1 arg, and includes the number of args that were passed.

I can't think of any reason why (quote) would be useful, so I decided to throw on that too. Very easy to change of course.

Also added a test that (eval '(quote 1 2 3)) throws.

Show
Gary Fredericks added a comment - - edited Attached p1, which throws an IllegalArgumentException (wrapped in a CompilerException of course) for anything but 1 arg, and includes the number of args that were passed. I can't think of any reason why (quote) would be useful, so I decided to throw on that too. Very easy to change of course. Also added a test that (eval '(quote 1 2 3)) throws.
Hide
Gary Fredericks added a comment -

(quote) currently returns nil. Do we have an opinion about that?

Show
Gary Fredericks added a comment - (quote) currently returns nil. Do we have an opinion about that?
Hide
Alex Miller added a comment -

That's why I left the original in there too.

Show
Alex Miller added a comment - That's why I left the original in there too.
Hide
Howard Lewis Ship added a comment - - edited

I just wanted to point out that your description is shorter, but makes it appear that such a use is unlikely and therefore unimportant; the detail of my description is to point out a reasonable situation where something explicable, but completely counterintuitive and confusing, does occur.

Show
Howard Lewis Ship added a comment - - edited I just wanted to point out that your description is shorter, but makes it appear that such a use is unlikely and therefore unimportant; the detail of my description is to point out a reasonable situation where something explicable, but completely counterintuitive and confusing, does occur.
Hide
Howard Lewis Ship added a comment -

Sorry, can't edit the description now to correct the formatting errors.

Show
Howard Lewis Ship added a comment - Sorry, can't edit the description now to correct the formatting errors.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated: