Skip to content

Commit

Permalink
[#89 #150] [Fix] Boxed Booleans incorrectly freezing to primitive `tr…
Browse files Browse the repository at this point in the history
…ue` (@RolT)

Before this commit:
  (freeze true)                  => froze as primitive `true`
  (freeze false)                 => froze as primitive `false`
  (freeze (Boolean. <anything>)) => froze as primitive `true`

After this commit:
  Boxed Booleans are first unboxed to correct primitive value before freezing

This was a long-standing bug, though thankfully unlikely to have affected most
users since boxed Booleans are rarely used in Clojure. Cases with Java interop
are the most likely to have been affected.

A big thanks to Roland Thiolliere (@RolT) for this fix!
  • Loading branch information
ptaoussanis committed Jun 23, 2022
1 parent ba88277 commit 8909a32
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 1 deletion.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ nippy/stress-data
:nil nil
:true true
:false false
:False (Boolean. false)
:char \ಬ
:str-short "ಬಾ ಇಲ್ಲಿ ಸಂಭವಿಸ"
:str-long (apply str (range 1000))
Expand Down
3 changes: 2 additions & 1 deletion src/taoensso/nippy.clj
Original file line number Diff line number Diff line change
Expand Up @@ -1151,7 +1151,7 @@
(.writeLong out (.getMostSignificantBits x))
(.writeLong out (.getLeastSignificantBits x)))

(freezer Boolean (if x (write-id out id-true) (write-id out id-false)))
(freezer Boolean (if (boolean x) (write-id out id-true) (write-id out id-false)))
(freezer (Class/forName "[B") (write-bytes out x))
(freezer (Class/forName "[Ljava.lang.Object;") (write-objects out x))
(freezer String (write-str out x))
Expand Down Expand Up @@ -2020,6 +2020,7 @@
:nil nil
:true true
:false false
:False (Boolean. false)
:char \ಬ
:str-short "ಬಾ ಇಲ್ಲಿ ಸಂಭವಿಸ"
:str-long (apply str (range 1000))
Expand Down

4 comments on commit 8909a32

@edporras
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Peter, thanks for pushing this release.

I have one small observation however: according to the clojure reference, they suggest using (Boolean/valueOf false) and (Boolean/valueOf true) instead of the constructors. Would you like me to do a PR to update it to be on the safe side?

@ptaoussanis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edporras Hi Ed- I'm sorry, I'm not sure I follow. Where exactly are you proposing to use Boolean/valueOf?

@edporras
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, I apologize. I was looking at this through a collapsed view on GitHub and missed that the instance of (Boolean. false) was part of your stress data. Sorry about that!

@ptaoussanis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all, it's always useful to get extra eyes on things like this to help make sure that everything is correct. Much appreciated 👍 Cheers :-)

Please sign in to comment.