ingydotnet / yaml-libyaml-pm

Perl Binding to libyaml
http://search.cpan.org/dist/YAML-LibYAML/
33 stars 37 forks source link

true/false data structure prevents changing values #25

Closed zhengwy888 closed 6 years ago

zhengwy888 commented 9 years ago

When loading an yaml with true value, the value can't be modified as perl will complain 'changing a read-only value' . to reproduce

use YAML::XS qw(Load);
my $yaml = <<EOM;
hello: true
EOM
my $data = Load($yaml);
$data->{hello}= 1; #<-- this line will error out with 'Modification of a read-only value attempted ...'

I don't know if there is a reason why converting to a data structure that Perl can't modify.

ratsbane commented 9 years ago

I'm not able to reproduce this problem or the output you're seeing. The syntax around your heredoc doesn't look right to me - I think it should be something like this instead: my $yaml = <<EOM; hello: true EOM my $data = Load($yaml);

zhengwy888 commented 9 years ago

@ratsbane sorry markdown ate my formatting. I have updated the comment with the correct format. I was using perl 5.20.1 and YAML::XS 0.52.

oalders commented 9 years ago

I can reproduce this under perl 5.18.2

dolmen commented 9 years ago

Here is a minimalist test case (verified on perl 5.20.1, YAML::XS 0.59):

$ perl -MYAML::XS -E 'Load("a: true")->{a} = !!0'
Modification of a read-only value attempted at -e line 1

This should not die.

rurban commented 8 years ago

Looks like I have to implement the same trick as in JSON::XS, special overloaded symbols representing true and false. A copy via newSVsv(&PL_sv_yes); helps with the readonly problem, but leads to lossage in the roundtrips:

test/boolean.t .......... 1/5
#   Failed test 'Booleans YNY roundtrip'
#   at test/boolean.t line 22.
#          got: '---
# a: 1
# b: 1
# c: ''
# d: ''
# '
#     expected: '---
# a: true
# b: 1
# c: false
# d: ''
# '
mithun commented 7 years ago

The workaround is to delete the key and reset it

use YAML::XS qw(Load);
my $yaml = <<EOM;
hello: true
EOM
my $data = Load($yaml);
# This fails
# $data->{hello}= 1;
# This works
delete $data->{hello};
$data->{hello} = 1;
zhengwy888 commented 7 years ago

@rurban actually I think a round trip loss is not so much of an issue this case, since perl doesn't natively support true value so I don't think people expect that. plus we can have a flag to turn on/off this round trip value. @mithun to avoid manually going into keys to find true/false I used JSON to dump than load the object to get rid of the true/false read only values.

rurban commented 7 years ago

perl supports sv_yes and sv_no via the well-known constructs !!1 and !!0.

perl -MDevel::Peek -e'Dump !!1'
SV = PVNV(0x7fb312003610) at 0x10433ba00
  REFCNT = 2147483644
  FLAGS = (IOK,NOK,POK,READONLY,PROTECT,pIOK,pNOK,pPOK)
  IV = 1
  NV = 1
  PV = 0x104300702 "1"
  CUR = 1
  LEN = 0

Note the overlarge refcount and PROTECT denoting an immortal protected special value, i.e. &PL_sv_yes.

perlpunk commented 6 years ago

Starting with YAML::XS 0.66_001, you can use this:

local $YAML::XS::Boolean = "JSON::PP"; # or "boolean"
my $data = Load("booltrue: true");
$data->{boolfalse} = JSON::PP::false;
my $yaml = Dump($data);
# boolfalse: false
# booltrue: true

See more about this in the docs. This also does not have the issue of readonly values. I will close this issue, when 0.66_001 was tested by cpantesters and 0.67 was released. Unless someone objects. I was thinking about a third option "perl" that uses simple 1/'' instead of the special variables, but I'm not sure if this is really needed.

SineSwiper commented 2 months ago

I'd like to re-open this because this seems like the load_scalar code in perl_libyaml.c is doing this wrong: scalar = &PL_sv_yes;

Consider the following (using Perl 5.30):

$ perl -MDevel::Peek -e'Dump !!1'
SV = PVNV(0x558433cb0f80) at 0x558433caf3d8
  REFCNT = 2147483644
  FLAGS = (IOK,NOK,POK,READONLY,PROTECT,pIOK,pNOK,pPOK)
  IV = 1
  NV = 1
  PV = 0x558431d9b864 "1"
  CUR = 1
  LEN = 0
$ perl -MDevel::Peek -e'my $t = !!1; Dump $t'
SV = PVNV(0x55e954ed6040) at 0x55e954f04938
  REFCNT = 1
  FLAGS = (IOK,NOK,POK,pIOK,pNOK,pPOK)
  IV = 1
  NV = 1
  PV = 0x55e954f27130 "1"\0
  CUR = 1
  LEN = 10

One is READONLY,PROTECT, and the other is not. I don't think the scalar can be set directly as a reference to the constant itself, like the code in perl_libyaml.c. It needs a newSV, and that constant stashed into the new SV.

perlpunk commented 1 month ago

The current solution was chosen to be able to roundtrip booleans. I've now created #118 which makes them not readonly for perl >= 5.36, where we got core boolean support.

perlpunk commented 1 month ago

118 is now released as v0.902.0