Clojure

Reader literals cannot contain periods

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Approval:
    Vetted

Description

The reader tries to read a record instead of a literal if the tag contains periods.

user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x  java.lang.Class.forName0 (Class.java:-2)

Summary of reader forms:

Kind Example Constraint Status
Record #user.Foo[1] record class name OK
Class #java.lang.String["abc"] class name OK
Clojure reader tag #uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d" not a class name, no "/" OK
Library reader tag #my/card "5H" not a class name, has "/" OK
  #my.ns/card "5H" not a class name, has "/" OK
  #my/playing.card "5H" not a class name, has "/" BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

Cause: In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

Proposed: Change the discriminator in CtorReader.

Alternative 1 (purely string inspection):

  • If name has a "/" -> readTagged (not a legal class name)
  • If name has no "/" or "." -> readTagged (records must have qualified names)
  • Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2 (prioritize Class check):

  • Attempt readRecord (also covers Java classes)
  • If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

Patch:

Screened by:

Activity

Kevin Lynagh made changes -
Field Original Value New Value
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.
The EDN spec is unclear on this:

    edn supports extensibility through a simple mechanism. # followed immediately by a symbol starting with an alphabetic character indicates that that symbol is a tag.

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.
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.
The EDN spec is unclear on this:

    edn supports extensibility through a simple mechanism. # followed immediately by a symbol starting with an alphabetic character indicates that that symbol is a tag.

(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.
Kevin Lynagh made changes -
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.
The EDN spec is unclear on this:

    edn supports extensibility through a simple mechanism. # followed immediately by a symbol starting with an alphabetic character indicates that that symbol is a tag.

(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.
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.
The EDN spec is unclear on this:

bq. edn supports extensibility through a simple mechanism. # followed immediately by a symbol starting with an alphabetic character indicates that that symbol is a tag.

(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.
Kevin Lynagh made changes -
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.
The EDN spec is unclear on this:

bq. edn supports extensibility through a simple mechanism. # followed immediately by a symbol starting with an alphabetic character indicates that that symbol is a tag.

(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.
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.
The EDN spec is unclear on this:

bq. edn supports extensibility through a simple mechanism. # followed immediately by a symbol starting with an alphabetic character indicates that that symbol is a tag.

(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.
Rich Hickey made changes -
Fix Version/s Release 1.5 [ 10150 ]
Rich Hickey made changes -
Approval Vetted [ 10003 ]
Kevin Lynagh made changes -
Patch Code and Test [ 10002 ]
Attachment clj-1100-reader-literal-periods.patch [ 11665 ]
Hide
Steve Miner added a comment - - edited

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})))))
Show
Steve Miner added a comment - - edited 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})))))
Steve Miner made changes -
Attachment CLJ-1100-allow-periods-in-data-reader-tags.patch [ 11666 ]
Steve Miner made changes -
Attachment CLJ-1100-allow-periods-in-data-reader-tags.patch [ 11666 ]
Steve Miner made changes -
Attachment CLJ-1100-periods-in-data-reader-tags.patch [ 11716 ]
Hide
Rich Hickey added a comment -

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.

Show
Rich Hickey added a comment - 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.
Rich Hickey made changes -
Fix Version/s Release 1.5 [ 10150 ]
Steve Miner made changes -
Attachment CLJ-1100-periods-in-data-reader-tags.patch [ 11716 ]
Hide
Steve Miner added a comment -

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
defined tags are required to be qualified but they may now have periods in the name. Tests added to show that
data readers cannot override record classes. Note: Clojure-defined data reader tags may be unqualified, but they should not contain periods in order to avoid confusion with record classes.

Show
Steve Miner added a comment - 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 defined tags are required to be qualified but they may now have periods in the name. Tests added to show that data readers cannot override record classes. Note: Clojure-defined data reader tags may be unqualified, but they should not contain periods in order to avoid confusion with record classes.
Steve Miner made changes -
Attachment CLJ-1100-periods-in-data-reader-tags.patch [ 11731 ]
Steve Miner made changes -
Comment [ Allow periods in data-reader tags. Tries \*data-readers* first, then default-data-readers as usual. Then tries tag as a possible record class, and finally tries the \*default-data-reader-fn*. More flexible than assuming any tag with a period is a record class. Note, with this patch, \*data-readers* can override a record class. I'm calling that a feature. ]
Steve Miner made changes -
Comment [ Revised my patch to add more tests and be more discriminating when considering if a tag could a record class name. The previous comments still apply. ]
Hide
Steve Miner added a comment - - edited

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.

Show
Steve Miner added a comment - - edited 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.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Andy Fingerhut made changes -
Attachment clj-1100-periods-in-data-reader-tags-patch-v2.txt [ 11842 ]
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Andy Fingerhut made changes -
Attachment clj-1100-periods-in-data-reader-tags-patch-v2.txt [ 11842 ]
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Steve Miner made changes -
Attachment CLJ-1100-periods-in-data-reader-tags.patch [ 11731 ]
Hide
Steve Miner added a comment -

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.

Show
Steve Miner added a comment - 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.
Steve Miner made changes -
Patch Code and Test [ 10002 ]
Hide
Kevin Lynagh added a comment -

I cannot figure out how to remove my attachment (clj-1100-reader-literal-periods.patch) in JIRA.

Show
Kevin Lynagh added a comment - I cannot figure out how to remove my attachment (clj-1100-reader-literal-periods.patch) in JIRA.
Hide
Steve Miner added a comment -

Downarrow (popup) menu to the right of the "Attachments" section. Choose "manager attachments".

Show
Steve Miner added a comment - Downarrow (popup) menu to the right of the "Attachments" section. Choose "manager attachments".
Kevin Lynagh made changes -
Attachment clj-1100-reader-literal-periods.patch [ 11665 ]
Hide
Kevin Lynagh added a comment -

Great, thanks Steve. Are you going to take another pass at this issue, or should I give it a go?

Show
Kevin Lynagh added a comment - Great, thanks Steve. Are you going to take another pass at this issue, or should I give it a go?
Hide
Steve Miner added a comment -

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.

Show
Steve Miner added a comment - 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.
Hide
Alex Miller added a comment -

Updated description based on my understanding.

Show
Alex Miller added a comment - Updated description based on my understanding.
Alex Miller made changes -
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.
The EDN spec is unclear on this:

bq. edn supports extensibility through a simple mechanism. # followed immediately by a symbol starting with an alphabetic character indicates that that symbol is a tag.

(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.
The reader tries to read a record instead of a literal if the tag contains periods.

{code}
user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x java.lang.Class.forName0 (Class.java:-2)
{code}

Summary of reader forms:

|Kind|Example|Constraint|Status
|Record|#user.Foo[1]|no "/", dotted name|OK
|Class|#java.lang.String["abc"]|no "/", dotted name|OK
|Clojure reader tag|#uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d"|no "/", not a class name|OK
|Library reader tag|#my/card "5H"|has "/"|OK
||#my.ns/card "5H"|has "/"|OK
||#my/playing.card "5H"|has "/"|BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

*Cause:* In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

*Proposed:* Change the discriminator in CtorReader.

Alternative 1:
- If name has a "/" -> readTagged (not a legal class name)
- If name has no "/" or "." -> readTagged (records must have qualified names)
- Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2:
- Attempt readRecord (also covers Java classes)
- If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

*Patch:*

*Screened by:*
Alex Miller made changes -
Description The reader tries to read a record instead of a literal if the tag contains periods.

{code}
user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x java.lang.Class.forName0 (Class.java:-2)
{code}

Summary of reader forms:

|Kind|Example|Constraint|Status
|Record|#user.Foo[1]|no "/", dotted name|OK
|Class|#java.lang.String["abc"]|no "/", dotted name|OK
|Clojure reader tag|#uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d"|no "/", not a class name|OK
|Library reader tag|#my/card "5H"|has "/"|OK
||#my.ns/card "5H"|has "/"|OK
||#my/playing.card "5H"|has "/"|BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

*Cause:* In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

*Proposed:* Change the discriminator in CtorReader.

Alternative 1:
- If name has a "/" -> readTagged (not a legal class name)
- If name has no "/" or "." -> readTagged (records must have qualified names)
- Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2:
- Attempt readRecord (also covers Java classes)
- If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

*Patch:*

*Screened by:*
The reader tries to read a record instead of a literal if the tag contains periods.

{code}
user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x java.lang.Class.forName0 (Class.java:-2)
{code}

Summary of reader forms:

|Kind|Example|Constraint|Status
|Record|#user.Foo[1]|no "/", dotted name|OK
|Class|#java.lang.String["abc"]|no "/", dotted name|OK
|Clojure reader tag|#uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d"|no "/", not a class name|OK
|Library reader tag|#my/card "5H"|has "/"|OK
| |#my.ns/card "5H"|has "/"|OK
| |#my/playing.card "5H"|has "/"|BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

*Cause:* In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

*Proposed:* Change the discriminator in CtorReader.

Alternative 1:
- If name has a "/" -> readTagged (not a legal class name)
- If name has no "/" or "." -> readTagged (records must have qualified names)
- Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2:
- Attempt readRecord (also covers Java classes)
- If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

*Patch:*

*Screened by:*
Alex Miller made changes -
Description The reader tries to read a record instead of a literal if the tag contains periods.

{code}
user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x java.lang.Class.forName0 (Class.java:-2)
{code}

Summary of reader forms:

|Kind|Example|Constraint|Status
|Record|#user.Foo[1]|no "/", dotted name|OK
|Class|#java.lang.String["abc"]|no "/", dotted name|OK
|Clojure reader tag|#uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d"|no "/", not a class name|OK
|Library reader tag|#my/card "5H"|has "/"|OK
| |#my.ns/card "5H"|has "/"|OK
| |#my/playing.card "5H"|has "/"|BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

*Cause:* In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

*Proposed:* Change the discriminator in CtorReader.

Alternative 1:
- If name has a "/" -> readTagged (not a legal class name)
- If name has no "/" or "." -> readTagged (records must have qualified names)
- Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2:
- Attempt readRecord (also covers Java classes)
- If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

*Patch:*

*Screened by:*
The reader tries to read a record instead of a literal if the tag contains periods.

{code}
user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x java.lang.Class.forName0 (Class.java:-2)
{code}

Summary of reader forms:

|Kind|Example|Constraint|Status
|Record|#user.Foo[1]|record class name|OK
|Class|#java.lang.String["abc"]|class name|OK
|Clojure reader tag|#uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d"|not a class name, no "/"|OK
|Library reader tag|#my/card "5H"|not a class name, has "/"|OK
| |#my.ns/card "5H"|not a class name, has "/"|OK
| |#my/playing.card "5H"|not a class name, has "/"|BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

*Cause:* In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

*Proposed:* Change the discriminator in CtorReader.

Alternative 1:
- If name has a "/" -> readTagged (not a legal class name)
- If name has no "/" or "." -> readTagged (records must have qualified names)
- Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2:
- Attempt readRecord (also covers Java classes)
- If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

*Patch:*

*Screened by:*
Alex Miller made changes -
Description The reader tries to read a record instead of a literal if the tag contains periods.

{code}
user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x java.lang.Class.forName0 (Class.java:-2)
{code}

Summary of reader forms:

|Kind|Example|Constraint|Status
|Record|#user.Foo[1]|record class name|OK
|Class|#java.lang.String["abc"]|class name|OK
|Clojure reader tag|#uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d"|not a class name, no "/"|OK
|Library reader tag|#my/card "5H"|not a class name, has "/"|OK
| |#my.ns/card "5H"|not a class name, has "/"|OK
| |#my/playing.card "5H"|not a class name, has "/"|BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

*Cause:* In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

*Proposed:* Change the discriminator in CtorReader.

Alternative 1:
- If name has a "/" -> readTagged (not a legal class name)
- If name has no "/" or "." -> readTagged (records must have qualified names)
- Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2:
- Attempt readRecord (also covers Java classes)
- If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

*Patch:*

*Screened by:*
The reader tries to read a record instead of a literal if the tag contains periods.

{code}
user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x java.lang.Class.forName0 (Class.java:-2)
{code}

Summary of reader forms:

|Kind|Example|Constraint|Status
|Record|#user.Foo[1]|record class name|OK
|Class|#java.lang.String["abc"]|class name|OK
|Clojure reader tag|#uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d"|not a class name, no "/"|OK
|Library reader tag|#my/card "5H"|not a class name, has "/"|OK
| |#my.ns/card "5H"|not a class name, has "/"|OK
| |#my/playing.card "5H"|not a class name, has "/"|BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

*Cause:* In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

*Proposed:* Change the discriminator in CtorReader.

Alternative 1 (purely string inspection):
- If name has a "/" -> readTagged (not a legal class name)
- If name has no "/" or "." -> readTagged (records must have qualified names)
- Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2 (prioritize Class check):
- Attempt readRecord (also covers Java classes)
- If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

*Patch:*

*Screened by:*
Alex Miller made changes -
Priority Major [ 3 ] Minor [ 4 ]
Rich Hickey made changes -
Fix Version/s Release 1.7 [ 10250 ]

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated: