tobyink / p5-type-tiny

Perl 5 distribution Type-Tiny; see homepage for downloads and documentation.
https://typetiny.toby.ink/
54 stars 48 forks source link

Use of uninitialized value $i in pattern match (m//) #138

Open XSven opened 1 year ago

XSven commented 1 year ago

Type-Tiny: 2.002001

This

use Eval::Closure   qw( eval_closure );
use Types::Standard qw( ArrayRef StrMatch );

my $KeyList = Type::Tiny->new(
  name   => 'KeyList',
  parent => ArrayRef [ StrMatch [ qr/\A [0-9A-Z]{3} \z/x ], 1 ]
);

my $assertion = eval_closure( source => 'sub { ' . $KeyList->inline_assert( '$_[0]' ) . '}' );

$assertion->( [ 'ABC', undef, 'DEF' ] );

defines a KeyList type constraint that is applied to a list with 3 elements. The second element is undef. If I set the environment variable EVAL_CLOSURE_PRINT_SOURCE to true the output is

package Eval::Closure::Sandbox_1;
sub {
  sub {
    do {
      no warnings "void";

      package Type::Tiny;
      (
        do {

          package Type::Tiny;
          ( ref( $_[ 0 ] ) eq 'ARRAY' ) and @{ $_[ 0 ] } >= 1 and do {
            my $ok = 1;
            for my $i ( @{ $_[ 0 ] } ) {
              ( $ok = 0, last ) unless do {

                package Type::Tiny;
                !ref( $i )
                  and !!( $i =~ $Types::Standard::StrMatch::expressions{ "Regexp|(?^x:\\A [0-9A-Z]{3} \\z)" } );
              }
            };
            $ok;
          }
        }
      ) or Type::Tiny::_failed_check( 42, "KeyList", $_[ 0 ], );
      $_[ 0 ];
    };
  }
  }

Use of uninitialized value $i in pattern match (m//) at (eval 129) line 3.
Reference ["ABC",undef,"DEF"] did not pass type constraint "KeyList" at strmatch.pl line 15
    "KeyList" is a subtype of "ArrayRef[StrMatch[(?^x:\A [0-9A-Z]{3} \z)],1]"
    Reference ["ABC",undef,"DEF"] did not pass type constraint "ArrayRef[StrMatch[(?^x:\A [0-9A-Z]{3} \z)],1]"
    "ArrayRef[StrMatch[(?^x:\A [0-9A-Z]{3} \z)],1]" constrains each value in the array with "StrMatch[(?^x:\A [0-9A-Z]{3} \z)]"
    "StrMatch[(?^x:\A [0-9A-Z]{3} \z)]" is a subtype of "StrMatch"
    "StrMatch" is a subtype of "Str"
    "Str" is a subtype of "Value"
    "Value" is a subtype of "Defined"
    Undef did not pass type constraint "Defined" (in $_->[1])
    "Defined" is defined as: (defined($_))

Can this warning

Use of uninitialized value $i in pattern match (m//) at (eval 124) line 3.

be avoided?

tobyink commented 1 year ago

Hmmm, interesting. StrMatch is supposed to be a child of Str, which does do a definedness check. It looks like StrMatch's inlining code is not being strict as its parent type, so isn't checking definedness. That's definitely a bug.

A workaround would be something like:

ArrayRef[
  ( Defined ) & ( StrMatch[ qr/\A [0-9A-Z]{3} \z/x ] ),
  1,
]

I'll get this fixed in 2.4.1 though.

tobyink commented 1 year ago

I actually can't reproduce this error, as eval_closure normally compiles code without use warnings. However, I still think StrMatch ought to be checking definedness, so I'll add that check.

tobyink commented 1 year ago

Ah, I see. You're using Eval::Closure, which does switch on warnings. Eval::TypeTiny does not.

XSven commented 1 year ago

I have used Params::ValidationCompiler that depends ot Eval::Closure. Now I have switched to Type::Params. I don't understand your decision not to turn on warnings in Eval::TypeTiny. How should we have detected such an issue without warnings being raised?

tobyink commented 1 year ago

These two lines are exactly equivalent in functionality:

                    !ref( $foo ) and $foo =~ m/.../
defined( $foo ) and !ref( $foo ) and $foo =~ m/.../

The only difference is that the first one will raise a warning if warnings are enabled and $foo is undef. They'll always give exactly the same boolean result.

How should we have detected such an issue without warnings being raised?

Skipping defined( $foo ) is only an issue because warnings are being raised.

XSven commented 1 year ago

I have understood this before. I like warnings and I treat them as issues. This is a matter of taste of course. I am very much appreciating your effort. I have triggered an initiative in the company that I am working for to sponsor your work. Let's see. You may close this issue unless you need it to track the changes included in the next Type::Tiny release.

tobyink commented 1 year ago

Oh yeah, in normal code, I'd keep the warnings on for sure and would check definedness. Inline-generated code, I tend to value performance over maintainability and readability though.

I'll keep the issue open until the next release. Just adds a bit of visibility in case somebody else tried to report the same issue. (Unlikely, but can happen.)