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

Different behavior of $foo ~~ 0 in 5.10.0. #59

Closed
schwern opened this issue Oct 27, 2012 · 5 comments
Closed

Different behavior of $foo ~~ 0 in 5.10.0. #59

schwern opened this issue Oct 27, 2012 · 5 comments
Labels

Comments

@schwern
Copy link
Contributor

schwern commented Oct 27, 2012

t/zero_defaults.t is failing on 5.10.0. The problem is the copy_cat() method. It does not consider an empty string to be a default. The issue is with how $foo ~~ 0 behaves.

copy_cat expands out into this:

sub copy_cat {
    my $self = shift;
    Method::Signatures->required_arg('$this') unless (@_ > 0);
    my $this = $_[0];
    my $that = !(@_ > 1) ? ( $this ) : do {
        no warnings;
        my $arg = $_[1];
        $arg ~~ 0 ? $this : $arg
    };
    Method::Signatures->too_many_args_error(2) if @_ > 2; 

    return $that;
}

On 5.10.0, "" ~~ 0 is false. From 5.10.1 on it's true.

Smart match before 5.10.1 is unstable. Simplest thing to do might be to boost the minimum requirement for that feature to 5.10.1.

@thoughtstream
Copy link
Contributor

Arguably it's post-5.10.1 that's broken... for MS purposes. :-)

Actually, though, it's the test that's broken.
Cut-and-paste error. Mea culpa!

The line:

    is( Stuff->copy_cat("wibble", ""), "wibble" );

should be:

    is( Stuff->copy_cat("wibble", 0), "wibble" );

@thoughtstream
Copy link
Contributor

BTW, the new new smartmatch semantics currently proposed
for 5.18 are going to mess this mechanism up again and will
necessitate a rewrite to support backwards compatibility. I'll volunteer
to provide that rewrite when the time comes, as I'll have to do the
same for various other modules that rely on 5.10.1 smartmatching
semantics.

@schwern
Copy link
Contributor Author

schwern commented Oct 28, 2012

I think the behavior of when needs to be clarified. The examples in the
docs make it pretty clear that it's an exact match.

# Use default if no argument passed OR argument is undef
method get_results($how_many = 1 when undef) {...}

# Use default if no argument passed OR argument is empty string
method get_results($how_many = 1 when "") {...}

# Use default if no argument passed OR argument is zero
method get_results($how_many = 1 when 0) {...}

If that's the case, if when is going to try and differentiate undef from 0
from empty string, the test needs all of them.

is( Stuff->copy_cat("wibble"),        "wibble" );
is( Stuff->copy_cat("wibble", 0),     "wibble" );
is( Stuff->copy_cat("wibble", ""),    "" );       # fails in 5.10.1 and up
is( Stuff->copy_cat("wibble", undef), undef );

I would suggest not using smart match. It's unstable across versions and
doing it wrong for a large spread of them (5.10.1 to now). I'm guessing its
there to avoid having to play the "is this a number or a string" game which is
an equally obnoxious hassle.

Alternatively we special case empty string.

Alternatively we define "when" as "whatever smart match does".

If we're going to use smart match, I like the last one. We don't have to work
around smart match changes. It also means things like "when []" or "when {}"
are legit without us having to explain anything.

@thoughtstream
Copy link
Contributor

I think we ought to go with "whatever smartmatch does".

If they need to be more specific, there's always:

 method get_results($how_many = 1 when qr/^0$/) {...}     

or:

use Smart::Match 'numwise';
method get_results($how_many = 1 when numwise 0) {...} 

They're going to have to get used to being more specific anyway,
once 5.18 is out.

Damian

@barefootcoder
Copy link
Contributor

Okay, this was causing 12 of the 17 failures on CPAN Testers. Fixed as per @thoughtstream suggested. I'm closing this one down.

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

No branches or pull requests

3 participants