Clojure

Evaling #{do ...} or [do ...] is treated as the do special form

Details

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

Description

Problem: Evaluating a persistent collection for which the function first returns the symbol do leads to that collection being treated as the do special form, even though it may be a vector or even a set. IMHO, the expected result would be to report that do cannot be resolved. 

[do 1 2]
;=> 2

#{"hello" "goodbye" do}
;=> "hello"
; Wat?

Cause: The check for do is checking for IPersistentCollection instead of ISeq.

Solution: Change the cast (occurs in two places) for the do form check from IPersistentCollection to ISeq:

if(form instanceof IPersistentCollection && Util.equals(RT.first(form), DO))

to

if(form instanceof ISeq && Util.equals(RT.first(form), DO))

Current patch: CLJ-1184-p4.patch

Screened by: Alex Miller

Attachments

  1. Text File
    CLJ-1184-p1.patch
    26/May/13 2:13 PM
    2 kB
    Gary Fredericks
  2. Text File
    CLJ-1184-p2.patch
    02/Jul/13 9:52 PM
    3 kB
    Gary Fredericks
  3. Text File
    CLJ-1184-p3.patch
    23/Jul/13 10:49 PM
    3 kB
    Alex Miller
  4. Text File
    CLJ-1184-p4.patch
    14/Aug/13 9:23 PM
    3 kB
    Gary Fredericks

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Gary Fredericks added a comment -

Attached a patch that changes IPersistentCollection to ISeq on the referenced line, and a regression test.

Show
Gary Fredericks added a comment - Attached a patch that changes IPersistentCollection to ISeq on the referenced line, and a regression test.
Hide
Permalink
Nicola Mometto added a comment -

As found out on #clojure, there are still some cases where the symbol "do" behaves in unexpected ways that this patch doesn't address.

user=> ((fn [do] do) 1)
nil
user=> (let [do 1] do)
nil

Show
Nicola Mometto added a comment - As found out on #clojure, there are still some cases where the symbol "do" behaves in unexpected ways that this patch doesn't address. user=> ((fn [do] do) 1) nil user=> (let [do 1] do) nil
Hide
Permalink
Gary Fredericks added a comment -

Presumably the same issue is the difference between

(let [x 5] do x)

which returns 5 and

(let [x 5] do do x)

which gives a compile error.

Show
Gary Fredericks added a comment - Presumably the same issue is the difference between 
(let [x 5] do x)
which returns 5 and 
(let [x 5] do do x)
which gives a compile error.
Hide
Permalink
Nicola Mometto added a comment -

This is actually a different bug.
I created another ticket with patch+test see http://dev.clojure.org/jira/browse/CLJ-1216

Show
Nicola Mometto added a comment - This is actually a different bug. I created another ticket with patch+test see http://dev.clojure.org/jira/browse/CLJ-1216
Hide
Permalink
Alex Miller added a comment -

There is a similar case that shows up in Compiler.compile1() around line 7139. Should this also change?

Whatever case that is, would also be nice to have a test for it too.

Show
Alex Miller added a comment - There is a similar case that shows up in Compiler.compile1() around line 7139. Should this also change? Whatever case that is, would also be nice to have a test for it too.
Hide
Permalink
Gary Fredericks added a comment -

Good catch; this one's a bit trickier to test, since you either have to have a resource on the classpath to compile using clojure.core/compile, or else call the lower-level Compiler/compile directly. To keep the filesystem clean I'm opting for the second one. Path coming shortly.

Show
Gary Fredericks added a comment - Good catch; this one's a bit trickier to test, since you either have to have a resource on the classpath to compile using clojure.core/compile, or else call the lower-level Compiler/compile directly. To keep the filesystem clean I'm opting for the second one. Path coming shortly.
Hide
Permalink
Gary Fredericks added a comment -

Attached a replacement patch with the second fix. For testing I couldn't call Compiler.compile directly without getting an NPE (that I couldn't reproduce at the repl), so instead I opted to write to a file, use clojure.core/compile, and cleanup the files inside the test.

Show
Gary Fredericks added a comment - Attached a replacement patch with the second fix. For testing I couldn't call Compiler.compile directly without getting an NPE (that I couldn't reproduce at the repl), so instead I opted to write to a file, use clojure.core/compile, and cleanup the files inside the test.
Hide
Permalink
Andy Fingerhut added a comment -

Presumptuously changing back to its former Vetted state, since the latest patch seems to address the reasons it was marked Incomplete on July 2, 2013.

Show
Andy Fingerhut added a comment - Presumptuously changing back to its former Vetted state, since the latest patch seems to address the reasons it was marked Incomplete on July 2, 2013.
Hide
Permalink
Alex Miller added a comment -

Updated patch very slightly to fix a spelling typo.

Show
Alex Miller added a comment - Updated patch very slightly to fix a spelling typo.
Hide
Permalink
Alex Miller added a comment -

Updated description to help it along a bit and marked Screened.

Show
Alex Miller added a comment - Updated description to help it along a bit and marked Screened.
Hide
Permalink
Andy Fingerhut added a comment -

All 3 of the attached patches no longer apply cleanly to latest master as of Aug 14 2013. This may simply be due to extra tests added to file compilation.clj by the patch for CLJ-1154, which was committed earlier today. If so, it should be pretty straightforward to update the stale patch(es). See the section "Updating Stale Patches" at http://dev.clojure.org/display/community/Developing+Patches

Show
Andy Fingerhut added a comment - All 3 of the attached patches no longer apply cleanly to latest master as of Aug 14 2013. This may simply be due to extra tests added to file compilation.clj by the patch for CLJ-1154, which was committed earlier today. If so, it should be pretty straightforward to update the stale patch(es). See the section "Updating Stale Patches" at http://dev.clojure.org/display/community/Developing+Patches
Hide
Permalink
Gary Fredericks added a comment - - edited

Attached a new patch (p4) that should apply. Also halfway reverted a change that Alex made regarding which files are cleaned up after the test. When I run the tests on my machine with his version, several class files are leftover.

Show
Gary Fredericks added a comment - - edited Attached a new patch (p4) that should apply. Also halfway reverted a change that Alex made regarding which files are cleaned up after the test. When I run the tests on my machine with his version, several class files are leftover.
Hide
Permalink
Alex Miller added a comment -

Screened.

Show
Alex Miller added a comment - Screened.

People

Vote (0)
Watch (2)

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.