Readonly not applied on arrayrefs or hashrefs

[dakkar]

Minimal test case:

#!/usr/bin/env perl
use strict;
use warnings;
use Readonly;
use Test::More;

subtest 'hashref' => sub {
    Readonly my $a => {a=>1};
    my $ok = eval { $a->{b}=2;1 };
    ok !$ok, 'not modified';
    like $@,qr{Modification},'correct exception';
};

subtest 'arrayref' => sub {
    Readonly my $b => [1,2];
    my $ok = eval { $b->[0]=3;1};
    ok !$ok, 'not modified';
    like $@,qr{Modification},'correct exception';
};

done_testing;

This passes on 1.5, fails on 1.6.

Was this ever supported? Should we change our code to:

Readonly my %a => { a => 1 };
my $a=\%a;

?

[sanko]

Responding on my phone via email so this might not be well formatted.

The function Readonly() creates shallow read only vars. What I think you want is a deep read only var. For that you need to use the verbose form:

Readonly::Hash my ...;

I'm already standing in a TSA line this morning so my brain is... ehhhhh... but try that. I know it's counter intuitive; the verbose api does what most people (including myself) wish simple Readonly() itself did.

[dakkar]

Thank you for the prompt reply! Turns out we need to do:

Readonly my %a => {a=>1};
my $a=\%a;

Or, maybe:

my $a = do { Readonly my %a => { a => 1 }; \%a };

[ethanherbertson]

The documentation on this project states:

"If any of the values you pass to Scalar, Array, Hash, or the standard Readonly are references, then those functions recurse over the data structures, marking everything as Readonly. The entire structure is nonmodifiable. This is normally what you want."

The only interpretation of that is that the following should halt and catch fire:

Readonly my $hr => {1 => 'a'};
$hr->{1} = 'b';

[sanko]

Yeah the documentation is even more misleading than the ironically named Readonly() function. What you expect to happen here is what the Readonly::Scalar(), ::Hash(), and ::Array() functions do.

Plain Readonly() creates what the original author calls a "shallow" read only variable which is great if you don't plan to use it on anything but one dimensional scalar values. Readonly::Scalar() makes the variable 'deeply' read only so the following snippet kills over as you expect:

use Readonly;

Readonly::Scalar my $hr => {1 => 'a'};
$hr->{1} = 'b';

I really need to stop messing around and rewrite the docs from scratch... Among other problems, it lacks clarity and contradicts both the internals and common practice.

[ethanherbertson]

But then why would he have created the Array1, Scalar1, and Hash1 functions?

If most people believe (or expect) that the Readonly() function does (or will do) a "deep" readonly and it doesn't, and the documentation also SAYS that it does recursion, then it's a bug in the code, not a shortcoming of the documentation. At least in my opinion. On Sep 3, 2014 6:52 PM, "Sanko Robinson" [email protected] wrote:

Yeah the documentation is even more misleading than the ironically named Readonly() function. What you (and anyone other than the original author) expect to happen here is what the Readonly::Scalar(), ::Hash(), and ::Array() functions do.

Plain Readonly() creates what the original author calls a "shallow" read only variable which is great if you don't plan to use it on anything but one dimensional scalar values. Readonly::Scalar() makes the variable 'deeply' readonly so the following snippet kills over as you expect:

use Readonly; Readonly::Scalar my $hr => {1 => 'a'};$hr->{1} = 4;

I really need to rewrite the docs from scratch...

— Reply to this email directly or view it on GitHub https://github.com/sanko/readonly/issues/12#issuecomment-54386032.

[sanko]

This is going to get wordy...

I have no idea why he started expanding the API with poorly named functions that do the same thing Readonly() does. I agree with you but for historical reasons, I'm not comfortable 'fixing' it beyond fixing the documentation. I originally had a TODO file included with the dist and mentioned a lot of these same problems. A lot of "wtf are you planning to do to Readonly?" and "You can't do X or Y because Z" messages were posted here on github and sent to me via email. I had them listed as just possible changes and it caused a minor panic.

The problem is that Readonly's API hasn't changed since 2004 (I only inherited it a few months ago, btw) so I hate to say it but the API at this point has to be considered stable even though it just plain doesn't make sense in places. Hundreds of modules on CPAN and a decade of code in the wild depend on Readonly and was written around the way it currently behaves rather than how it's ambiguously or even incorrectly documented. If written according to the documentation, they'd end up with broken code like your original example.

I'm in a tough spot here: do I go the cheap route and document the current behavior or do I potentially break untold amounts of code that has worked for years and anger very vocal parts of the community? I might be more daring in the future but that would be a very major shift in functionality that I'd make very public and prepare everyone for several months in advance. For now though, I just can't do it.

If I could wipe the slate clean, though, I'd have three basic functions: Readonly::Deep(), Readony::Shallow() and Readonly::Clone(). That would cover all the bases and clear up all API ambiguity.

[andrui]

It is possible to extend functionality of Readonly() function in order to detect HashRefs and ArrayRefs:

diff --git a/lib/Readonly.pm b/lib/Readonly.pm
index 0f7f97d..4aa3720 100644
--- a/lib/Readonly.pm
+++ b/lib/Readonly.pm
@@ -254,6 +254,11 @@ sub Array (\@;@) {
     my $aref   = shift;
     my @values = @_;

+    # If only one value, and it's an arrayref, expand it
+    if (@_ == 1 && ref $_[0] eq 'ARRAY') {
+        @values = @{$_[0]};
+    }
+
 # Recursively check passed elements for references; if any, make them Readonly
     foreach (@values) {
         if    (ref eq 'SCALAR') { Scalar my $v => $$_; $_ = \$v }
@@ -299,6 +304,13 @@ eval q{sub Readonly} . ($] < 5.008 ? '' : '(\[$@%]@)') . <<'SUB_READONLY';
         croak "$REASSIGN $badtype" if $badtype;
         croak "Readonly scalar must have only one value" if @_ > 2;

+        if (defined $_[1] && ref $_[1] eq 'ARRAY') {
+            return Array @{${$_[0]}}, $_[1];
+        }
+        elsif (defined $_[1] && ref $_[1] eq 'HASH') {
+            return Hash %{${$_[0]}}, $_[1];
+        }
+
         my $tieobj = eval {tie ${$_[0]}, 'Readonly::Scalar', $_[1]};
         # Tie may have failed because user tried to tie a constant, or we screwed up somehow.
         if ($@)
@@ -372,6 +384,9 @@ Readonly - Facility for creating read-only scalars, arrays, hashes
     Readonly    %has => (key => value, key => value, ...);
     Readonly my %has => (key => value, key => value, ...);
     Readonly my $sca; # Implicit undef, readonly value
+    # or also it is possible to create read-only array/hash references from the scratch:
+    Readonly my $arrref => [value, value, ...];
+    Readonly my $hasref => {key => value, key => value, ...};

     # Alternate form (for Perls earlier than v5.8)
     Readonly    \$sca => $initial_value;