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

Regression: "Not enough arguments for Readonly" in v1.500.0 #8

Open
rohanpm opened this issue Jun 27, 2014 · 4 comments
Open

Regression: "Not enough arguments for Readonly" in v1.500.0 #8

rohanpm opened this issue Jun 27, 2014 · 4 comments

Comments

@rohanpm
Copy link

rohanpm commented Jun 27, 2014

Certain Readonly constructs which previously worked fine now fail with "Not enough arguments for Readonly" as of v1.500.0 - as in this testcase:

$ perl -MReadonly -E 'Readonly my $X => ("foo" =~ /bar/);'
Not enough arguments for Readonly at -e line 1.

Not sure what's going on there.

The non-contrived example which I actually hit is that my module had a line like this:

Readonly my $WINDOWS => ($OSNAME =~ m{win32}i);

...and it suddenly broke when Readonly was upgraded.

rohanpm added a commit to rohanpm/QMake-Project that referenced this issue Jun 27, 2014
@pghmcfc
Copy link

pghmcfc commented Jun 27, 2014

This might also be why t/23-readonly.t in Params-Validate 1.11 fails with the new Readonly:

use strict;
use warnings;

use Test::Requires {
    Readonly       => '1.03',
    'Scalar::Util' => '1.20',
};

use Params::Validate qw(validate validate_pos SCALAR);
use Test::More;

{
    Readonly my $spec => { foo => 1 };
    my @p = ( foo => 'hello' );

    eval { validate( @p, $spec ) };
    is( $@, q{}, 'validate() call succeeded with Readonly spec hashref' );
}

{
    Readonly my $spec => { type => SCALAR };
    my @p = 'hello';

    eval { validate_pos( @p, $spec ) };
    is( $@, q{}, 'validate_pos() call succeeded with Readonly spec hashref' );
}

{
    Readonly my %spec => ( foo => { type => SCALAR } );
    my @p = ( foo => 'hello' );

    eval { validate( @p, \%spec ) };
    is( $@, q{}, 'validate() call succeeded with Readonly spec hash' );
}

done_testing();

Result:

#   Failed test 'validate_pos() call succeeded with Readonly spec hashref'
#   at t/23-readonly.t line 25.
#          got: 'Parameter #1 ("hello") has a type specification which is not a number. It is undef.
#  Use the constants exported by Params::Validate to declare types. at t/23-readonly.t line 24.
#       eval {...} called at t/23-readonly.t line 24
# '
#     expected: ''
# Looks like you failed 1 test of 3.
t/23-readonly.t .........................
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests

https://rt.cpan.org/Public/Bug/Display.html?id=96758

@sanko
Copy link
Owner

sanko commented Jun 27, 2014

Sigh. Reverting and documenting.

Here's what went wrong: The prototype for Readonly::Readonly(...) is made to accept just about everything (simple scalars, hashref, etc.) so it doesn't care about undefined (not verbosely undef values) but the verbose forms (Readonly::Scalar(), et al) throw fatal errors during compile thanks to their prototypes and gag on them if passed at runtime.

Readonly::Scalar my $blah => undef; # perfectly fine
Readonly::Scalar my $blah; # fatal if caught in compile
Readonly my $blah; # slips on through; you get an undef readonly value

The documentation never mentioned this really obvious difference in the right places for some so when I received a report asking about closing what they saw as a bug in Readonly::Readonly(), I went for API consistency and made it croak(). That was stupid.

Aside: too many people have been using undocumented functionality for me to correct anything meaningful in the API without breaking a lot of code. I need to print a banner of that and hang it somewhere to remind me. :) Anyway, yeah, reverting and documenting. Next version will be on PAUSE in a few hours. I'll close this with the commit later.

Edit: Oh, and thanks for the report! :) ...more annoyed with the API than I am with users. With the sighing and all it may come off a little gruff. Really, though, thanks for the report and for using Readonly in the first place.

@sanko
Copy link
Owner

sanko commented Jun 27, 2014

@pghmcfc that seems to be something else entirely. Works fine over the old tie() API (if I change the test to use Readonly::Scalar1 ...) but not the XS/built-in version (uses SvREADONLY). I'll need to dig to see where it's going wrong.

@sanko
Copy link
Owner

sanko commented Jun 27, 2014

@pghmcfc Found it. Weird that a smoker with Readonly::XS installed didn't shake this one out long ago. Should be resolved in 1.60.

sanko added a commit that referenced this issue Jun 27, 2014
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

3 participants