[CLJ-1100] Reader literals cannot contain periods Created: 02/Nov/12 Updated: 14/Feb/13 |
|
| Status: | Open |
| Project: | Clojure |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Defect | Priority: | Major |
| Reporter: | Kevin Lynagh | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 1 |
| Labels: | reader | ||
| Approval: | Vetted |
| Description |
|
The LispReader tries to read a record instead of a literal if the tag contains periods: https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LispReader.java#L1171 Which effectively means that reader tags cannot contain periods.
(issue opened: https://github.com/edn-format/edn/issues/39) If periods are allowed, then the LispReader should first check to see if the tag is in *data-readers* and only then if not try to initialize a Java class. I'm happy to write the patch if this behavior is what is desired. |
| Comments |
| Comment by Steve Miner [ 06/Nov/12 9:41 AM ] |
|
The suggested patch (clj-1100-reader-literal-periods.patch) will break reading records when *default-data-reader-fn* is set. Try adding a test like this: (deftest tags-containing-periods-with-default ;; we need a predefined record for this test so we (mis)use clojure.reflect.Field for convenience (let [v "#clojure.reflect.Field{:name \"fake\" :type :fake :declaring-class \"Fake\" :flags nil}"] (binding [*default-data-reader-fn* nil] (is (= (read-string v) #clojure.reflect.Field{:name "fake" :type :fake :declaring-class "Fake" :flags nil}))) (binding [*default-data-reader-fn* (fn [tag val] (assoc val :meaning 42))] (is (= (read-string v) #clojure.reflect.Field{:name "fake" :type :fake :declaring-class "Fake" :flags nil}))))) |
| Comment by Rich Hickey [ 29/Nov/12 9:36 AM ] |
|
The problem assessment is ok, but the resolution approach may not be. What happens should be based not upon what is in data-readers but whether or not the name names a class. Is the intent here to allow readers to circumvent records? I'm not in favor of that. |
| Comment by Steve Miner [ 29/Nov/12 4:01 PM ] |
|
New patch following Rich's comments. The decision to read a record is now based on the symbol containing periods and not having a namespace. Otherwise, it is considered a data reader tag. User |
| Comment by Steve Miner [ 29/Nov/12 4:17 PM ] |
|
I deleted my old patch and some comments referring to it to avoid confusion. In Clojure 1.5 beta 1, # followed by a qualified symbol with a period in the name is considered a record and causes an exception for the missing record class. With the patch, only non-qualified symbols containing periods are considered records. That allows user-defined qualified symbols with periods in their names to be used as data reader tags. |
| Comment by Andy Fingerhut [ 07/Feb/13 9:05 AM ] |
|
clj-1100-periods-in-data-reader-tags-patch-v2.txt dated Feb 7 2013 is identical to CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012, except it applies cleanly to latest master. The only change appears to be in some white space in the context lines. |
| Comment by Andy Fingerhut [ 07/Feb/13 12:53 PM ] |
|
I've removed clj-1100-periods-in-data-reader-tags-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012 applies cleanly to latest master and passes all tests if you use this command to apply it. % git am --keep-cr -s --ignore-whitespace < CLJ-1100-periods-in-data-reader-tags.patch I've already updated the JIRA workflow and screening patches wiki pages to mention this --ignore-whitespace option. |
| Comment by Andy Fingerhut [ 13/Feb/13 11:31 AM ] |
|
Both of the current patches, CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012, and clj-1100-reader-literal-periods.patch dated Nov 6 2012, fail to apply cleanly to latest master (1.5.0-RC15) as of today, although they did last week. Given all of the changes around read / read-string and edn recently, they should probably be evaluated by their authors to see how they should be updated. |
| Comment by Steve Miner [ 14/Feb/13 12:23 PM ] |
|
I deleted my patch: CLJ-1100-periods-in-data-reader-tags.patch. clj-1100-reader-literal-periods.patch is clearly wrong, but the original author or an administrator has to delete that. |
| Comment by Kevin Lynagh [ 14/Feb/13 1:28 PM ] |
|
I cannot figure out how to remove my attachment (clj-1100-reader-literal-periods.patch) in JIRA. |
| Comment by Steve Miner [ 14/Feb/13 1:43 PM ] |
|
Downarrow (popup) menu to the right of the "Attachments" section. Choose "manager attachments". |
| Comment by Kevin Lynagh [ 14/Feb/13 2:02 PM ] |
|
Great, thanks Steve. Are you going to take another pass at this issue, or should I give it a go? |
| Comment by Steve Miner [ 14/Feb/13 3:04 PM ] |
|
Kevin, I'm not planning to work on this right now as 1.5 is pretty much done. It might be worthwhile discussing the issue a bit on the dev mailing list before working on a patch, but that's up to you. I think my approach was correct, although now changes would have to be applied to both LispReader and EdnReader. |