readonly icon indicating copy to clipboard operation
readonly copied to clipboard

`Readonly::Clone` does not clone a partial of a constant

Open michael-on-github opened this issue 9 years ago • 5 comments

#!/usr/bin/env perl

use Readonly 2.05;

Readonly::Scalar our $MAP => {
    'record' => {
        id    => 1,
        title => 'Record',
    },
};

my $map_partial_copy = Readonly::Clone $MAP->{record};
# $map_partial_copy->{id} = 42; # Modification of a read-only value attempted

michael-on-github avatar Jul 08 '16 09:07 michael-on-github

Right now, it looks like another upstream bug in perl itself. Simply accessing the value inside Readonly::Clone(...) 'fixes' it. I'm trying to narrow it down to a minimal example that I can report but it looks like another bug I reported on perl's RT a few years ago. That one was related to ignored scalar magic while this one seems to be hash magic... I'd guess arrays will turn out to also be broken.

I created a branch for this issue that shows that just accessing the variable can 'fix' this bug. If you comment out the line that dumps it and run your code example, the output is...

SCALAR at ../../lib/Readonly.pm line 392.
SCALAR at ../../lib/Readonly.pm line 395.
Modification of a read-only value attempted at 025_partial_clone.t line 23.

...notice that ref() gives us incorrect reftypes. But dumping the value first...

# Readonly.pm:391: \{
#   # tied Readonly::Hash
#   id => 1,
#   title => "Record",
# }
REF at ../../lib/Readonly.pm line 392.
HASH at ../../lib/Readonly.pm line 395.`

...fixes everything. It correctly acknowledges that we're using references to a hash. Weird... trying to narrow it down.

sanko avatar Jul 09 '16 14:07 sanko

Take two! Got it down to a generic example that doesn't use Readonly.

use strict;
use warnings;
use Tie::Hash;
use Tie::Scalar;
use Data::Dump;
$|++;

sub _debug {
    warn ref $_[0];
    use Data::Dump;
    ddx $_[0];    # Accessing part of it for printing makes it all better
    warn ref $_[0];
}

# Test with tied hash ref
tie my %tied, 'Tie::StdHash';
$tied{record} = {id    => 1,
                 title => 'Record'
};
_debug(\$tied{record});

# Verify with simple hashref
my %simple;
$simple{record} = {id    => 1,
                   title => 'Record'
};
_debug(\$simple{record});

...the output from this reads this way:

SCALAR at debug.pl line 10.
# debug.pl:12: \{ id => 1, title => "Record" }
REF at debug.pl line 13.
REF at debug.pl line 10.
# debug.pl:12: \{ id => 1, title => "Record" }
REF at debug.pl line 13.

With the tied hash, we get a SCALAR reftype until we access any of the contents. Just dumping the structure is enough to wake perl's internals up to the tied magic. I'll have to report this upstream with our example once I do more testing.

sanko avatar Jul 09 '16 20:07 sanko

Reported as https://rt.perl.org/Ticket/Display.html?id=128588.

Unfortunately, tie is kinda the redheaded stepchild of perl. I've had trouble with it ever since I took over Readonly and would rather throw all that classic code away. Smartmatch, select(), and now ref()... I hate bothering p5p for basically the same thing but a bug is a bug. :\

sanko avatar Jul 09 '16 21:07 sanko

There is a workaround for this case:

my $map_partial_orig = $MAP->{record};
my $map_partial      = Readonly::Clone $map_partial_orig;
$map_partial->{id}      = 42; # ok
$map_partial_orig->{id} = 42; # Modification of a read-only value attempted

michael-on-github avatar Aug 11 '16 11:08 michael-on-github

Have you tried the drop-in replacement for Readonly I sent you about a month ago? https://metacpan.org/pod/ReadonlyX

It isn't encumbered by tie.

sanko avatar Aug 11 '16 11:08 sanko