Clownfish nullable bug

[Peter Karman <pe...@peknet.com>]

fyi, I've just committed a failing test to KS trunk as r5886. The issue
manifests when an abstract nullable method is overridden in a subclass. The
'nullable' flag is not being parsed correctly in Clownfish and so when the
overridden method returns undef, KS croaks.

It appears to be a bug in the grammar passed to Parse::RecDescent in
Clownfish::Parser but that RecDescent stuff is some serious voodoo and I don't
even know where to start.

-- 
Peter Karman  .  http://peknet.com/  .  peter@peknet.com

Re: [Lucy] Re: Clownfish nullable bug

[Peter Karman <pe...@peknet.com>]

Marvin Humphrey wrote on 3/3/10 10:16 AM:
> On Tue, Mar 02, 2010 at 11:16:49PM -0600, Peter Karman wrote:
>> fyi, I've just committed a failing test to KS trunk as r5886. The issue
>> manifests when an abstract nullable method is overridden in a subclass. The
>> 'nullable' flag is not being parsed correctly in Clownfish and so when the
>> overridden method returns undef, KS croaks.
> 
> "Croak" is a polite way of putting it, right?  I imagine it was segfaulting.  :)
> 

No, it was correctly croaking with "cannot be null" messages. That's how I
tracked the problem. So in that case, the feature was working -- it was just
being incorrectly applied.

[snip]

> I committed this change to the KinoSearch repository as r5887; I'll shortly
> commit both your test and the fix to the Lucy repository.  Thanks for the test!

cool. thanks for the thorough explanation on Clownfish parsing too. I am sure it
will help me in the future.

-- 
Peter Karman  .  http://peknet.com/  .  peter@peknet.com

Re: Clownfish nullable bug

[Marvin Humphrey <ma...@rectangular.com>]

On Tue, Mar 02, 2010 at 11:16:49PM -0600, Peter Karman wrote:
> fyi, I've just committed a failing test to KS trunk as r5886. The issue
> manifests when an abstract nullable method is overridden in a subclass. The
> 'nullable' flag is not being parsed correctly in Clownfish and so when the
> overridden method returns undef, KS croaks.

"Croak" is a polite way of putting it, right?  I imagine it was segfaulting.  :)

> It appears to be a bug in the grammar passed to Parse::RecDescent in
> Clownfish::Parser but that RecDescent stuff is some serious voodoo and I don't
> even know where to start.

Troubleshooting Parse::RecDescent starts with dumping either %item or @item,
the match variables that are created by P::RD for each production.  By looking
at %item for the production "type"...

    type:
        nullable(?) simple_type type_postfix(s?)
        { 
            warn Data::Dumper::Dumper(\%item);   # <------ debugging dump
            Clownfish::Parser->simple_or_composite_type(\%item) 
        }

... I was able to see that "nullable" wasn't part of that match:

    $VAR1 = {
          '__RULE__' => 'type',
          'type_postfix(s?)' => [],
          'simple_type' => bless( {
                                    'is_string_type' => 0,
                                    'decremented' => 0,
                                    'incremented' => 1,
                                    'c_string' => 'Thing*',
                                    'parcel' => bless( {
                                                         'Prefix' => '',
                                                         'name' => 'DEFAULT',
                                                         'PREFIX' => '',
                                                         'prefix' => '',
                                                         'cnick' => ''
                                                       }, 'Clownfish::Parcel' ),
                                    'const' => undef,
                                    'indirection' => 1,
                                    'specifier' => 'Thing'
                                  }, 'Clownfish::Type::Object' ),
          'nullable(?)' => []  # <-------- Missing.
        };

Therefore, it had to have been swallowed by the "simple_type" production, which
leads back to "object_type":

    simple_type:
          object_type          # <------- Match here.
        | primitive_type
        | void_type
        | va_list_type
        | arbitrary_type
        { $item[1] }

    object_type:
        type_qualifier(s?) object_type_specifier '*'
        { Clownfish::Parser->new_object_type(\%item); }  # <------ calls constructor

Clownfish::Parser->new_object_type() calls the constructor for
Clownfish::Type::Object, which is ultimately where the problem lay.  The
"nullable" parameter was being passed correctly, but the constructor was just
dropping it on the ground.  

    @@ -30,6 +30,7 @@
         $self->{incremented} = $incremented;
         $self->{decremented} = $decremented;
         $self->{indirection} = $indirection;
    +    $self->{nullable}    = $nullable;
         $self->{parcel} ||= Clownfish::Parcel->default_parcel;
         my $prefix = $self->{parcel}->get_prefix;

I committed this change to the KinoSearch repository as r5887; I'll shortly
commit both your test and the fix to the Lucy repository.  Thanks for the test!

Marvin Humphrey