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

Readonly::Clone does not clone a partial of a constant #25

Open
michael-on-github opened this issue Jul 8, 2016 · 5 comments
Open

Readonly::Clone does not clone a partial of a constant #25

michael-on-github opened this issue Jul 8, 2016 · 5 comments

Comments

@michael-on-github
Copy link
Contributor

#!/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
@sanko
Copy link
Owner

sanko commented Jul 9, 2016

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
Copy link
Owner

sanko commented Jul 9, 2016

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
Copy link
Owner

sanko commented Jul 9, 2016

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. :\

@michael-on-github
Copy link
Contributor Author

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

@sanko
Copy link
Owner

sanko commented Aug 11, 2016

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.

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

No branches or pull requests

2 participants