Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boolean encoding is inconsistent #161

Closed
karenetheridge opened this issue Mar 26, 2020 · 6 comments
Closed

boolean encoding is inconsistent #161

karenetheridge opened this issue Mar 26, 2020 · 6 comments
Labels

Comments

@karenetheridge
Copy link
Contributor

perl -MCpanel::JSON::XS -wle'print Cpanel::JSON::XS->new->allow_nonref(1)->encode(!0)'
true
perl -MCpanel::JSON::XS -wle'print Cpanel::JSON::XS->new->allow_nonref(1)->encode([!0])'
[1]

This is with version 4.19.

@karenetheridge
Copy link
Contributor Author

13:37 < tinita> I think the problem is with !0 itself. as soon as you save it in an arrayref, its special status gets lost
13:38 < tinita> perl -wE'use Devel::Peek;Dump !0;Dump [!0]'
13:38 <@kraih> lol

so I guess this means it's not actually fixable. I hope I'm wrong though.

@Grinnz
Copy link

Grinnz commented Mar 26, 2020

SV_yes and SV_no should not be interpreted as JSON booleans (unless via Cpanel::JSON::XS::Type with the boolean type). The user cannot easily tell if something they have is one of these special constants, they might accidentally encode one of them when trying to encode the number 1 (for example), and as demonstrated here they can de-special themselves. They are simply not reliably distinct from 0 and 1 in implementation or interface.

@karenetheridge
Copy link
Contributor Author

as demonstrated here they can de-special themselves

I don't agree with this part, as that's the same problem we have with the difference between the string "6" and the number 6, depending on how the value has been used be earlier code.

@Grinnz
Copy link

Grinnz commented Mar 26, 2020

It is similar, and it's not good that that happens either. But in this case, we can avoid the problem and everything still works intuitively.

@rurban
Copy link
Owner

rurban commented May 17, 2020

Not really, !0 is always SV_YES, even if as arrayref.

perl -Dx -e'print [!0]' =>  SV = SV_YES
perl -Dx -e'print !1' =>  SV = SV_NO

It's the anonlist in perl before the XS encode which changes the [yes] to [1].
With -Dts

(-e:1)	pushmark
    =>  **  \PVMG(""\0)  *  
(-e:1)	const(SV_YES)
    =>  **  \PVMG(""\0)  *  SV_YES  
(-e:1)	anonlist
    =>  **  \PVMG(""\0)  \AV()  
(-e:1)	method_named
    =>  **  \PVMG(""\0)  \AV()  CV(encode)  
(-e:1)	entersub
    =>  *  PV("[1]"\0) [UTF8 "[1]"]  

So it's av_make in anonlist which turns the SV_YES into 1, esp. sv_setsv_flags does not support the special immortals, like YES, NO, ... Upstream bug, sorry

BTW: Trivially fixable there, but it's already too late.

@Grinnz
Copy link

Grinnz commented May 17, 2020

My concern is even if that were reliable, it's very common to already be using these values and expecting a 1 on encode, because it's generally indistinguishable from 1 in perl code. The boolean object is the only consistent and obvious way to represent something to be encoded to a boolean without Cpanel::JSON::XS::Type, and people are already used to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants