Clojure

munge should not munge $ (which isJavaIdentifierPart), should munge ' (which is not)

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.3
  • Component/s: None
  • Labels:
    None
  • Approval:
    Ok

Description

munge is overeager in converting $ to DOLLARSIGN, since $ is a valid
character in Java identifiers:

user> (filter #(Character/isJavaIdentifierPart %) (keys
clojure.lang.Compiler/CHAR_MAP))
(\$)

On a related point, ' (single quote) is not admissible in Java
identifiers, but it is not munged on master:

user=> (Character/isJavaIdentifierPart \')
false
user=> (munge "'")
"'"

This leads to e.g.

user=> *'
#<core$_STAR_' clojure.core$_STAR_'@5adf48c4>

(note the ' after STAR).

Originally reported on the Dev list.

See also this thread on the (regular) Clojure ggroup.

The attached patch applies cleanly against current master.

Activity

Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/433 Attachments: 0001-munge-no-longer-changes-to-DOLLARSIGN.patch - https://www.assembla.com/spaces/clojure/documents/dp5tziVaer34yIeJe5cbLr/download/dp5tziVaer34yIeJe5cbLr
Stuart Halloway made changes -
Field Original Value New Value
Assignee Rich Hickey [ richhickey ]
Reporter Assembla Importer [ importer ]
Priority Major [ 3 ]
Description munge is overeager in converting $ to _DOLLARSIGN_, since $ is a valid
character in Java identifiers:

{code}user> (filter #(Character/isJavaIdentifierPart %) (keys
clojure.lang.Compiler/CHAR_MAP))
(\$)
</code></pre>
 
On a related point, ' (single quote) is not admissible in Java
identifiers, but it is not munged on master:

<pre><code>user=> (Character/isJavaIdentifierPart \')
false
user=> (munge "'")
"'"
</code></pre>

This leads to e.g.

<pre><code>user=> *'
#<core$_STAR_' clojure.core$_STAR_'@5adf48c4>
{code}

(note the ' after _STAR_).

Originally reported [on the Dev list|http://groups.google.com/group/clojure-dev/browse_thread/thread/9caab13eafa10f80].

See also [this thread|http://groups.google.com/group/clojure/browse_thread/thread/7753adc5453d7410] on the (regular) Clojure ggroup.

The attached patch applies cleanly against current master.
munge is overeager in converting $ to _DOLLARSIGN_, since $ is a valid
character in Java identifiers:

{code}user> (filter #(Character/isJavaIdentifierPart %) (keys
clojure.lang.Compiler/CHAR_MAP))
(\$)
{code}
 
On a related point, ' (single quote) is not admissible in Java
identifiers, but it is not munged on master:

{code}user=> (Character/isJavaIdentifierPart \')
false
user=> (munge "'")
"'"
{code}

This leads to e.g.

{code}user=> *'
#<core$_STAR_' clojure.core$_STAR_'@5adf48c4>
{code}

(note the ' after _STAR_).

Originally reported [on the Dev list|http://groups.google.com/group/clojure-dev/browse_thread/thread/9caab13eafa10f80].

See also [this thread|http://groups.google.com/group/clojure/browse_thread/thread/7753adc5453d7410] on the (regular) Clojure ggroup.

The attached patch applies cleanly against current master.
Hide
Stuart Halloway added a comment -

Patch is clean, the real issue here is doing the right thing. Two concerns:

  • was the original choice to munge $ motivated, and if so do we need a more subtle patch that preserves that original intent?
  • presumably this is (yet another) binary-compatibility-breaking change. 1.3 already has major change, so now is a good a time as any...
Show
Stuart Halloway added a comment - Patch is clean, the real issue here is doing the right thing. Two concerns:
  • was the original choice to munge $ motivated, and if so do we need a more subtle patch that preserves that original intent?
  • presumably this is (yet another) binary-compatibility-breaking change. 1.3 already has major change, so now is a good a time as any...
Stuart Halloway made changes -
Approval Test Screened
Hide
Rich Hickey added a comment -

This issue mentions two problems but patch fixes only one. Should add the single-quote handling

Show
Rich Hickey added a comment - This issue mentions two problems but patch fixes only one. Should add the single-quote handling
Stuart Halloway made changes -
Approval Screened Incomplete
Hide
Michał Marczyk added a comment - - edited

Right, I forgot about that in the patch somehow. Also, I just noticed that " is also not JavaIdentifierPart and yet is not currently munged. The newly attached patch fixes all three issues.

Show
Michał Marczyk added a comment - - edited Right, I forgot about that in the patch somehow. Also, I just noticed that " is also not JavaIdentifierPart and yet is not currently munged. The newly attached patch fixes all three issues.
Michał Marczyk made changes -
Attachment 0001-fix-munge-handling-of.patch [ 10046 ]
Rich Hickey made changes -
Assignee Rich Hickey [ richhickey ]
Approval Incomplete Test
Rich Hickey made changes -
Fix Version/s Release.Next [ 10038 ]
Fix Version/s Backlog [ 10035 ]
Stuart Halloway made changes -
Approval Test Screened
Rich Hickey made changes -
Approval Screened Ok
Stuart Halloway made changes -
Status In Progress [ 3 ] Resolved [ 5 ]
Resolution Completed [ 1 ]
Stuart Halloway made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: